From e66536a99ed958f81ac18f926799bebe49f44ca6 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 2 Aug 2026 14:16:36 +0200 Subject: [PATCH] fix(apple): keep a view's service alive while the view is used `odr::HtmlView` looks self-sufficient - it holds a `shared_ptr` - but the impl behind it, `internal::html::HtmlView`, keeps only a bare `abstract::HtmlService *` and forwards `config()` and `write_html()` through it. A view handed out by `-[ODRHtmlService views]` therefore dangles as soon as the service goes, and `try service().views.first` - a temporary service, which is how anyone would write it - segfaulted on both macOS and the simulator. `ODRHtmlView` now carries its `ODRHtmlService` the way `ODRElement` carries its `ODRDocument`. The other two bindings already do this: JNI passes an `owner` to the `NativeResource` constructor, python pins the service onto the view as `_service`. `testRenderedHtmlCarriesItsOwnStyles` has been red on main since it landed - it is the crash, not a flake. `testViewsKeepTheirServiceAlive` states the invariant directly, next to the element suite's equivalent. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YSePnR4Jcj5cq3H1D11xcd --- apple/AGENTS.md | 17 +++++++++++------ apple/src/ODRHtml.mm | 9 +++++++-- apple/src/ODRPrivate.h | 2 +- apple/tests/OdrCoreTests.swift | 14 ++++++++++++++ 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/apple/AGENTS.md b/apple/AGENTS.md index f8a134ba..cfd6176d 100644 --- a/apple/AGENTS.md +++ b/apple/AGENTS.md @@ -76,12 +76,17 @@ consumer, and a SwiftPM binary target gives the consumer no way to pass `jni/src/odr_jni.cpp::throw_java` — keep the two in step. - **Elements carry their owner.** Most public C++ handles own a `shared_ptr`, so a wrapper holding one by value is self-sufficient and needs no keep-alive. - `odr::Element` is the exception: it holds a bare pointer into the document's - adapter. Every `ODRElement` therefore keeps a strong reference to its - `ODRDocument`, and navigation goes through `-derive:` so that reference is - carried along by construction rather than by remembering to pass it. This is - the JNI bindings' owner chain, and it is what lets a caller keep a subtree - after dropping the document. + `odr::Element` and `odr::HtmlView` are the exceptions: the first holds a bare + pointer into the document's adapter, the second one into its service. Every + `ODRElement` therefore keeps a strong reference to its `ODRDocument`, and + navigation goes through `-derive:` so that reference is carried along by + construction rather than by remembering to pass it; every `ODRHtmlView` + likewise keeps its `ODRHtmlService`. This is the JNI bindings' owner chain + (and python's `_service` attribute), and it is what lets a caller keep a + subtree after dropping the document, or render a view off a service it no + longer holds. **Check a new wrapper for a bare pointer before assuming the + `shared_ptr` makes it self-sufficient** — `HtmlView` looks like it owns + everything it needs, and does not. - **Strings**: `odr::apple::to_string` / `to_nsstring`, real UTF-8 ↔ UTF-16. Never hand a `-UTF8String` pointer to something that outlives the autorelease pool. diff --git a/apple/src/ODRHtml.mm b/apple/src/ODRHtml.mm index 9000a5b8..d7a4ed86 100644 --- a/apple/src/ODRHtml.mm +++ b/apple/src/ODRHtml.mm @@ -306,11 +306,16 @@ + (instancetype)htmlWithHandle:(const odr::Html &)handle { @implementation ODRHtmlView { std::optional _handle; + // The service the view belongs to. The view's impl holds a bare pointer to + // it, so without this a view handed out by `-views` could outlive what it + // points into — the same owner chain `ODRElement` keeps to its document. + id _owner; } -+ (instancetype)viewWithHandle:(odr::HtmlView)handle { ++ (instancetype)viewWithHandle:(odr::HtmlView)handle owner:(id)owner { ODRHtmlView *const result = [[ODRHtmlView alloc] init]; result->_handle = std::move(handle); + result->_owner = owner; return result; } @@ -377,7 +382,7 @@ + (instancetype)serviceWithHandle:(odr::HtmlService)handle { NSMutableArray *const result = [NSMutableArray arrayWithCapacity:views.size()]; for (const odr::HtmlView &view : views) { - [result addObject:[ODRHtmlView viewWithHandle:view]]; + [result addObject:[ODRHtmlView viewWithHandle:view owner:self]]; } return result; }, diff --git a/apple/src/ODRPrivate.h b/apple/src/ODRPrivate.h index a861c9dc..bfb4405b 100644 --- a/apple/src/ODRPrivate.h +++ b/apple/src/ODRPrivate.h @@ -158,7 +158,7 @@ ODRMeasure *_Nullable box(const std::optional &measure); @end @interface ODRHtmlView (Private) -+ (instancetype)viewWithHandle:(odr::HtmlView)handle; ++ (instancetype)viewWithHandle:(odr::HtmlView)handle owner:(id)owner; - (const odr::HtmlView &)handle; @end diff --git a/apple/tests/OdrCoreTests.swift b/apple/tests/OdrCoreTests.swift index fe9e2422..f8a5522e 100644 --- a/apple/tests/OdrCoreTests.swift +++ b/apple/tests/OdrCoreTests.swift @@ -118,6 +118,20 @@ final class HtmlTests: XCTestCase { XCTAssertTrue(html.contains(" HtmlView { + try XCTUnwrap(try service().views.first) + } + let view = try viewOnly() + XCTAssertFalse(view.path.isEmpty) + var resources: NSArray? + XCTAssertFalse(try view.writeHtml(resources: &resources).isEmpty) + } + func testBringOfflineWritesFiles() throws { let output = try temporaryDirectory() let html = try service().bringOffline(to: output)