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

RESOLVED FIXED in Firefox 67

Status

()

defect
P1
critical
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: philipp, Assigned: perry)

Tracking

(Regression, {crash, regression})

67 Branch
mozilla68
All
Windows
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67+ fixed, firefox68 fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

3 months ago

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.

Assignee

Updated

2 months ago
Priority: -- → P3

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.

No longer blocks: 1530223
Regressed by: 1530223
Flags: needinfo?(overholt)
Assignee

Comment 2

2 months ago

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.

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

Flags: needinfo?(overholt)
Assignee

Updated

2 months ago
Assignee: nobody → perry

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

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

Priority: P3 → P1
Assignee

Updated

2 months ago
Keywords: checkin-needed

Comment 7

2 months ago

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18af3ac7686b
Add a (temporary) null check in RemoteWorkerChild::CloseWorkerOnMainThread r=asuth

Keywords: checkin-needed

Comment 8

2 months ago
bugherder
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Flags: needinfo?(perry)
Assignee

Comment 10

2 months ago

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:
Flags: needinfo?(perry)
Attachment #9057059 - Flags: approval-mozilla-beta?

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.

Attachment #9057059 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.