fix(core): perform microtask checkpoint after user callbacks when stack is empty - #36207
fix(core): perform microtask checkpoint after user callbacks when stack is empty#36207jmao0001 wants to merge 5 commits into
Conversation
Deno Individual Contributor License AgreementAll contributors have signed the CLA. Thank you! This is an automated message from CLA Assistant |
So which one is it? The implementation looks totally AI written. |
|
Hello @bartlomieju, sorry for the confusion about the PR description. To clarify: I utilized an AI to write the actual implementation and diffs based on your architectural design. I spent my time carefully reviewing the output, orchestrating the fix, and testing it to make sure it solves the issue, but the code generation itself was assisted by AI. The contradictory checklist was a mistake in my PR description that I missed before hitting submit. I've updated the description to be fully accurate. I would appreciate it if you could do one more review of this PR. |
|
Hello @bartlomieju, just a quick heads up: I have been doing some extra local verification in a Codespace while waiting for the CI approval. I caught a minor compilation issue (an invalid ASCII character in a JS comment and an immutable |
|
Hello @bartlomieju and maintainers, just a quick ping on this. Let me know if you need any changes from me before triggering the CI. |
|
Follow-up to #36207 to fix the failing unit and spec tests. WPT was already passing, so the core spec logic was fine, but we hit a few edge cases in the Deno tests. Here is what this fixes:
Just in case, Web API event listeners still use |
Summary
This PR implements the user-code depth counter design proposed by @bartlomieju in this comment on #11731, closing the remaining scope of that issue (the event-listener / internal-JS-callback case).
Background. Web IDL's "clean up after running script" algorithm says browsers must perform a microtask checkpoint after a user callback returns if no other user script is on the stack. Deno, however, was running microtasks only when the entire JS stack emptied — with no distinction between Deno-internal JS and user JS. This produced spec-incompatible ordering for events dispatched from internal code:
Browsers print
listener 1, microtask 1, listener 2, microtask 2; Deno printedlistener 1, listener 2, microtask 1, microtask 2.The 2021-vintage objection ("V8 auto-microtask policy can't be influenced") no longer applies:
deno_corenow runs underv8::MicrotasksPolicy::Explicitand already exposesop_run_microtasks. The remaining missing piece was knowing whether user script was on the stack — which is exactly what this PR adds.What changed
libs/core/runtime/jsrealm.rs— Added auser_code_depth: Box<[u32; 1]>field toContextState(sized to match the existingtick_info/immediate_info/timer_infoshared-buffer pattern). Bumped the counter aroundscript.run(tc_scope)inexecute_scriptandexecute_script_with_cache.libs/core/runtime/jsruntime.rs— Instore_js_callbacks, created aUint32Arraybacked byuser_code_depthand passed it to JS via a newDeno.core.__setUserCodeDepthinit-only setter (mirroring the existing__setTickInfo/__setImmediateInfo/__setTimerInfopattern). Also bumped the counter around the synchronous portion ofJsRuntime::mod_evaluate.libs/core/01_core.js— Added theuserCodeDepthshared-buffer reference, the__setUserCodeDepth(buf)setter, and theinvokeUserCallback(cb, thisArg, args)helper. The helper increments the counter, calls the user callback viaReflectApply, decrements infinally, and callsop_run_microtasks()when the counter returns to zero. V8'sPerformMicrotaskCheckpointno-ops when already running microtasks, so nested invocations from inside a promise reaction are safe and match the HTML spec's "if performing a microtask checkpoint is true, return" guard.ext/web/02_event.js— RoutedinnerInvokeEventListeners's listener callback invocation (both the function andhandleEventpaths) throughcore.invokeUserCallback. This is the single chokepoint that fixes the AbortSignal / MessagePort / unhandledrejection cases for free.ext/web/02_timers.js— RoutedsetTimeoutandsetIntervalcallbacks throughcore.invokeUserCallback.ext/webidl/00_webidl.js— RoutedinvokeCallbackFunctionthroughcore.invokeUserCallback. This covers all stream underlying-source/sink/size algorithms, queueing-strategy callbacks, and any other Web IDL callback entry point without requiring each call site to be updated individually.Why this is correct
target.dispatchEvent(...)from top-level user code): the outer top-level scope holds depth = 1, so inner listener invocations see depth ≥ 2 and skip the checkpoint. Microtasks defer until the user script unwinds → browser-canonical ordering.op_run_microtasks, so anyinvokeUserCallbackcalls inside them see the outer checkpoint in progress and the nestedop_run_microtasks()becomes a V8-level no-op. Newly queued microtasks are picked up by the outer checkpoint — matching the HTML spec's recursive-checkpoint guard.Uint32Array, so both Rust and internal JS touch it with a plain indexed read/write — same pattern already used fortick_infoandimmediate_info.Tests
tests/unit/event_target_test.ts: added two regression tests — one async test asserting browser-canonical ordering when an event is dispatched from internal code (viasetTimeout(0)), and one sync test asserting microtasks do not run between listeners when the event is dispatched from user code.libs/core_testing/unit/microtask_test.ts: added structural tests forDeno.core.invokeUserCallback— return-value pass-through,thisbinding, and exception propagation with the microtask checkpoint still firing infinally.Checklist
fix(module): brief descriptionconvention)./x fmt— to be run locally before merging./x lint— to be run locally before mergingAI disclosure: An AI assistant was used to help draft this PR description and to translate the design sketch from the issue thread into diffs. The design itself (user-code depth counter,
invokeUserCallbackhelper, bumping at top-level entry points) is the one proposed by @bartlomieju in the issue thread. All code was reviewed and adjusted to match the existingtick_info/immediate_info/timer_infoshared-buffer pattern already present indeno_core.Closes #11731