1428652 - Crash in mozilla::dom::workers::ServiceWorkerManager::RemoveScopeAndRegistration

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-23ba0bbb-89a9-4c51-a185-41d910180107.
=============================================================

Seen while looking at nightly crash stats: http://bit.ly/2CMM9bH. Crashes started using 20180106220101.

MOZ_RELEASE_ASSERT(false) (controlled client when removing registration)

ni on :bkelly since the code was touched in Bug 1425975

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::workers::ServiceWorkerManager::RemoveScopeAndRegistration dom/workers/ServiceWorkerManager.cpp:2351
1 xul.dll mozilla::dom::workers::ServiceWorkerUnregisterJob::Unregister dom/workers/ServiceWorkerUnregisterJob.cpp:143
2 xul.dll mozilla::dom::workers::ServiceWorkerUnregisterJob::PushUnsubscribeCallback::OnUnsubscribe dom/workers/ServiceWorkerUnregisterJob.cpp:33
3 xul.dll XPTC__InvokebyIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm:97
4  @0x61ffffffff 
5 xul.dll xptiInterfaceInfo::GetMethodInfo xpcom/reflect/xptinfo/xptiprivate.h:347
6 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1234
7 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:929
8 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:473
9 xul.dll Interpret js/src/vm/Interpreter.cpp:3096

=============================================================

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This crash is due to this code not checking the principal:

https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/dom/workers/ServiceWorkerManager.cpp#2349

When the first version of this code was written we did not have origin attributes and the scope completely captured the principal.  That is no longer the case, though.

I can reproduce this crash by:

1. Open https://fetch-event-echo.glitch.me/ in container tab A
2. Open https://fetch-event-echo.glitch.me/ in container tab B
3. In first tab, run this from the web console: navigator.serviceWorker.getRegistration().then(reg => reg.unregister());
4. Close the first tab
5. Observe the crash.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make service worker registration removal take container principals into account. r=asuth

This fixes it for me locally.  I need to figure out how to write a test.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: 2 Verify that unregistering a service worker in one container does not break a SW with same scope in different container. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=120115794206fad8f3efc0f5e8284f22ab023334

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8940865 [details] [diff] [review]
P1 Make service worker registration removal take container principals into account. r=asuth

Andrew, this patch fixes a bug where we compare registrations only using the scope.  We really need to use the principal as well due origin attributes.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941156 [details] [diff] [review]
2 Verify that unregistering a service worker in one container does not break a SW with same scope in different container. r=asuth

This test triggers the crash on m-c, but passes with P1 patch applied.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8941156 [details] [diff] [review]
2 Verify that unregistering a service worker in one container does not break a SW with same scope in different container. r=asuth

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

This is a very readable test!  Kudos!

Not in this patch, but maybe we should think about moving the helpers, especially doUnregister into a head.js.  (It seems browser chrome tests support head.js which is handy.)

::: dom/workers/test/serviceworkers/browser_unregister_with_containers.js
@@ +50,5 @@
> +}
> +
> +function getController(browser) {
> +  return ContentTask.spawn(browser, null, function() {
> +    return content.navigator.serviceWorker.controller;

per IRC, please normalize here so that the message manager doesn't need to fallback to JSON stringifying which ends up weird.  Which I guess means either !!blah or if you still want null to pass-through, blah ? true : null.

@@ +66,5 @@
> +}
> +
> +add_task(async function test() {
> +  await SpecialPowers.pushPrefEnv({"set": [
> +    ["dom.ipc.processCount", 1],

A comment here about the rationale for the single process would be good again.

@@ +83,5 @@
> +
> +  await doRegister(containerBrowser1);
> +  await doRegister(containerBrowser2);
> +
> +  checkUncontrolled(containerBrowser1);

you need to await these async functions here and below as well or have a very good comment about why the next awaits are always guaranteed to bound these awaits.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Verify that unregistering a service worker in one container does not break a SW with same scope in different container. r=asuth

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d553d8df009
P1 Make service worker registration removal take container principals into account. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c7e33c6c64
P2 Verify that unregistering a service worker in one container does not break a SW with same scope in different container. r=asuth

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48c2c76a7873
P3 Fix typo in browser_unregister_with_containers.js. r=me

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/0d553d8df009
https://hg.mozilla.org/mozilla-central/rev/56c7e33c6c64
https://hg.mozilla.org/mozilla-central/rev/48c2c76a7873