The API is the same and we need to make sure that it works the same in all three contexts.

Nikhil, I have a question about ServiceWorkerContainer.register() not working: With these patches applied, if you run both of the tests in dom/cache, the second call to navigator.serviceWorker.register("worker_wrapper.js") fails to start up the service worker. The code in serviceworker_driver.js properly unregister()'s the SW at the end of the test run, so by the time that the second test runs, we must have registered worker_wrapper.js once, unregistered and registered it again. But the second registration doesn't start the SW as can be verified by putting a dump() statement at the global scope of worker_wrapper.js. There's nothing printed to stdout and as far as I could follow the control flow under the debugger, everything seems to go on normally. If I modify the test like below to randomize the script URL passed to register(), things start to work. diff --git a/dom/cache/test/mochitest/serviceworker_driver.js b/dom/cache/test/mochitest/serviceworker_driver.js index 6d732d6..be3aa04 100644 --- a/dom/cache/test/mochitest/serviceworker_driver.js +++ b/dom/cache/test/mochitest/serviceworker_driver.js @@ -28,7 +28,7 @@ function serviceWorkerTestExec(testFile) { document.body.appendChild(iframe); } - navigator.serviceWorker.register("worker_wrapper.js", {scope: "."}) + navigator.serviceWorker.register("worker_wrapper.js" + "?" + (Math.random()), {scope: "."}) .then(function(registration) { if (registration.installing) { registration.installing.onstatechange = function(e) { Do you have any idea what could be going on here? Thanks!

