Fix async context preservation across await for Bun - #135
Conversation
|
/build-preview |
|
🔄 Preview build started Release tag: |
WalkthroughAdds conditional Bun-specific async-context capture into an InternalFieldTuple, propagates that tuple through promise reactions and microtasks, sets Bun async context before executing microtasks/async resumes, and restores it afterward across Promise, async-function, async-generator, and async-iterator pathways when USE(BUN_JSC_ADDITIONS) is enabled. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
|
This may have been caused by a newer build being triggered for the same PR. |
|
/build-preview |
|
🔄 Preview build started Release tag: |
|
✅ Preview build completed Release: autobuild-preview-pr-135-a6fa914b |
aec04e2 to
4276058
Compare
|
/build-preview |
|
🔄 Preview build started Release tag: |
4276058 to
691e0d8
Compare
|
/build-preview |
|
This may have been caused by a newer build being triggered for the same PR. |
|
🔄 Preview build started Release tag: |
|
✅ Preview build completed Release: autobuild-preview-pr-135-691e0d80 |
53dc5e2 to
6ba5c16
Compare
|
/preview-build |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @Source/JavaScriptCore/runtime/JSMicrotask.cpp:
- Around line 1219-1324: The repeated async context save/restore logic in the
AsyncGenerator cases (AsyncGeneratorYieldAwaited, AsyncGeneratorBodyCallNormal,
AsyncGeneratorBodyCallReturn, AsyncGeneratorResumeNext) should be extracted into
a single helper to avoid duplication: add a helper (e.g., prepareAsyncContext or
AsyncContextScope helper struct with explicit restore() method) that accepts
VM&, JSGlobalObject*, and a JSValue contextArg reference, unwraps
InternalFieldTuple if present, sets globalObject->m_asyncContextData internal
field and returns the unwrapped context plus a token (InternalFieldTuple* and
JSValue restore) or an object with a restore() method; then replace the
duplicated blocks in those cases to call the helper, call scope.release(),
invoke asyncGeneratorYieldAwaited / asyncGeneratorBodyCallNormal /
asyncGeneratorBodyCallReturn / asyncGeneratorResumeNextReturn with the unwrapped
context, and finally call the helper’s restore() (or use its RAII destructor if
you pass VM for destructor) to put back restoreAsyncContext.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 691e0d80a4ad068845345bb2a4157e3a5a3c4759 and 6ba5c16.
📒 Files selected for processing (3)
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/Source/JavaScriptCore/runtime/**/*.{h,cpp,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
Add or modify runtime functions within the JavaScriptCore runtime/ directory
Files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
**/Source/JavaScriptCore/**/*.{cpp,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
Use dataLog() for printf-style debugging in JavaScriptCore code
Files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
Source/JavaScriptCore/**/*.{cpp,h}
📄 CodeRabbit inference engine (Source/JavaScriptCore/CLAUDE.md)
Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Applied to files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:28.399Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:28.399Z
Learning: Applies to **/Source/JavaScriptCore/runtime/**/*.{h,cpp,mm} : Add or modify runtime functions within the JavaScriptCore runtime/ directory
Applied to files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/builtins/**/*.js : Prefer implementing and modifying JavaScript built-ins in builtins/*.js before changing C++
Applied to files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/{runtime/JSModuleLoader.cpp,builtins/ModuleLoader.js} : Coordinate module loading changes across runtime/JSModuleLoader.cpp (C++) and builtins/ModuleLoader.js (JS)
Applied to files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:28.399Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:28.399Z
Learning: Applies to **/Source/JavaScriptCore/heap/**/*.{h,cpp,mm} : Make heap/garbage-collection changes within the heap/ components
Applied to files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp : Define new or modified JavaScript global functions using the JSC_DEFINE_HOST_FUNCTION macro
Applied to files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cppSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/CommonIdentifiers.h : Add or reference well-known property names in runtime/CommonIdentifiers.h
Applied to files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/JSGlobalObject.cpp : Register new global functions in runtime/JSGlobalObject.cpp after defining them
Applied to files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
🔇 Additional comments (14)
Source/JavaScriptCore/runtime/JSPromisePrototype.cpp (2)
36-38: LGTM!The conditional include for
InternalFieldTuple.his properly guarded withUSE(BUN_JSC_ADDITIONS), following the coding guidelines for Bun-specific features.
277-292: LGTM!The async context wrapping logic is correct:
- Only creates the tuple when
asyncContextis not undefined (avoiding unnecessary allocations)- Tuple structure
[context, asyncContext]matches the extraction pattern inPromiseFinallyReactionJob- Properly guarded with
USE(BUN_JSC_ADDITIONS)Source/JavaScriptCore/runtime/JSPromise.cpp (6)
47-49: LGTM!Conditional include properly guarded.
277-322: LGTM!The async context capture for promise reactions is well-implemented:
- Tuple structure
[userContext, asyncContext]is consistent with extraction inPromiseReactionJob- Properly guards allocation when no async context exists
- Context is correctly propagated to all status cases
325-367: LGTM!The new
performPromiseThenWithContextfunction correctly handles the case where an explicit user context is provided:
- Creates tuple when either
userContextorasyncContextis meaningful- Properly propagates context to all promise status cases
- Follows the same pattern as
performPromiseThen
458-499: LGTM!Both the fast-path and slow-path thenable resolution correctly capture and propagate the async context to their respective microtasks. This ensures context is preserved when resolving promises with thenables.
708-757: LGTM!This is the core fix for the async context preservation issue. The implementation correctly:
- Captures Bun's async context at the point of
await- Wraps the generator context with async context in an
InternalFieldTuple- Uses the
BUN_CONTEXTmacro to cleanly switch between wrapped and original context- Properly
#undefs the macro at the end to avoid polluting the namespace
692-701: LGTM!The
triggerPromiseReactionschanges correctly handle the context propagation:
- When Bun additions are enabled and context exists, uses the 6-argument
queueMicrotask- Non-Bun builds retain the assertion that context is always undefined/null
- Falls through to the original behavior when no context is present
Source/JavaScriptCore/runtime/JSMicrotask.cpp (6)
57-59: LGTM!Conditional include properly guarded.
801-829: LGTM!The async context setup/restore pattern is correctly implemented:
- Saves current context before modification
- Only sets context when
asyncContextDataexists- Restores original context after execution
- Guard conditions prevent null pointer access
867-923: LGTM!Both thenable job cases correctly handle async context:
PromiseResolveThenableJobextracts direct async context from argumentsPromiseResolveThenableJobWithInternalMicrotaskextracts from wrapped tuple- Both properly save, set, and restore the async context
963-1093: LGTM!The
PromiseReactionJobchanges correctly implement async context handling:
- Properly extracts both
userContextandasyncContextfrom either tuple or direct value- All exit points (exception handlers, early returns, normal completion) restore the async context
- The comment at lines 1042-1044 correctly explains keeping context active during
resolvePromise/rejectPromiseThe handling of the context argument is robust, covering:
InternalFieldTuple(lines 975-978)- Direct cell values (lines 979-980)
- Non-cell values (lines 981-983)
1103-1203: LGTM!This is the core fix for the PR's main issue. The
AsyncFunctionResumecase correctly:
- Extracts generator and async context from
InternalFieldTuplewhen wrapped- Falls back to direct cast when not wrapped (backward compatibility)
- Sets up Bun's async context before resuming the async function
- Restores context at all exit points:
- Exception during termination check
- Error path (reject)
- Completed state (resolve)
- Next await iteration (captures new context for continuation)
1397-1433: LGTM!The new Bun-specific microtask types are correctly implemented:
BunPerformMicrotaskJobcollects all non-empty arguments starting from index 1BunInvokeJobWithArgumentsfollows the same pattern- Both properly guard against argument buffer overflow
- These entry points don't need async context save/restore as they're called by Bun's own microtask handling
| case InternalMicrotask::PromiseFinallyReactionJob: { | ||
| // Phase 1: Original promise settled | ||
| // arguments[0] = resultPromise | ||
| // arguments[1] = value/reason from original promise | ||
| // arguments[2] = context (JSPromiseCombinatorsGlobalContext: promise=resultPromise, values=onFinally) | ||
| // OR InternalFieldTuple: [context, asyncContext] when Bun async context is present | ||
| // payload = Fulfilled/Rejected status | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| // Extract context and async context from InternalFieldTuple if wrapped | ||
| JSValue contextArg = arguments[2]; | ||
| JSPromiseCombinatorsGlobalContext* context; | ||
| JSValue asyncContext = jsUndefined(); | ||
|
|
||
| if (contextArg.isCell()) { | ||
| if (auto* tuple = jsDynamicCast<InternalFieldTuple*>(contextArg)) { | ||
| context = jsCast<JSPromiseCombinatorsGlobalContext*>(tuple->getInternalField(0)); | ||
| asyncContext = tuple->getInternalField(1); | ||
| } else { | ||
| context = jsCast<JSPromiseCombinatorsGlobalContext*>(contextArg); | ||
| } | ||
| } else { | ||
| context = jsCast<JSPromiseCombinatorsGlobalContext*>(contextArg); | ||
| } | ||
|
|
||
| // Set up async context before calling onFinally | ||
| InternalFieldTuple* asyncContextData = nullptr; | ||
| JSValue restoreAsyncContext; | ||
| if (!asyncContext.isUndefined()) { | ||
| asyncContextData = globalObject->m_asyncContextData.get(); | ||
| if (asyncContextData) { | ||
| restoreAsyncContext = asyncContextData->getInternalField(0); | ||
| asyncContextData->putInternalField(vm, 0, asyncContext); | ||
| } | ||
| } | ||
|
|
||
| scope.release(); | ||
| promiseFinallyReactionJob(globalObject, vm, | ||
| jsCast<JSPromise*>(arguments[0]), | ||
| arguments[1], | ||
| context, | ||
| static_cast<JSPromise::Status>(payload)); | ||
|
|
||
| // Restore async context | ||
| if (asyncContextData) | ||
| asyncContextData->putInternalField(vm, 0, restoreAsyncContext); | ||
| #else | ||
| scope.release(); | ||
| promiseFinallyReactionJob(globalObject, vm, | ||
| jsCast<JSPromise*>(arguments[0]), | ||
| arguments[1], | ||
| jsCast<JSPromiseCombinatorsGlobalContext*>(arguments[2]), | ||
| static_cast<JSPromise::Status>(payload)); | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the context extraction logic.
The context extraction at lines 1340-1348 has redundant handling. Since jsDynamicCast<InternalFieldTuple*> already handles non-cell values safely, the outer isCell() check may be unnecessary.
♻️ Suggested simplification
- if (contextArg.isCell()) {
- if (auto* tuple = jsDynamicCast<InternalFieldTuple*>(contextArg)) {
- context = jsCast<JSPromiseCombinatorsGlobalContext*>(tuple->getInternalField(0));
- asyncContext = tuple->getInternalField(1);
- } else {
- context = jsCast<JSPromiseCombinatorsGlobalContext*>(contextArg);
- }
- } else {
+ if (auto* tuple = jsDynamicCast<InternalFieldTuple*>(contextArg)) {
+ context = jsCast<JSPromiseCombinatorsGlobalContext*>(tuple->getInternalField(0));
+ asyncContext = tuple->getInternalField(1);
+ } else {
context = jsCast<JSPromiseCombinatorsGlobalContext*>(contextArg);
- }
+ }|
/build-preview |
|
🔄 Preview build started Release tag: |
|
✅ Preview build completed Release: autobuild-preview-pr-135-6ba5c161 |
Additional Fix: Promise.resolve(InternalPromise) behaviorAdded a fix for ProblemThe C++ SolutionAdded a check in TestsBoth tests now pass:
/build-preview |
|
🔄 Preview build started Release tag: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Source/JavaScriptCore/runtime/JSPromise.cpp:
- Around line 916-933: The current logic uses a goto createNewPromise to force
creation of a new Promise when an InternalPromise's constructor doesn't match;
replace the goto by computing a boolean (e.g., needNewPromise) after the checks
in the block that calls promiseSpeciesWatchpointIsValid(vm, promise), set
needNewPromise = true when promise->structure()->classInfoForCells() ==
JSInternalPromise::info() and constructor !=
globalObject->internalPromiseConstructor(), otherwise return promise; then after
retrieving property via promise->get(...) and comparing to constructor, return
promise when equal, and finally if needNewPromise is true fall through to the
existing createNewPromise logic (or call the same creation path) instead of
jumping with goto, keeping all semantics of promiseSpeciesWatchpointIsValid,
JSInternalPromise::info(), constructor, and
globalObject->internalPromiseConstructor() intact.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Source/JavaScriptCore/runtime/JSPromise.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/Source/JavaScriptCore/runtime/**/*.{h,cpp,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
Add or modify runtime functions within the JavaScriptCore runtime/ directory
Files:
Source/JavaScriptCore/runtime/JSPromise.cpp
**/Source/JavaScriptCore/**/*.{cpp,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
Use dataLog() for printf-style debugging in JavaScriptCore code
Files:
Source/JavaScriptCore/runtime/JSPromise.cpp
Source/JavaScriptCore/**/*.{cpp,h}
📄 CodeRabbit inference engine (Source/JavaScriptCore/CLAUDE.md)
Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Files:
Source/JavaScriptCore/runtime/JSPromise.cpp
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:28.399Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:28.399Z
Learning: Applies to **/Source/JavaScriptCore/runtime/**/*.{h,cpp,mm} : Add or modify runtime functions within the JavaScriptCore runtime/ directory
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/{runtime/JSModuleLoader.cpp,builtins/ModuleLoader.js} : Coordinate module loading changes across runtime/JSModuleLoader.cpp (C++) and builtins/ModuleLoader.js (JS)
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:28.399Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:28.399Z
Learning: Applies to **/Source/JavaScriptCore/heap/**/*.{h,cpp,mm} : Make heap/garbage-collection changes within the heap/ components
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp : Define new or modified JavaScript global functions using the JSC_DEFINE_HOST_FUNCTION macro
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/JSGlobalObject.cpp : Register new global functions in runtime/JSGlobalObject.cpp after defining them
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
🔇 Additional comments (9)
Source/JavaScriptCore/runtime/JSPromise.cpp (9)
47-49: LGTM!The conditional include is properly guarded with
USE(BUN_JSC_ADDITIONS)as per the coding guidelines for Bun-specific features.
277-290: LGTM!The async context capture pattern is correctly implemented:
- Properly guarded with
USE(BUN_JSC_ADDITIONS)- Only allocates the tuple when
asyncContextis meaningful (not undefined)- Correctly structures the tuple as
[userContext, asyncContext]
295-319: LGTM!All three promise status cases (Pending, Rejected, Fulfilled) consistently propagate the captured async context when
USE(BUN_JSC_ADDITIONS)is enabled, while preserving the original behavior for non-Bun builds.
325-367: LGTM!The new
performPromiseThenWithContextmethod correctly handles the thenable continuation case by:
- Wrapping both user context and async context in an
InternalFieldTuplewhen either is present- Using appropriate null checks (
isUndefinedOrNullfor user context vsisUndefinedfor async context)- Maintaining consistent handling across all promise status branches
458-468: LGTM!Async context is correctly captured and propagated for the fast-path thenable resolution when the promise is
isThenFastAndNonObservable().
491-499: LGTM!Async context propagation for the generic thenable resolution path mirrors the fast path, ensuring consistent behavior across all thenable resolution scenarios.
692-701: LGTM!The conditional microtask queuing correctly handles the context propagation:
- When context is present, passes it to the microtask
- Falls through to the no-context path otherwise
- Non-Bun builds retain the assertion that context should be undefined/null
708-757: LGTM! This is the core fix for async context preservation acrossawait.The implementation correctly:
- Captures Bun's async context at the await point
- Wraps it together with the generator context in an
InternalFieldTuple- Propagates the wrapped context through all code paths (fulfilled, rejected, thenable)
- Uses the
BUN_CONTEXTmacro to reduce code duplication while maintaining the non-Bun path- Properly
#undefs the macro after use- Guards Bun-specific features with
USE(BUN_JSC_ADDITIONS)as requiredThis ensures
JSMicrotask.cpp'sAsyncFunctionResumecase can extract and restore the async context when resuming the async function. Verify thatInternalFieldTuple::createwith the 4-argument overload (taking vm, structure, val1, val2) exists in the codebase.
277-290: Verify consistent tuple slot indexing across consumer sites.The
InternalFieldTuplelayout is[0: userContext, 1: asyncContext]. Ensure thatJSMicrotask.cppextracts these fields in the same order when restoring the async context. Check that allgetInternalField()calls in JSMicrotask.cpp use indices 0 and 1 in the same sequence as they are set in JSPromise.cpp.Also applies to: 334-345
| if (promiseSpeciesWatchpointIsValid(vm, promise)) [[likely]] { | ||
| // For InternalPromise, we can only return the same promise if the constructor | ||
| // matches. This preserves the behavior where Promise.resolve(InternalPromise) | ||
| // creates a new regular Promise, ensuring user-facing APIs return regular Promises. | ||
| if (promise->structure()->classInfoForCells() == JSInternalPromise::info()) { | ||
| if (constructor != globalObject->internalPromiseConstructor()) | ||
| goto createNewPromise; | ||
| } | ||
| return promise; | ||
| } | ||
|
|
||
| auto property = promise->get(globalObject, vm.propertyNames->constructor); | ||
| RETURN_IF_EXCEPTION(scope, { }); | ||
|
|
||
| if (property == constructor) | ||
| return promise; | ||
| } | ||
| createNewPromise: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider refactoring to avoid goto for improved readability.
The logic is correct: when Promise.resolve receives an InternalPromise but the constructor is not internalPromiseConstructor, a new regular Promise must be created to ensure it passes instanceof Promise checks.
However, the goto createNewPromise pattern reduces readability. Consider restructuring:
♻️ Suggested refactor
if (argument.inherits<JSPromise>()) {
auto* promise = jsCast<JSPromise*>(argument);
- if (promiseSpeciesWatchpointIsValid(vm, promise)) [[likely]] {
- // For InternalPromise, we can only return the same promise if the constructor
- // matches. This preserves the behavior where Promise.resolve(InternalPromise)
- // creates a new regular Promise, ensuring user-facing APIs return regular Promises.
- if (promise->structure()->classInfoForCells() == JSInternalPromise::info()) {
- if (constructor != globalObject->internalPromiseConstructor())
- goto createNewPromise;
- }
+ if (promiseSpeciesWatchpointIsValid(vm, promise)) [[likely]] {
+ // For InternalPromise, we can only return the same promise if the constructor
+ // matches. This preserves the behavior where Promise.resolve(InternalPromise)
+ // creates a new regular Promise, ensuring user-facing APIs return regular Promises.
+ bool isInternalPromise = promise->structure()->classInfoForCells() == JSInternalPromise::info();
+ bool constructorMatches = !isInternalPromise || constructor == globalObject->internalPromiseConstructor();
+ if (constructorMatches)
return promise;
- }
+ } else {
auto property = promise->get(globalObject, vm.propertyNames->constructor);
RETURN_IF_EXCEPTION(scope, { });
if (property == constructor)
return promise;
+ }
}
-createNewPromise:
if (constructor == globalObject->promiseConstructor()) [[likely]] {🤖 Prompt for AI Agents
In @Source/JavaScriptCore/runtime/JSPromise.cpp around lines 916 - 933, The
current logic uses a goto createNewPromise to force creation of a new Promise
when an InternalPromise's constructor doesn't match; replace the goto by
computing a boolean (e.g., needNewPromise) after the checks in the block that
calls promiseSpeciesWatchpointIsValid(vm, promise), set needNewPromise = true
when promise->structure()->classInfoForCells() == JSInternalPromise::info() and
constructor != globalObject->internalPromiseConstructor(), otherwise return
promise; then after retrieving property via promise->get(...) and comparing to
constructor, return promise when equal, and finally if needNewPromise is true
fall through to the existing createNewPromise logic (or call the same creation
path) instead of jumping with goto, keeping all semantics of
promiseSpeciesWatchpointIsValid, JSInternalPromise::info(), constructor, and
globalObject->internalPromiseConstructor() intact.
|
❌ Preview build failed Check the workflow run for details. |
When an async function awaits a promise, Bun's AsyncLocalStorage context was being lost. This happened because the upstream WebKit change moved the Microtask handling to C++, and the InternalMicrotask::AsyncFunctionResume case was not restoring Bun's async context. The fix: 1. In JSPromise::resolveWithInternalMicrotaskForAsyncAwait: Capture the current async context and wrap it together with the generator in an InternalFieldTuple. 2. In runInternalMicrotask (AsyncFunctionResume case): Extract the async context from the InternalFieldTuple and restore it during the microtask execution. This mirrors the existing async context handling in PromiseReactionJob and ensures that AsyncLocalStorage.getStore() returns the correct context after await points. Fixes: oven-sh/bun#25958
When Promise.resolve() is called with an InternalPromise argument, it should create a new regular Promise instead of returning the same InternalPromise. This ensures user-facing APIs return regular Promises that pass instanceof Promise checks. The issue was that the C++ promiseResolve implementation used promiseSpeciesWatchpointIsValid() which returns true for InternalPromise, causing it to bypass the constructor check. The old JavaScript implementation explicitly checked value.constructor === constructor before returning the same promise. This fix adds a check for InternalPromise: if the target constructor is not internalPromiseConstructor, we fall through to create a new Promise.
a1f3614 to
8cbd8ed
Compare
|
/build-preview |
|
🔄 Preview build started Release tag: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Source/JavaScriptCore/runtime/JSMicrotask.cpp`:
- Around line 967-983: The context extraction logic for arguments[3] is
redundant: remove the unnecessary contextArg.isCell() check and rely directly on
jsDynamicCast<InternalFieldTuple*> to detect an InternalFieldTuple; keep the
existing fallback paths that set userContext = contextArg for non-tuple values
and handle undefined/null by leaving userContext and asyncContext as
jsUndefined(). Specifically, simplify the branch around contextArg, using
jsDynamicCast<InternalFieldTuple*> to populate userContext and asyncContext when
it returns non-null and otherwise assign userContext = contextArg when
contextArg is not undefined/null.
♻️ Duplicate comments (2)
Source/JavaScriptCore/runtime/JSMicrotask.cpp (1)
1340-1349: Simplify redundant context extraction logic.The context extraction has redundant branches. The outer
elseat line 1347-1349 is unreachable because it's only entered whencontextArg.isCell()is false, but then it callsjsCast<JSPromiseCombinatorsGlobalContext*>(contextArg)which would be invalid for non-cell values.♻️ Suggested fix
- if (contextArg.isCell()) { - if (auto* tuple = jsDynamicCast<InternalFieldTuple*>(contextArg)) { - context = jsCast<JSPromiseCombinatorsGlobalContext*>(tuple->getInternalField(0)); - asyncContext = tuple->getInternalField(1); - } else { - context = jsCast<JSPromiseCombinatorsGlobalContext*>(contextArg); - } - } else { + if (auto* tuple = jsDynamicCast<InternalFieldTuple*>(contextArg)) { + context = jsCast<JSPromiseCombinatorsGlobalContext*>(tuple->getInternalField(0)); + asyncContext = tuple->getInternalField(1); + } else { context = jsCast<JSPromiseCombinatorsGlobalContext*>(contextArg); - } + }Source/JavaScriptCore/runtime/JSPromise.cpp (1)
916-933: Consider refactoring to avoidgotofor improved readability.The logic is correct: when
Promise.resolvereceives anInternalPromisebut the constructor is notinternalPromiseConstructor, a new regularPromisemust be created to ensure it passesinstanceof Promisechecks.However, the
goto createNewPromisepattern reduces readability.♻️ Suggested refactor
if (argument.inherits<JSPromise>()) { auto* promise = jsCast<JSPromise*>(argument); if (promiseSpeciesWatchpointIsValid(vm, promise)) [[likely]] { // For InternalPromise, we can only return the same promise if the constructor // matches. This preserves the behavior where Promise.resolve(InternalPromise) // creates a new regular Promise, ensuring user-facing APIs return regular Promises. - if (promise->structure()->classInfoForCells() == JSInternalPromise::info()) { - if (constructor != globalObject->internalPromiseConstructor()) - goto createNewPromise; - } - return promise; + bool isInternalPromise = promise->structure()->classInfoForCells() == JSInternalPromise::info(); + if (!isInternalPromise || constructor == globalObject->internalPromiseConstructor()) + return promise; + } else { + auto property = promise->get(globalObject, vm.propertyNames->constructor); + RETURN_IF_EXCEPTION(scope, { }); + + if (property == constructor) + return promise; } - - auto property = promise->get(globalObject, vm.propertyNames->constructor); - RETURN_IF_EXCEPTION(scope, { }); - - if (property == constructor) - return promise; } -createNewPromise:
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/Source/JavaScriptCore/runtime/**/*.{h,cpp,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
Add or modify runtime functions within the JavaScriptCore runtime/ directory
Files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
**/Source/JavaScriptCore/**/*.{cpp,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
Use dataLog() for printf-style debugging in JavaScriptCore code
Files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
Source/JavaScriptCore/**/*.{cpp,h}
📄 CodeRabbit inference engine (Source/JavaScriptCore/CLAUDE.md)
Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Applied to files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
📚 Learning: 2025-10-01T01:58:28.399Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:28.399Z
Learning: Applies to **/Source/JavaScriptCore/runtime/**/*.{h,cpp,mm} : Add or modify runtime functions within the JavaScriptCore runtime/ directory
Applied to files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
📚 Learning: 2025-10-01T01:58:28.399Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:28.399Z
Learning: Applies to **/Source/JavaScriptCore/heap/**/*.{h,cpp,mm} : Make heap/garbage-collection changes within the heap/ components
Applied to files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/builtins/**/*.js : Prefer implementing and modifying JavaScript built-ins in builtins/*.js before changing C++
Applied to files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/{runtime/JSModuleLoader.cpp,builtins/ModuleLoader.js} : Coordinate module loading changes across runtime/JSModuleLoader.cpp (C++) and builtins/ModuleLoader.js (JS)
Applied to files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp : Define new or modified JavaScript global functions using the JSC_DEFINE_HOST_FUNCTION macro
Applied to files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/JSGlobalObject.cpp : Register new global functions in runtime/JSGlobalObject.cpp after defining them
Applied to files:
Source/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/CommonIdentifiers.h : Add or reference well-known property names in runtime/CommonIdentifiers.h
Applied to files:
Source/JavaScriptCore/runtime/JSPromisePrototype.cpp
🔇 Additional comments (15)
Source/JavaScriptCore/runtime/JSMicrotask.cpp (8)
57-59: LGTM: Properly guarded include for InternalFieldTuple.The conditional include follows the coding guidelines for guarding Bun-specific features with
USE(BUN_JSC_ADDITIONS).
801-829: LGTM: Async context preservation for PromiseResolveThenableJobFast.The save/set/execute/restore pattern correctly preserves async context across the microtask execution.
867-891: LGTM: Async context handling for PromiseResolveThenableJob.Correctly captures and restores async context around the thenable's
thenmethod invocation.
899-924: LGTM: Async context extraction from InternalFieldTuple for thenable resolution.The extraction logic correctly handles both tuple-wrapped and direct context values.
1103-1203: LGTM: Core fix for async context preservation across await.This is the primary fix for the PR objective. The implementation correctly:
- Extracts the generator and async context from the
InternalFieldTuplewrapper- Restores Bun's async context before resuming the async function
- Ensures context is properly restored at all exit points (exception, completion, continuation)
1206-1217: LGTM: InternalFieldTuple unwrapping for async iterator operations.Correctly extracts the context from the tuple wrapper when present.
1219-1325: LGTM: Consistent async context handling for async generator operations.All four async generator cases follow the same correct pattern for async context save/restore. The slight variations in the called functions justify the inline implementation over a shared helper.
1397-1434: LGTM: New Bun-specific microtask job types.Both
BunPerformMicrotaskJobandBunInvokeJobWithArgumentsare properly guarded and provide clean entry points for Bun's custom microtask handling.Source/JavaScriptCore/runtime/JSPromisePrototype.cpp (2)
36-38: LGTM: Properly guarded include.The conditional include follows the coding guidelines.
277-293: LGTM: Async context capture for Promise.finally().Correctly captures the async context at the
.finally()call site and wraps it with the combinator context in anInternalFieldTuple. This ensures theonFinallycallback executes with the correct async context.Source/JavaScriptCore/runtime/JSPromise.cpp (5)
47-49: LGTM: Properly guarded include.Follows coding guidelines for Bun-specific features.
277-323: LGTM: Async context propagation in performPromiseThen.Correctly captures the async context at the point of
.then()registration and propagates it through the promise reaction mechanism. The tuple structure[userContext, asyncContext]is consistent with the extraction logic inJSMicrotask.cpp.
325-367: LGTM: New performPromiseThenWithContext for explicit user context.This new method correctly extends the async context propagation pattern to support explicit user context values, wrapping both in an
InternalFieldTuplewhen either is meaningful.
458-500: LGTM: Async context capture in resolvePromise.Both the fast-path (JSPromise thenable) and slow-path (generic thenable) correctly capture the async context to pass to the thenable resolution microtasks.
708-757: LGTM: Core async context capture for async/await.This is the key implementation that fixes the PR objective. The
BUN_CONTEXTmacro approach cleanly handles the context wrapping without duplicating conditional logic throughout the function. TheInternalFieldTuple::createcall with 4 arguments at line 716 requires verification that this overload exists in the codebase.
| // Extract userContext and asyncContext from arguments[3] | ||
| // If it's an InternalFieldTuple: [userContext, asyncContext] | ||
| // Otherwise: it's userContext directly (legacy behavior) | ||
| JSValue contextArg = arguments[3]; | ||
| JSValue userContext = jsUndefined(); | ||
| JSValue asyncContext = jsUndefined(); | ||
|
|
||
| if (!contextArg.isEmpty() && contextArg.isCell()) { | ||
| if (auto* tuple = jsDynamicCast<InternalFieldTuple*>(contextArg)) { | ||
| userContext = tuple->getInternalField(0); | ||
| asyncContext = tuple->getInternalField(1); | ||
| } else { | ||
| userContext = contextArg; | ||
| } | ||
| } else if (!contextArg.isEmpty() && !contextArg.isUndefinedOrNull()) { | ||
| userContext = contextArg; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the context extraction logic.
The extraction logic is correct but has some redundancy. The isCell() check before jsDynamicCast<InternalFieldTuple*> is unnecessary since jsDynamicCast safely handles non-cell values.
♻️ Suggested simplification
- if (!contextArg.isEmpty() && contextArg.isCell()) {
- if (auto* tuple = jsDynamicCast<InternalFieldTuple*>(contextArg)) {
- userContext = tuple->getInternalField(0);
- asyncContext = tuple->getInternalField(1);
- } else {
- userContext = contextArg;
- }
- } else if (!contextArg.isEmpty() && !contextArg.isUndefinedOrNull()) {
- userContext = contextArg;
- }
+ if (!contextArg.isEmpty()) {
+ if (auto* tuple = jsDynamicCast<InternalFieldTuple*>(contextArg)) {
+ userContext = tuple->getInternalField(0);
+ asyncContext = tuple->getInternalField(1);
+ } else if (!contextArg.isUndefinedOrNull()) {
+ userContext = contextArg;
+ }
+ }🤖 Prompt for AI Agents
In `@Source/JavaScriptCore/runtime/JSMicrotask.cpp` around lines 967 - 983, The
context extraction logic for arguments[3] is redundant: remove the unnecessary
contextArg.isCell() check and rely directly on
jsDynamicCast<InternalFieldTuple*> to detect an InternalFieldTuple; keep the
existing fallback paths that set userContext = contextArg for non-tuple values
and handle undefined/null by leaving userContext and asyncContext as
jsUndefined(). Specifically, simplify the branch around contextArg, using
jsDynamicCast<InternalFieldTuple*> to populate userContext and asyncContext when
it returns non-null and otherwise assign userContext = contextArg when
contextArg is not undefined/null.
|
✅ Preview build completed Release: autobuild-preview-pr-135-8cbd8edf |
When Promise.resolve(InternalPromise) is called, instead of using the regular resolve() which triggers thenable resolution and calls user-visible .then(), use performPromiseThenWithInternalMicrotask to adopt the state internally. This preserves InternalPromise isolation while returning a regular Promise for instanceof checks.
|
/build-preview |
|
🔄 Preview build started Release tag: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Source/JavaScriptCore/runtime/JSPromise.cpp (1)
916-933: Consider refactoring to avoidgoto.The
goto createNewPromisepattern reduces readability. This concern was raised in a previous review suggesting restructuring with a boolean flag instead.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Source/JavaScriptCore/runtime/JSPromise.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/Source/JavaScriptCore/runtime/**/*.{h,cpp,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
Add or modify runtime functions within the JavaScriptCore runtime/ directory
Files:
Source/JavaScriptCore/runtime/JSPromise.cpp
**/Source/JavaScriptCore/**/*.{cpp,mm}
📄 CodeRabbit inference engine (CLAUDE.md)
Use dataLog() for printf-style debugging in JavaScriptCore code
Files:
Source/JavaScriptCore/runtime/JSPromise.cpp
Source/JavaScriptCore/**/*.{cpp,h}
📄 CodeRabbit inference engine (Source/JavaScriptCore/CLAUDE.md)
Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Files:
Source/JavaScriptCore/runtime/JSPromise.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/**/*.{cpp,h} : Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:28.399Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:28.399Z
Learning: Applies to **/Source/JavaScriptCore/runtime/**/*.{h,cpp,mm} : Add or modify runtime functions within the JavaScriptCore runtime/ directory
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/{runtime/JSModuleLoader.cpp,builtins/ModuleLoader.js} : Coordinate module loading changes across runtime/JSModuleLoader.cpp (C++) and builtins/ModuleLoader.js (JS)
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:28.399Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:28.399Z
Learning: Applies to **/Source/JavaScriptCore/heap/**/*.{h,cpp,mm} : Make heap/garbage-collection changes within the heap/ components
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/builtins/**/*.js : Prefer implementing and modifying JavaScript built-ins in builtins/*.js before changing C++
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp : Define new or modified JavaScript global functions using the JSC_DEFINE_HOST_FUNCTION macro
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
📚 Learning: 2025-10-01T01:58:56.267Z
Learnt from: CR
Repo: oven-sh/WebKit PR: 0
File: Source/JavaScriptCore/CLAUDE.md:0-0
Timestamp: 2025-10-01T01:58:56.267Z
Learning: Applies to Source/JavaScriptCore/runtime/JSGlobalObject.cpp : Register new global functions in runtime/JSGlobalObject.cpp after defining them
Applied to files:
Source/JavaScriptCore/runtime/JSPromise.cpp
🔇 Additional comments (9)
Source/JavaScriptCore/runtime/JSPromise.cpp (9)
47-49: LGTM!The conditional include is properly guarded with
USE(BUN_JSC_ADDITIONS)as required by the coding guidelines.
277-290: LGTM!The async context capture pattern is correctly implemented with proper
USE(BUN_JSC_ADDITIONS)guards. TheInternalFieldTuplestructure with[userContext, asyncContext]layout is consistent with the pattern used inperformPromiseThenWithContext.
295-319: LGTM!All three promise status cases (Pending, Rejected, Fulfilled) consistently pass the captured async context to their respective handlers. The
USE(BUN_JSC_ADDITIONS)guards are correctly applied with fallback tojsUndefined()for non-Bun builds.
325-367: LGTM!The function correctly handles the case where a
userContextis explicitly provided. The tuple creation condition!userContext.isUndefinedOrNull() || !asyncContext.isUndefined()appropriately covers both scenarios where either the user-provided context or the async context needs preservation.
458-468: LGTM!The async context is correctly captured and passed to the
PromiseResolveThenableJobFastmicrotask for the fast-path resolution of already-resolved promises.
491-499: LGTM!Consistent async context propagation for the general thenable resolution path via
PromiseResolveThenableJob.
692-701: LGTM!The conditional logic correctly distinguishes between reactions with async context (Bun) and those without, with the
ASSERTin the non-Bun path ensuring the invariant is maintained.
938-948: VerifyInternalMicrotask::PromiseResolveWithoutHandlerJobenum value exists.This code path introduces usage of
InternalMicrotask::PromiseResolveWithoutHandlerJobto adopt state from anInternalPromisewithout triggering user-visible.then()handlers. The approach is sound for preserving InternalPromise isolation while returning a regular Promise, and the code is properly guarded withUSE(BUN_JSC_ADDITIONS). However, the existence and correctness of this specific enum value needs confirmation.
708-758: Verify theInternalFieldTuple::create4-argument overload exists.The code uses
InternalFieldTuple::create(vm, globalObject->internalFieldTupleStructure(), context, asyncContext)(Line 716), which is a different constructor signature than used elsewhere (e.g., Lines 284, 340) whereInternalFieldTuple::create(vm, structure)is called followed byputInternalField. Ensure this 4-argument factory method exists and correctly initializes both internal fields.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| #if USE(BUN_JSC_ADDITIONS) | ||
| void JSPromise::performPromiseThenWithContext(VM& vm, JSGlobalObject* globalObject, JSValue onFulfilled, JSValue onRejected, JSValue promiseOrCapability, JSValue context) | ||
| void JSPromise::performPromiseThenWithContext(VM& vm, JSGlobalObject* globalObject, JSValue onFulfilled, JSValue onRejected, JSValue promiseOrCapability, JSValue userContext) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the InternalFieldTuple layout convention.
The code consistently uses InternalFieldTuple with slot 0 for user/generator context and slot 1 for async context across multiple locations (Lines 285-286, 341-342, 716). Consider adding a brief comment or constant definitions to formalize this layout convention, which would improve maintainability and reduce the risk of slot index errors in future changes.
💡 Example documentation
// InternalFieldTuple layout for async context propagation:
// Slot 0: userContext or generatorContext (from caller)
// Slot 1: Bun asyncContext (from globalObject->m_asyncContextData)
constexpr unsigned ContextTupleUserContextSlot = 0;
constexpr unsigned ContextTupleAsyncContextSlot = 1;|
✅ Preview build completed Release: autobuild-preview-pr-135-d2c9cd9e |
|
/build-preview |
|
🔄 Preview build started Release tag: |
|
✅ Preview build completed Release: autobuild-preview-pr-135-814e1b6e |
|
/build-preview |
|
🔄 Preview build started Release tag: |
|
✅ Preview build completed Release: autobuild-preview-pr-135-b2edf332 |
Summary
When an async function awaits a promise, Bun's AsyncLocalStorage context was being lost. This happened because the upstream WebKit change moved Microtask handling to C++, and the
InternalMicrotask::AsyncFunctionResumecase was not restoring Bun's async context.Problem
The test
test-diagnostics-channel-tracing-channel-promise-run-stores.jswas failing becausestore.getStore()returnedundefinedafterawait setTimeout(1)instead of the expected context.Fix
JSPromise.cpp (
resolveWithInternalMicrotaskForAsyncAwait):InternalFieldTupleJSMicrotask.cpp (
AsyncFunctionResumecase):InternalFieldTupleTesting
All related tests pass:
test-diagnostics-channel-tracing-channel-promise-run-stores.js✅test-async-local-storage-contexts.js✅test-diagnostics-channel-tracing-channel-callback-run-stores.js✅test-diagnostics-channel-tracing-channel-sync-run-stores.js✅Related