1461181 - Crash in mozilla::dom::ClientHandle::Control

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-f33d376f-c841-485d-9110-608710180513.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ClientHandle::Control dom/clients/manager/ClientHandle.cpp:123
1 xul.dll mozilla::dom::ServiceWorkerManager::StartControllingClient dom/serviceworkers/ServiceWorkerManager.cpp:344
2 xul.dll mozilla::dom::ServiceWorkerManager::StartControlling dom/serviceworkers/ServiceWorkerManager.cpp:1886
3 xul.dll nsDocShell::MaybeCreateInitialClientSource docshell/base/nsDocShell.cpp:2862
4 xul.dll nsDocShell::DoChannelLoad docshell/base/nsDocShell.cpp:11172
5 xul.dll nsDocShell::DoURILoad docshell/base/nsDocShell.cpp:10968
6 xul.dll nsDocShell::InternalLoad docshell/base/nsDocShell.cpp:10291
7 xul.dll nsDocShell::LoadURI docshell/base/nsDocShell.cpp:1011
8 xul.dll nsFrameLoader::ReallyStartLoadingInternal dom/base/nsFrameLoader.cpp:558
9 xul.dll nsIDocument::MaybeInitializeFinalizeFrameLoaders dom/base/nsDocument.cpp:6719

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

this crash signature is rising in volume on the beta channel with the start of the 61.0b rollout. perhaps that's related to bug 1425975.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Address suggests a nullptr de-ref somewhere.  I'll investigate on Monday.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Its not obvious to me how we could be getting a nullptr here unless TLS is lying to us.  I'm going to add some diagnostic assertions.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

AFAICT none of my new assertions in bug 1461326 have triggered.  That's good, I guess.

Looking closer at this code I think I see maybe what could be happening now.  The SWM method is effectively doing:


  MOZ_DIAGNOSTIC_ASSERT(aRegistrationInfo->GetActive());

  const ServiceWorkerDescriptor& active =
    aRegistrationInfo->GetActive()->Descriptor();

  RefPtr<ClientHandle> clientHandle =
    ClientManager::CreateHandle(aClientInfo,
                                SystemGroup::EventTargetFor(TaskCategory::Other));

  ref = Move(clientHandle->Control(active));

I think maybe GetActive() is returning nullptr.  The diagnostic assert does not catch it because we are on beta.  Perhaps accessing GetActive()->Descriptor() does not trigger a crash either because we're just storing a pointer reference.  Its not until we try to access the contents of that pointer reference later that we crash.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't call ServiceWorkerManager::StartControllingClient() if there is no active worker. r=asuth

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8979238 [details] [diff] [review]
Don't call ServiceWorkerManager::StartControllingClient() if there is no active worker. r=asuth

Andrew, see comment 3 for what I think is happening.  This patch tries to ensure that we don't call StartControllingClient() if the registration does not have an active worker.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ec7b9c344a
Don't call ServiceWorkerManager::StartControllingClient() if there is no active worker. r=asuth

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Ryan, if comment 6 fixes this issue, do you think we should revert the release asserts I have added back to diagnostic asserts to avoid shipping them unnecessarily on release?  In theory it should be fine, but it is more release asserts than we normally use.

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d5ec7b9c344a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

What are the trade-offs to keeping them? Perf? I assume we'd just crash somewhere else, so stability wouldn't be affected much one way or the other?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> What are the trade-offs to keeping them? Perf? I assume we'd just crash
> somewhere else, so stability wouldn't be affected much one way or the other?

Some very minor perf hit.  Its possible they might trigger on general memory corruption issues which might be more noisy in general release population.  Not sure catching those are bad, though.

I guess I'll leave them for now.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8979238 [details] [diff] [review]
Don't call ServiceWorkerManager::StartControllingClient() if there is no active worker. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1425975
[User impact if declined]: Low frequency crashes.  I believe this probably only triggers when a service worker does clients.claim() on activation and takes control of a page in a different process.  Its basically our cross-process service worker propagation problem.
[Is this code covered by automated tests?]: We have a lot of tests, but it does not trigger the race condition.
[Has the fix been verified in Nightly?]: Its landed in nightly, but we don't have STR to verify.
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal
[Why is the change risky/not risky?]: This adds some additional checks for nullptr and a release assertion that we don't ever pass a nullptr here.
[String changes made/needed]: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8979238 [details] [diff] [review]
Don't call ServiceWorkerManager::StartControllingClient() if there is no active worker. r=asuth

Simple fix for a ServiceWorker crash. Approved for 61.0b8.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b6f094b0dcf2