[objective_c] Fix GC safepoint crash in ObjCProtocolBuilder.implementMethod

[henawey-t]

Fixes #3209

Root cause

In ObjCProtocolBuilder.implementMethod, the call to block.ref.pointer.cast() extracts a raw pointer and passes it across the FFI boundary. After that expression the Dart compiler considers block dead — no more Dart-level references. A GC safepoint inside the native call can fire before ObjC has had a chance to retain the block pointer, triggering objc_release through the finalizer chain and producing EXC_BAD_ACCESS at protocol.m:33.

Why the existing Finalizable on _ObjCReference / ObjCBlockRef doesn't help: Dart's Finalizable guarantee only protects local variables whose static type is a subtype of Finalizable. The parameter block has static type ObjCBlockBase, which was not in the Finalizable hierarchy. The transient field access block.ref is never stored in a local variable of a Finalizable type, so no reachabilityFence was inserted for any part of the chain.

Fix

Add implements Finalizable to ObjCBlockBase in _ObjCRefHolder's subclass chain:

class ObjCBlockBase extends _ObjCRefHolder<c.ObjCBlockImpl, ObjCBlockRef>
    implements Finalizable {

The Dart compiler now keeps block alive until the end of implementMethod's scope — including across the native safepoint — giving ObjC time to retain the block pointer before any finalizer can fire.

Why ObjCObject is intentionally excluded: ObjCObject instances can be sent across Dart isolates (e.g. NSInputStream uses Isolate.run). Finalizable objects are non-sendable; adding it to ObjCObject would break that. Blocks capture Dart closures and are never sent across isolates, so Finalizable is safe for ObjCBlockBase only.

Production validation

Applied this fix (as implements Finalizable on ObjCBlockBase) to a forked objective_c 9.1.0 via dependency_override. Crashlytics confirmed crashes dropped from ~1.1K events/week to zero in the app version that previously exhibited the crash. No new crashes or memory regressions observed.

Testing

• Compile-time assertion (finalizable_test.dart): a static type check that dart analyze enforces — if ObjCBlockBase ever loses implements Finalizable a type error is emitted immediately.
• Runtime assertion: expect(obj, isNot(isA<Finalizable>())) on NSObject verifies ObjCObject remains non-Finalizable (preserving isolate sendability).
• GC pressure test: builds a protocol object, forces doGC(), and verifies the retain count remains non-zero.
• The exact race (GC at a safepoint inside an FFI call) cannot be triggered with doGC(), which runs between Dart instructions rather than at FFI safepoints. The compile-time assertion is the primary regression guard.

Pre-existing test failures (unrelated)

Two tests fail identically on main before any of these changes:

• nsdate_test: NSDate from DateTime — date format locale difference
• nsdictionary/nsarray_test: ref counting — flaky GC-timing interference between test files

🤖 Generated with Claude Code

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[github-actions]

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?
objective_c	Breaking	9.3.0	9.4.0-wip	9.4.0-wip	✔️

This check can be disabled by tagging the PR with skip-breaking-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbol	Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

[liamappelbe]

Thanks for the PR @henawey-t.

The first thing I'm interested in is the regression test. I'm still confused about what's going on with this bug, and I'd really like to be able to reproduce the failure locally. If this regression test is a simple repro of the error, that alone would be extremely helpful.

But when I clone the test into my local main branch, it passes (after I comment out the compile time Finalizable check), even when I add a for loop to run it 1000 times. I think the first step is to focus on trying to repro the failure, even if it's a 0.1% flake. For example, if it's a threading issue, we have other tests that call into native code, spawn multiple threads, and even poke at the Dart embedder APIs. Take a look at the pkgs/ffigen/test/native_objc_test directory for examples (a good example is callOnSameThreadOutsideIsolate in block_test.m)

I have some ideas for how to fix this bug without making ObjCBlockBase finalizable, but I can't try them out without a repro.

[henawey-t]

Thanks for the PR @henawey-t.

The first thing I'm interested in is the regression test. I'm still confused about what's going on with this bug, and I'd really like to be able to reproduce the failure locally. If this regression test is a simple repro of the error, that alone would be extremely helpful.

But when I clone the test into my local main branch, it passes (after I comment out the compile time Finalizable check), even when I add a for loop to run it 1000 times. I think the first step is to focus on trying to repro the failure, even if it's a 0.1% flake. For example, if it's a threading issue, we have other tests that call into native code, spawn multiple threads, and even poke at the Dart embedder APIs. Take a look at the pkgs/ffigen/test/native_objc_test directory for examples (a good example is callOnSameThreadOutsideIsolate in block_test.m)

I have some ideas for how to fix this bug without making ObjCBlockBase finalizable, but I can't try them out without a repro.

I was able to reproduce it, but I have to enforce Dart GC from the Objc before calling the actual implementation of -[DOBJCDartProtocolBuilder implementMethod:withBlock:...], which produces the same issue we are facing. And I enforce it exactly at the start of the swizzled method before calling the original objc method.

I documented everything in the test/REPRODUCTION.md, let me know if anything need to clarfication.

[liamappelbe]

Great work on the test. Looks like it was pretty fiddly to implement, but it works. I can repro now, so I'll try some alternative fixes.

[liamappelbe]

Played around with some options. Here's a simple fix that doesn't require changing the block inheritance heirarchy:

  void implementMethod(
    Pointer<r.ObjCSelector> sel,
    Pointer<Char> signature,
    Pointer<Void> trampoline,
    ObjCBlockBase block,
  ) {
    if (_built) {
      throw StateError('Protocol is already built');
    }
    final blockRef = block.ref;  // Store the ref in a local.
    _builder.implementMethod(
      sel,
      withBlock: blockRef.pointer.cast(),
      withTrampoline: trampoline,
      withSignature: signature,
    );
  }

We should probably do a quick scan of the repo to see if there's anywhere else that we're doing block.ref.pointer or object.ref.pointer, and change them to store the .ref in a local.

[henawey-t]

I did the changes as recommended and updated the tests. However, the repo scan is done. Running dart run ffigen revealed 24 unsafe sites in objective_c_bindings_generated.dart where block parameters have .ref.pointer passed inline to non-leaf FFI calls, for example:

  // NSArray.enumerateObjectsAtIndexes:options:usingBlock:
  _objc_msgSend_a3wp08(
    object$.ref.pointer,
    _sel_enumerateObjectsAtIndexes_options_usingBlock_,
    s.ref.pointer,
    options,
    usingBlock.ref.pointer,  // ← unsafe, same pattern
  );

One important distinction though: the object$.ref.pointer and s.ref.pointer usages are safe — those variables are NSArray/NSIndexSet which extend ObjCObject, and ObjCObjectRef transitively implementsFinalizable via _ObjCReference. The VM keeps them live. Only block parameters are at risk because ObjCBlockBase does not implement Finalizable.

Since objective_c_bindings_generated.dart is auto-generated, patching it directly would be overwritten on the next dart run ffigen. The fix should go into the FFIgen generator, whenever a method parameter has type ObjCBlock, emit the .ref extraction pattern instead of inlining .ref.pointer in the argument list.

[liamappelbe]

Since objective_c_bindings_generated.dart is auto-generated, patching it directly would be overwritten on the next dart run ffigen. The fix should go into the FFIgen generator, whenever a method parameter has type ObjCBlock, emit the .ref extraction pattern instead of inlining .ref.pointer in the argument list.

Figures. I'll fix FFIgen myself in a follow up PR. Did you find any locations using this pattern outside of generated code?

Did you want to try to land this PR, or just show me the repro? If you want to land it, start by removing REPRODUCTION.md, and reduce the amount of documentation in finalizable_test.dart and gc_inject.m. It's too verbose atm. A short comment of every non-obvious function, and maybe comment on any particularly obscure lines of code is sufficient. Also, the CHANGELOG entry is a bit too long. The first sentence is good, but the second is unnecessary. If people want to know the details of the fix they can click through on the bug link.

[henawey-t]

Figures. I'll fix FFIgen myself in a follow up PR. Did you find any locations using this pattern outside of generated code?

No unsafe manual usages outside generated code — all 24 sites were in objective_c_bindings_generated.dart (auto-generated, so FFIgen itself needs fixing).

Did you want to try to land this PR, or just show me the repro? If you want to land it, start by removing REPRODUCTION.md, and reduce the amount of documentation in finalizable_test.dart and gc_inject.m. It's too verbose atm. A short comment of every non-obvious function, and maybe comment on any particularly obscure lines of code is sufficient. Also, the CHANGELOG entry is a bit too long. The first sentence is good, but the second is unnecessary. If people want to know the details of the fix they can click through on the bug link.

I'm pushing a commit for these changes. Let me know if anything is unclear.

[liamappelbe]

Formatting errors: https://github.com/dart-lang/native/actions/runs/24822639870/job/72798914910?pr=3307

[henawey-t]

Formatting errors: https://github.com/dart-lang/native/actions/runs/24822639870/job/72798914910?pr=3307

@liamappelbe done

[henawey-t]

Great, the PR is green now. @liamappelbe let me know if anything is needed from my side to merge the PR.

[liamappelbe]

Looks good, but you'll have a merge conflict with #3325. You're adding a test-only util, so it should be gated behind the includeTestUtils flag, just like test/util.c

[liamappelbe]

Actually it's a small enough merge that I can do it myself through github's UI