1904059 - Crash in [@ JSContext::checkImpl] via DebuggerImmediateRunnable::WorkerRun

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/cc5a52ba-9a28-4f2a-b1ca-f0e040240620

MOZ_CRASH Reason: *** Compartment mismatch 1f723407c10 vs. 1f727919e50 at argument 1

Top 10 frames:

0  xul.dll  JSContext::checkImpl(JS::Handle<JSObject*> const&, JS::Handle<JS::Value> cons...  js/src/vm/JSContext-inl.h:207
0  xul.dll  JSContext::check(JS::Handle<JSObject*> const&, JS::Handle<JS::Value> const&, ...  js/src/vm/JSContext-inl.h:214
0  xul.dll  JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>...  js/src/vm/CallAndConstruct.cpp:47
1  xul.dll  mozilla::dom::(anonymous namespace)::DebuggerImmediateRunnable::WorkerRun(JSC...  dom/workers/WorkerPrivate.cpp:638
2  xul.dll  mozilla::dom::WorkerThreadRunnable::Run()  dom/workers/WorkerRunnable.cpp:439
3  xul.dll  mozilla::dom::WorkerPrivate::ProcessSingleDebuggerRunnable()  dom/workers/WorkerPrivate.cpp:4225
4  xul.dll  mozilla::dom::WorkerPrivate::InterruptCallback(JSContext*)  dom/workers/WorkerPrivate.cpp:3979
4  xul.dll  mozilla::dom::workerinternals::(anonymous namespace)::InterruptCallback(JSCon...  dom/workers/RuntimeService.cpp:473
5  xul.dll  HandleInterrupt(JSContext*, bool)  js/src/vm/Runtime.cpp:415
5  xul.dll  JSContext::handleInterrupt()  js/src/vm/Runtime.cpp:488

A similar-looking stack got fixed about a year ago in bug 1849265, but I'm still seeing a fair number of these (this is the highest volume compartment mismatch). Maybe you could take another look, Eden? Thanks.

Other crashes that look similar from the last week:
bp-e69d180f-30c7-4f17-8a58-631dd0240620
bp-fb4b6adc-0c39-4bc5-b592-087c80240619
bp-53051626-7780-4116-ae26-b0a670240616
bp-e631789f-3587-4866-9b32-95bd60240615

Comment 1

[Andrew McCreight [:mccr8]]

It kind of looks like a regression in 129. This isn't enabled on all branches, but looking back at the last 6 months, I only see a single occurrence of this stack (from January: bp-45a0897a-8cbd-43f8-9205-daccb0240104) until the 20240613093628, and then there have been around a half dozen of these, which is a decent chunk of all crashes with this signature in the last 6 months.

Comment 2

[Andrew McCreight [:mccr8]]

There's also a similar-looking stack with a different signature, [@ js::ContextChecks::fail ]. Maybe this is the Android version?
bp-fde09198-2544-449b-b51d-1515d0240619

Comment 3

[Andrew McCreight [:mccr8]]

This is still the most common compartment mismatch by far. I found a few variations. The signature has changed since I initially filed it because I did some signature work.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #1)

It kind of looks like a regression in 129. This isn't enabled on all branches, but looking back at the last 6 months, I only see a single occurrence of this stack (from January: bp-45a0897a-8cbd-43f8-9205-daccb0240104) until the 20240613093628, and then there have been around a half dozen of these, which is a decent chunk of all crashes with this signature in the last 6 months.

Note that the specific signatures here relate to the debugger interrupt mechanism introduced in bug 1821250 in Firefox 128. We do not expect this exact signature to have been possible before that point, but the logic resulting in the mismatch for the general signature could have happened if like a sync XHR was on the stack. Although some of the stacks are truncated, they generally show that we are interrupting top-level script evaluation which means we do have a nested event loop on the stack... which ties into:

As discussed at the team meeting today:

• Eden indicated he suspects mozilla::dom::WorkerPrivate::GetCurrentEventLoopGlobal is likely involved, which makes sense since it only returns non-null values when there is a syncloop. We check that in WorkerThreadRunnable::Run as part of changes in Fx127 in bug 1769913 to use it as the basis for the global we choose to use.
• :smaug indicated it's weird how DebuggerImmediateRunnable::WorkerRun manually invokes JS_CallFunctionValue (with the global) after pulling the function out of the dom::Function mHandler where the CallbackObject straight up tracks the mozilla::dom::CallbackObject::mCallbackGlobal and if we just had the CallbackObject/dom::Function do the call for us, we should be fine.
  • It makes sense that there probably was some weird evolutionary story that got us here, but our current calling is not a best practice.

Comment 6

[Andrew McCreight [:mccr8]]

Looks like there are some STR in bug 1917312.

Comment 7

[Eden Chuang[:edenchuang]]

Attachment: Bug 1904059 - Using DOM::Function::Call instead JS_CallFunctionValue in DebuggerImmediateRunnable::WorkerRun. r=#dom-worker-reviewers

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2da041f4b00
Using DOM::Function::Call instead JS_CallFunctionValue in DebuggerImmediateRunnable::WorkerRun. r=asuth

Comment 9

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

Backed out for build bustage in dom/workers/WorkerPrivate.cpp:
https://hg.mozilla.org/integration/autoland/rev/c8a25946ad59

Push with failures
Failure log

[task 2024-09-25T11:32:20.448Z] 11:32:20    ERROR -  /builds/worker/checkouts/gecko/dom/workers/WorkerPrivate.cpp:666:5: error: arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument).  'mHandler->' is neither.
[task 2024-09-25T11:32:20.449Z] 11:32:20     INFO -    666 |     mHandler->Call({}, &rval, rv);
[task 2024-09-25T11:32:20.449Z] 11:32:20     INFO -        |     ^~~~~~~~~~

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8a25946ad59
Backed out changeset c2da041f4b00 for causing build bustages @ WorkerPrivate.cpp CLOSED TREE

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Ah, yeah the log below does accurately reflect that we were previously sidestepping the MOZ_CAN_RUN_SCRIPT annotations, but now we do in fact need to integrate a bit more with that.

[task 2024-09-25T11:32:20.448Z] 11:32:20    ERROR -  /builds/worker/checkouts/gecko/dom/workers/WorkerPrivate.cpp:666:5: error: arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument).  'mHandler->' is neither.
[task 2024-09-25T11:32:20.449Z] 11:32:20     INFO -    666 |     mHandler->Call({}, &rval, rv);
[task 2024-09-25T11:32:20.449Z] 11:32:20     INFO -        |     ^~~~~~~~~~
[task 2024-09-25T11:32:20.449Z] 11:32:20    ERROR -  /builds/worker/checkouts/gecko/dom/workers/WorkerPrivate.cpp:666:5: error: functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT
[task 2024-09-25T11:32:20.450Z] 11:32:20     INFO -    666 |     mHandler->Call({}, &rval, rv);
[task 2024-09-25T11:32:20.450Z] 11:32:20     INFO -        |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2024-09-25T11:32:20.450Z] 11:32:20     INFO -  /builds/worker/checkouts/gecko/dom/workers/WorkerPrivate.cpp:662:16: note: caller function declared here
[task 2024-09-25T11:32:20.451Z] 11:32:20     INFO -    662 |   virtual bool WorkerRun(JSContext* aCx,
[task 2024-09-25T11:32:20.451Z] 11:32:20     INFO -        |                ^

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e177ae5f5ac8
Using DOM::Function::Call instead JS_CallFunctionValue in DebuggerImmediateRunnable::WorkerRun. r=asuth

Comment 13

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

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR128 approval when you get a chance. It grafts cleanly.

Comment 15

[Eden Chuang[:edenchuang]]

Comment on attachment 9425248 [details]
Bug 1904059 - Using DOM::Function::Call instead JS_CallFunctionValue in DebuggerImmediateRunnable::WorkerRun. r=#dom-worker-reviewers

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: When using the debugger on the Worker, the content process could meet the assertion for JS compartment mismatching if SetImmediate() is used in the debugger script.
• User impact if declined: Users might not meet problem, if the debugger is not used. However, developers could meet a tab crash when using SetImmediate() in the debugger for Worker.
• Fix Landed on Version: 132
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The reason is WorkerRunnable::Run() uses the GetCurrentEventLoopTarget() to get the global for the runnable. However, the current event loop stack could be modified by other operation.
  Fortunately, DebuggerImmediateRunnable() is always created by the debugger global, and the callback function knows what the correct global to use. So this patch lets DebuggerImmediateRunnable::WorkerRun call Callback::Function::Call() directly to use the correct global.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9425248 [details]
Bug 1904059 - Using DOM::Function::Call instead JS_CallFunctionValue in DebuggerImmediateRunnable::WorkerRun. r=#dom-worker-reviewers

Approved for 128.4esr.

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/301b45e11abf