770770 - refactor webapp runtime test harness to reduce complexity/special-casing

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P2)

Description

[Drew Willcoxon :adw]

Currently the ShutdownLeakLogger isn't used for WebappRT chrome tests.  See bug 733631 comment 46 and onward for why.  It should be enabled and the non-meaningful leaks reported by the logger should be fixed.

Comment 1

[Drew Willcoxon :adw]

Attachment: patch

This creates a new content browser per test.*  It keeps the original browser around (and collapsed) so that the new browsers can be cloned from it instead of creating them from scratch with the same attributes as the original.

It also keeps some unrelated improvements I made during the course of working on this, like removing the webapprt-test-did-install observer when the runtime window is closed, and having head.js load the webapp rather than install.html doing it, and cleaning up a couple of small things from the original landing.  I hope that's OK.

* I couldn't get the technique of going back in session history and then calling PurgeHistory (like I mentioned in bug 733631 comment 63) to work with multiple tests.  PurgeHistory purges the first n entries in session history, not the last.  When you start with about:blank, load page A, load page B, and then go back to about:blank and PurgeHistory(2) when the test is done, you end up starting the next test at page B, not about:blank, and ultimately page B is leaked.  I also tried various eviction-related methods on nsISHistoryInternal, nsISHEntry, and nsIBFCacheEntry, but none worked, some resulting in strange errors probably because I was using them in strange ways.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 640444 [details] [diff] [review]
patch

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

Looks good, r=myk!

::: webapprt/test/chrome/head.js
@@ +74,5 @@
> +  let originalContent = document.getElementById(contentID);
> +  let newContent = originalContent.cloneNode(false);
> +  originalContent.id = originalContentID;
> +  originalContent.collapsed = true;
> +  originalContent.parentNode.appendChild(newContent);

Nit: if we ever add bottom chrome, this would make the new <browser> appear below it.  To avoid the possibility of that happening, do |originalContent.parentNode.insertBefore(newContent, originalContent.nextSibling)| (the equivalent of `insertAfter`) instead.

Comment 3

[Drew Willcoxon :adw]

I had to unbitrot this patch for bug 771395, and now browser_window-title.js is leaking the docshell of the new browser that's created when the test starts -- but only if browser_window-title.js loads both of the URLs it loads.  If it loads only one, no leak.  Whatever's different between loading one URL and loading two is causing something to hold on to the docshell.  (This kind of stuff always makes me want to quit Mozilla and start a farm or something.)

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: unbitrotted and fixes window title test failures

I unbitrotted the patch again, and I fixed a window title test failure (by adding the progress listener that updates the window title to each new <browser> the test harness creates).

I don't see a leak:

TEST-PASS | automationutils.processLeakLog() | no leaks detected!

But I do still see a failure:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_sample.js | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIFileInputStream.init] at resource://webapprt/modules/WebappRT.jsm:47

Some debugging reveals that the file not found is /Users/myk/Library/Application Support/Webapp Runtime/webapp.json.  That's as far as I've got.

Comment 5

[Drew Willcoxon :adw]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> But I do still see a failure:

That's because progressListener is being notified when the test navigates to install.html to install its test app.  But of course by that point no app has been installed, so WebappRT.config has not been set up properly, so when progressListener accesses WebappRT.config, WebappRT.config goes looking for webapp.json, which isn't there.  So it's time to add more workarounds for this crappy, horrible test framework I've made.

