Details

Reviewers

nbp
arai

Group Reviewers

spidermonkey-reviewers

Commits

rFIREFOXAUTOLAND7c8b91d6d8b4: Bug 2068540 - Abort off-thread script compilation when the request is cancelled.

Bugzilla Bug ID

2068540

Summary

Add a cancellation flag to js::FrontendContext, polled where the frontend
starts on a script, a function or its bytecode, so that a cancelled
StencilCompileOrDecodeTask aborts instead of finishing work whose result is
discarded. Decode and wasm compilation have no poll points.

This saves CPU cycles on pages which cancel compiles.

Diff Detail

Repository: rFIREFOXAUTOLAND firefox-autoland

Event Timeline

leggert planned changes to this revision.Sep 2 2026, 2:37 PM

leggert created this revision.

Herald added a project: secure-revision. · View Herald TranscriptSep 2 2026, 2:37 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 2 2026, 2:38 PM

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

As with D322872, I'm again touching code here I'm very unfamiliar with, so please feel free to tell me this is completely broken and wrong.

leggert added a parent revision: D322872: Bug 2068481 - Don't block the main thread when cancelling off-thread script compilation. r=arai,smaug,#dom-core-reviewers.Sep 2 2026, 2:39 PM

Harbormaster failed remote builds in B1034800: Diff 1364021!Sep 2 2026, 2:49 PM

leggert requested review of this revision.Sep 3 2026, 10:59 AM

leggert updated this revision to Diff 1365118.

leggert retitled this revision from WIP: Bug 2068540 - Abort off-thread script compilation when the request is cancelled. r?nbp,arai,#dom-core-reviewers to Bug 2068540 - Abort off-thread script compilation when the request is cancelled. r?nbp,arai,#dom-core-reviewers.

leggert edited the summary of this revision. (Show Details)

leggert added reviewers: nbp, arai, dom-core-reviewers.

leggert mentioned this in D323212: Bug 2068540 - Abort off-thread script compilation when the request is cancelled. r?nbp,arai,#dom-core-reviewers.Sep 3 2026, 11:02 AM

