Skip to content

fix: return object from generated retain override - #480

Open
rbudnar wants to merge 1 commit into
NativeScript:mainfrom
rbudnar:codex/fix-intel-retain-abi
Open

rbudnar wants to merge 1 commit into
NativeScript:mainfrom
rbudnar:codex/fix-intel-retain-abi

Conversation

@rbudnar

@rbudnar rbudnar commented Sep 28, 2026 •

Copy link
Copy Markdown

Summary

Generated Objective-C subclasses override retain with an IMP declared as a void-returning block even though the installed method is encoded @@: and retain must return the object. Make both the original IMP function pointer and replacement block return id.

Reproduction and verification

On an Intel/x86_64 macOS 15.7.9 host with Xcode 16.4 and an iOS 18.6 iPhone 16 Plus Simulator, a NativeScript 9.1.0 app crashed at -[UIApplication setDelegate:] before UI. LLDB showed the NativeScript-generated delegate's retain IMP returning an invalid object pointer. The same crash occurred in Debug and Release builds and in a clean base app unrelated to the feature being tested.

Rebuilding the 9.1.0 runtime with this three-line return-type correction allowed the installed app to launch. Its native Files picker then restored a 30,695,814-byte JSON backup (1,700 workout rows, 243 tombstones) and exported a 30,695,812-byte backup whose canonical data matched the input. This is simulator evidence from the 9.1.0 source; I have not run the upstream main test suite and cannot claim Apple Silicon or device verification.

Related: #292 reports an Intel simulator startup crash, though that issue does not include a matching stack trace.

Scope

This changes only the ABI declaration for the generated retain override. It does not alter retain-count policy or release behavior. PR #403 touches the same section and currently retains the void return signature; this change may need to be carried through that refactor.

Summary by CodeRabbit

  • Bug Fixes
    • Improved runtime object-retention handling so retain operations preserve and return their expected result. Existing retain-count checks, garbage-collection protection, and release behavior are unchanged. This update does not add or remove user-facing functionality, but helps ensure object-retention operations continue to behave consistently within the runtime.

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 3678c84f-e0f4-4786-a623-d7ec8cf3dea8

📥 Commits

Reviewing files that changed from the base of the PR and between 1736146 and c3a56c9.

📒 Files selected for processing (1)
  • NativeScript/runtime/ClassBuilder.mm

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

The retain override now stores the original retain implementation with an id return type and returns the result of forwarding the call.

Changes

Retain override

Layer / File(s) Summary
Forward retain result
NativeScript/runtime/ClassBuilder.mm
The saved retain function pointer now returns id. The replacement block returns the forwarded call’s result.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Suggested reviewers: edusperoni

Merge Risk: ⚪ Minimal · up to c3a56

The override now returns the object produced by retain, matching the method contract. No concrete merge-blocking issue is evident in the supplied change context.

Architecture Summary

Architecture risk: 🔵 Low · up to c3a56

The change affects 1 system.

Changed systems: NativeScript

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — NativeScript (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in NativeScript/runtime/ClassBuilder.mm: The retain function pointer changes its return type from void to id, matching the Objective-C retain method; the replacement block continues to forward to the original implementation and now returns its object result.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: returning the object from the generated retain override.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit checks the retain call,
Its object result comes back to all.
The pointer’s type is now id,
The forwarded value is returned with pride.
One small change, then off I hop,
Through clover fields, I gladly stop.

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

This branch has not been deployed

No deployments
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