1674115 - Crash in [@ mozilla::dom::WorkerPrivate::SetIsDebuggerReady]

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Crashes from 3+ installations, only with 84.0a1, oldest reported build ID is 20201027212052.

Crash report: https://crash-stats.mozilla.org/report/index/cbb7f803-85a1-4f6a-8f25-06d000201029

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::WorkerPrivate::SetIsDebuggerReady dom/workers/WorkerPrivate.cpp:2448
1 xul.dll XPTC__InvokebyIndex 
2 xul.dll trunc 
3 xul.dll trunc 
4 xul.dll static XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1142
5 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:925
6 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:598
7 xul.dll Interpret js/src/vm/Interpreter.cpp:3336
8 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:635
9 xul.dll js::Call js/src/vm/Interpreter.cpp:680

Comment 1

[Neha Kochar [:neha]]

This crash had Fission enabled.

Comment 2

[u608768]

Also seeing these without Fission enabled: https://crash-stats.mozilla.org/report/index/1109f9fe-9191-4126-bd6f-e96540201030.

Comment 3

[Jens Stutte [:jstutte]]

Attachment: WorkerPrivate nullptr exception

Comment 4

[Jens Stutte [:jstutte]]

I am not sure how to read all those IPC inline frames in order to understand where the nullptr comes from. Yaron, can you take a look?

Comment 5

[Yaron Tausky [:ytausky]]

The nullptr is mWorkerPrivate here. It is set to nullptr here. Looking at the callers of WorkerDebugger::SetDebuggerReady, this seems suspicious. onConnectToWorker is the Promise returned by connectToWorker, which returns a rejected Promise here if the debugger is already closed. This code was came from bug 1633712 which landed on 27 October, the day before the first crash report, so that also seems like an indication that it's related.

Comment 6

[Jens Stutte [:jstutte]]

Thanks Yaron!
Nicolas, can you check this?

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jens Stutte [:jstutte] from comment #6)

Thanks Yaron!
Nicolas, can you check this?

Sure

(In reply to Yaron Tausky [:ytausky] from comment #5)

The nullptr is mWorkerPrivate here. It is set to nullptr here. Looking at the callers of WorkerDebugger::SetDebuggerReady, this seems suspicious. onConnectToWorker is the Promise returned by connectToWorker, which returns a rejected Promise here if the debugger is already closed. This code was came from bug 1633712 which landed on 27 October, the day before the first crash report, so that also seems like an indication that it's related.

Thanks for investigating this Yaron. So this call was made to unpause the worker in case we couldn't connect to it; but yes, as you point it out, we only seems to reject if the worker is closed, so we shouldn't need that.
I'm going to remove that from the js code, but I also wonder if we should fix https://searchfox.org/mozilla-central/rev/c938c7416c633639a5c8ce4412be586eefb48005/dom/workers/WorkerDebugger.cpp#389 so we would check mWorkerPrivate before calling SetIsDebuggerReady ?

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1674115 - [devtools] Fix crash caused by calling WorkerDebugger.setDebuggerReady. r=ochameau.

We were trying to resume the worker debugger in case we
couldn't connect to the worker. But connectToWorker only
explicitly rejects when the worker is closed, which means
we were trying to resume a closed worker, which could cause
a crash.
This patch simply removes the line where we were resuming the
worker debugger.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd01a13c1a4f
[devtools] Fix crash caused by calling WorkerDebugger.setDebuggerReady. r=ochameau.

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/cd01a13c1a4f