1609665 - Crash in [@ mozilla::dom::ServiceWorkerRegistrationInfo::MaybeScheduleTimeCheckAndUpdate]

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug is for crash report bp-3f7cd4e1-1124-45ef-a3d4-6dddc0200116.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ServiceWorkerRegistrationInfo::MaybeScheduleTimeCheckAndUpdate dom/serviceworkers/ServiceWorkerRegistrationInfo.cpp:460
1 xul.dll std::_Func_impl_no_alloc<`lambda at z:/task_1579054948/build/src/dom/serviceworkers/ServiceWorkerPrivateImpl.cpp:460:7', void, mozilla::dom::ServiceWorkerOpResult&&>::_Do_call 
2 xul.dll mozilla::MozPromise<mozilla::dom::ServiceWorkerOpResult, mozilla::ipc::ResponseRejectReason, 1>::ThenValue<`lambda at z:/task_1579054948/build/src/dom/serviceworkers/ServiceWorkerPrivateImpl.cpp:840:7'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:793
3 xul.dll mozilla::MozPromise<RefPtr<mozilla::AudioData>, mozilla::MediaResult, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:402
4 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1241
5 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
6 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290
9 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137

another serviceworker-related crash signature that's popping up during the 73.0b cycle.

Comment 1

[Jens Stutte [:jstutte]]

This seems to be caused by a ServiceWorkerRegistrationInfo nullptr. I see several calls to MaybeScheduleTimeCheckAndUpdate:

	dom/serviceworkers/FetchEventOpChild.cpp
165	mRegistration->MaybeScheduleTimeCheckAndUpdate(); // found in mozilla::dom::(anonymous namespace)::SynthesizeResponseWatcher::CancelInterception
470	mRegistration->MaybeScheduleTimeCheckAndUpdate(); // found in mozilla::dom::FetchEventOpChild::Recv__delete__
678	mRegistration->MaybeScheduleTimeCheckAndUpdate(); // found in mozilla::dom::FetchEventOpChild::MaybeScheduleRegistrationUpdate
	dom/serviceworkers/ServiceWorkerPrivate.cpp
386	mRegistration->MaybeScheduleTimeCheckAndUpdate(); // found in mozilla::dom::(anonymous namespace)::RegistrationUpdateRunnable::Run
1573	registration->MaybeScheduleTimeCheckAndUpdate(); // found in mozilla::dom::ServiceWorkerPrivate::SendFetchEvent
	dom/serviceworkers/ServiceWorkerPrivateImpl.cpp
463	registration->MaybeScheduleTimeCheckAndUpdate(); // found in mozilla::dom::ServiceWorkerPrivateImpl::SendPushEventInternal
466	registration->MaybeScheduleTimeCheckAndUpdate(); // found in mozilla::dom::ServiceWorkerPrivateImpl::SendPushEventInternal

in this case we are inside ServiceWorkerPrivateImpl. None of these callers checks before, if mRegistration is nullptr. But I see several points where it is assigned mRegistration = nullptr;, so probably we need to understand, if we can do anything more meaningful than crash if mRegistration is nullptr (we could crash/assert only in debug builds?).

Comment 2

[Perry Jiang [:perry] [no longer employee, use ni?]]

Based on stack frame 2 (or 1, depending on the report) in the crash report

