1898029 - Interrupts should switch back to the main stack to execute user code

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, task, P3)

Description

[Ryan Hunt [:rhunt]]

If wasm code is running on a suspendable stack and is interrupted, the interrupt code may run a user callback which can do whatever it wants (including re-entering the engine and running scripts). This could lead to JS code unexpectedly running on a suspendable stack.

I'm not sure the best solution here. The interrupt code always runs through a trap, maybe we should have the trap exit always switch back to the main stack before calling WasmHandleTrap? That may also help out the debugger, which will have a similar problem and also uses traps.

Comment 1

[Ryan Hunt [:rhunt]]

It looks like atomics.wait can cause an interrupt too [1].

[1] https://searchfox.org/mozilla-central/rev/c84d3db8d7d6503b1208a0378db640095e106355/js/src/builtin/AtomicsObject.cpp#1037

Comment 2

[Ryan Hunt [:rhunt]]

An assertion in CheckInterrupt that we're not on a suspendable stack would be useful here too.

Comment 3

[Yury Delendik (:yury)]

This could lead to JS code unexpectedly running on a suspendable stack.

Currently we do that for imports (at CallImportOnMainThread) -- we probably want to develop a common solution based on that.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:rhunt, could you have a look please?

For more information, please visit BugBot documentation.

Comment 5

[Yury Delendik (:yury)]

Bug 1898031 introduced ForwardToMainStack utility that temporary switches stacks for debugger handler. The JSContext::handleInterrupt can perform the same thing to execute any HandleInterrupt handlers on the main stack.

Comment 6

[Yury Delendik (:yury)]

Attachment: Bug 1898029 - [wasm-jspi] Check if on suspendable stack before handling interrupts. r?jandem

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46e3983957d4
[wasm-jspi] Check if on suspendable stack before handling interrupts. r=jandem

Comment 8

[tszentpeteri]

Backed out for causing sm bustages on timeout.js

• Backout link
• Push with failures
• Failure Log:

[task 2024-10-18T14:11:07.093Z] TEST-PASS | js/src/jit-test/tests/wasm/js-promise-integration/shutdown.js | Success (code 0, args "--setpref=wasm_js_promise_integration=true --no-blinterp --no-baseline --no-ion --more-compartments") [0.0 s]
[task 2024-10-18T14:11:07.093Z] Exit code: 0
[task 2024-10-18T14:11:07.093Z] FAIL - wasm/js-promise-integration/timeout.js
[task 2024-10-18T14:11:07.093Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/js-promise-integration/timeout.js | Unknown (code 0, args "--setpref=wasm_js_promise_integration=true") [0.0 s]
[task 2024-10-18T14:11:07.093Z] INFO exit-status     : 0
[task 2024-10-18T14:11:07.093Z] INFO timed-out       : False
[task 2024-10-18T14:11:07.093Z] Exit code: 0
[task 2024-10-18T14:11:07.093Z] FAIL - wasm/js-promise-integration/timeout.js
[task 2024-10-18T14:11:07.093Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/js-promise-integration/timeout.js | Unknown (code 0, args "--setpref=wasm_js_promise_integration=true --ion-eager --ion-offthread-compile=off --more-compartments") [0.0 s]
[task 2024-10-18T14:11:07.093Z] INFO exit-status     : 0
[task 2024-10-18T14:11:07.093Z] INFO timed-out       : False
[task 2024-10-18T14:11:07.093Z] Exit code: 0
[task 2024-10-18T14:11:07.093Z] FAIL - wasm/js-promise-integration/timeout.js
[task 2024-10-18T14:11:07.093Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/js-promise-integration/timeout.js | Unknown (code 0, args "--setpref=wasm_js_promise_integration=true --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads") [0.0 s]
[task 2024-10-18T14:11:07.093Z] INFO exit-status     : 0
[task 2024-10-18T14:11:07.093Z] INFO timed-out       : False
[task 2024-10-18T14:11:07.093Z] Exit code: 0
[task 2024-10-18T14:11:07.093Z] FAIL - wasm/js-promise-integration/timeout.js
[task 2024-10-18T14:11:07.093Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/js-promise-integration/timeout.js | Unknown (code 0, args "--setpref=wasm_js_promise_integration=true --baseline-eager --write-protect-code=off") [0.0 s]
[task 2024-10-18T14:11:07.093Z] INFO exit-status     : 0
[task 2024-10-18T14:11:07.093Z] INFO timed-out       : False
[task 2024-10-18T14:11:07.093Z] Exit code: 0
[task 2024-10-18T14:11:07.093Z] FAIL - wasm/js-promise-integration/timeout.js
[task 2024-10-18T14:11:07.093Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/js-promise-integration/timeout.js | Unknown (code 0, args "--setpref=wasm_js_promise_integration=true --no-blinterp --no-baseline --no-ion --more-compartments") [0.0 s]
[task 2024-10-18T14:11:07.093Z] INFO exit-status     : 0
[task 2024-10-18T14:11:07.093Z] INFO timed-out       : False
[task 2024-10-18T14:11:07.101Z] Exit code: 0
[task 2024-10-18T14:11:07.101Z] FAIL - wasm/js-promise-integration/timeout.js
[task 2024-10-18T14:11:07.101Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/js-promise-integration/timeout.js | Unknown (code 0, args "--setpref=wasm_js_promise_integration=true --blinterp-eager") [0.0 s]
[task 2024-10-18T14:11:07.101Z] INFO exit-status     : 0
[task 2024-10-18T14:11:07.101Z] INFO timed-out       : False
[task 2024-10-18T14:11:07.101Z] TEST-PASS | js/src/jit-test/tests/wasm/js-types/function-ctor-callable.js | Success (code 0, args "") [0.0 s]

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/728ebf6a9ef7
[wasm-jspi] Check if on suspendable stack before handling interrupts. r=jandem

Comment 10

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/728ebf6a9ef7

Comment 11

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #9)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/728ebf6a9ef7
[wasm-jspi] Check if on suspendable stack before handling interrupts.
r=jandem

Perfherder has detected a browsertime performance change from push 728ebf6a9ef746a1918df142fdb9a51289086603.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
6%	speedometer3 cpuTimeSupport	macosx1015-64-shippable-qr	fission webrender	94,050.40 -> 88,074.00	Before/After
6%	speedometer3 cpuTimeSupport	macosx1015-64-shippable-qr	fission webrender	93,228.25 -> 87,431.00	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 42451

For more information on performance sheriffing please see our FAQ.