Comment on attachment 8574244 [details] [diff] [review] Part 1: Create a mini-framework for running tests in the worker, service worker and window contexts Review of attachment 8574244 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Just a few idle comments and nits. Thanks! Is it worth sticking this somewhere it can be shared with other tests? I think this is something you asked me in the past as well, but I just don't know where to put this stuff. It would be nice if we had some common dir for mochitest utility libraries. ::: dom/cache/test/mochitest/driver.js @@ +7,5 @@ > +// 1. Regular Worker context > +// 2. Service Worker context > +// 3. Window context > +// The function returns a promise which will get resolved once all tests > +// finish. Specify that the caller is responsible for SimpleTest.finish(). @@ +14,5 @@ > + function setupPrefs() { > + return new Promise(function(resolve, reject) { > + SpecialPowers.pushPrefEnv({ > + "set": [["dom.caches.enabled", true], > + ["dom.fetch.enabled", true], I don't think this needed any more. Fetch is defaulted on as of this weekend. @@ +24,5 @@ > + }); > + }); > + } > + > + function importScript(script) { This is really close to |WorkerGlobalScope.importScripts()|. I guess it doesn't matter, though, since its not exported from this file. @@ +74,5 @@ > + return setupPrefs() > + .then(importDrivers) > + .then(runWorkerTest) > + .then(runServiceWorkerTest) > + .then(runFrameTest) An interesting alternative or stress test might be to make these three tests run in parallel. return setupPrefs() .then(importDrivers) .then(Promise.All([runWorkerTest, runServiceWorkerTest, runFrameTest])) .catch(...)

Comment on attachment 8574245 [details] [diff] [review] Part 2: Merge test_cache.js and test_cache_frame.html Review of attachment 8574245 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/test/mochitest/test_cache.js @@ +63,5 @@ > }); > }).then(function() { > + // FIXME(nsm): Can't use a Request object for now since the operations > + // consume it's 'body'. See > + // https://github.com/slightlyoff/ServiceWorker/issues/510. I believe this comment is long defunct. You can address this in later bugs, though, as the tests get fleshed out.

Ehsan, IIUC you are not removing the frame after it sends 'finish'. This means the document is still controlled, and the unregister() will only mark the registration to remove it in the future (when all documents controlled by it go away). If you call the next register before that, the registration, and the existing set of workers, will be re-used.

(In reply to Ben Kelly [:bkelly] from comment #7) > Is it worth sticking this somewhere it can be shared with other tests? I > think this is something you asked me in the past as well, but I just don't > know where to put this stuff. It would be nice if we had some common dir > for mochitest utility libraries. Yeah, maybe, but perhaps later. We don't really have a good place to share helper scripts globally but perhaps we can consider moving all of these tests to dom/workers/test/serviceworker... > @@ +14,5 @@ > > + function setupPrefs() { > > + return new Promise(function(resolve, reject) { > > + SpecialPowers.pushPrefEnv({ > > + "set": [["dom.caches.enabled", true], > > + ["dom.fetch.enabled", true], > > I don't think this needed any more. Fetch is defaulted on as of this > weekend. Yep, fixed locally. > @@ +74,5 @@ > > + return setupPrefs() > > + .then(importDrivers) > > + .then(runWorkerTest) > > + .then(runServiceWorkerTest) > > + .then(runFrameTest) > > An interesting alternative or stress test might be to make these three tests > run in parallel. > > return setupPrefs() > .then(importDrivers) > .then(Promise.All([runWorkerTest, > runServiceWorkerTest, > runFrameTest])) > .catch(...) The current driver script doesn't know how to differentiate between incoming message events from different sources so this will currently break the tests but we can consider fixing this in the future.

I'm going to land this stuff by randomizing the SW script URL for now, pending bug 1141256 to investigate the issue in comment 6.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c96b6474bdac https://hg.mozilla.org/integration/mozilla-inbound/rev/8827f51084f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/0438058b908a https://hg.mozilla.org/integration/mozilla-inbound/rev/2e62e8ce8c3e https://hg.mozilla.org/integration/mozilla-inbound/rev/f646f28b05d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d91550919d2

sorry had to back this out since i think this caused https://treeherder.mozilla.org/logviewer.html#?job_id=7400465&repo=mozilla-inbound

also seems this caused https://treeherder.mozilla.org/logviewer.html#?job_id=7393921&repo=mozilla-inbound

(In reply to Carsten Book [:Tomcat] from comment #13) > sorry had to back this out since i think this caused > https://treeherder.mozilla.org/logviewer.html#?job_id=7400465&repo=mozilla- > inbound This means that we couldn't remove a body file on windows: Assertion failure: ((bool)(!!(!NS_FAILED_impl(rv)))), at c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\src\dom\cache\FileUtils.cpp:247 It would be good to know what the error code is there.

(In reply to Carsten Book [:Tomcat] from comment #14) > also seems this caused > https://treeherder.mozilla.org/logviewer.html#?job_id=7393921&repo=mozilla- > inbound Note these errors further up in the log: 21:13:58 INFO - [Child 787] WARNING: 'NS_FAILED(rv)', file ../../../gecko/dom/workers/ServiceWorkerManager.cpp, line 718 21:13:59 INFO - [Child 787] WARNING: 'NS_FAILED(aStatus)', file ../../../gecko/dom/workers/ServiceWorkerManager.cpp, line 117 21:14:17 INFO - JavaScript error: , line 0: NS_ERROR_ILLEGAL_VALUE:

(In reply to Ben Kelly [:bkelly] from comment #15) > (In reply to Carsten Book [:Tomcat] from comment #13) > > sorry had to back this out since i think this caused > > https://treeherder.mozilla.org/logviewer.html#?job_id=7400465&repo=mozilla- > > inbound > > This means that we couldn't remove a body file on windows: > > Assertion failure: ((bool)(!!(!NS_FAILED_impl(rv)))), at > c:\builds\moz2_slave\m-in-w32-d- > 0000000000000000000\build\src\dom\cache\FileUtils.cpp:247 > > It would be good to know what the error code is there. The Windows error code is ERROR_SHARING_VIOLATION, which gets translated to NS_ERROR_FILE_IS_LOCKED <https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#498>.

This error can occur on Windows when removing a file that is open in another application.

(In reply to Ben Kelly [:bkelly] from comment #16) > (In reply to Carsten Book [:Tomcat] from comment #14) > > also seems this caused > > https://treeherder.mozilla.org/logviewer.html#?job_id=7393921&repo=mozilla- > > inbound > > Note these errors further up in the log: > > 21:13:58 INFO - [Child 787] WARNING: 'NS_FAILED(rv)', file > ../../../gecko/dom/workers/ServiceWorkerManager.cpp, line 718 > 21:13:59 INFO - [Child 787] WARNING: 'NS_FAILED(aStatus)', file > ../../../gecko/dom/workers/ServiceWorkerManager.cpp, line 117 > 21:14:17 INFO - JavaScript error: , line 0: NS_ERROR_ILLEGAL_VALUE: This happens because bug 1137683 is not fixed yet, so we cannot create a service worker on b2g. I will adjust the test to skip the SW part on b2g for now, and will add a comment to bug 1137683 to make sure that is backed out as part of the fix to that bug.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #18) > Created attachment 8575593 [details] [diff] [review] > Part 7: Ignore NS_ERROR_FILE_IS_LOCKED when removing a cache object > > This error can occur on Windows when removing a file that is open > in another application. How do we know its not gecko that's holding the file open here? Did you verify with the debugger or instrumentation that we closed all our references? What other process in a mochitest could hold it open? If it is gecko, then its a bug that needs to be fixed instead of ignoring the error code.

See question in comment 22.

(In reply to Ben Kelly [:bkelly] from comment #22) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #18) > > Created attachment 8575593 [details] [diff] [review] > > Part 7: Ignore NS_ERROR_FILE_IS_LOCKED when removing a cache object > > > > This error can occur on Windows when removing a file that is open > > in another application. > > How do we know its not gecko that's holding the file open here? Did you > verify with the debugger or instrumentation that we closed all our > references? > > What other process in a mochitest could hold it open? > > If it is gecko, then its a bug that needs to be fixed instead of ignoring > the error code. I can't reproduce this locally, so I don't know which process is holding the file open. I've seen at least one case on Windows where the indexing service keeps files that you modify and close open for a bit longer in order to index them, and attempting to remove them immediately would fail. But it's hard to tell whether that's what's happening here. Can you think of a way to debug this without reproducing? Note that I think no matter what is keeping the file open, we don't want to crash a debug build when it happens.

Can we make it an NS_ASSERTION instead of a MOZ_ASSERT? That way if it happens a lot we will still get a treeherder failure, but it won't kill the browser process.

Comment on attachment 8575617 [details] [diff] [review] Part 8: Disable the service worker part of these tests on b2g while bug 1137683 gets fixed Review of attachment 8575617 [details] [diff] [review]: ----------------------------------------------------------------- Can you print a warning that tests have been suppressed? Otherwise, LGTM.

I'm going to see if I can reproduce the file error.

Ehsan, can you do some try pushes with this patch instead of loosening the deletion assertion? I think there might be some races in how we report the stream is closed from the child process. This patch tries to close those races by always closing the underlying stream before reporting to the parent that the close has happened.

Missing a semi-colon in previous patch. This one actually compiles and passes tests locally.

Is there a Part 6 to this bug? :-) I see Part 7 in the bug list, but no Part 6.

I'm just going to do the try before the west coast wakes up and kills the try server.

(In reply to Ben Kelly [:bkelly] from comment #25) > Can we make it an NS_ASSERTION instead of a MOZ_ASSERT? That way if it > happens a lot we will still get a treeherder failure, but it won't kill the > browser process. Yes, a non-fatal assertion is better here, but that will at the very least turn into an intermittent failure. (In reply to Ben Kelly [:bkelly] from comment #29) > Missing a semi-colon in previous patch. This one actually compiles and > passes tests locally. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f7c5eb30203 (In reply to Ben Kelly [:bkelly] from comment #30) > Is there a Part 6 to this bug? :-) I see Part 7 in the bug list, but no > Part 6. Part 6 is comment 11. :-)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95716badb0c includes Ben's part 7 instead of mine.

Comment on attachment 8575994 [details] [diff] [review] P7 Close underlying file stream in ReadStream before reporting closed. Review of attachment 8575994 [details] [diff] [review]: ----------------------------------------------------------------- Looks like this was indeed a race! Thanks for the fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/98cfaf5f3907 https://hg.mozilla.org/integration/mozilla-inbound/rev/bfef86393692 https://hg.mozilla.org/integration/mozilla-inbound/rev/5b73358e99de https://hg.mozilla.org/integration/mozilla-inbound/rev/93b30945b980 https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a721743ffd https://hg.mozilla.org/integration/mozilla-inbound/rev/1b071963e442 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c612012a232 https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c20c565d00

https://hg.mozilla.org/mozilla-central/rev/98cfaf5f3907 https://hg.mozilla.org/mozilla-central/rev/bfef86393692 https://hg.mozilla.org/mozilla-central/rev/5b73358e99de https://hg.mozilla.org/mozilla-central/rev/93b30945b980 https://hg.mozilla.org/mozilla-central/rev/e7a721743ffd https://hg.mozilla.org/mozilla-central/rev/1b071963e442 https://hg.mozilla.org/mozilla-central/rev/2c612012a232 https://hg.mozilla.org/mozilla-central/rev/e6c20c565d00