1439099 - Crash in mozilla::dom::ServiceWorkerInfo::AddServiceWorker

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorker::Create() get the ServiceWorkerInfo using a full descriptor match instead of just by ID. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove ServiceWorkerRegistrationInfo::GetByID(). r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove ServiceWorkerRegistrationInfo::GetByID(). r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove ServiceWorker::MatchesDescriptor() in favor of Descriptor().Matches(). r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/e645473ee112
https://hg.mozilla.org/mozilla-central/rev/3c0e2869bb10
https://hg.mozilla.org/mozilla-central/rev/a2ebde19d278