mozilla::MozPromise<mozilla::dom::ServiceWorkerOpResult, mozilla::ipc::ResponseRejectReason, 1>::ThenValue<`lambda at z:/task_1579058349/build/src/dom/serviceworkers/ServiceWorkerPrivateImpl.cpp:840:7'>::DoResolveOrRejectInternal(mozilla::MozPromise<mozilla::dom::ServiceWorkerOpResult, mozilla::ipc::ResponseRejectReason, 1>::ResolveOrRejectValue&)

the null ServiceWorkerRegistrationInfo is used in ExecServiceWorkerOp's callback(s) (ServiceWorkerPrivateImpl.cpp:840:7 is ExecServiceWorkerOp). Out of all calls to ExecServiceWorkerOp, only SendPushEventInternal does MaybeScheduleTimeCheckAndUpdate in the callbacks.

Comment 3

[Perry Jiang [:perry] [no longer employee, use ni?]]

Going to add some assertions to narrow down the crash, so marking leave-open.

Comment 4

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1609665 - promote diagnostic assert to release assert r?asuth

Attempting to narrow down the crash of a null ServiceWorkerRegistrationInfo.

Comment 5

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1609665 - refactor: use std::lower_bound to find ordered scope insertion index r?#dom-workers-and-storage,asuth

Many things may go wrong if mOrderedScopes isn't actually ordered. We can put
more of that responsibility on the STL.

Depends on D60326

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e65f67152a00
promote diagnostic assert to release assert r=asuth

Comment 7

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 9121667 [details]
Bug 1609665 - promote diagnostic assert to release assert r?asuth

Beta/Release Uplift Approval Request

• User impact if declined: We believe there will be no impact because this MOZ_RELEASE_ASSERT should catch the nullptr dereference before it actually happens and crash earlier instead. If so, then the patch won't change the amount of crashes. If not, then there will be a new assertion failure to fix.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky because it is only changing a MOZ_DIAGNOSTIC_ASSERT to MOZ_RELEASE_ASSERT. This bug's crash is only showing up on beta, where MOZ_DIAGNOSTIC_ASSERTs are removed.
• String changes made/needed:

Comment 8

[Cristina Coroiu [:ccoroiu]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121667 [details]
Bug 1609665 - promote diagnostic assert to release assert r?asuth

Shifts the crash from a nullptr crash to a release assert so we can hopefully get better diagnostics. Approved for 73.0b7.

Comment 10

[Razvan Maries]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121667 [details]
Bug 1609665 - promote diagnostic assert to release assert r?asuth

Clearing the approval flag to get this off the needs-uplift radar.

Comment 12

[Razvan Maries]

Backed out for build bustages on ServiceWorkerManager.cpp.

Push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&selectedJob=285503789&resultStatus=testfailed%2Cbusted%2Cexception&revision=b9df24255be1c53df4a89c86d6311befdffc73b9

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285503789&repo=mozilla-beta&lineNumber=44201

Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/fe6f34b2859f0706c88ec07199e30395041af258

There was a conflict on ServiceWorkerManager.cpp when patch was imported for uplift.

Comment 13

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1609665 - promote diagnostic assert to release assert r?asuth

Comment 14

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 9122518 [details]
Bug 1609665 - promote diagnostic assert to release assert r?asuth

Beta/Release Uplift Approval Request

• User impact if declined: See https://bugzilla.mozilla.org/show_bug.cgi?id=1609665#c7. Patch modified to land cleanly on mozilla-beta.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See https://bugzilla.mozilla.org/show_bug.cgi?id=1609665#c7
• String changes made/needed:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9122518 [details]
Bug 1609665 - promote diagnostic assert to release assert r?asuth

Same as comment 9. Approved for 73.0b9.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9122518 [details]
Bug 1609665 - promote diagnostic assert to release assert r?asuth

Pushed to Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/eca62a891f1d

Also clearing the approval flag to get this off the needs-uplift radar.

Comment 17

[Julien Cristau [:jcristau] (back April 22)]

sw-e10s was disabled in 73.0b9

Comment 18

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1609665 - use ScopeContainer instead of plain nsTArray for SW scopes r?#dom-workers-and-storage

Comment 19

[Perry Jiang [:perry] [no longer employee, use ni?]]

I'm working on this, but the crash is coming from a callback for push notifications and it's difficult to tell what the state is when the callback is registered. It's possible that the Service Worker's "ordered scopes" is buggy (which could explain the crash), so I'll upload a patch for that as a first step.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/353a831e9eab
use ScopeContainer instead of plain nsTArray for SW scopes r=dom-workers-and-storage-reviewers,sg,asuth

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/353a831e9eab

Comment 22

[[:philipp]]

crashes with the assert are now showing up with the [@ mozilla::dom::ServiceWorkerManager::SendPushEvent ] signature

Comment 23

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1609665 - push notifications should use registrations with an exact scope match r?#dom-workers-and-storage-reviewers

Also adds ScopeToPrincipal function to convert scope nsIURI/strings to principals.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d889781b71a
push notifications should use registrations with an exact scope match r=dom-workers-and-storage-reviewers,asuth

Comment 25

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/2d889781b71a

Comment 26

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 9128265 [details]
Bug 1609665 - push notifications should use registrations with an exact scope match r?#dom-workers-and-storage-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: nullptr dereference when handling push notifications, which may crash.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky because it just does an early-return when it sees a particular nullptr, instead of saving it and trying to de-reference later.
• String changes made/needed:

Comment 27

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9128265 [details]
Bug 1609665 - push notifications should use registrations with an exact scope match r?#dom-workers-and-storage-reviewers

Service-worker crash fix, one of the dependencies to allow shipping sw-e10s ship in 74, uplift approved for 74.0b8, thanks.

Comment 28

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/708d1df5809f

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Perry, this seems to have spiked big again on Nightly. Any ideas why?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Pushlog range for the spike:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7766d0b4a12&tochange=a7a6063bd5a4b49911b5a956c252a16961aec5a0

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Bug 1407276 looks like the only plausible change in that range.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

With any luck, the spike should go away in the next set of Nightlies with bug 1407276 backed out.

Comment 33

[Eden Chuang[:edenchuang]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)

Bug 1407276 looks like the only plausible change in that range.

Yes, it is caused by bug 1407276's patch. The core dump stack must be only produced by the patch.