Skip to content

Enable CHECK_REF_COUNTED_LIFECYCLE - #121

Merged
sosukesuzuki merged 1 commit into
mainfrom
enable-ref-counted-lifecycle
Dec 27, 2025
Merged

sosukesuzuki merged 1 commit into
mainfrom
enable-ref-counted-lifecycle

Conversation

@sosukesuzuki

Copy link
Copy Markdown
Member

We disable CHECK_REF_COUNTED_LIFECYCLE temporary to build latest WebKit.

But we can enable it now.

@sosukesuzuki

Copy link
Copy Markdown
Member Author

/build-preview

@github-actions

Copy link
Copy Markdown

🔄 Preview build started

Release tag: autobuild-preview-pr-121-f24072bb
Workflow: View progress

@coderabbitai

coderabbitai Bot commented Dec 26, 2025

Copy link
Copy Markdown

Walkthrough

Two WebKit files remove Bun-specific preprocessor conditionals. The WasmMemory.h file unconditionally includes assertion logic in checkLifetime(), while RefCountDebugger.h simplifies CHECK_REF_COUNTED_LIFECYCLE to depend solely on assertion and security settings, removing the prior Bun-specific branch.

Changes

Cohort / File(s) Summary
Bun-specific conditional removal
Source/JavaScriptCore/wasm/WasmMemory.h, Source/WTF/wtf/RefCountDebugger.h
Removes USE(BUN_JSC_ADDITIONS) preprocessor gates. WasmMemory.h unconditionally defines checkLifetime() with assertion logic, eliminating empty stub path. RefCountDebugger.h simplifies CHECK_REF_COUNTED_LIFECYCLE macro to depend only on ASSERT_ENABLED and ENABLE(SECURITY_ASSERTIONS), removing Bun-specific override.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description lacks critical details required by the WebKit template: no Bugzilla bug reference, no 'Reviewed by' line, and minimal explanation of why the change is now safe to enable. Add a Bugzilla bug reference link, include 'Reviewed by' attribution, and provide a more detailed explanation of why CHECK_REF_COUNTED_LIFECYCLE can now be safely enabled.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Enable CHECK_REF_COUNTED_LIFECYCLE' is clear and directly summarizes the main change: re-enabling the CHECK_REF_COUNTED_LIFECYCLE conditional checks across the codebase by removing Bun-specific gating.

📜 Recent 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 2317e14 and f24072b.

📒 Files selected for processing (2)
  • Source/JavaScriptCore/wasm/WasmMemory.h
  • Source/WTF/wtf/RefCountDebugger.h
💤 Files with no reviewable changes (1)
  • Source/JavaScriptCore/wasm/WasmMemory.h
🧰 Additional context used
🧠 Learnings (3)
📓 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)
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
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++
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
📚 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/WTF/wtf/RefCountDebugger.h
📚 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/WTF/wtf/RefCountDebugger.h
🔇 Additional comments (1)
Source/WTF/wtf/RefCountDebugger.h (1)

32-36: Since the repository cannot be cloned and the claims in the review comment cannot be verified independently, manual verification of the following is required:

  1. Confirm that the previous state of RefCountDebugger.h actually contained a USE(BUN_JSC_ADDITIONS) guard that was removed in this change
  2. Verify that re-enabling CHECK_REF_COUNTED_LIFECYCLE for Bun builds does not break existing functionality
  3. Ensure no other Bun-specific ref-counting workarounds conflict with this change
  4. Run relevant tests with lifecycle checks enabled to validate the change works correctly

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown

⚠️ Preview build was cancelled

This may have been caused by a newer build being triggered for the same PR.

@sosukesuzuki

Copy link
Copy Markdown
Member Author

/build-preview

@github-actions

Copy link
Copy Markdown

🔄 Preview build started

Release tag: autobuild-preview-pr-121-f24072bb
Workflow: View progress

@github-actions

Copy link
Copy Markdown

✅ Preview build completed

Release: autobuild-preview-pr-121-f24072bb

@sosukesuzuki
sosukesuzuki merged commit 8637781 into main Dec 27, 2025
1 check passed
Jarred-Sumner pushed a commit to oven-sh/bun that referenced this pull request Dec 27, 2025
### What does this PR do?

Enables `CHECK_REF_COUNTED_LIFECYCLE` in WebKit (
oven-sh/WebKit#121 )

See also
WebKit/WebKit@a978fae

#### `CHECK_REF_COUNTED_LIFECYCLE`?

A compile-time macro that enables lifecycle validation for
reference-counted objects in debug builds.

**Definition**
```cpp
  #if ASSERT_ENABLED || ENABLE(SECURITY_ASSERTIONS)
  #define CHECK_REF_COUNTED_LIFECYCLE 1
  #else
  #define CHECK_REF_COUNTED_LIFECYCLE 0
  #endif
```
**Purpose**

  Detects three categories of bugs:
1. Missing adoption - Objects stored in RefPtr without using adoptRef()
2. Ref during destruction - ref() called while destructor is running
(causes dangling pointers)
  3. Thread safety violations - Unsafe ref/deref across threads

**Implementation**

  When enabled, RefCountDebugger adds two tracking flags:
  - m_deletionHasBegun - Set when destructor starts
  - m_adoptionIsRequired - Cleared when adoptRef() is called

These flags are checked on every ref()/deref() call, with assertions
failing on violations.

**Motivation**

  Refactored debug code into a separate RefCountDebugger class to:
  - Improve readability of core refcount logic
- Eliminate duplicate code across RefCounted, ThreadSafeRefCounted, etc.
  - Simplify adding new refcount classes

 **Overhead**

Zero in release builds - the flags and checks are compiled out entirely.

### How did you verify your code works?

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
liooil pushed a commit to liooil/poly that referenced this pull request Aug 7, 2026
### What does this PR do?

Enables `CHECK_REF_COUNTED_LIFECYCLE` in WebKit (
oven-sh/WebKit#121 )

See also
WebKit/WebKit@a978fae

#### `CHECK_REF_COUNTED_LIFECYCLE`?

A compile-time macro that enables lifecycle validation for
reference-counted objects in debug builds.

**Definition**
```cpp
  #if ASSERT_ENABLED || ENABLE(SECURITY_ASSERTIONS)
  #define CHECK_REF_COUNTED_LIFECYCLE 1
  #else
  #define CHECK_REF_COUNTED_LIFECYCLE 0
  #endif
```
**Purpose**

  Detects three categories of bugs:
1. Missing adoption - Objects stored in RefPtr without using adoptRef()
2. Ref during destruction - ref() called while destructor is running
(causes dangling pointers)
  3. Thread safety violations - Unsafe ref/deref across threads

**Implementation**

  When enabled, RefCountDebugger adds two tracking flags:
  - m_deletionHasBegun - Set when destructor starts
  - m_adoptionIsRequired - Cleared when adoptRef() is called

These flags are checked on every ref()/deref() call, with assertions
failing on violations.

**Motivation**

  Refactored debug code into a separate RefCountDebugger class to:
  - Improve readability of core refcount logic
- Eliminate duplicate code across RefCounted, ThreadSafeRefCounted, etc.
  - Simplify adding new refcount classes

 **Overhead**

Zero in release builds - the flags and checks are compiled out entirely.

### How did you verify your code works?

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
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.

1 participant