Closed Bug 1462077 Opened 6 years ago Closed 6 years ago

Crash in mozilla::dom::ClientSource::SetController

Categories

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

x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- disabled
firefox60 --- wontfix
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

It appears we are hitting our safety release assert in FF60 that prevents a cross-origin service worker controller.  Fortunately the assertion prevents this from being a security issue, but its a bad problem and we need to figure out how this is happening.

This bug was filed from the Socorro interface and is
report bp-09d62ec8-7bf4-4e50-b863-aa3bd0180516.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ClientSource::SetController dom/clients/manager/ClientSource.cpp:383
1 xul.dll mozilla::dom::ClientSource::Control dom/clients/manager/ClientSource.cpp:433
2 xul.dll mozilla::dom::ClientSourceOpChild::DoSourceOp<RefPtr<mozilla::MozPromise<mozilla::dom::ClientOpResult, nsresult, 0> >  dom/clients/manager/ClientSourceOpChild.cpp:45
3 xul.dll mozilla::dom::ClientSourceOpChild::Init dom/clients/manager/ClientSourceOpChild.cpp:79
4 xul.dll mozilla::dom::ClientSourceChild::RecvPClientSourceOpConstructor dom/clients/manager/ClientSourceChild.cpp:49
5 xul.dll mozilla::dom::PClientSourceChild::OnMessageReceived ipc/ipdl/PClientSourceChild.cpp:241
6 xul.dll mozilla::ipc::PBackgroundChild::OnMessageReceived ipc/ipdl/PBackgroundChild.cpp:1968
7 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2133
8 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2063
9 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1909

=============================================================
I think I might see the problem.  We need to update the clientInfo stack variable when we do this:

https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/dom/serviceworkers/ServiceWorkerManager.cpp#2207-2221

That code is creating a new reserved client, but it goes ahead and tries to mark the original client controlled via the SWM.  Its probably a race as to whether the necko-based control takes effect before the SWM-based control.
Priority: -- → P1
Ah, there must be an initial about:blank that gets destroyed when the load completes with the new reserved ClientSource.  If the SWM control operation completes before that happens then the crash will trigger.  I think, anyway.
Andrew, this patch fixes a bug in DispatchFetchEvent() when we try to do an STS upgrade in e10s mode.  The code was already trying to force create a new ClientSource object, but it neglected to update clientInfo stack variable.  So the ServiceWorkerManager::StartControllingClient() method was being called on the wrong client.
Attachment #8976286 - Flags: review?(bugmail)
This makes us apply the same release assertion currently in ClientSource::SetController() also in ClientHandle::Control().  This would have caught the problem in our existing automation tests.  It also might be more useful for some crash reports since we can see what is calling the async ClientHandle::Control() method.
Attachment #8976287 - Flags: review?(bugmail)
Comment on attachment 8976286 [details] [diff] [review]
P1 Make ServiceWorkerManager::DispatchFetchEvent() use the new client if we he create one for an STS upgrade. r=asuth

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

Thanks for the excellent characterization of the problem.
Attachment #8976286 - Flags: review?(bugmail) → review+
Attachment #8976287 - Flags: review?(bugmail) → review+
This was a mistake from when I implemented bug 1440705.
Blocks: 1440705
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd63f2afbb7
P1 Make ServiceWorkerManager::DispatchFetchEvent() use the new client if we create one for an STS upgrade. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5fb8f83dbf0
P2 Add a release assert in ClientHandle::Control() that enforces same-origin policy. r=asuth
https://hg.mozilla.org/mozilla-central/rev/2bd63f2afbb7
https://hg.mozilla.org/mozilla-central/rev/e5fb8f83dbf0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment on attachment 8976286 [details] [diff] [review]
P1 Make ServiceWorkerManager::DispatchFetchEvent() use the new client if we he create one for an STS upgrade. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1440705
[User impact if declined]: A race condition during STS upgrade can trigger crashes if a race condition triggers.
[Is this code covered by automated tests?]: Yes.  The P2 patch here adds an additional assertion that allows our current tests to catch this problem.
[Has the fix been verified in Nightly?]: Its landed in nightly, but we never saw the race condition trigger in nightly.
[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 patch adds a one line fix to populate a value that was clearly missed before.  The assertion is checking for the same condition we already assert on in other places.
[String changes made/needed]: None
Attachment #8976286 - Flags: approval-mozilla-beta?
Comment on attachment 8976287 [details] [diff] [review]
P2 Add a release assert in ClientHandle::Control() that enforces same-origin policy. r=asuth

See comment 10.
Attachment #8976287 - Flags: approval-mozilla-beta?
Comment on attachment 8976286 [details] [diff] [review]
P1 Make ServiceWorkerManager::DispatchFetchEvent() use the new client if we he create one for an STS upgrade. r=asuth

Simple fix for a Service Workers crash, and a bonus release assert to help catch it in the future. Approved for 61.0b6.
Attachment #8976286 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8976287 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.