Closed Bug 1004418 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Attachment #8475154 - Flags: review?(ttaubert)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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? :-)
Iteration: --- → 34.3
Points: --- → 2
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Flags: needinfo?(mmucci) → qe-verify?
Flags: qe-verify? → qe-verify-
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.
Attachment #8475154 - Flags: review?(ttaubert) → review+
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
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #8475154 - Attachment is obsolete: true
Attachment #8477344 - Flags: review+
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.
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
Attachment #8477344 - Attachment is obsolete: true
w/ nits + disabling looking at jsloader/jssubloader entries:

remote:   https://hg.mozilla.org/integration/fx-team/rev/966c24f8e64d
Whiteboard: [fixed-in-fx-team]
Followup to disable on debug: https://hg.mozilla.org/integration/fx-team/rev/b1c83316ffd6
it's the skipping that keeps on... skipping?

remote:   https://hg.mozilla.org/integration/fx-team/rev/2daf21638de1
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(paolo.mozmail)
(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. :-)
Flags: needinfo?(paolo.mozmail)
Blocks: 1065446
You need to log in before you can comment on or make changes to this bug.