Closed Bug 1376089 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::workers::WorkerPrivateParent<T>::MaybeWrapAsWorkerRunnable

Categories

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

55 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: philipp, Assigned: baku)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-03d86fcd-1ef8-427a-a57c-c589c0170620.
=============================================================
Crashing Thread (94), Name: DOM Worker
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::workers::WorkerPrivateParent<mozilla::dom::workers::WorkerPrivate>::MaybeWrapAsWorkerRunnable(already_AddRefed<nsIRunnable>) 	dom/workers/WorkerPrivate.cpp:3002
1 	xul.dll 	mozilla::dom::workers::WorkerThread::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) 	dom/workers/WorkerThread.cpp:265
2 	xul.dll 	NS_DispatchToCurrentThread(already_AddRefed<nsIRunnable>&&) 	xpcom/threads/nsThreadUtils.cpp:213
3 	xul.dll 	NS_DispatchToCurrentThread(nsIRunnable*) 	xpcom/threads/nsThreadUtils.cpp:230
4 	xul.dll 	mozilla::CycleCollectedJSRuntime::FinalizeDeferredThings(mozilla::CycleCollectedJSContext::DeferredFinalizeType) 	xpcom/base/CycleCollectedJSRuntime.cpp:1401
5 	xul.dll 	mozilla::CycleCollectedJSRuntime::OnGC(JSContext*, JSGCStatus) 	xpcom/base/CycleCollectedJSRuntime.cpp:1453
6 	xul.dll 	mozilla::CycleCollectedJSRuntime::GCCallback(JSContext*, JSGCStatus, void*) 	xpcom/base/CycleCollectedJSRuntime.cpp:822
7 	xul.dll 	js::gc::GCRuntime::callGCCallback(JSGCStatus) 	js/src/jsgc.cpp:1465
8 	xul.dll 	`anonymous namespace'::AutoNotifyGCActivity::~AutoNotifyGCActivity 	js/src/jsgc.cpp:1493
9 	xul.dll 	js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) 	js/src/jsgc.cpp:6633
10 	xul.dll 	js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) 	js/src/jsgc.cpp:6765

reports with this signature started increasingly showing up in firefox 55 - first in 55.0a1 build 20170610030207.
i forgot to add: all with MOZ_CRASH(All runnables destined for a worker thread must be cancelable!)
baku, can you take a look here, please?
Flags: needinfo?(amarchesini)
Attached patch gc.patchSplinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8888745 - Flags: review?(continuation)
Attachment #8888745 - Flags: review?(continuation) → review+
Why aren't we hitting this assert in automation?
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96941e476121
IncrementalFinalizeRunnable must be a CancelableStream in order to be dispatch to workers, r=mccr8
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01ec9f33545a
Backed out changeset 96941e476121 for bustage.
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a2235600af
IncrementalFinalizeRunnable must be a CancelableStream in order to be dispatch to workers, r=mccr8
https://hg.mozilla.org/mozilla-central/rev/d6a2235600af
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Please request Beta approval on this when you get a chance.
Flags: needinfo?(amarchesini)
Comment on attachment 8888745 [details] [diff] [review]
gc.patch

Approval Request Comment
[Feature/Bug causing the regression]: CC in workers
[User impact if declined]: incremental GC are not executed in workers. In debug build, FF crashes because of an assertion.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: This patch makes the runnable a CancelableRunnable. It is just a different base class.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8888745 - Flags: approval-mozilla-beta?
Would you be able to answer the question in comment 3?
Flags: needinfo?(amarchesini)
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Why aren't we hitting this assert in automation?

It seems that we don't have enough tests. Could this be a good answer? How can write a test where I force the execution of this runnable?
Flags: needinfo?(amarchesini) → needinfo?(continuation)
Comment on attachment 8888745 [details] [diff] [review]
gc.patch

make IncrementalFinalizeRunnable cancelable, beta55+
Attachment #8888745 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Andrea Marchesini [:baku] from comment #13)
> It seems that we don't have enough tests. Could this be a good answer? How
> can write a test where I force the execution of this runnable?

Hmm, I'm not really sure. I wonder if this is fallout from bug 1364528. That made us use incremental finalization when there's an exception pending, in CycleCollectedJSRuntime::OnGC. Maybe prior to that we wouldn't ever use it on workers? I'm not sure if there's any easy way to test that condition.
Flags: needinfo?(continuation)
(In reply to Andrea Marchesini [:baku] from comment #11)
> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no 

Setting qe-verify- based on Andrea's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.