Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions apple/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
9 changes: 7 additions & 2 deletions apple/src/ODRHtml.mm
Original file line number Diff line number Diff line change
Expand Up @@ -306,11 +306,16 @@ + (instancetype)htmlWithHandle:(const odr::Html &)handle {

@implementation ODRHtmlView {
std::optional<odr::HtmlView> _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;
}

Expand Down Expand Up @@ -377,7 +382,7 @@ + (instancetype)serviceWithHandle:(odr::HtmlService)handle {
NSMutableArray<ODRHtmlView *> *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;
},
Expand Down
2 changes: 1 addition & 1 deletion apple/src/ODRPrivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ ODRMeasure *_Nullable box(const std::optional<odr::Measure> &measure);
@end

@interface ODRHtmlView (Private)
+ (instancetype)viewWithHandle:(odr::HtmlView)handle;
+ (instancetype)viewWithHandle:(odr::HtmlView)handle owner:(id)owner;
- (const odr::HtmlView &)handle;
@end

Expand Down
14 changes: 14 additions & 0 deletions apple/tests/OdrCoreTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,20 @@ final class HtmlTests: XCTestCase {
XCTAssertTrue(html.contains("<style"), "the html has no stylesheet")
}

/// A view's impl points into its service without owning it, so the view has
/// to keep the service alive itself β€” the analogue of
/// `ElementTreeTests.testElementsKeepTheirDocumentAlive`. Rendering off a
/// service that only ever existed as a temporary used to segfault.
func testViewsKeepTheirServiceAlive() throws {
func viewOnly() throws -> 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)
Expand Down
Loading