1539535 - Crash in [@ mozilla::dom::RemoteWorkerChild::CloseWorkerOnMainThread]

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug is for crash report bp-04f73f3a-30be-4ba8-8312-3b3400190327.

Top 10 frames of crashing thread:

0 xul.dll void mozilla::dom::RemoteWorkerChild::CloseWorkerOnMainThread dom/workers/remoteworkers/RemoteWorkerChild.cpp:459
1 xul.dll nsresult mozilla::detail::RunnableFunction<`lambda at z:/task_1553215353/build/src/dom/workers/remoteworkers/RemoteWorkerChild.cpp:525:9'>::Run xpcom/threads/nsThreadUtils.h:562
2 xul.dll nsresult mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:295
3 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1179
4 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:482
5 xul.dll void mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:88
6 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
7 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290
8 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137
9 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:411

crash reports with this signature and MOZ_RELEASE_ASSERT(lock->mWorkerPrivate) added in bug 1530223 are newly appearing in firefox 67.

Comment 1

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

These crashes are new in 67 and are caused by the fix for the crasher bug 1530223 but the volume looks equivalent. Andrew can we get this investigated please? Thanks.

Comment 2

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

This is caused by a race between RemoteWorkerChild::CloseWorkerOnMainThread and RemoteWorkerChild::ShutdownOnWorker. This should be fixed by one of the patches from bug 1231213, but until all that stuff has landed it may be worth just returning from the function if (!lock->mWorkerPrivate) instead of MOZ_RELEASE_ASSERTing.

Comment 3

[Andrew Overholt [:overholt]]

During our team meeting tomorrow we will discuss who can take Perry's suggestion from comment 2.

Comment 4

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

Attachment: Bug 1539535 - Add a (temporary) null check in RemoteWorkerChild::CloseWorkerOnMainThread r?asuth

Comment 5

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

Tracking for 67 as the crashes spiked in beta 8 (138 crashes which is more crashes than betas 1 to 7 combined)

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See How Do You Triage for more information

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18af3ac7686b
Add a (temporary) null check in RemoteWorkerChild::CloseWorkerOnMainThread r=asuth

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/18af3ac7686b

Comment 9

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

Perry, could you request an uplift to beta so as that we get it in the next beta? I don't think that there is enough volume on Nightly to evaluate if the fix works while we have this volume of crashes on beta. Thanks

Comment 10

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

Comment on attachment 9057059 [details]
Bug 1539535 - Add a (temporary) null check in RemoteWorkerChild::CloseWorkerOnMainThread r?asuth

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1530223
• User impact if declined: Browser may crash non-deterministically.
• 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): The patch changes an assertion that asserts a non-null pointer to a check that handles a null pointer accordingly.
• String changes made/needed:

Comment 11

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

Comment on attachment 9057059 [details]
Bug 1539535 - Add a (temporary) null check in RemoteWorkerChild::CloseWorkerOnMainThread r?asuth

Fix for a crash spiking in 67 beta but with no volume on 68, uplift approved for 67 beta 11 as we need the beta volume to understand if the patch has an impact on crashes.

Comment 12

[Andreea Pavel [:apavel]]

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