1428650 - Crash in mozilla::dom::workers::ServiceWorkerManager::StartControllingClient

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-2552b8ba-e686-4a17-ab77-d24680180107.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20180106100308: http://bit.ly/2CCFttm.

MOZ_RELEASE_ASSERT(aRegistrationInfo->GetActive())

Bug 1425975 is in the range. ni on :bkelly

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::workers::ServiceWorkerManager::StartControllingClient dom/workers/ServiceWorkerManager.cpp:320
1 xul.dll mozilla::dom::workers::ServiceWorkerManager::MaybeClaimClient dom/workers/ServiceWorkerManager.cpp:3269
2 xul.dll mozilla::dom::workers::ServiceWorkerManager::MaybeClaimClient dom/workers/ServiceWorkerManager.cpp:3293
3 xul.dll mozilla::dom::ClientSource::Claim dom/clients/manager/ClientSource.cpp:635
4 xul.dll mozilla::dom::ClientSourceOpChild::DoSourceOp<RefPtr<mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0> >  dom/clients/manager/ClientSourceOpChild.cpp:45
5 xul.dll mozilla::dom::ClientSourceChild::RecvPClientSourceOpConstructor dom/clients/manager/ClientSourceChild.cpp:49
6 xul.dll mozilla::dom::PClientSourceChild::OnMessageReceived ipc/ipdl/PClientSourceChild.cpp:241
7 xul.dll mozilla::ipc::PBackgroundChild::OnMessageReceived ipc/ipdl/PBackgroundChild.cpp:1812
8 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2110
9 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2040

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

At first glance this might be a problem with claiming clients across processes before the active service worker has propagated.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make ServiceWorkerManager::MaybeClaimClient() handle inconsistent child-process SWM state. r=asuth

I don't have steps to reproduce, but I think this should fix it.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I looked closer and there is definitely a propagation problem:

1. SW is registered in process A.
2. Process A propagates the registration and installing worker to process B.
3. Process A begins activating SW and dispatches activate event.
4. Activate event handler calls clients.claim().
5. Client in process B is claimed.
6. Process A propagates final activated state to process B.

The problem is that at step (5) process B does not know the worker is active.

One possible fix would be to just try to propagate to active state around step (3), but that's not quite right either.  While the worker is activating we need to queue up and delay events like FetchEvent.  So we would actually have to build a separate propagating for activating, wait for activation, and then activate.

That is possible to do, but I don't think its worth the effort.  We are actively trying to remove all of this propagation code.  We should just accept that some clients may not be claimed until we finish that work.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make ServiceWorkerManager::MaybeClaimClient() handle inconsistent child-process SWM state. r=asuth

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941058 [details] [diff] [review]
Make ServiceWorkerManager::MaybeClaimClient() handle inconsistent child-process SWM state. r=asuth

Andrew, this makes us not assert if we try to claim a client without an active worker yet.  Comment 3 describes how this can happen with multi-e10s and our propagation code today.

Comment 6

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

Comment on attachment 8941058 [details] [diff] [review]
Make ServiceWorkerManager::MaybeClaimClient() handle inconsistent child-process SWM state. r=asuth

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

Restating: This changes the logic in the MaybeClaimClient(...const ServiceWorkerDescriptor&) path to fast-fail by resolving with false in the event the registration doesn't have an active SW.  This avoids going further down the claim call stack where StartControllingClient() would assert.  Content-wise, Clients.claim() returns a Promise<void> so doesn't really care about the result as long as it resolves.  (Clients::Claim explicitly ignores the ClientOpResult and does MaybeResolveWithUndefined.)  Some assertions are also upgraded to diagnostic asserts because Ben loves diagnostic asserts.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82d983031ec7
Make ServiceWorkerManager::MaybeClaimClient() handle inconsistent child-process SWM state. r=asuth

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/82d983031ec7