636835 - Validator misses several errors for the attached XPI

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P2)

Description

[Kris Maglione [:kmag]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre
Build Identifier: 

The attached XPI is missing quite a few error flags, including:

 • components/scriptish.js:313:Cu.evalInSandbox( 
 • components/scriptish.js:324:Cu.evalInSandbox(src, aSandbox, jsVer, fileURLPrefix+fileURL, 1);
 • components/scriptish.js:329:Cu.evalInSandbox( 
 • content/js/install.js:40:function delayedClose() setTimeout(close, 0); 
 • modules/constants.js:30: get process() Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess),  
 • modules/script/script.js:512: var scriptNode = doc.createElement("Script");
 • modules/third-party/Scriptish_openFolder.js:47: if (fParent.exists()) fParent.launch(); 



Reproducible: Always

Comment 1

[Kris Maglione [:kmag]]

Attachment: Failing XPI: Scriptish 0.1b9

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

All of those look pretty important.

Comment 3

[Matt Basta [:basta]]

Just fixed the second to last error; the validator was doing a case-sensitive search.

Comment 4

[Matt Basta [:basta]]

There were a number of bugs which were causing the JS engine to fail out. They're now fixed. All of the evalInSandbox errors are showing up, as is the aforementioned case sensitivity bug. The setTimeout message is also being shown.

The problem with the Ci.nsIProcess() bug is that I have no idea how to map ObjectPatterns from JS 1.7 onto the local scope when they're declared. I'll be looking into it.

As for the fParent.launch() message, what exactly are we testing for? Any call to "launch()"? In the code, fParent is derived from aFile, which is passed as an argument to a function. There's no way to determine their context or map it to anything in the predefined entities tree. Perhaps this is something we should be testing for with Regex?

Comment 5

[Kris Maglione [:kmag]]

Re nsILocalFile.launch, I think you may as well flag calling any method called 'launch'. That's what the old validator did and it didn't seem to cause any undue noise. It would be great if it could only be done for nsILocalFile instances, but that doesn't seem feasible and I don't think it will be a major issue in practice.

Comment 6

[Matt Basta [:basta]]

I'll just include a regex for launch() since searching for <nsILocalFile>.launch() will duplicate the error.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Another failing XPI

This XPI has a similar failed detection of a setTimeout call with a string argument. I only caught it because of my runtime validator. Forgive me for dumping this into this bug. I'll file a separate report if you want.

chrome://osokker/content/oSokker.js:3401:		setTimeout( 'com.bz.oSokker.os_mailAlert()', mailAlertInterval );

Looking at the source, there seem to be quite a lot of other similarly missed calls. Perhaps tests are not descending into with bodies?

Comment 8

[Kris Maglione [:kmag]]

Attachment: Minimized testcase

Yes, that would appear to be the case.

Comment 9

[Matt Basta [:basta]]

Hmmm...looks like I had an incomplete thought. If the validator encounters a setTimeout with a literal string, it should load up Spidermonkey and parse the string, then traverse the nodes as if the code was being called inline. Right now, it just kind of skims over that last part. Just proves that I'm easily distracted. Will fix with the rest of the stuff, but maybe not until tomorrow (spring break starts after class today and I have a wonderful rush-hour drive home).

Comment 10

[Matt Basta [:basta]]

This should fix all of the issues from the original XPI:

https://github.com/mattbasta/amo-validator/commit/c73adad815f7b77247cd2504c418008a2743e15d

All of the errors mentioned are reported properly.

I'm working on the second XPI. Not the same problem, but I'd might as well get it done here anyway.

Comment 11

[Matt Basta [:basta]]

This should fix the last of it:

https://github.com/mattbasta/amo-validator/commit/c6a83a3931d915e7c7b17d38b430f22472c3c9c2

One of the action handlers was returning False instead of None, causing the JS engine to ignore certain branches of the code, namely the ones that deal with the body of With statements.

I also fixed some bugs with division by zero errors.

Give it a look and let me know what you think.

Comment 12

[Wil Clouser [:clouserw]]

Reopening because I don't think it's on mozilla/amo-validator yet, and that's the one we're pulling from these days.  Most of the time we get r? requests on IRC, but using github's pull request works too.

Comment 13

[Matt Basta [:basta]]

Fair enough, will do.

Comment 14

[Matt Basta [:basta]]

Pushed:

https://github.com/mozilla/amo-validator/commit/2892e17735aa64f5042c6f93c502d5fe9655711f

Comment 15

[Kris Maglione [:kmag]]

Everything looks good.

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...