leggert added a project: testing-exception-unchanged (Doesn't change behavior for users).Sep 3 2026, 11:03 AM

Harbormaster completed remote builds in B1035648: Diff 1365118.Sep 3 2026, 11:25 AM

This is basically JS engine only.

leggert added a reviewer: spidermonkey-reviewers.Sep 3 2026, 12:53 PM

The fundamental approach looks okay, but we should add several restriction and assertions, and rename some functions/variables to be more descriptive, to ensure that we never misuse it.

js/public/experimental/CompileScript.h
101	This should be called "RequestCancelling..." or something, as it doesn't immediately "cancel" it, and also it doesn't guarantee that it actually "cancel"s (`HadFrontendCancelled` can return false). It would depend on the definition of "cancel", but at least this function and the next one contradicts.
103–104	"by" sounds misleading here as well, especially with the current function name which implies it immediately "cancels".
110	This is very strong restriction, and error-prone thing. We should ensure that it's very difficult to misuse. For example, we could have the following: The consumer should explicitly specify that the newly created FrontendContext can be cancelled, on creation. e.g. let `JS::NewFrontendContext` receive an optional enum class parameter `AllowCancellingCompilation::Yes`, store it into debug-only field, and assert that it's `Yes` when cancellation is requested Restrict the use of cancellation only when the FrontendContext has no associated JSContext
js/src/frontend/FrontendContext.h
50–51	"Not cleared by clearErrors" sounds very scary. This means any `hadErrors()` -> `clearErrors()` flow can have a bug.
98–100	The name is misleading, as it's not clear that it's a request, not that it's already cancelled. (the consumer below calls `isCancelled()` and the `onCancelled`, which is very confusing) It should be `isCancellationRequestedForCompilation_` or something.
js/src/jsapi-tests/testFrontendErrors.cpp
386–387	This check is unnecessary, as this function doesn't touch cx.
452–455	This test passes even if the cancellation doesn't work at all. I'm not sure if the testcase worth the cost and the complexity.
464	This is unnecessary.
js/src/jsfriendapi.cpp
368–372 ↗	(On Diff #1365118)	This requires a performance test for the compilation. browsertime and talos could be used.

This revision now requires changes to proceed.Sep 3 2026, 1:58 PM

leggert added inline comments.Sep 3 2026, 2:34 PM

js/public/experimental/CompileScript.h
110	Thanks for the quick review on this. I'm probably missing something fundamental, since I'm not familiar with the code, but are there downsides to allowing this kind of interruption? Asking, since to me naively it seems like a good general addition...

arai added inline comments.Sep 3 2026, 2:47 PM

js/public/experimental/CompileScript.h
110	The interruption itself is good. The runtime part of the JS engine also has interrupt (implemented in slightly different way tho). But the fact that the interruption makes the entire FrontendContext not-reusable is not intuitive, as the function names disagree with the actual behavior ("clear" doesn't clear). So we should make it very obvious, opt-in, and very limited to the actual use case, so that we can avoid bugs in future.

leggert added inline comments.Sep 3 2026, 2:50 PM

js/public/experimental/CompileScript.h
110	Thanks, that makes sense.

I'm not sure about adding the check to AutoCheckRecursionLimit because it seems these are pretty different use cases. It might be simpler and more predictable perf-wise to check the cancel flag at the beginning of each JS function we parse, constant fold, and emit bytecode for, or something? (If we want to be more precise we could change that to the beginning of each scope.)

leggert requested review of this revision.Sep 3 2026, 3:21 PM

leggert updated this revision to Diff 1365421.

leggert retitled this revision from Bug 2068540 - Abort off-thread script compilation when the request is cancelled. r?nbp,arai,#dom-core-reviewers to Bug 2068540 - Abort off-thread script compilation when the request is cancelled. r?nbp,arai,#spidermonkey-reviewers.

Harbormaster completed remote builds in B1035898: Diff 1365421.Sep 3 2026, 3:43 PM

In D322930#11228679, @jandem wrote:

I'm not sure about adding the check to AutoCheckRecursionLimit because it seems these are pretty different use cases. It might be simpler and more predictable perf-wise to check the cancel flag at the beginning of each JS function we parse, constant fold, and emit bytecode for, or something? (If we want to be more precise we could change that to the beginning of each scope.)

Yeah, good point.
If the main goal is to reduce the power usage, not anything security sensitive (what the recursion limit is for), checking the flag in different and less places would work, and that will simplify things.

@leggert, can you move the cancellation check to a separate function, called in the following places?

GeneralParser::parse
Parser::moduleBody
GeneralParser::functionDefinition
Parser::trySyntaxParseInnerFunction's fallback path (see the comment below)
FoldConstants
BytecodeEmitter::emitScript
BytecodeEmitter::emitFunctionScript

js/public/experimental/CompileScript.h
30
js/src/frontend/FrontendContext.cpp
134–135	Off-thread-ness is unrelated here. More important thing is that we shouldn't cancel the compilation if it's associated with JSContext, as it makes the scope of cancellation and the not-reusable-ness vague. Also, adding another assertion `!allowCancellingCompilation_` to `FrontendContext::setCurrentJSContext` will make the comment unnecessary.
199–200	Please assert `!allowCancellingCompilation_` here as well.
213	Please assert `!allowCancellingCompilation_` here as well.
334	Please put the default parameter as a comment.
js/src/frontend/Parser.cpp
3153	So, for example, this case can just early return on the cancellation.

This revision now requires changes to proceed.Sep 3 2026, 3:46 PM

Thanks, I'll move it. (I basically piggybacked on what seemed easiest.) Will also address the other comments.

leggert requested review of this revision.Sep 4 2026, 6:28 AM

leggert updated this revision to Diff 1366332.

leggert edited the summary of this revision. (Show Details)

Thanks again for the detailed comments.

Harbormaster completed remote builds in B1036601: Diff 1366332.Sep 4 2026, 6:59 AM

Thank you so much for working on this!
r+ with the comments addressed.

js/src/frontend/FrontendContext.h
195	"compilation unwinds" is not what we really want to do. We just return an error. The "unwind" operation done around the syntax parsing is that, performing "full" parsing is costly, given it allocates AST nodes etc, and in general a large JS file has many functions that's never executed in single page load (e.g. large JS library). Thus we try to skip full parsing for inner functions as much as possible. That's called syntax parsing, which only checks syntax errors. The syntax parser doesn't support the entire syntax, and thus in some case the syntax parsing aborts, and in that case we perform full parsing on the same function, by rewinding the state (collected functions, tokenizer position, etc). So, the "unwind" is only for the successful compilation, and we don't need it if we're to cancel the compilation and discard the result.
js/src/frontend/Parser.cpp
3141–3153	Oh, sorry, I should've clarified. We don't have to perform the rewind operations above, given that we're performing early return.

This revision is now accepted and ready to land.Sep 4 2026, 8:09 AM

leggert updated this revision to Diff 1366495.Sep 4 2026, 9:33 AM

Thank you!

Harbormaster completed remote builds in B1036722: Diff 1366495.Sep 4 2026, 10:04 AM

Closed by commit rFIREFOXAUTOLAND7c8b91d6d8b4: Bug 2068540 - Abort off-thread script compilation when the request is cancelled. (authored by leggert). · Explain WhySep 4 2026, 10:46 AM

This revision was automatically updated to reflect the committed changes.

leggert added a commit: rFIREFOXAUTOLAND7c8b91d6d8b4: Bug 2068540 - Abort off-thread script compilation when the request is cancelled..

leggert mentioned this in rFIREFOXBETA7c8b91d6d8b4: Bug 2068540 - Abort off-thread script compilation when the request is cancelled..Sep 10 2026, 11:50 AM

leggert mentioned this in rFIREFOXRELEASE7c8b91d6d8b4: Bug 2068540 - Abort off-thread script compilation when the request is cancelled..Thu, Sep 24, 7:32 AM

Bug 2068540 - Abort off-thread script compilation when the request is cancelled. r?nbp,arai,#spidermonkey-reviewers
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1366551

dom/script/ScriptLoadContext.h

dom/script/ScriptLoader.cpp

js/public/experimental/CompileScript.h

js/src/frontend/BytecodeEmitter.cpp

js/src/frontend/FoldConstants.cpp

js/src/frontend/FrontendContext.h

js/src/frontend/FrontendContext.cpp

js/src/frontend/Parser.cpp

js/src/jsapi-tests/testFrontendErrors.cpp

Bug 2068540 - Abort off-thread script compilation when the request is cancelled. r?nbp,arai,#spidermonkey-reviewersClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1366551

dom/script/ScriptLoadContext.h

dom/script/ScriptLoader.cpp

js/public/experimental/CompileScript.h

js/src/frontend/BytecodeEmitter.cpp

js/src/frontend/FoldConstants.cpp

js/src/frontend/FrontendContext.h

js/src/frontend/FrontendContext.cpp

js/src/frontend/Parser.cpp

js/src/jsapi-tests/testFrontendErrors.cpp

Bug 2068540 - Abort off-thread script compilation when the request is cancelled. r?nbp,arai,#spidermonkey-reviewers
ClosedPublic
Actions

Revision Contents
Changeset List