1004418 - Create a mochitest to verify that all JS we ship is parsable (and doesn't do dumb things) in our JS implementation

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gijs (he/him)]

Split off from bug 971096.

I have some ideas on how to do this. In particular, I'd like to:

1) have a set of windows with a JS function that opens them and returns a promise that gets resolved when they're ready
2) for each window, get a list/generator of all the scripts loaded in that global (I believe the debugger API can do this, but I've not checked; if necessary we can walk the DOM, but that's imperfect)
3) parse each script into an AST (fail test if this doesn't work, obviously)
4) create a set of analysers to run against the AST that check for the global variables that get created, and the use of undefined variables or other brokenness.

As a first step, I think we should shortcut to step 3 by just looking for all files with a .js/.jsm extension ala bug 971096.

Comment 1

[:Gijs (he/him)]

Attachment: create a mochitest to verify all JS we ship parses,

This seems to work. I've kind of changed approach completely here and just parse everything using Reflect.jsm. This gets us ASTs, which I've been too lazy to do much with. I think this is probably an OK first step? The AST checks I think can be split off into their own bug; I'm afraid of running into significant numbers of false positives there, where globals are defined in different files but end up in the same scope, and that leading to 'undefined variable' problems.

Comment 2

[:Gijs (he/him)]

Marco, can you add this to the spreadsheet? I worked on this on Tuesday morning, because I was stuck on a train that (contrary to expectations) did not have wifi, and so I couldn't access the bugs/assets I was planning to work on this iteration. According to the spreadsheet there's still room to pick stuff up this iteration, so I presume this is OK? :-)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8475154 [details] [diff] [review]
create a mochitest to verify all JS we ship parses,

Review of attachment 8475154 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: browser/base/content/test/general/browser_parsable_script.js
@@ +30,5 @@
> +  return false;
> +}
> +
> +function parsePromise(uri) {
> +  let deferred = Promise.defer();

Please use the official ES6 API, i.e. |new Promise(resolve => ...|

@@ +45,5 @@
> +        ok(false, "Script error reading " + uri.spec + ": " + ex);
> +        deferred.resolve(false);
> +        return;
> +      }
> +      deferred.resolve(true);

Could put that in the try {} block and remove the return above.

@@ +49,5 @@
> +      deferred.resolve(true);
> +    }
> +  };
> +  xhr.send(null);
> +  return deferred.promise;

xhr.onerror = reject; (when using the official API)

