Crash in mozilla::dom::ServiceWorkerInfo::AddServiceWorker

RESOLVED FIXED in Firefox 60

Status

()

defect
P2
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: calixte, Assigned: bkelly)

Tracking

(Blocks 1 bug, {crash, regression})

Trunk
mozilla60
Unspecified
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 unaffected, firefox60 fixed)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

a year ago
This bug was filed from the Socorro interface and is
report bp-055d61b2-72dd-4081-a769-bfc1e0180217.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ServiceWorkerInfo::AddServiceWorker dom/serviceworkers/ServiceWorkerInfo.cpp:235
1 xul.dll mozilla::dom::ServiceWorker::ServiceWorker dom/serviceworkers/ServiceWorker.cpp:82
2 xul.dll mozilla::dom::ServiceWorker::Create dom/serviceworkers/ServiceWorker.cpp:64
3 xul.dll nsGlobalWindowInner::GetOrCreateServiceWorker dom/base/nsGlobalWindowInner.cpp:6395
4 xul.dll mozilla::dom::ClientSource::PostMessage dom/clients/manager/ClientSource.cpp:575
5 xul.dll mozilla::dom::ClientSourceOpChild::DoSourceOp<RefPtr<mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0> >  dom/clients/manager/ClientSourceOpChild.cpp:45
6 xul.dll mozilla::dom::ClientSourceOpChild::Init dom/clients/manager/ClientSourceOpChild.cpp:89
7 xul.dll mozilla::dom::ClientSourceChild::RecvPClientSourceOpConstructor dom/clients/manager/ClientSourceChild.cpp:49
8 xul.dll mozilla::dom::PClientSourceChild::OnMessageReceived ipc/ipdl/PClientSourceChild.cpp:241
9 xul.dll mozilla::ipc::PBackgroundChild::OnMessageReceived ipc/ipdl/PBackgroundChild.cpp:1812

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

There are 12 crashes (from 7 installs) in nightly 60 starting with buildid 20180203220331.
The moz_crash_reason is always MOZ_RELEASE_ASSERT(workerURL.Equals(NS_ConvertUTF8toUTF16(mDescriptor.ScriptURL()))) and this assertion is has been added in patch [1] to fix bug 1434342.
:bkelly, could you investigate please ?

[1] https://hg.mozilla.org/mozilla-central/rev/1cf205e9e3b0
Flags: needinfo?(bkelly)
This is probably raciness from our lame SWM cross process sync mechanism.  I'll look at adding some extra checks on Monday.  Once our multi-e10s work is done we should be able to then remove those checks.
Priority: -- → P2
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
Comment on attachment 8952195 [details] [diff] [review]
P1 Make ServiceWorker::Create() get the ServiceWorkerInfo using a full descriptor match instead of just by ID. r=asuth

Andrew, this crash is happening because:

1. A service worker is calling Client.postMessage() to a client .
2. That client runs in a separate process where we have a different ServiceWorker identifier space.
3. The ServiceWorkerDescriptor.Id() value exists on the ServiceWorkerRegistration, but points to the wrong ServiceWorker.

This is something I originally worked around by simply leaving the MessageEvent.source empty if there was no matching ID, but that old code does not handle when there is a match with a worker with a different scriptURL.

This patch tightens things up by add MatchByDescriptor().  This compares Id, ScriptURL, Scope, and PrincipalInfo.

Ultimately this problem will go away when we have a single process managing the ServiceWorker Id values.  We're just not there yet, though.
Attachment #8952195 - Flags: review?(bugmail)
Comment on attachment 8952197 [details] [diff] [review]
P2 Remove ServiceWorkerRegistrationInfo::GetByID(). r=asuth

This patch removes the old ServiceWorkerRegistrationInfo::GetById() method.  We don't need its one call-site now since GetOrCreateServiceWorker() can return nullptr safely.
Attachment #8952197 - Flags: review?(bugmail)
Comment on attachment 8952199 [details] [diff] [review]
P3 Remove ServiceWorker::MatchesDescriptor() in favor of Descriptor().Matches(). r=asuth

Since we added ServiceWorkerDescriptor::Matches() in P1, lets remove the ServiceWorker::MatcheDescriptor() and instead just expose a Descriptor() getter.
Attachment #8952199 - Flags: review?(bugmail)
Attachment #8952195 - Flags: review?(bugmail) → review+
Attachment #8952197 - Flags: review?(bugmail) → review+
Attachment #8952199 - Flags: review?(bugmail) → review+

Comment 10

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e645473ee112
P1 Make ServiceWorker::Create() get the ServiceWorkerInfo using a full descriptor match instead of just by ID. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c0e2869bb10
P2 Remove ServiceWorkerRegistrationInfo::GetByID(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2ebde19d278
P3 Remove ServiceWorker::MatchesDescriptor() in favor of Descriptor().Matches(). r=asuth
You need to log in before you can comment on or make changes to this bug.