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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[[:philipp]]

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.

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 2

[Gerry Chang [:gchang]]

Track this as this crashes on 48/49/50.

Comment 3

[Andrew Overholt [:overholt]]

baku, any thoughts?

Comment 4

[Andrea Marchesini [:baku]]

This is super generic! And I don't see anything strange happening in other threads.
Khuey, any thoughts?

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Not seeing a lot of crashes for this particular signature so with current info we have, marking fix-optional.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This could happen if we never call WorkerPrivate::SetThread(nullptr).  From looking at WorkerThreadPrimaryRunnable::Run it looks like that can happen ...

Comment 7

[Andrea Marchesini [:baku]]

Attachment: foo.patch

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2026686ebec1
Force WorkerPrivate->SetThread(nullptr) using a RAII, r=khuey

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2026686ebec1

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Andre, can we have an uplift request to beta(49)? Thanks

Comment 12

[Andrea Marchesini [:baku]]

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.

Comment 13

[Gerry Chang [:gchang]]

Hi Andre,
The fix is already in 50 aurora. The patch should be uplift to 49 beta.

Comment 14

[Andrea Marchesini [:baku]]

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.

Comment 15

[Gerry Chang [:gchang]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7846d91a3d6b