Skip to content

runtime: fix small leak in Blob deinit#24802

Merged
Jarred-Sumner merged 2 commits into
mainfrom
nektro-patch-31
Nov 18, 2025
Merged

runtime: fix small leak in Blob deinit#24802
Jarred-Sumner merged 2 commits into
mainfrom
nektro-patch-31

Conversation

@nektro

@nektro nektro commented Nov 18, 2025

Copy link
Copy Markdown
Contributor

pulled out of #21663

@robobun

robobun commented Nov 18, 2025

Copy link
Copy Markdown
Collaborator
Updated 7:41 PM PT - Nov 17th, 2025

@nektro, your commit 0248b7c has 3 failures in Build #31939 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 24802

That installs a local version of the PR into your bun-24802 executable, so you can run:

bun-24802 --bun

@nektro nektro marked this pull request as ready for review November 18, 2025 03:45
@coderabbitai

This comment was marked as resolved.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

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 9513c1d and 0248b7c.

📒 Files selected for processing (2)
  • scripts/runner.node.mjs (1 hunks)
  • src/bun.js/webcore/Blob.zig (1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:60-76
Timestamp: 2025-09-02T17:41:07.869Z
Learning: In bun's memory utilities, when handling const pointers in deinit operations, prefer compile-time errors over silent skipping to avoid hard-to-find memory leaks. Users expect explicit failures rather than silent omissions in memory management.
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.

Applied to files:

  • scripts/runner.node.mjs
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.

Applied to files:

  • src/bun.js/webcore/Blob.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.

Applied to files:

  • src/bun.js/webcore/Blob.zig
📚 Learning: 2025-10-01T21:49:27.862Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.

Applied to files:

  • src/bun.js/webcore/Blob.zig
📚 Learning: 2025-10-21T20:57:01.892Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23889
File: src/string.zig:751-752
Timestamp: 2025-10-21T20:57:01.892Z
Learning: In Zig code handling bun.String: calling deref() on an empty bun.String (String.empty or after being set to .empty) is well-defined behavior and safely does nothing (no-op). While such calls may be redundant and could be removed for code clarity, their presence is not a correctness issue.

Applied to files:

  • src/bun.js/webcore/Blob.zig
📚 Learning: 2025-09-02T17:41:07.869Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/memory.zig:60-76
Timestamp: 2025-09-02T17:41:07.869Z
Learning: In bun's memory utilities, when handling const pointers in deinit operations, prefer compile-time errors over silent skipping to avoid hard-to-find memory leaks. Users expect explicit failures rather than silent omissions in memory management.

Applied to files:

  • src/bun.js/webcore/Blob.zig
📚 Learning: 2025-09-02T18:32:32.309Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/bun.zig:668-675
Timestamp: 2025-09-02T18:32:32.309Z
Learning: The `std.meta.trait` module has been removed from the Zig standard library, which will cause compilation failures in functions that use it like `std.meta.trait.isSingleItemPtr()`. This affects functions like `isHeapMemory()` in `src/bun.zig`.

Applied to files:

  • src/bun.js/webcore/Blob.zig
🔇 Additional comments (1)
scripts/runner.node.mjs (1)

494-510: Buildkite annotation styling change is consistent with intent

Tying style strictly to flaky ("warning" for flaky, "error" otherwise) cleanly separates flaky from hard failures and ensures vendor failures are always annotated as errors. Annotation grouping via context is preserved and no other behavior changes.

Comment thread src/bun.js/webcore/Blob.zig
@nektro nektro requested a review from cirospaciari November 18, 2025 05:12
@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

Confirming this does fix the leak. It would be better if it had a test. It would also be better if we added a separate deinit function that calls destroy explicitly instead of incrementing the reference count in the deref function.
image

@Jarred-Sumner Jarred-Sumner merged commit af498a0 into main Nov 18, 2025
59 of 62 checks passed
@Jarred-Sumner Jarred-Sumner deleted the nektro-patch-31 branch November 18, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants