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.

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.

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.

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.

This was a mistake from when I implemented bug 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

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

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.

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.

https://hg.mozilla.org/releases/mozilla-beta/rev/d5c088330e6b https://hg.mozilla.org/releases/mozilla-beta/rev/39f739efe104