::: browser/base/content/test/general/parsingTestHelpers.jsm
@@ +21,5 @@
> + * @param dir the directory which to scan for files (nsIFile)
> + * @param extensions the extensions of files we're interested in (Array).
> + */
> +function generateURIsFromDirTree(dir, extensions) {
> +  let rv = [];

Looks like |rv| should be defined inside the task.

@@ +30,5 @@
> +  return Task.spawn(function*() {
> +    while (dirQueue.length) {
> +      let nextDir = dirQueue.shift();
> +      let {subdirs, files} = yield iterateOverPath(nextDir, extensions);
> +      dirQueue = dirQueue.concat(subdirs);

dirQueue.push(...subdirs)?

@@ +31,5 @@
> +    while (dirQueue.length) {
> +      let nextDir = dirQueue.shift();
> +      let {subdirs, files} = yield iterateOverPath(nextDir, extensions);
> +      dirQueue = dirQueue.concat(subdirs);
> +      rv = rv.concat(files);

rv.push(...files)?

@@ +39,5 @@
> +}
> +
> +/* Shorthand constructors to construct an nsI(Local)File and zip reader: */
> +const LocalFile = new Components.Constructor("@mozilla.org/file/local;1", Ci.nsIFile, "initWithPath");
> +const ZipReader = new Components.Constructor("@mozilla.org/libjar/zip-reader;1", "nsIZipReader", "open");

I'd put those at the top of the file.

@@ +53,5 @@
> + * @param extensions the file extensions we're interested in.
> + */
> +function iterateOverPath(path, extensions) {
> +  let iterator = new OS.File.DirectoryIterator(path);
> +  let parentDir = new LocalFile(path);

Using LocalFile shouldn't be necessary.

@@ +62,5 @@
> +    function onEntry(entry) {
> +      if (entry.isDir) {
> +        let subdir = parentDir.clone();
> +        subdir.append(entry.name);
> +        subdirs.push(subdir.path);

subdirs.push(entry.path);

@@ +66,5 @@
> +        subdirs.push(subdir.path);
> +      } else if (extensions.some((extension) => entry.name.endsWith(extension))) {
> +        let file = parentDir.clone();
> +        file.append(entry.name);
> +        let uriSpec = getURLForFile(file);

let uriSpec = getURLForFile(entry.path);

@@ +70,5 @@
> +        let uriSpec = getURLForFile(file);
> +        files.push(Services.io.newURI(uriSpec, null, null));
> +      } else if (entry.name.endsWith(".ja") || entry.name.endsWith(".jar")) {
> +        let file = parentDir.clone();
> +        file.append(entry.name);

You can use entry.path here too.

@@ +74,5 @@
> +        file.append(entry.name);
> +        for (let extension of extensions) {
> +          let entryIterator = generateEntriesFromJarFile(file, extension);
> +          let subentries = [uri for (uri of entryIterator)];
> +          files = files.concat(subentries);

files.push(...entryIterator)? That should work I think.

@@ +88,5 @@
> +  }, function(e) {
> +    outerPromise.reject(e);
> +    iterator.close();
> +  });
> +  return outerPromise.promise;

Maybe:

return Task.spawn(function* () {
  try {
    return (yield promise);
  } finally {
    iterator.close();
  }
});

Although instead of using |promise| here defining an iterator function and doing |yield iterator.forEach(iterateEntry)| would be a little easier to grasp, imo.

Comment 4

[:Gijs (he/him)]

Try push:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=fc8feeb2f07b
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fc8feeb2f07b

Comment 5

[:Gijs (he/him)]

Attachment: Patch for checkin

Comment 6

[:Gijs (he/him)]

Sigh. That was fun:

 2011 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_parsable_script.js | There should be 0 parsing errors - Got 227, expected 0 

I expect the jar stuff is broken; --app-override is broken on mac, so I had trouble testing it.

Comment 7

[:Gijs (he/him)]

Attachment: Patch for checkin v1.1

Well, that was fun. We store jsloader/jssubloader caches in omni.ja, and that affects this test against packaged builds. Renewed trypush ignoring those folders:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=23ead8e7658d
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=23ead8e7658d

Comment 8

[:Gijs (he/him)]

w/ nits + disabling looking at jsloader/jssubloader entries:

remote:   https://hg.mozilla.org/integration/fx-team/rev/966c24f8e64d

Comment 9

[:Gijs (he/him)]

Followup to disable on debug: https://hg.mozilla.org/integration/fx-team/rev/b1c83316ffd6

Comment 10

[:Gijs (he/him)]

it's the skipping that keeps on... skipping?

remote:   https://hg.mozilla.org/integration/fx-team/rev/2daf21638de1

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/966c24f8e64d
https://hg.mozilla.org/mozilla-central/rev/b1c83316ffd6
https://hg.mozilla.org/mozilla-central/rev/2daf21638de1

Comment 12

[:Paolo Amadini]

This test failed after a change to MozLoopAPI.jsm, but the log doesn't tell me anything useful about the cause of the error, and the file itself apparently parses and works correctly:

https://tbpl.mozilla.org/?tree=Try&rev=e67adcc1cb39

Strangely, the test doesn't work on debug builds. Is there a way I can get more information about the failure, other than the expensive process of setting up an optimized build? I couldn't find any documentation about what this test does exactly, and how to debug it.

Comment 13

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #12)
> This test failed after a change to MozLoopAPI.jsm, but the log doesn't tell
> me anything useful about the cause of the error, and the file itself
> apparently parses and works correctly:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=e67adcc1cb39
> 
> Strangely, the test doesn't work on debug builds.

It works just fine, but it takes so long that it's disabled there because our automation infra would complain otherwise.

> Is there a way I can get
> more information about the failure, other than the expensive process of
> setting up an optimized build?

The test just uses XMLHttpRequest to load each js/jsm file, and the uses Reflect.jsm to parse it. Your push managed to cause an internal parse error, which seems like it's a bug in Reflect.jsm and/or the JS parser. I would suggest filing a bug there. If this is blocking critical stuff, you can add an exception to the test at the top of the file.

If you can't run the entire test yourself in debug mode, I would suggest just using a scratchpad snippet to run the test just for your jsm, in a debug build, and then figuring out what's going wrong in Reflect.jsm/JS.

>  I couldn't find any documentation about what
> this test does exactly, and how to debug it.

Where would you expect such documentation, and/or how would debugging the test be any different from any other browser mochitest?

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/try/rev/b6e989cca2af#l1.77

Was the removal of the return in the value getter there intentional? Not sure if that causes the issue here, but it seems wrong on the face of it.

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> https://hg.mozilla.org/try/rev/b6e989cca2af#l1.77
> 
> Was the removal of the return in the value getter there intentional? Not
> sure if that causes the issue here, but it seems wrong on the face of it.

Egh, nevermind, I misread. :-)