(no longer active) Assignee
	Description • 9 years ago

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

	(no longer active) Assignee
	Comment 6 • 9 years ago

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!

	Ben Kelly [:bkelly, not reviewing]
	Comment 7 • 9 years ago

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(...)

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 9 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 9 • 9 years ago

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.

	(no longer active) Assignee
	Comment 10 • 9 years ago

(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.

	(no longer active) Assignee
	Comment 11 • 9 years ago

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.

	(no longer active) Assignee
	Comment 12 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 13 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 14 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 15 • 9 years ago

(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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 16 • 9 years ago

(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:

	(no longer active) Assignee
	Comment 17 • 9 years ago

(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>.

	(no longer active) Assignee
	Comment 18 • 9 years ago

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

	(no longer active) Assignee
	Comment 19 • 9 years ago

(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.

	(no longer active) Assignee
	Comment 21 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cbc09877fe9

	Ben Kelly [:bkelly, not reviewing]
	Comment 22 • 9 years ago

(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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 23 • 9 years ago

See question in comment 22.

	(no longer active) Assignee
	Comment 24 • 9 years ago

(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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 25 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 26 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 27 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 28 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 29 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 30 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 31 • 9 years ago

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

	(no longer active) Assignee
	Comment 32 • 9 years ago

(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. :-)

	(no longer active) Assignee
	Comment 33 • 9 years ago

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

	(no longer active) Assignee
	Comment 34 • 9 years ago

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.

	(no longer active) Assignee
	Comment 35 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 36 • 9 years ago

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