Update WebKit#26381
Conversation
|
Updated 3:51 AM PT - Jan 25th, 2026
❌ @dylan-conway, your commit e6af6de has 2 failures in
🧪 To try this PR locally: bunx bun-pr 26381That installs a local version of the PR into your bun-26381 --bun |
WalkthroughReplaces CatchScope with TopExceptionScope across C++, Zig, and generated bindings; removes CatchScope binding and adds TopExceptionScope binding; updates Zig exports and many callsites; adjusts mimalloc compile guards to USE_BUN_MIMALLOC; updates WEBKIT_VERSION fallback and adds Windows bmalloc link flags in CMake. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/BunPlugin.cpp (1)
649-654: UsetopExceptionScopefor the exception check/clear.This block creates a
TopExceptionScopebut checks/clears the outerscope(THROW_SCOPE), leaving the new scope unused. The exception fromobject->getshould be checked against the localtopExceptionScopeinstead. This pattern is already used correctly elsewhere in the same file (lines 546–549).🛠️ Suggested fix
- if (scope.exception()) [[unlikely]] { - (void)scope.tryClearException(); + if (topExceptionScope.exception()) [[unlikely]] { + (void)topExceptionScope.tryClearException(); value = jsUndefined(); }
🤖 Fix all issues with AI agents
In `@src/bun.js/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 1662-1665: The conversion pathValue.toWTFString is using a
separate DECLARE_TOP_EXCEPTION_SCOPE/topExceptionScope and a dead (unreachable)
topExceptionScope.tryClearException() call; remove the
DECLARE_TOP_EXCEPTION_SCOPE and the tryClearException() call and instead use the
existing ThrowScope already declared earlier in the function (the ThrowScope
variable used for the rest of the function) for exception handling, replacing
RETURN_IF_EXCEPTION(topExceptionScope, ...) with the equivalent
RETURN_IF_EXCEPTION using that ThrowScope so exception handling is consistent
and the dead code is eliminated.
In `@src/bun.js/modules/NodeProcessModule.h`:
- Around line 36-41: Add the C++ branch prediction attribute to the exception
check to match other modules: change the if condition that checks
topExceptionScope.exception() in the DECLARE_TOP_EXCEPTION_SCOPE block (the code
calling process->get and assigning jsUndefined() on exception) to use the
attribute form (if ([[unlikely]] topExceptionScope.exception()) { ... }),
leaving the exception handling and tryClearException call untouched.
| auto topExceptionScope = DECLARE_TOP_EXCEPTION_SCOPE(vm); | ||
| String path = pathValue.toWTFString(lexicalGlobalObject); | ||
| RETURN_IF_EXCEPTION(catchScope, JSValue::encode(jsUndefined())); | ||
| (void)catchScope.tryClearException(); | ||
| RETURN_IF_EXCEPTION(topExceptionScope, JSValue::encode(jsUndefined())); | ||
| (void)topExceptionScope.tryClearException(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git clone --depth 1 https://github.com/oven-sh/bun.git 2>&1 | head -20Repository: oven-sh/bun
Length of output: 138
🏁 Script executed:
# First, let's look at the file and the specific lines in context
sed -n '1630,1680p' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cppRepository: oven-sh/bun
Length of output: 152
🏁 Script executed:
# Search for how TopExceptionScope is used elsewhere in this file
rg 'TopExceptionScope|tryClearException' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp -B2 -A2Repository: oven-sh/bun
Length of output: 149
🏁 Script executed:
# Check return value patterns in this file - how exceptions are handled
rg 'RETURN_IF_EXCEPTION.*JSValue::encode|RETURN_IF_EXCEPTION.*\{\}' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp -A1 -B1Repository: oven-sh/bun
Length of output: 149
🏁 Script executed:
# Look for TopExceptionScope usage patterns across bindings
rg 'DECLARE_TOP_EXCEPTION_SCOPE|tryClearException' /tmp/bun/src/bun.js/bindings/ -l | head -10Repository: oven-sh/bun
Length of output: 124
🏁 Script executed:
find /tmp/bun -name "JSSQLStatement.cpp" -type f 2>/dev/nullRepository: oven-sh/bun
Length of output: 109
🏁 Script executed:
# Also check what exists in the bindings directory
ls -la /tmp/bun/src/bun.js/bindings/ 2>&1 | head -20Repository: oven-sh/bun
Length of output: 1390
🏁 Script executed:
# Try a broader search
fd "JSSQLStatement" /tmp/bun 2>/dev/nullRepository: oven-sh/bun
Length of output: 162
🏁 Script executed:
sed -n '1630,1680p' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cppRepository: oven-sh/bun
Length of output: 2031
🏁 Script executed:
# Check the return patterns in this function and nearby functions
sed -n '1600,1700p' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp | grep -n 'RETURN_IF_EXCEPTION\|DECLARE_THROW_SCOPE\|DECLARE_TOP_EXCEPTION_SCOPE\|tryClearException'Repository: oven-sh/bun
Length of output: 216
🏁 Script executed:
# Look for other uses of TopExceptionScope and tryClearException in the bindings
rg 'DECLARE_TOP_EXCEPTION_SCOPE' /tmp/bun/src/bun.js/bindings/ -A5 -B1 | head -50Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check return value patterns in this specific file
rg 'RETURN_IF_EXCEPTION' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp | head -20Repository: oven-sh/bun
Length of output: 1012
🏁 Script executed:
# Search for DECLARE_CATCH_SCOPE documentation and usage patterns
rg 'DECLARE_CATCH_SCOPE' /tmp/bun/src/bun.js/bindings/ -B2 -A8 | head -80Repository: oven-sh/bun
Length of output: 6677
🏁 Script executed:
# Check the WebKit headers to understand CatchScope semantics
find /tmp/bun -name "*.h" -path "*/JSC/*" -o -name "*.h" -path "*/webcore/*" | xargs grep -l "DECLARE_CATCH_SCOPE\|tryClearException" 2>/dev/null | head -5Repository: oven-sh/bun
Length of output: 114
🏁 Script executed:
# Let's look more broadly at how DECLARE_CATCH_SCOPE + RETURN_IF_EXCEPTION + tryClearException pattern is used
rg 'DECLARE_CATCH_SCOPE.*\n.*RETURN_IF_EXCEPTION.*\n.*tryClearException' /tmp/bun/src/bun.js/bindings/ -U | head -100Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Search for the exact pattern to see if it's used elsewhere
rg -A2 'DECLARE_CATCH_SCOPE' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cppRepository: oven-sh/bun
Length of output: 232
🏁 Script executed:
# Let's verify the actual line numbers and context from the file
sed -n '1658,1675p' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp | cat -nRepository: oven-sh/bun
Length of output: 810
🏁 Script executed:
# Search for documentation on CatchScope vs ThrowScope
rg 'DECLARE_CATCH_SCOPE|DECLARE_THROW_SCOPE' /tmp/bun -A1 | grep -E '(CATCH|THROW)' | head -30Repository: oven-sh/bun
Length of output: 2825
🏁 Script executed:
# Look at how the function uses both scopes
sed -n '1628,1680p' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp | cat -nRepository: oven-sh/bun
Length of output: 2405
🏁 Script executed:
# Search for WebKit documentation on CatchScope behavior
rg 'class CatchScope|struct CatchScope' /tmp/bun -A5 | head -40Repository: oven-sh/bun
Length of output: 762
🏁 Script executed:
# Look at the semantics of RETURN_IF_EXCEPTION with CatchScope
rg 'RETURN_IF_EXCEPTION.*catchScope' /tmp/bun/src/bun.js/bindings/ -B3 -A3 | head -60Repository: oven-sh/bun
Length of output: 2216
🏁 Script executed:
# Check if there are other similar patterns in the codebase
rg 'DECLARE_CATCH_SCOPE.*\n.*toWTFString' /tmp/bun -U -B2 -A5 | head -80Repository: oven-sh/bun
Length of output: 2350
🏁 Script executed:
# Look at what WebKit exception scopes do - search for defines
rg 'define.*DECLARE_CATCH_SCOPE|define.*RETURN_IF_EXCEPTION' /tmp/bun -A2 | head -40Repository: oven-sh/bun
Length of output: 3365
🏁 Script executed:
# Look for the RETURN_IF_EXCEPTION macro definition to understand its semantics
rg -i 'define.*RETURN_IF_EXCEPTION' /tmp/bun --type-add 'cpp:include,*.h' -A3 | head -30Repository: oven-sh/bun
Length of output: 3265
🏁 Script executed:
# Check what tryClearException does - search for its definition/usage context
rg 'tryClearException|clearExceptionExceptTermination' /tmp/bun/src/bun.js/bindings/ -B2 -A2 | head -80Repository: oven-sh/bun
Length of output: 6483
🏁 Script executed:
# Look at ErrorStackTrace.cpp in more detail to see the correct pattern
sed -n '40,60p' /tmp/bun/src/bun.js/bindings/ErrorStackTrace.cppRepository: oven-sh/bun
Length of output: 818
🏁 Script executed:
# Search for the actual RETURN_IF_EXCEPTION macro definition from WebKit
rg 'define RETURN_IF_EXCEPTION' /tmp/bun --type-add 'cpp:include,*.h' -A5Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look at how other similar patterns handle exceptions - look for the correct pattern
sed -n '45,65p' /tmp/bun/src/bun.js/bindings/ErrorStackTrace.cppRepository: oven-sh/bun
Length of output: 850
🏁 Script executed:
# Check if there's a pattern where RETURN_IF_EXCEPTION is used with a CatchScope that's followed by anything
rg 'RETURN_IF_EXCEPTION.*catchScope' /tmp/bun/src/bun.js/bindings/ -A5 -B2Repository: oven-sh/bun
Length of output: 2614
🏁 Script executed:
# Let's look at the full context of ErrorStackTrace to understand the correct pattern
sed -n '40,70p' /tmp/bun/src/bun.js/bindings/ErrorStackTrace.cpp | cat -nRepository: oven-sh/bun
Length of output: 1330
🏁 Script executed:
# Search for WebKit macro definitions - they might be in JSC headers
find /tmp/bun -name "*.h" | xargs grep -l "RETURN_IF_EXCEPTION" | head -5Repository: oven-sh/bun
Length of output: 298
🏁 Script executed:
# Look at the git history or comments to understand the intent
git -C /tmp/bun log --oneline -n 20 -- src/bun.js/bindings/sqlite/JSSQLStatement.cpp 2>/dev/null | head -5Repository: oven-sh/bun
Length of output: 144
🏁 Script executed:
# Let's look at the control flow more carefully - see what happens after line 1666
sed -n '1663,1700p' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp | cat -nRepository: oven-sh/bun
Length of output: 1697
🏁 Script executed:
# Let me check the context before and after this snippet more carefully
sed -n '1630,1670p' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp | cat -nRepository: oven-sh/bun
Length of output: 2053
🏁 Script executed:
# Look at how similar conversions use exception handling in this file
rg 'toWTFString|toInt32' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp -B2 -A2 | head -50Repository: oven-sh/bun
Length of output: 1869
🏁 Script executed:
# Check what exception scopes are available and understand the difference
rg 'DECLARE_THROW_SCOPE|DECLARE_CATCH_SCOPE' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp -B1 -A8Repository: oven-sh/bun
Length of output: 13093
🏁 Script executed:
# Let me check what the actual behavior should be - search for comments explaining CatchScope vs ThrowScope
rg 'CatchScope.*exception|ThrowScope.*exception' /tmp/bun/src/bun.js/bindings/ -B2 -A2 | head -60Repository: oven-sh/bun
Length of output: 4981
🏁 Script executed:
# Look at the full pattern in ErrorStackTrace to see how it's supposed to work
sed -n '80,110p' /tmp/bun/src/bun.js/bindings/ErrorStackTrace.cppRepository: oven-sh/bun
Length of output: 1057
🏁 Script executed:
# Check if there's a specific reason to use JSValue::encode(jsUndefined()) vs {}
rg 'RETURN_IF_EXCEPTION.*JSValue::encode\(jsUndefined' /tmp/bun/src/bun.js/bindings/ -B2Repository: oven-sh/bun
Length of output: 2833
🏁 Script executed:
# Search for the definition and documentation of RETURN_IF_EXCEPTION behavior
rg 'RETURN_IF_EXCEPTION|Exception\(' /tmp/bun/src/bun.js/bindings/ErrorCode.h -A2 -B1 | head -40Repository: oven-sh/bun
Length of output: 406
🏁 Script executed:
# Look at the immediate context - does execution ever reach line 36?
sed -n '1661,1700p' /tmp/bun/src/bun.js/bindings/sqlite/JSSQLStatement.cpp | cat -nRepository: oven-sh/bun
Length of output: 1759
🏁 Script executed:
# Compare: how is the exception-suppression pattern supposed to work?
sed -n '520,545p' /tmp/bun/src/bun.js/bindings/ErrorStackTrace.cppRepository: oven-sh/bun
Length of output: 609
Problematic exception handling with unreachable tryClearException() call and inconsistent scope usage.
The tryClearException() call on line 1665 is dead code—if toWTFString() throws, RETURN_IF_EXCEPTION returns immediately and this line is never reached; if it succeeds, there's no exception to clear. Additionally, this function already uses a ThrowScope (declared at line 1632) throughout the rest of its logic. Using a separate CatchScope for just this one conversion is inconsistent with the rest of the function's exception handling pattern. Update to use the existing scope for consistency:
- auto catchScope = DECLARE_CATCH_SCOPE(vm);
String path = pathValue.toWTFString(lexicalGlobalObject);
- RETURN_IF_EXCEPTION(catchScope, JSValue::encode(jsUndefined()));
- (void)catchScope.tryClearException();
+ RETURN_IF_EXCEPTION(scope, {});🤖 Prompt for AI Agents
In `@src/bun.js/bindings/sqlite/JSSQLStatement.cpp` around lines 1662 - 1665, The
conversion pathValue.toWTFString is using a separate
DECLARE_TOP_EXCEPTION_SCOPE/topExceptionScope and a dead (unreachable)
topExceptionScope.tryClearException() call; remove the
DECLARE_TOP_EXCEPTION_SCOPE and the tryClearException() call and instead use the
existing ThrowScope already declared earlier in the function (the ThrowScope
variable used for the rest of the function) for exception handling, replacing
RETURN_IF_EXCEPTION(topExceptionScope, ...) with the equivalent
RETURN_IF_EXCEPTION using that ThrowScope so exception handling is consistent
and the dead code is eliminated.
| auto topExceptionScope = DECLARE_TOP_EXCEPTION_SCOPE(vm); | ||
| JSValue result = process->get(globalObject, entry); | ||
| if (catchScope.exception()) { | ||
| if (topExceptionScope.exception()) { | ||
| result = jsUndefined(); | ||
| (void)catchScope.tryClearException(); | ||
| (void)topExceptionScope.tryClearException(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TopExceptionScope definition and usage patterns
echo "=== DECLARE_TOP_EXCEPTION_SCOPE macro definition ==="
rg "DECLARE_TOP_EXCEPTION_SCOPE" --type cpp --type-add 'cpp:*.h' -B 2 -A 5 | head -60
echo ""
echo "=== TopExceptionScope class/struct definition ==="
rg "class TopExceptionScope|struct TopExceptionScope" --type cpp --type-add 'cpp:*.h' -B 2 -A 15 | head -80
echo ""
echo "=== Check if this file previously used CatchScope ==="
git log --oneline -p src/bun.js/modules/NodeProcessModule.h 2>/dev/null | grep -A 5 -B 5 "CatchScope\|TopExceptionScope" | head -50Repository: oven-sh/bun
Length of output: 6147
🏁 Script executed:
#!/bin/bash
# Check similar exception handling patterns with TopExceptionScope in the codebase
echo "=== Other uses of topExceptionScope.exception() pattern ==="
rg "topExceptionScope\.exception\(\)" --type cpp --type-add 'cpp:*.h' -B 3 -A 3 | head -80
echo ""
echo "=== Other uses of tryClearException() ==="
rg "tryClearException\(\)" --type cpp --type-add 'cpp:*.h' -B 2 -A 2 | head -60
echo ""
echo "=== Verify if this is inside a loop and context ==="
sed -n '30,50p' src/bun.js/modules/NodeProcessModule.hRepository: oven-sh/bun
Length of output: 11065
🏁 Script executed:
#!/bin/bash
# Check if [[unlikely]] should be present in NodeProcessModule.h
echo "=== ObjectModule.cpp exception check ==="
rg -A 4 "scope\.exception\(\)" src/bun.js/modules/ObjectModule.cpp
echo ""
echo "=== BunTestModule.h exception check ==="
rg -A 4 "topExceptionScope\.exception\(\)" src/bun.js/modules/BunTestModule.h | head -20
echo ""
echo "=== NodeProcessModule.h exception check (under review) ==="
sed -n '36,42p' src/bun.js/modules/NodeProcessModule.hRepository: oven-sh/bun
Length of output: 994
🏁 Script executed:
#!/bin/bash
# Search for all TopExceptionScope patterns to see [[unlikely]] usage
echo "=== All TopExceptionScope.exception() patterns with context ==="
rg "topExceptionScope\.exception\(\)" --type cpp --type-add 'cpp:*.h' -B 1 -A 1 | grep -E "(topExceptionScope|unlikely)"Repository: oven-sh/bun
Length of output: 2616
Add [[unlikely]] annotation for consistency with similar property enumeration patterns.
The exception handling is correct and sound. TopExceptionScope with graceful fallback to jsUndefined() is the established pattern across the codebase for property enumeration. However, for consistency with identical patterns in ObjectModule.cpp and BunTestModule.h, add the [[unlikely]] hint:
Suggested change
auto topExceptionScope = DECLARE_TOP_EXCEPTION_SCOPE(vm);
JSValue result = process->get(globalObject, entry);
- if (topExceptionScope.exception()) {
+ if (topExceptionScope.exception()) [[unlikely]] {
result = jsUndefined();
(void)topExceptionScope.tryClearException();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto topExceptionScope = DECLARE_TOP_EXCEPTION_SCOPE(vm); | |
| JSValue result = process->get(globalObject, entry); | |
| if (catchScope.exception()) { | |
| if (topExceptionScope.exception()) { | |
| result = jsUndefined(); | |
| (void)catchScope.tryClearException(); | |
| (void)topExceptionScope.tryClearException(); | |
| } | |
| auto topExceptionScope = DECLARE_TOP_EXCEPTION_SCOPE(vm); | |
| JSValue result = process->get(globalObject, entry); | |
| if (topExceptionScope.exception()) [[unlikely]] { | |
| result = jsUndefined(); | |
| (void)topExceptionScope.tryClearException(); | |
| } |
🤖 Prompt for AI Agents
In `@src/bun.js/modules/NodeProcessModule.h` around lines 36 - 41, Add the C++
branch prediction attribute to the exception check to match other modules:
change the if condition that checks topExceptionScope.exception() in the
DECLARE_TOP_EXCEPTION_SCOPE block (the code calling process->get and assigning
jsUndefined() on exception) to use the attribute form (if ([[unlikely]]
topExceptionScope.exception()) { ... }), leaving the exception handling and
tryClearException call untouched.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bun.js/bindings/ZigGlobalObject.cpp (2)
1557-1612: Consider removing unused exception scope.The
scopevariable is declared but never used in this function. Exception handling is done throughexceptionPtrfromprofiledCall. If the scope is only needed for JSC's internal exception tracking mechanism, a comment clarifying its purpose would be helpful.
1614-1664: Same observation asjsFunctionPerformMicrotaskabove — thescopevariable is declared but not used; exception handling is viaexceptionPtr.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmake/tools/SetupWebKit.cmake`:
- Around line 4-6: The WEBKIT_VERSION value was changed to commit
f151e5c43c3546da23c5c1e709452da25d47e168 but the PR description mentions
5b6a0ac49b6f0f505b7165850f1318a622652123; please confirm which commit is
intended and either (a) update the WEBKIT_VERSION constant in SetupWebKit.cmake
to the correct commit hash if the description was right (look for the
WEBKIT_VERSION set(...) line) or (b) update the PR description to reference
f151e5c43c3 if the newer commit was intentionally chosen so the code and PR
message match. Ensure the chosen commit hash and any explanatory text are
consistent.
Remove bmalloc.lib from Windows link targets in both DEBUG and RELEASE builds. libpas was originally disabled on Windows in PR #20931 due to stability issues (pas_assertion_failed crashes after extended runtime), but was inadvertently re-enabled in PR #26381 ("Update WebKit"). No changes were made to fix the underlying libpas stability issues on Windows between the disable and re-enable, so the same class of crashes (documented in #21192, #24576, #20505) resurfaced. Fixes #26982 Co-Authored-By: Claude <noreply@anthropic.com>
bmalloc/libpas was originally disabled on Windows in PR #20931 due to assertion failures (pas_assertion_failed) causing crashes. It was inadvertently re-enabled in PR #26381 (WebKit update). This removes bmalloc.lib from Windows link targets to restore the intended behavior. Fixes #26985 Fixes #26982 Co-Authored-By: Claude <noreply@anthropic.com>
bmalloc/libpas has a documented history of instability on Windows, causing crashes during garbage collection under memory pressure. It was originally disabled in PR #20931, accidentally re-enabled in PR #26381 (WebKit update), and fixed in commits 91949f7/9b87e4a3f3 — but those fixes were never merged to main. The recent CMake-to-TypeScript build migration also carried over the unconditional bmalloc linking. This removes bmalloc.lib from Windows link targets in both the legacy CMake build and the new TypeScript (ninja) build system. Fixes #28097 Fixes #26985 Fixes #26982 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
### What does this PR do? Updates WebKit to oven-sh/WebKit@5b6a0ac ### How did you verify your code works? --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: Claude Bot <claude-bot@bun.sh> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
What does this PR do?
Updates WebKit to oven-sh/WebKit@5b6a0ac
How did you verify your code works?