Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1820,6 +1820,10 @@ pub struct RuntimeHooks {
/// never lazily created. Called from `WebWorker::shutdown` / `global_exit`
/// right after `close_all_socket_groups`.
pub close_dns_for_terminate: fn(),
/// `ObjectURLRegistry::revoke_all_for_context` — drops every blob URL a
/// worker registered, called from `WebWorker::shutdown`. Registry lives in
/// `bun_runtime::webcore`.
Comment on lines +1823 to +1825

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

pub revoke_object_urls_for_context: fn(context_id: i32),
}

/// Canonical `EventLoopCtx` vtable for a `*mut VirtualMachine` owner — the JS
Expand Down
3 changes: 3 additions & 0 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1251,6 +1251,9 @@ impl WebWorker {
vm.on_exit();
if let Some(hooks) = runtime_hooks() {
(hooks.cron_clear_all_teardown)(vm);
// Before `dispatchExit` so the parent never observes `exit`
// with this worker's blob URLs still live.
Comment on lines +1254 to +1255

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

(hooks.revoke_object_urls_for_context)(self.execution_context_id as i32);
// Drain `TimeoutObject`s from this worker's timer heap before
// `close_all_socket_groups` / `WebWorker__teardownJSCVM` so
// their heap nodes are unlinked while `runtime_state` and the
Expand Down
7 changes: 7 additions & 0 deletions src/runtime/jsc_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1274,6 +1274,12 @@ fn has_blob_url(blob_id: &[u8]) -> bool {
crate::webcore::object_url_registry::ObjectURLRegistry::singleton().has(blob_id)
}

/// `WebCore.ObjectURLRegistry.singleton().revoke_all_for_context(context_id)`.
fn revoke_object_urls_for_context(context_id: i32) {
crate::webcore::object_url_registry::ObjectURLRegistry::singleton()
.revoke_all_for_context(context_id)
}

/// `Response::get_blob_without_call_frame` /
/// `Request::get_blob_without_call_frame`. Downcasts
/// `value` to a `Response`/`Request` (whose data shapes + `BodyMixin` impl live
Expand Down Expand Up @@ -1487,6 +1493,7 @@ pub(crate) static __BUN_RUNTIME_HOOKS: RuntimeHooks = RuntimeHooks {
retroactively_report_discovered_tests,
cancel_all_timers,
close_dns_for_terminate,
revoke_object_urls_for_context,
};

// ════════════════════════════════════════════════════════════════════════════
Expand Down
20 changes: 18 additions & 2 deletions src/runtime/webcore/ObjectURLRegistry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ impl Default for ObjectURLRegistry {

pub struct Entry {
blob: Blob,
/// Registering realm; see [`ObjectURLRegistry::revoke_all_for_context`].
context_id: i32,
}

// `Entry` is auto-`Send`: its sole field is `Blob`, which already asserts
Expand All @@ -38,9 +40,10 @@ const _: fn() = || {
};

impl Entry {
pub fn init(blob: &Blob) -> Box<Entry> {
pub fn init(blob: &Blob, context_id: i32) -> Box<Entry> {
Box::new(Entry {
blob: blob.dupe_with_content_type(true),
context_id,
})
}
}
Expand All @@ -55,7 +58,7 @@ impl Drop for Entry {
impl ObjectURLRegistry {
pub fn register(&self, vm: &mut VirtualMachine, blob: &Blob) -> UUID {
let uuid = vm.rare_data().next_uuid();
let entry = Entry::init(blob);
let entry = Entry::init(blob, vm.initial_script_execution_context_identifier);

self.map.lock().insert(uuid.bytes, entry);
uuid
Expand Down Expand Up @@ -91,6 +94,19 @@ impl ObjectURLRegistry {
let _ = self.map.lock().remove(&uuid.bytes);
}

/// Remove every entry registered by `context_id` (worker teardown).
pub fn revoke_all_for_context(&self, context_id: i32) {
let mut map = self.map.lock();
let keys: Vec<[u8; 16]> = map
.iter()
.filter(|(_, e)| e.context_id == context_id)
.map(|(k, _)| *k)
.collect();
for k in keys {
let _ = map.remove(&k);
}
}

pub fn has(&self, pathname: &[u8]) -> bool {
let Some(uuid) = uuid_from_pathname(pathname) else {
return false;
Expand Down
36 changes: 36 additions & 0 deletions test/js/web/workers/worker_blob.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { expect, test } from "bun:test";
import { resolveObjectURL } from "node:buffer";
import { Worker as NodeWorker } from "node:worker_threads";

test("Worker from a Blob", async () => {
const worker = new Worker(
Expand Down Expand Up @@ -124,3 +126,37 @@ test("Worker on a revoked blob still works", async () => {

expect(revoked).toBe("revoked.");
});

test("Blob URLs created inside a Worker are revoked when the worker exits", async () => {
const worker = new NodeWorker(
`const { parentPort } = require("node:worker_threads");
const u8 = new Uint8Array(1024).fill(7);
parentPort.postMessage(URL.createObjectURL(new Blob([u8])));`,
{ eval: true },
);
const url = await new Promise<string>(resolve => worker.once("message", resolve));
await new Promise(resolve => worker.once("exit", resolve));
Comment on lines +137 to +138

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: both new tests await new Promise(resolve => worker.once("message", resolve)) without wiring worker.once("error", reject). If a regression breaks Worker eval startup or URL.createObjectURL inside the worker, message never fires and the test hangs to the file timeout instead of failing fast with the actual error. Same shape at line 152. REVIEW.md: "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise."

Extended reasoning...

What the issue is

Both new tests added in this PR wait for the worker's first message with a resolve-only promise:

// line 137
const url = await new Promise<string>(resolve => worker.once("message", resolve));
// line 152
const workerUrl = await new Promise<string>(resolve => worker.once("message", resolve));

Neither promise is wired to reject on the worker's error event. REVIEW.md's Tests reviewers reject section is explicit on this shape:

Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise — never throw inside event callbacks.

How it manifests

The eval scripts here are trivial and pass today, so nothing breaks on merge. The concern is diagnosability under a future regression: if new NodeWorker(src, { eval: true }) startup regresses, or URL.createObjectURL / Blob construction inside the worker starts throwing, the worker emits error (and then exit) but never message. The awaited promise on line 137 never settles, and the test sits idle until the file-level timeout fires — surfacing as a generic "timed out" instead of the actual worker error.

The exit awaits on lines 138/153 are fine on their own (a node:worker_threads Worker always emits exit, even on error), but execution never reaches them because it's blocked on the preceding message await.

Why existing code doesn't prevent it

There is no .on("error", ...) handler on either NodeWorker instance. With node:worker_threads, an unhandled error event is re-thrown on the parent's next tick, which in Bun's test runner would crash the test process rather than silently pass — so the test wouldn't go green on a broken build. But the failure mode is a process-level uncaught exception (or a hang, depending on ordering), not a clean test failure pointing at the worker's error. The REVIEW.md rule exists precisely so the awaited promise is the thing that fails, carrying the real error into the test's failure output.

Pre-existing tests in this file (e.g. the Web Worker tests at lines 19–27, 109–125) share the resolve-only shape, but per the guidelines local precedent doesn't weaken the rule for new code.

Step-by-step proof

  1. Suppose a regression makes URL.createObjectURL throw inside a worker (or the eval-mode entry fails to parse).
  2. The worker never reaches parentPort.postMessage(...), so "message" never fires.
  3. The NodeWorker emits "error" with the thrown value, then "exit".
  4. On line 137, worker.once("message", resolve) was the only handler registered; resolve is never called and the promise stays pending.
  5. await on line 137 blocks; line 138's exit await is never reached.
  6. The test hangs until the runner's timeout, reporting "timed out" with no mention of the actual worker error.

Fix

Wire error to reject on the message await (both tests):

const url = await new Promise<string>((resolve, reject) => {
  worker.once("message", resolve);
  worker.once("error", reject);
});

The subsequent exit await can stay as-is (it always fires), or be folded into the same pattern for symmetry.

Severity

nit — this is test-diagnosability, not a correctness bug in the shipped runtime code. The happy path works and a regression would still fail (just less cleanly and more slowly).


expect(resolveObjectURL(url)).toBeUndefined();
await expect(fetch(url)).rejects.toThrow();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Bare .rejects.toThrow() passes on any rejection, so this line doesn't actually pin the revoked-URL behavior — REVIEW.md asks for a specific error class/code/message (never bare toThrow()). Consider asserting the actual error, e.g. .rejects.toThrow(TypeError) or matching /Failed to resolve blob:/ (which is what fetch on a missing blob URL produces). Nit only — line 140's resolveObjectURL(url)undefined already proves revocation.

Extended reasoning...

What the issue is

REVIEW.md, under Every assertion must be able to fail, and assert the strongest invariant, states: "specific error class/code/message (never bare toThrow())". Line 141 asserts await expect(fetch(url)).rejects.toThrow(); with no error class or message argument. Any rejection — including one from an unrelated regression in fetch(blob:...) (a parse failure, a network-layer bug, an OOM) — satisfies this assertion, so the line does not actually pin the reason the fetch fails.

Code path

When fetch() is given a blob: URL whose UUID is not in ObjectURLRegistry, src/runtime/webcore/fetch.rs (around the ObjectURLRegistry::singleton().resolve_and_dupe(...) call) rejects with a TypeError (ErrorCode INVALID_ARG_VALUE) whose message is Failed to resolve blob:<uuid>. That is the specific, stable error this test should assert.

Why the existing check on line 140 doesn't fully cover it

Line 140 (expect(resolveObjectURL(url)).toBeUndefined()) already proves the registry entry is gone via a completely separate code path (node:buffer's resolveObjectURL), so the test as a whole is not vacuous — the primary property (revocation on worker exit) is pinned. The fetch assertion on line 141 is a secondary check that the registry removal is also observable through fetch. But in its bare form it would still pass if, say, a future refactor made fetch(blob:...) reject before ever consulting the registry, or reject for a different reason on this URL shape — masking a regression in the very path being exercised.

Step-by-step proof

  1. Worker exits → revoke_all_for_context removes the entry.
  2. resolveObjectURL(url) returns undefined → line 140 passes (correct).
  3. Suppose a regression makes fetch on any blob: URL throw RangeError: invalid URL before the registry lookup. fetch(url) rejects → bare .rejects.toThrow() on line 141 still passes.
  4. The test is green even though fetch no longer proves anything about revocation. With .rejects.toThrow(/Failed to resolve blob:/) the test would fail and catch the regression.

Impact

Low. Nothing breaks if this merges as-is: line 140 already asserts the load-bearing invariant, and the runtime change is fully covered. This is a test-quality tightening per the repo's stated review rule, not a correctness bug.

How to fix

await expect(fetch(url)).rejects.toThrow(TypeError);
// or, more precisely:
await expect(fetch(url)).rejects.toThrow(/Failed to resolve blob:/);

Addressing the refutation

One verifier refuted this as a duplicate of another report on the same line. That objection is about deduplication bookkeeping, not validity — it explicitly does not dispute the finding itself. Synthesis has consolidated the reports into this single entry, so the duplication concern is moot; only one comment will be posted.

});

test("Worker exit does not revoke Blob URLs created by other threads", async () => {
const parentUrl = URL.createObjectURL(new Blob([new Uint8Array(64).fill(1)]));
try {
const worker = new NodeWorker(
`const { parentPort } = require("node:worker_threads");
parentPort.postMessage(URL.createObjectURL(new Blob([new Uint8Array(64)])));`,
{ eval: true },
);
const workerUrl = await new Promise<string>(resolve => worker.once("message", resolve));
await new Promise(resolve => worker.once("exit", resolve));

expect(resolveObjectURL(workerUrl)).toBeUndefined();
const blob = resolveObjectURL(parentUrl);
expect(blob).toBeInstanceOf(Blob);
expect(new Uint8Array(await blob!.arrayBuffer())).toEqual(new Uint8Array(64).fill(1));
} finally {
URL.revokeObjectURL(parentUrl);
}
});
Loading