I'm still seeing the leak, even when I comment out the body of progressListener.onLocationChange so the test can carry on.  I tried Tim's patch in bug 728294 as suggested by Tim and Gavin, and no leak is reported, but as far as I can tell it's missing the class of leak that my leak belongs to.  It appears that something is holding on to the docshell until the chrome window closes, but only in certain cases:  I've reduced the test down to doing nothing more than loading a page, and unbelievably the only difference between a leaking run and a nonleaking run is the URL of the page that the test loads.  (http://example.com/ leaks, http://mochi.test/ doesn't.  There are other examples.)  When I break in gdb, I see that in the nonleaking case the docshell is collected when browser-test.js calls schedulePreciseGC, which is what I'd expect.  In the leaking case, the docshell is not collected until the chrome window is closed.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Drew: I have some ideas about some test harness changes that might resolve this issue.  Mind if I take this over?

Comment 7

[Drew Willcoxon :adw]

Sure.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: refactor test harness

Ok, here's a patch that causes the leak to go away (in conjunction with the fix for bug 780686) but does a whole bunch of other things besides.  They may not all be needed to fix the leak, but they're a valuable set of changes that are worth doing in any case, and I'm not actually sure which set fixes the leak, as the changes are architectural in nature and were not amenable to incremental testing.

(I can create a separate bug to track this work, and make this one depend on that one, if you'd prefer.)

The changes mainly focus on isolating test-specific functionality from the general functionality of the runtime.  I wanted as few files as possible to know that a runtime session is in test mode and put as much test functionality as possible into test-specific files.

With these changes, of the files that implement general functionality, only CommandLineHandler.js (and WebappRT.jsm, to a very small extent) knows about test mode; the rest of the test-specific code is in test-specific files, including a singleton XUL window "shim" (mochitest.xul) that loads in test mode.

That shim window is now where chrome tests load.  Each test then opens a webapp window when it installs an app and closes it once it finishes testing it.

The functionality available to the chrome tests remains the same, although obviously the means of access are a bit different, since the webapp window is a separate window.

But the functionality of content tests has changed in one important respect: they can't install webapps.  Instead, they all load within a single webapp that the shim installs before running them, and they only have access to the same functionality that webapp content can access.

That makes it much simpler to create a content test, since one only has to provide the test itself, cutting the requirement down from three files (webapp installer, webapp manifest, test) to one (test).  And it doesn't particularly limit the functionality one can test, since one can still create a chrome test to test functionality that needs a new app to be installed.  So I think it's better than worse.  And more consistent with the browser content tests.


In the process, I made a couple more changes that weren't strictly necessary but simplify the code and testing codepaths:

1. Stop freezing WebappRT.config, since doing so doesn't provide any actual protection when consumers can (and do, in the case of the test code) just replace it wholesale.  (I originally froze it in a misguided attempt to protect the internal state of this module the way Jetpack modules do.  But it doesn't make sense to do that for JSMs, especially not as a one-off like this.)

2. Move startup code to a JavaScript module, where it loads after the other modules on which it depends are definitely initialized, and where it doesn't potentially interfere with the command line handler.


There are also a few Mochitest changes in this patch that are not webapprt-specific:

1. On Windows, options.failureFile is a Windows/Unix hybrid filepath (drive-prefixed, but forward-slashed, like C:/Users/myk/m-c/obj/...), but Mochitest passes it to nsILocalFile.initWithPath(), which throws on such paths (it expects Windows-native paths like C:\Users\myk\m-c\obj\...).  So the patch canonicalizes it first via Mochitest.getFullPath().

2. SimpleTest/setup.js's parseQueryString() parses non-boolean parameters into arrays, but then it passes them to functions that use them as strings, which happens to work in the common case (because ["foo"] stringifies to "foo") but is brittle.  So the patch makes the code pass the first value of each such array instead.

Comment 9

[Drew Willcoxon :adw]

I haven't looked at the patch yet, but that all sounds good except maybe this:

(In reply to Myk Melez [:myk] [@mykmelez] from comment #8)
> That shim window is now where chrome tests load.  Each test then opens a
> webapp window when it installs an app and closes it once it finishes testing
> it.

Is it possible that this could hide leaks in the webapp window?  Dao made a comment along those lines when I talked about using a window per test: bug 733631 comment 62.

The docshell leak I was seeing when using a xul:browser per test is no longer reported now that bug 728294 has landed.  (Although the docshell still outlives its test...)

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: clean up inconsistent usage of nsIURI

This is a followup patch with very minor changes, fixing some inconsistent usage of nsIURI.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to Drew Willcoxon :adw from comment #9)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #8)
> > That shim window is now where chrome tests load.  Each test then opens a
> > webapp window when it installs an app and closes it once it finishes testing
> > it.
> 
> Is it possible that this could hide leaks in the webapp window?  Dao made a
> comment along those lines when I talked about using a window per test: bug
> 733631 comment 62.

I'm not sure, but I don't think so.  In both cases, it looks like the leak detection takes place after the last webapp window is closed, whether that's the single webapp window of the current implementation or the last in the series of such windows created by this change.  So I don't think this patch affects whether or not leaks within such windows will be detected.

Comment 12

[Drew Willcoxon :adw]

browser-test.js checks for leaks on the webapp window (and the Firefox window, for browser chrome tests) at the end of each test:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#237

If a test were to leak an object on the window or exercise some part of the runtime that leaked an object, we wouldn't catch it if the window is closed at that point.  Currently no webapp test closes the one and only webapp window, so it's not a problem now.

But with this patch, browser-test.js no longer runs in the scope of webapp windows, so leaks on them won't be caught anyway.  I think that might be a problem.  What do you think?

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: refactor while still using single webapp window

(In reply to Drew Willcoxon :adw from comment #12)
> But with this patch, browser-test.js no longer runs in the scope of webapp
> windows, so leaks on them won't be caught anyway.  I think that might be a
> problem.  What do you think?

Hmm, I see what you mean.  Ok, here's an alternative that still isolates test-specific code but runs browser-test.js and all tests in the scope of a single webapp window.

This version still opens a shim window, but the shim's role is restricted to installing a webapp and opening the webapp window that then runs all the tests.

Thus tests are still run in a single webapp window, as they are currently, so that leaks in it can be detected; but the runtime session is fully configured by the time that window opens, so webapp.js doesn't have to know about test mode.

Note that the shim unconditionally installs webapprt/test/content/test.webapp, whether it's running chrome or content mochitests.  The chrome mochitests don't test that app, but it's still necessary to install *some* app before opening the webapp window, and that one is available, so we might as well reuse it.

Speaking of reuse, both the shim and head.js want to install apps, and they can reuse the code to do it, but enabling them to do so proved unstraightforward.  In the end, I put the code into webapprt/content/mochitest.js, load it fairly normally in the shim document webapprt/content/mochitest.xul (albeit being careful about the order in which code in that document is loaded), and then use the subscript loader to load it in head.js.

Comment 14

[Drew Willcoxon :adw]

I don't understand the need for mochitest.xul.  Am I wrong that it's just a vehicle for running the script embedded in it and imported into it, and none of that script cares what window it's running in?  Is a separate window necessary?

Actually, I mentioned bug 728294 earlier, but its landing removed ShutdownLeakLogger, which was the thing that reported the DOMWindow "leaks" that are the subject of this bug.  There's no more ShutdownLeakLogger, and there are no more leaks reported by it, so this bug doesn't make sense anymore.  Your patches are really about refactoring, so let's continue review either in a new bug or a morphed version of this bug.  What do you think?

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to Drew Willcoxon :adw from comment #14)
> I don't understand the need for mochitest.xul.  Am I wrong that it's just a
> vehicle for running the script embedded in it and imported into it, and none
> of that script cares what window it's running in?

That's correct!  mochitest.xul is just a vehicle for the code that installs an app, and that code doesn't care what window it's running in.


> Is a separate window necessary?

It isn't strictly necessary, but the test-mode codepath has to install an app before certain startup/initialization code is evaluated, and app installation is asynchronous.

The current implementation jumps through a variety of hoops to delay startup/init until the first app is installed, and those hoops seem complex and error-prone, liable to trip us up as we make non-test changes to the implementation.  Thus I focused on reducing the footprint of test-mode code in non-test-specific files.

I considered putting the code into a module that CommandLineHandler.js imports before opening the initial webapp window.  But app installation uses a DOM API that isn't available in a module context, and it isn't clear how possible/safe it is to access/bypass that API to install an app outside of the context of a DOM window.  (I would love to be wrong about this, though, as it would be preferable.)

I also tried getting startup/init working without the shim code by faking an initial WebappRT.config in CommandLineHandler.js.  That works ok for the startup/init code currently in the codebase, because none of it looks up the app in the webapp registry.  But it'll fail the moment we add startup/init code that does access the registry.  And since content tests access the registry, it means they would need to retain their ability to install apps (and the requirement that they do so before running).


> Actually, I mentioned bug 728294 earlier, but its landing removed
> ShutdownLeakLogger, which was the thing that reported the DOMWindow "leaks"
> that are the subject of this bug.  There's no more ShutdownLeakLogger, and
> there are no more leaks reported by it, so this bug doesn't make sense
> anymore.  Your patches are really about refactoring, so let's continue
> review either in a new bug or a morphed version of this bug.  What do you
> think?

That makes sense.  Since there's plenty of context in this bug, and the bug is no longer useful for its original purpose, let's morph it.  I have chosen a new summary, but I'm happy for you to wordsmith it!

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 650677 [details] [diff] [review]
patch v3: refactor while still using single webapp window

TryServer run to make sure my Mochitest changes don't break other Mochitest suites:

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

Comment 17

[Drew Willcoxon :adw]

Comment on attachment 650677 [details] [diff] [review]
patch v3: refactor while still using single webapp window

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

::: webapprt/WebappRT.jsm
@@ +49,5 @@
> +  set config(newVal) {
> +    this._config = JSON.parse(JSON.stringify(newVal));
> +  },
> +
> +  get launchURL() {

This returns an nsIURI, not a URL; "URL" generally means the referent is a string.

::: webapprt/content/mochitest.js
@@ +7,5 @@
> +
> +const MANIFEST_URL_BASE = Services.io.newURI(
> +  "http://mochi.test:8888/webapprtChrome/webapprt/test/chrome/", null, null);
> +
> +// Dependencies implicitly provided by scope into which script is loaded:

Which "script"?  I think you mean "this script"?

@@ +9,5 @@
> +  "http://mochi.test:8888/webapprtChrome/webapprt/test/chrome/", null, null);
> +
> +// Dependencies implicitly provided by scope into which script is loaded:
> +//Cu.import("resource://gre/modules/Services.jsm");
> +//Cu.import("resource://webapprt/modules/WebappRT.jsm");

I don't understand why you don't just import these here.  Also, it doesn't seem like WebappRT.jsm is available when mochitest.js is evaluated due to its inclusion in mochitest.xul, since mochitest.xul imports WebappRT.jsm after including mochitest.js...?

@@ +24,5 @@
> +      win.addEventListener("load", function onLoadWindow() {
> +        if (win.location == "chrome://global/content/commonDialog.xul" &&
> +            win.opener == window) {
> +          win.close();
> +        }

I think you need an else branch that removes the load listener?  You could remove it unconditionally actually.

@@ +66,5 @@
> +  Services.obs.addObserver(observeInstall, "webapps-ask-install", false);
> +
> +  // Step 1: Install the app at the URL specified by the manifest.
> +  let url = Services.io.newURI(manifestURL, null, MANIFEST_URL_BASE);
> +  navigator.mozApps.install(url.spec, parameters);

I wonder whether calling this from chrome rather than content could potentially mess something up.  I thought that somewhere along the line mozApps.install examines the caller's origin to determine an "install origin," for example.

::: webapprt/content/mochitest.xul
@@ +13,5 @@
> +  const Ci = Components.interfaces;
> +  const Cu = Components.utils;
> +
> +  // mochitest.js needs this, so we must import it before we load that script.
> +  Cu.import("resource://gre/modules/Services.jsm");

I don't understand why mochitest.js can't just import this.

::: webapprt/test/chrome/browser_sample.js
@@ +1,4 @@
>  // This is a sample WebappRT chrome test.  It's just a browser-chrome mochitest.
>  
> +// Dependencies implicitly provided by scope into which script is loaded:
> +//Cu.import("resource://webapprt/modules/WebappRT.jsm");

I don't understand why you can't just import it here.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: addresses review nits

(In reply to Drew Willcoxon :adw from comment #17)

> ::: webapprt/WebappRT.jsm
> @@ +49,5 @@
> > +  set config(newVal) {
> > +    this._config = JSON.parse(JSON.stringify(newVal));
> > +  },
> > +
> > +  get launchURL() {
> 
> This returns an nsIURI, not a URL; "URL" generally means the referent is a
> string.

I'm aware of that convention in the Firefox codebase, but I don't find it compelling.  Most nsIURI objects represent URLs (as does this one).  And URLs are URIs, regardless of whether they are represented as strings or nsIURI objects.  

It is sometimes useful to distinguish between a string and an nsIURI representation of a URL, when both kinds of representations are used in the same function.  But I would use a more explicit mechanism in that case, such as appending "Str" to the string representation's name.

Nevertheless, the runtime is bundled with Firefox, is part of the Firefox module, and is hacked on mostly by Firefox engineers; so it's reasonable to follow Firefox conventions.  Thus I changed the name to launchURI.

https://github.com/mykmelez/mozilla-central/commit/ff9ec0bd9e9692d5cd3506a319fbc8ccd806fe67


> ::: webapprt/content/mochitest.js
> @@ +7,5 @@
> > +
> > +const MANIFEST_URL_BASE = Services.io.newURI(
> > +  "http://mochi.test:8888/webapprtChrome/webapprt/test/chrome/", null, null);
> > +
> > +// Dependencies implicitly provided by scope into which script is loaded:
> 
> Which "script"?  I think you mean "this script"?

Yes, this script!  For all occurrences of that comment.  I added "this" to the comment (although a subsequent change removed the comment entirely).

https://github.com/mykmelez/mozilla-central/commit/8f0762d6f9ce0a2fe2e470fb4494a20a690ce8d3


> @@ +9,5 @@
> > +  "http://mochi.test:8888/webapprtChrome/webapprt/test/chrome/", null, null);
> > +
> > +// Dependencies implicitly provided by scope into which script is loaded:
> > +//Cu.import("resource://gre/modules/Services.jsm");
> > +//Cu.import("resource://webapprt/modules/WebappRT.jsm");
> 
> I don't understand why you don't just import these here.

In earlier testing, some leak detector was reporting leaks of the symbols imported by these modules when mochitest.js was loaded by head.js.  But it no longer does so, so I uncommented the import statements and removed the comment.

https://github.com/mykmelez/mozilla-central/commit/31c465b0abb9fea547d40068940521e2aaaa79dd


> Also, it doesn't
> seem like WebappRT.jsm is available when mochitest.js is evaluated due to
> its inclusion in mochitest.xul, since mochitest.xul imports WebappRT.jsm
> after including mochitest.js...?

WebappRT.jsm is not yet available on evaluation of mochitest.js, but mochitest.js doesn't need it until its becomeWebapp() function is called, by which time WebappRT.jsm has been imported by the inline script in mochitest.xul.  Nevertheless, the previous change makes mochitest.js import it anyway, so there's no chance of confusion.


> @@ +24,5 @@
> > +      win.addEventListener("load", function onLoadWindow() {
> > +        if (win.location == "chrome://global/content/commonDialog.xul" &&
> > +            win.opener == window) {
> > +          win.close();
> > +        }
> 
> I think you need an else branch that removes the load listener?  You could
> remove it unconditionally actually.

Good point, listener removed.

https://github.com/mykmelez/mozilla-central/commit/485074018dc2478d78c63fd4ed4b5d884cc994c0


> @@ +66,5 @@
> > +  Services.obs.addObserver(observeInstall, "webapps-ask-install", false);
> > +
> > +  // Step 1: Install the app at the URL specified by the manifest.
> > +  let url = Services.io.newURI(manifestURL, null, MANIFEST_URL_BASE);
> > +  navigator.mozApps.install(url.spec, parameters);
> 
> I wonder whether calling this from chrome rather than content could
> potentially mess something up.  I thought that somewhere along the line
> mozApps.install examines the caller's origin to determine an "install
> origin," for example.

The mozApps implementation does determine an install origin, and that origin will be a chrome origin in this case (chrome://webapprt, I think).  But I don't know of any problems that'll cause.  A search through the code suggests the mozApps DOM API tests also install from a chrome origin (chrome://mochitests), while the AITC tests install from content origins (http://localhost and http://example.com).

http://mxr.mozilla.org/mozilla-central/search?string=installorigin


> ::: webapprt/content/mochitest.xul
> @@ +13,5 @@
> > +  const Ci = Components.interfaces;
> > +  const Cu = Components.utils;
> > +
> > +  // mochitest.js needs this, so we must import it before we load that script.
> > +  Cu.import("resource://gre/modules/Services.jsm");
> 
> I don't understand why mochitest.js can't just import this.

It can, now that doing so no longer leaks when mochitest.js is loaded by head.js.  Done.

https://github.com/mykmelez/mozilla-central/commit/77be41638daa98a3ad7c08935b0c786ecc3f57ff


> ::: webapprt/test/chrome/browser_sample.js
> @@ +1,4 @@
> >  // This is a sample WebappRT chrome test.  It's just a browser-chrome mochitest.
> >  
> > +// Dependencies implicitly provided by scope into which script is loaded:
> > +//Cu.import("resource://webapprt/modules/WebappRT.jsm");
> 
> I don't understand why you can't just import it here.

This import exhibited the same problem of triggering leak detection, but it no longer does so, so I have uncommented it.

https://github.com/mykmelez/mozilla-central/commit/31c465b0abb9fea547d40068940521e2aaaa79dd


All changes are fixes for review nits, so carrying forward review.  This is the version I'll commit.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 651906 [details] [diff] [review]
patch v4: addresses review nits

https://hg.mozilla.org/integration/mozilla-inbound/rev/07b53bdc212a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07b53bdc212a