Closed
Bug 1290116
Opened 8 years ago
Closed 8 years ago
service worker unregister() should wait for active worker to go idle before clearing the registration
Categories
(Core :: DOM: Service Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: rubys, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(7 files)
2.23 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
1.83 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
2.43 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
11.89 KB,
patch
|
lina
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160606113944
Steps to reproduce:
Attempted to use Basic Authentication with Service Workers. I started with the offline-fallback demo in serviceworke.rs and added a .htaccess and htpasswd file. Complete instructions here:
https://github.com/rubys/serviceworker-cookbook/tree/authenticated-offline-fallack/offline-fallback#reproduction-instructions-apache-httpd
Diff:
https://github.com/mozilla/serviceworker-cookbook/commit/554a5b86a378bc4783668388f3e3551eac112c3a
Actual results:
Got a Corrupted Content Error:
https://raw.githubusercontent.com/rubys/serviceworker-cookbook/authenticated-offline-fallack/offline-fallback/CorruptedContentError-firefox.png
Expected results:
Should have seen "Yay, you are online!" page.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bkelly)
Assignee | ||
Comment 1•8 years ago
|
||
I can reproduce. I didn't use apache, though. I used this SimpleHTTPServer python script:
https://gist.githubusercontent.com/fxsjy/5465353/raw/c5e5b66f5b5b35247560d19fd2ea5251f88e34b7/SimpleAuthServer.py
I don't think this is actually related to the basic auth. The failure only occurs when pressing the "View Complete Demo Again" button. This does an unregister() and reload() without waiting like this:
navigator.serviceWorker.getRegistration().then(function(registration) {
registration.unregister();
window.location.reload();
});
If I change this code to look like this:
navigator.serviceWorker.getRegistration().then(function(registration) {
if (registration) {
return registration.unregister();
}
}).then(_ => {
window.location.reload();
});
Then I can no longer reproduce. It seems we have some unsafe race when reloading before the unregister() promise completes.
Assignee: nobody → bkelly
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bkelly)
Assignee | ||
Comment 2•8 years ago
|
||
With the original code I see one of four results:
1) "Service worker change, registered the service worker" shown on the page and the web console shows "[oninstall] Cached offline page 'http://localhost:8080/offline.html'".
2) "An active service worker controller was found, no need to register" shown on the page and no logs in the web console.
3) Content corrupted page with this in the web console "Failed to load ‘http://localhost:8080/?1470016121244’. A ServiceWorker intercepted the request and encountered an unexpected error."
4) Only once I saw the same as (1), but also an error about the service worker script fetch failing.
With the code modifications in comment 1 I consistently get only result (1).
Assignee | ||
Comment 3•8 years ago
|
||
Sam, please be aware if you require basic auth on the offline.html page itself (or any pre-cached pages fetch'd in the install event handler) they will be cached as failed authorizations. We don't have a window to associate the authorization with in the install event, so I think you will always get a failed auth for those fetches.
I think this is unrelated to the content corrupted page, though.
Flags: needinfo?(rubys)
It is not clear what information you need from me.
I will say that my current code base loads the cache entirely from the "client" process (i.e., the browser), and I can verify that the cache contains successful results, and I do NO fetches from the service worker (not precisely true: the code will try a fetch if the object is not in the cache, but I have verified that that path is not being executed). Even so, I still get either content corrupted or failed authorization errors (again, despite verifying that the cache only contains successful responses).
In the case of failed authorization errors, a shift-F5 reload from the server, and in the process throw up a modal dialog prompting for user and password, and after that point the service worker generally behaves, but exiting the browser completely and starting again can cause me to get authentication errors again.
I'll be glad to share my code, but I thought it best to start the conversation with the posted service worker examples.
Assignee | ||
Comment 5•8 years ago
|
||
Sorry. I guess it was more just an fYI about a problem with the example code. If that does not reflect your real situation, then that is good. The behavior I was seeing in the example was a failed auth page when the server was offline.
Flags: needinfo?(rubys)
Assignee | ||
Comment 6•8 years ago
|
||
So, we are hitting this error on the serviceworke.rs site as well:
Failed to load ‘https://serviceworke.rs/offline-fallback/’. A ServiceWorker intercepted the request and encountered an unexpected error.service-worker.js:23:4
But its not showing the error because about:neterror is deemed non-secure and this is an iframe in a secure document:
Blocked loading mixed active content “about:neterror?e=corruptedContentErrorv2&u=https%3A//serviceworke.rs/offline-fallback/&c=UTF-8&f=regular&d=The%20site%20at%20https%3A//serviceworke.rs/offline-fallback/%20has%20experienced%20a%20network%20protocol%20violation%20that%20cannot%20be%20repaired.”[Learn More]
In localhost http testing I don't think we consider this to be mixed content.
Assignee | ||
Comment 7•8 years ago
|
||
Filed bug 1291448 about blocking about:neterror as mixed content.
See Also: → 1291448
Assignee | ||
Comment 8•8 years ago
|
||
For some reason the RespondWithHandler is being GC'd before the promise is fulfilled. Currently we cancel the intercepted request in this case:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#341
We could just reset the channel here, but I'd like to figure out why we are GC'ing the RespondWithHandler first.
Assignee | ||
Comment 9•8 years ago
|
||
I believe the problem here is that we unconditionally terminate workers and remove the registration when the last controlling document goes away:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerUnregisterJob.cpp#142
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2005
We really need to wait to do this until the active worker is idle. This is very similar to bug 1170543.
I'll make these entry points check the active worker idle state. I'll also add this to the spec issue.
Blocks: ServiceWorkers-compat
Depends on: 1170543
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
With these patches I can no longer trigger the failure locally. I will write a wpt test that exposes the problem next.
Assignee | ||
Comment 13•8 years ago
|
||
This fails without the P1, P2, and P3 patches. It succeeds with the patches.
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Summary: Attempting to use Basic Auth with ServiceWorkers results in Corrupted Content Error → service worker unregister() should wait for active worker to go idle before clearing the registration
Assignee | ||
Comment 15•8 years ago
|
||
We should release this with bug 1170543 if we can.
status-firefox48:
--- → wontfix
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox-esr45:
--- → disabled
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8777375 [details] [diff] [review]
P1 Expose a ServiceWorkerRegistrationInfo::IsIdle() method. r=asuth
Step one is to expose the "registration is idle" state in a convenient way. later patches will then use this method.
Attachment #8777375 -
Flags: review?(bugmail)
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8777377 [details] [diff] [review]
P2 Clean up ServiceWorkerManager::StopControllingADocument(). r=asuth
This patch simply cleans up the logic in ServiceWorkerManager::StopControllingADocument() to use our traditional "short circuit return" style. It has no functional change.
Attachment #8777377 -
Flags: review?(bugmail)
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8777378 [details] [diff] [review]
P3 Delay removing registration until active worker is idle. r=asuth
Do the actual work of delaying registration teardown until after the registration's active worker has gone idle.
Attachment #8777378 -
Flags: review?(bugmail)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8777396 [details] [diff] [review]
P4 Verify that unregister waits for active worker to go idle. r=asuth
Add a test case verifying we wait to teardown the registration until the active worker is idle.
I put this in activation.https.html because its nearly identical to the other tests there. Its just a bit of a misnomer because its not about activation but instead about unregister. It doesn't seem worth the hassle of renaming the test or creating a new test file with duplicate logic.
Attachment #8777396 -
Flags: review?(bugmail)
Updated•8 years ago
|
Attachment #8777375 -
Flags: review?(bugmail) → review+
Updated•8 years ago
|
Attachment #8777377 -
Flags: review?(bugmail) → review+
Updated•8 years ago
|
Attachment #8777378 -
Flags: review?(bugmail) → review+
Comment 20•8 years ago
|
||
Comment on attachment 8777396 [details] [diff] [review]
P4 Verify that unregister waits for active worker to go idle. r=asuth
Review of attachment 8777396 [details] [diff] [review]:
-----------------------------------------------------------------
Restating the test for my benefit:
- Step one retracts the controlled iframe document that "active" was controlling. "active" is now being kept alive by the (post)MessageEvent's waitUntil(). Prior to the code in P3 being introduced that checks for the worker being idle, this would have either resulted in the registration being removed in one of:
- ServiceWorkerUnregisterJob::Unregister if the document was already gone, or
- ServiceWorkerManager::StopControllingADocument if the document removal raced the unregister. For this specific test, it won't be this one because nsDocument::SetScriptGlobalObject should get synchronously called by the remove() and call through to this path prior to the unregister call in the test.
- Step two is gated on the unregister job completing (in step 5, after flagging uninstalling in 4, with step 6 skipping with us assuming that your proposed change of "using the service worker" at https://github.com/slightlyoff/ServiceWorker/issues/916#issuecomment-237272060 will go through). We wait for the the "waiting" SW to be marked redundant, which will occur when ServiceWorkerRegistrationInfo::Clear() is invoked by SWM::RemoveRegistration(). That happens when we postMessage the "active" SW which resolves its MessageEvent waitUntil() which results in SWM::WorkerIsIdle being invoked.
- Step three is just checking that the Clear() totally happened.
Attachment #8777396 -
Flags: review?(bugmail) → review+
Comment 21•8 years ago
|
||
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db014dc92795
P1 Expose a ServiceWorkerRegistrationInfo::IsIdle() method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c50283f07370
P2 Clean up ServiceWorkerManager::StopControllingADocument(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a336a1fa97
P3 Delay removing registration until active worker is idle. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e95eb4821b8
P4 Verify that unregister waits for active worker to go idle. r=asuth
Reporter | ||
Comment 22•8 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #1)
> I can reproduce.
> I don't think this is actually related to the basic auth.
I do think that what you are fixing is an important bug, but I'm increasingly convinced that the bug I wanted to report on is different than the one being fixed.
Once there is a stable version (preferably in a nightly build, but if necessary I can build it myself - it has been a while, but I used to be able to build firefox on my machine), I'll follow up (either in this bug or a new bug - I care not) with a reproduction scenario, this time using Python's SimpleHTTPServer.
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Sam Ruby from comment #22)
> (In reply to Ben Kelly [:bkelly] from comment #1)
> > I can reproduce.
> > I don't think this is actually related to the basic auth.
>
> I do think that what you are fixing is an important bug, but I'm
> increasingly convinced that the bug I wanted to report on is different than
> the one being fixed.
>
> Once there is a stable version (preferably in a nightly build, but if
> necessary I can build it myself - it has been a while, but I used to be able
> to build firefox on my machine), I'll follow up (either in this bug or a new
> bug - I care not) with a reproduction scenario, this time using Python's
> SimpleHTTPServer.
Please do! I was not aware you saw another error beyond the corrupted content after pressing "View Complete Demo Again".
If you run into more problems, please file them as a new bug and need-info me. Thanks!
Assignee | ||
Comment 24•8 years ago
|
||
By the way, I think you could test now with the builds from my tinderbox run:
Windows:
http://archive.mozilla.org/pub/firefox/try-builds/bkelly@mozilla.com-807f79bb95954a90361b418dcf1cd4131aba2377/try-win32/firefox-50.0a1.en-US.win32.zip
Mac:
http://archive.mozilla.org/pub/firefox/try-builds/bkelly@mozilla.com-807f79bb95954a90361b418dcf1cd4131aba2377/try-macosx64/firefox-50.0a1.en-US.mac.dmg
Linux:
http://archive.mozilla.org/pub/firefox/try-builds/bkelly@mozilla.com-807f79bb95954a90361b418dcf1cd4131aba2377/try-linux64/firefox-50.0a1.en-US.linux-x86_64.tar.bz2
Can you reproduce the problem with one of these builds?
Flags: needinfo?(rubys)
Reporter | ||
Comment 25•8 years ago
|
||
Will do once this lands in a build.
Meanwhile, the scenario I wanted to report on is one where the cache is loaded correctly, and the service worker correctly returns that data via event.respondWith many times. All is well and good until the browser is completely closed and then the user restarts the browser and attempts to visit the page. The result is a 401 response. There is no opportunity to provide a user and password, just an unconditional authentication error. In fact, no request is sent to the server in this scenario.
Because the cache is available to the browser (client) process, I can verify that the data in the cache is still correct.
Reporter | ||
Comment 26•8 years ago
|
||
I'll try one of those builds either late tonight or tomorrow (I'm East Coast US).
Assignee | ||
Comment 27•8 years ago
|
||
(In reply to Sam Ruby from comment #25)
> Will do once this lands in a build.
>
> Meanwhile, the scenario I wanted to report on is one where the cache is
> loaded correctly, and the service worker correctly returns that data via
> event.respondWith many times. All is well and good until the browser is
> completely closed and then the user restarts the browser and attempts to
> visit the page. The result is a 401 response. There is no opportunity to
> provide a user and password, just an unconditional authentication error. In
> fact, no request is sent to the server in this scenario.
Yes, I can reproduce this.
> Because the cache is available to the browser (client) process, I can verify
> that the data in the cache is still correct.
At least for the offline-fallback demo its not pulling any data from the Cache. The service worker just proxies the connection through.
I'll file a new bug. Thanks.
Flags: needinfo?(rubys)
Assignee | ||
Comment 28•8 years ago
|
||
See bug 1291893.
I had to back this out for w-e10s failures like https://treeherder.mozilla.org/logviewer.html#?job_id=33262919&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0eb2464ca17
Assignee | ||
Comment 30•8 years ago
|
||
This does not reproduce locally, but my best guess is we are delaying the unregister because we happen to catch the worker in the activating state while its processing the no-op activate event.
Assignee | ||
Comment 31•8 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #30)
> This does not reproduce locally, but my best guess is we are delaying the
> unregister because we happen to catch the worker in the activating state
> while its processing the no-op activate event.
Actually, that shouldn't matter. Because the uninstalling flag should be set.
Except... the spec does not say to ignore registrations with the uninstalling flag set. I think that is probably a spec bug. I filed:
https://github.com/slightlyoff/ServiceWorker/issues/943
Let's see if that fixes the issue here.
Assignee | ||
Comment 32•8 years ago
|
||
Assignee | ||
Comment 33•8 years ago
|
||
Since the other test case is racy, lets explicitly test getRegistrations() with the uninstalling flag set.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bca2a724cae8
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8777975 [details] [diff] [review]
P5 Make navigator.serviceWorker.getRegistrations() ignore uninstalling registrations. r=asuth
The try build indicates this fixes the problem with the previous landing.
I also got preliminary spec approval from Jake on IRC.
Basically we getRegistrations() should be doing the same thing as getRegistration(). A registration that has a pending uninstall should not be returned.
Attachment #8777975 -
Flags: review?(bugmail)
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8778002 [details] [diff] [review]
P6 Add a wpt test case verifying getRegistrations() does not return registrations with a pending uninstall. r=asuth
The additional test case verifies that getRegistrations() hides registrations with pending uninstalls. It does this by holding a controlled frame alive and calling unregister(). In this situation the registration gets the pending uninstall flag, but does not actually go away.
This test fails without the P5 patch. It succeeds with the P5 patch.
Attachment #8778002 -
Flags: review?(bugmail)
Comment 36•8 years ago
|
||
Comment on attachment 8777975 [details] [diff] [review]
P5 Make navigator.serviceWorker.getRegistrations() ignore uninstalling registrations. r=asuth
Review of attachment 8777975 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/workers/ServiceWorkerManager.cpp
@@ +696,5 @@
> }
>
> for (uint32_t i = 0; i < data->mOrderedScopes.Length(); ++i) {
> + RefPtr<ServiceWorkerRegistrationInfo> info =
> + data->mInfos.GetWeak(data->mOrderedScopes[i]);
Is there an idiom for using GetWeak() when we know the result will already exist?
Or an idiom change to favor this style since the pointer gets directly returned (and AddRefed by the refptr) rather than requiring a separate declaration line and manually typing getter_AddRefs for the obscured outval?
(I ask because I don't see a lot of GetWeak in dom/workers/ and "Weak" always raises my suspicions.)
Attachment #8777975 -
Flags: review?(bugmail) → review+
Updated•8 years ago
|
Attachment #8778002 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 37•8 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #36)
> > + RefPtr<ServiceWorkerRegistrationInfo> info =
> > + data->mInfos.GetWeak(data->mOrderedScopes[i]);
>
> Is there an idiom for using GetWeak() when we know the result will already
> exist?
Yea, I just wanted to assign to the RefPtr in a single statement.
Comment 38•8 years ago
|
||
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90006dffbe5b
P1 Expose a ServiceWorkerRegistrationInfo::IsIdle() method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/74ceee4dc2f4
P2 Clean up ServiceWorkerManager::StopControllingADocument(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca998a8cbe48
P3 Delay removing registration until active worker is idle. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/44b50089c079
P4 Verify that unregister waits for active worker to go idle. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9922c014310
P5 Make navigator.serviceWorker.getRegistrations() ignore uninstalling registrations. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa715b19902b
P6 Add a wpt test case verifying getRegistrations() does not return registrations with a pending uninstall. r=asuth
Comment 39•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/35b2715748a3 for 70% odds of WinXP debug timing out in test_subscription_change.html, https://treeherder.mozilla.org/logviewer.html#?job_id=33457497&repo=mozilla-inbound
Assignee | ||
Comment 40•8 years ago
|
||
Given the number of backouts here I don't intend to uplift any more. This can just ride the trains. (Once I figure out this latest failure.)
Assignee | ||
Comment 41•8 years ago
|
||
I think the push tests are generally a bit racy in that very few of them wait for the service worker to become active. Changing the timing could cause these races to finish in different ways.
Let's see if this fixes the problem:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c8bf9706520
Assignee | ||
Comment 42•8 years ago
|
||
Comment on attachment 8778958 [details] [diff] [review]
P7 Fixed activation races in push service worker tests. r=kitcambridge
Review of attachment 8778958 [details] [diff] [review]:
-----------------------------------------------------------------
Kit, this patch makes the dom/push/test mochitests consistently wait for service workers to be active. We were only waiting in a few cases here before. I think to avoid races we should generally wait for active workers all the time. It did not seem like any of these tests were purposefully trying to test push against installing or waiting workers.
Even if this does not fix the failures in the last backout its still probably a good thing to land.
Attachment #8778958 -
Flags: review?(kcambridge)
Comment 43•8 years ago
|
||
Comment on attachment 8778958 [details] [diff] [review]
P7 Fixed activation races in push service worker tests. r=kitcambridge
Review of attachment 8778958 [details] [diff] [review]:
-----------------------------------------------------------------
Ouch. Thanks for fixing these, Ben!
::: dom/push/test/test_utils.js
@@ +235,5 @@
> + resolve(swr);
> + return;
> + }
> + sw.addEventListener('statechange', function onStateChange(evt) {
> + if (sw.state === 'activated') {
Just so I understand: this is different than `navigator.serviceWorker.ready` because that resolves if the worker is "activating" or "activated", right?
Attachment #8778958 -
Flags: review?(kcambridge) → review+
Assignee | ||
Comment 44•8 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #43)
> Just so I understand: this is different than `navigator.serviceWorker.ready`
> because that resolves if the worker is "activating" or "activated", right?
No, I think .ready resolves on activated. Its more to avoid accidentally using a previously active service worker. If you already have an activated service worker .ready resolves immediately. So after a .register() you usually want to wait for .installing to really test your new worker.
Assignee | ||
Comment 45•8 years ago
|
||
Also, .ready doesn't let you wait for a different scope activation which some of these tests needed.
Assignee | ||
Comment 46•8 years ago
|
||
Ugh... After 24 hours I finally got two winxp test results. One of them is a failure.
Assignee | ||
Comment 47•8 years ago
|
||
Although it failed in a different way. Now its timing out right at the start. The previous test exercises our "long running SW, kill it with fire" code paths. I suspect we are leaving a token count non zero there which make the SW seem like its non-idle forever. This then prevents the previous worker from every being completely evicted.
Assignee | ||
Comment 48•8 years ago
|
||
In the winxp failures I get these additional error messages in test_serviceworker_lifetime.html:
14:16:48 INFO - [336] WARNING: '!JS::AddPromiseReactions(cx, promiseObj, resolveFunc, rejectFunc)', file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/promise/Promise.cpp, line 824
14:16:48 INFO - [336] WARNING: A runnable was posted to a worker that is already shutting down!: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/workers/WorkerPrivate.cpp, line 2443
Assignee | ||
Comment 49•8 years ago
|
||
Ugh, I think this is a race in worker holder code. I'm going to file a separate bug.
Assignee | ||
Comment 50•8 years ago
|
||
I am fairly confident bug 1293690 is the issue here. I added some NS_ASSERTION(false) calls to log stacks and managed to trigger the same timing that the winxp automation is hitting. My patch for the other bug seems to fix it. Will push a new try build in a bit.
Assignee | ||
Comment 51•8 years ago
|
||
Assignee | ||
Comment 52•8 years ago
|
||
The try build has all green winxp M(3) results with bug 1293690 in the mix. Just have to wait for baku to come back from PTO next week to review it.
Assignee | ||
Comment 53•8 years ago
|
||
And indeed, I see it hitting the expected error path:
20:02:57 INFO - [644] WARNING: '!JS::AddPromiseReactions(cx, promiseObj, resolveFunc, rejectFunc)', file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/promise/Promise.cpp, line 824
20:02:57 INFO - [644] WARNING: '!ref->UseWorkerHolder()', file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/workers/ServiceWorkerPrivate.cpp, line 342
20:02:57 INFO - [644] WARNING: '!handler', file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/workers/ServiceWorkerPrivate.cpp, line 379
20:02:57 INFO - [644] WARNING: A runnable was posted to a worker that is already shutting down!: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/workers/WorkerPrivate.cpp, line 2443
The patches in bug 1293690 make the UseWorkerHolder() call fail here if the worker is already terminated.
Comment 54•8 years ago
|
||
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a4e8c92faae
P1 Expose a ServiceWorkerRegistrationInfo::IsIdle() method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7da0dbfc72
P2 Clean up ServiceWorkerManager::StopControllingADocument(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5ad5e9ed20
P3 Delay removing registration until active worker is idle. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e9a9ba04ce5
P4 Verify that unregister waits for active worker to go idle. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0b9c25b6125
P5 Make navigator.serviceWorker.getRegistrations() ignore uninstalling registrations. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/a81b6ce5c25f
P6 Add a wpt test case verifying getRegistrations() does not return registrations with a pending uninstall. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc14d107fca
P7 Fixed activation races in push service worker tests. r=kitcambridge
Comment 55•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/252ff19cd746 for the build bustage from bug 1293690.
Comment 56•8 years ago
|
||
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0c8cd41219c
P1 Expose a ServiceWorkerRegistrationInfo::IsIdle() method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/465601c1b933
P2 Clean up ServiceWorkerManager::StopControllingADocument(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb891c00bc15
P3 Delay removing registration until active worker is idle. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/508299ab7a22
P4 Verify that unregister waits for active worker to go idle. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf520760c0df
P5 Make navigator.serviceWorker.getRegistrations() ignore uninstalling registrations. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8207a64e1cf2
P6 Add a wpt test case verifying getRegistrations() does not return registrations with a pending uninstall. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b6a4e0898f
P7 Fixed activation races in push service worker tests. r=kitcambridge
Comment 57•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c0c8cd41219c
https://hg.mozilla.org/mozilla-central/rev/465601c1b933
https://hg.mozilla.org/mozilla-central/rev/eb891c00bc15
https://hg.mozilla.org/mozilla-central/rev/508299ab7a22
https://hg.mozilla.org/mozilla-central/rev/cf520760c0df
https://hg.mozilla.org/mozilla-central/rev/8207a64e1cf2
https://hg.mozilla.org/mozilla-central/rev/c3b6a4e0898f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•