inspector: bump WebKit so Debugger.disable is idempotent on asserts builds - #36851
inspector: bump WebKit so Debugger.disable is idempotent on asserts builds#36851robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 4:08 PM PT - Aug 3rd, 2026
✅ @robobun, your commit 5561f3f7526bbdcb4dd916cb459ebfe1d76f52b1 passed in 🧪 To try this PR locally: bunx bun-pr 36851That installs a local version of the PR into your bun-36851 --bun |
There was a problem hiding this comment.
I didn't find any bugs, but this pins WEBKIT_VERSION to an ephemeral preview release (autobuild-preview-pr-385-...) that the description notes will be deleted once oven-sh/WebKit#385 merges — a maintainer should sign off on landing against a preview tag vs. waiting for the merged sha.
What was reviewed:
prebuiltUrl/prebuiltDestDirhandling of theautobuild-prefix — both branch on it correctly, so the tag resolves and the cache key is distinct.- New test: error/close events are wired to settle pending
send()promises, stderr is drained concurrently withchild.exited, andchild.kill()runs fromfinallyso a SIGABRT mid-sequence can't hang the test. skipIf(!isDebug && !isASAN)matches the assert beingASSERT_ENABLED-only.
Extended reasoning...
Overview
Two files: scripts/build/deps/webkit.ts swaps WEBKIT_VERSION from commit sha 34c01d13... to the preview tag autobuild-preview-pr-385-3a8fe8a1, and test/cli/inspect/inspect.test.ts gains a ~90-line test that connects to --inspect=127.0.0.1:0 over WebSocket and issues a Debugger.disable → enable → disable → disable → enable → disable sequence, asserting six {result: {}} replies and no SIGABRT. The actual code fix lives in oven-sh/WebKit#385, not here.
Security risks
None. The change is a dependency version pin plus a test that spawns a local subprocess bound to 127.0.0.1:0 and talks to it over a loopback WebSocket. No new user-facing surface, no auth/crypto/permissions paths.
Level of scrutiny
High, because WEBKIT_VERSION selects the JavaScriptCore build for every configuration. Even though the upstream diff is described as a one-line early return, pinning main to a preview release tag is a build-infrastructure decision: the PR description itself says preview releases are deleted when the source PR merges, so this value will need to be repointed. Whether to land now against the preview or wait for the merged sha is a maintainer call — see also the upgrade-webkit skill/process this repo uses for WebKit bumps.
Other factors
The test itself is solid against the review checklist: it awaits observable conditions (URL scraped from stderr, per-id reply promises), wires close/error to a shared fail() that resolves every pending sender so an abort mid-sequence can't deadlock, drains stderr in parallel with child.exited, kills the child from a finally, and asserts exact reply objects rather than counts. The rejectUrl after the drain loop and the stderrDrained.catch(() => {}) prevent an unhandled rejection when the child is killed after the URL is already resolved. prebuiltUrl() already handles the autobuild- prefix (skips prepending it) and prebuiltDestDir() uses the full tag as the cache key, so the non-sha version string flows through correctly.
No prior reviews from me on this PR; deferring so a human can confirm the preview-tag pin is acceptable to merge.
…uilds
InspectorDebuggerAgent::disable() called internalDisable() unconditionally,
so a Debugger.disable on an already-disabled agent (or before any enable)
re-ran m_debugger.setClient(nullptr) and tripped ASSERT(!!m_client != !!client)
in JSC::Debugger::setClient on debug/ASAN builds. Release answers {} and
continues. Frontends send Debugger.disable liberally on teardown (detach +
close, or two attached tools), so this is an ordinary sequence.
oven-sh/WebKit#385 adds the same if (!enabled()) return { }; guard that
enable(), willDestroyFrontendAndBackend(), InspectorHeapAgent::disable()
and InspectorConsoleAgent::disable() already have.
ec9dfac to
5561f3f
Compare
|
Rebased onto 2ed8ed6 after #36794 landed and moved On the preview-tag question: this follows the same pattern as #35754 / #36472 / #36361 and the other WebKit bump PRs in the queue; the body notes that |
A
Debugger.disablesent tobun --inspecton an already-disabled Debugger agent (or before anyDebugger.enable) aborts an asserts build:Release just answers
{}and continues. Reproduces over both the WebSocket and--inspect=unix:transports; the--inspectserver forwards messages straight to JSC's backend dispatcher, so the crash is in WebKit. Frontends sendDebugger.disableliberally on teardown (detach + close, or two attached tools each disabling), so this is an ordinary sequence rather than an exotic one.Cause
InspectorDebuggerAgent::disable()callsinternalDisable(false)unconditionally.internalDisable()runsm_debugger.setClient(nullptr), andJSC::Debugger::setClientasserts the client is transitioning (!!m_client != !!client). Whenm_clientis already null, either because the agent was never enabled or becausedisable()already ran, the assert fails.enable()guards withif (enabled()) return makeUnexpected(...);willDestroyFrontendAndBackend()guardsinternalDisable()withif (enabled());InspectorHeapAgent::disable()andInspectorConsoleAgent::disable()both early-return when already disabled.InspectorDebuggerAgent::disable()is the odd one out.Fix
oven-sh/WebKit#385 adds
if (!enabled()) return { };toInspectorDebuggerAgent::disable(), mirroring the other agents. Branched offe6e37cda21(currentWEBKIT_VERSION) so the preview bump is minimal.This PR bumps
WEBKIT_VERSIONto the preview build and addstest/cli/inspect/inspect-debugger-disable.test.tswhich drivesdisable → enable → disable → disable → enable → disableover the--inspectWebSocket and asserts all six replies arrive and the inspectee does not SIGABRT. The test istest.skipIf(!isDebug && !isASAN)since the assert isASSERT_ENABLED-only.Verification
test/js/node/inspector/inspector.test.ts23/23 andtest/js/bun/jsc/webkit-upgrade-3722912f.test.ts5/5 with the bump.Once oven-sh/WebKit#385 merges,
WEBKIT_VERSIONshould be repointed at the merged main sha (preview releases are deleted at that point).[decide:webkit] gate passed · iteration 2 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 2
evidence per changed file