Closed Bug 1279498 Opened 4 years ago Closed 4 years ago

Crash in mozilla::dom::workers::WorkerPrivate::OnProcessNextEvent | mozilla::dom::workers::WorkerThread::Observer::OnProcessNextEvent | nsThread::ProcessNextEvent | NS_ProcessNextEvent | mozilla::ipc::MessagePumpForNonMainThreads::Run

Categories

(Core :: DOM: Workers, defect)

47 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 + wontfix
firefox49 + fixed
firefox50 + 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-d15f9240-f71b-4bd0-a077-6deee2160610.
=============================================================
Crashing Thread (64)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::workers::WorkerPrivate::OnProcessNextEvent() 	dom/workers/WorkerPrivate.cpp:4553
1 	xul.dll 	mozilla::dom::workers::WorkerThread::Observer::OnProcessNextEvent(nsIThreadInternal*, bool) 	dom/workers/WorkerThread.cpp:328
2 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:968
3 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:297
4 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:326
5 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:227
6 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:201
7 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:396
8 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:397
9 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:95
10 	msvcr120.dll 	_callthreadstartex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:376
11 	msvcr120.dll 	msvcr120.dll@0x2c000 	
12 	kernel32.dll 	BaseThreadInitThunk 	
13 	ntdll.dll 	__RtlUserThreadStart 	
14 	ntdll.dll 	_RtlUserThreadStart

these signatures are showing up in crash data starting with 47 builds.
I looked at the volume of the first signature and it is at ~400 instances on 47 release in 2 weeks. The volume is not so high to make this a top crasher. Nom'ing for tracking in 48. This will be a wontfix for 47.
Track this as this crashes on 48/49/50.
baku, any thoughts?
Component: General → DOM: Workers
Flags: needinfo?(amarchesini)
This is super generic! And I don't see anything strange happening in other threads.
Khuey, any thoughts?
Flags: needinfo?(amarchesini) → needinfo?(khuey)
Not seeing a lot of crashes for this particular signature so with current info we have, marking fix-optional.
This could happen if we never call WorkerPrivate::SetThread(nullptr).  From looking at WorkerThreadPrimaryRunnable::Run it looks like that can happen ...
Flags: needinfo?(khuey)
Attached patch foo.patchSplinter Review
Assignee: nobody → amarchesini
Attachment #8771905 - Flags: review?(khuey)
Comment on attachment 8771905 [details] [diff] [review]
foo.patch

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

::: dom/workers/RuntimeService.cpp
@@ +2526,5 @@
>    }
>  
> +  class MOZ_STACK_CLASS SetThreadHelper final
> +  {
> +    // Raw pointer: this class is in stack.

is on the stack
Attachment #8771905 - Flags: review?(khuey) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2026686ebec1
Force WorkerPrivate->SetThread(nullptr) using a RAII, r=khuey
https://hg.mozilla.org/mozilla-central/rev/2026686ebec1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Andre, can we have an uplift request to beta(49)? Thanks
Flags: needinfo?(amarchesini)
Comment on attachment 8771905 [details] [diff] [review]
foo.patch

Approval Request Comment
[Feature/regressing bug #]: Workers. Not a particular bug generated this regression.
[User impact if declined]: a crash can happen.
[Describe test coverage new/current, TreeHerder]: none because racy.
[Risks and why]: none. This patch uses a RAII class to set the correct thread always.
[String/UUID change made/needed]: none.
Flags: needinfo?(amarchesini)
Attachment #8771905 - Flags: approval-mozilla-aurora?
Hi Andre,
The fix is already in 50 aurora. The patch should be uplift to 49 beta.
Flags: needinfo?(amarchesini)
Comment on attachment 8771905 [details] [diff] [review]
foo.patch

Approval Request Comment
[Feature/regressing bug #]: Workers. Not a particular bug generated this regression.
[User impact if declined]: a crash can happen.
[Describe test coverage new/current, TreeHerder]: none because racy.
[Risks and why]: none. This patch uses a RAII class to set the correct thread always.
[String/UUID change made/needed]: none.
Flags: needinfo?(amarchesini)
Attachment #8771905 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8771905 [details] [diff] [review]
foo.patch

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

This patch fixes a crash. Take it in 49 beta. Should be in 49 beta 5.
Attachment #8771905 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.