Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

iOS: Add null checks on shell dereference #57099

Merged
merged 1 commit into from
Dec 10, 2024
Merged

iOS: Add null checks on shell dereference #57099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 20 additions & 5 deletions shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,27 +320,37 @@ - (void)dispatchPointerDataPacket:(std::unique_ptr<flutter::PointerDataPacket>)p
}

- (fml::WeakPtr<flutter::PlatformView>)platformView {
FML_DCHECK(_shell);
if (!_shell) {
return {};
}
return _shell->GetPlatformView();
}

- (flutter::PlatformViewIOS*)iosPlatformView {
FML_DCHECK(_shell);
if (!_shell) {
return nullptr;
Copy link
Contributor

@hellohuanlin hellohuanlin Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why we use WeakPtr in previous line, but raw pointer here?

Copy link
Contributor

@hellohuanlin hellohuanlin Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and RefPtr below? (Should they be WeakPtr as well?)

Copy link
Member Author

@cbracken cbracken Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use cases:

  • WeakPtr: A raw pointer wrapper that, in unopt builds, asserts that all dereferences occur on the thread on which it was allocated. We use this to ensure that all derefs of shell occur on the platform thread. Does not do refcounting. We use this where we want to enforce threading invariants but object ownership/lifetime is managed by someone else.
  • RefPtr: A refcounted pointer. We use this in the engine where an object ownership is shared between a pool of owners. You could think of this as somewhat similar to std::shared_ptr. There's other utility code in ref_counted.h built around this that supports threading invariants.
  • raw C/C++ pointers: In cases where we want a non-owning pointer to an object whose lifetime is managed by someone else and where we don't care about threading invariants, we often use raw C/C++ pointers.

When I wrote this patch, I looked and these getters are only really used by FlutterEngine itself, FlutterViewController, and tests. There's no reason we couldn't be consistent and return at raw pointer instead of the WeakPtr in the platformView getter, but that's orthogonal to this fix.

My thinking is that rather than do that though, I'd rather send a followup that eliminates these getters altogether and provides better API instead. For example, engine.iosPlatformView is used by accessibility code in FlutterViewController; the relevant non-view-specific code should simply be refactored into FlutterEngine instead.

}
return static_cast<flutter::PlatformViewIOS*>(_shell->GetPlatformView().get());
}

- (fml::RefPtr<fml::TaskRunner>)platformTaskRunner {
FML_DCHECK(_shell);
if (!_shell) {
return {};
}
return _shell->GetTaskRunners().GetPlatformTaskRunner();
}

- (fml::RefPtr<fml::TaskRunner>)uiTaskRunner {
FML_DCHECK(_shell);
if (!_shell) {
return {};
}
return _shell->GetTaskRunners().GetUITaskRunner();
}

- (fml::RefPtr<fml::TaskRunner>)rasterTaskRunner {
FML_DCHECK(_shell);
if (!_shell) {
return {};
}
return _shell->GetTaskRunners().GetRasterTaskRunner();
}

Expand Down Expand Up @@ -392,6 +402,9 @@ - (void)sendKeyEvent:(const FlutterKeyEvent&)event
}

- (void)ensureSemanticsEnabled {
if (!self.iosPlatformView) {
return;
}
self.iosPlatformView->SetSemanticsEnabled(true);
}

Expand Down Expand Up @@ -419,6 +432,7 @@ - (void)setViewController:(FlutterViewController*)viewController {
}

- (void)attachView {
FML_DCHECK(self.iosPlatformView);
self.iosPlatformView->attachView();
}

Expand Down Expand Up @@ -1224,6 +1238,7 @@ - (void)cleanUpConnection:(FlutterBinaryMessengerConnection)connection {
#pragma mark - FlutterTextureRegistry

- (int64_t)registerTexture:(NSObject<FlutterTexture>*)texture {
FML_DCHECK(self.iosPlatformView);
int64_t textureId = self.nextTextureId++;
self.iosPlatformView->RegisterExternalTexture(textureId, texture);
return textureId;
Expand Down
13 changes: 13 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,19 @@ - (void)testCreate {
XCTAssertNotNil(engine);
}

- (void)testShellGetters {
FlutterDartProject* project = [[FlutterDartProject alloc] init];
FlutterEngine* engine = [[FlutterEngine alloc] initWithName:@"foobar" project:project];
XCTAssertNotNil(engine);

// Ensure getters don't deref _shell when it's null, and instead return nullptr.
XCTAssertEqual(engine.platformView.get(), nullptr);
XCTAssertEqual(engine.iosPlatformView, nullptr);
XCTAssertEqual(engine.platformTaskRunner.get(), nullptr);
XCTAssertEqual(engine.uiTaskRunner.get(), nullptr);
XCTAssertEqual(engine.rasterTaskRunner.get(), nullptr);
}

- (void)testInfoPlist {
// Check the embedded Flutter.framework Info.plist, not the linked dylib.
NSURL* flutterFrameworkURL =
Expand Down
7 changes: 6 additions & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,8 @@ - (void)installFirstFrameCallback {
weakPlatformView->SetNextFrameCallback([weakSelf,
platformTaskRunner = self.engine.platformTaskRunner,
rasterTaskRunner = self.engine.rasterTaskRunner]() {
FML_DCHECK(platformTaskRunner);
FML_DCHECK(rasterTaskRunner);
FML_DCHECK(rasterTaskRunner->RunsTasksOnCurrentThread());
// Get callback on raster thread and jump back to platform thread.
platformTaskRunner->PostTask([weakSelf]() { [weakSelf onFirstFrameRendered]; });
Expand Down Expand Up @@ -1822,7 +1824,7 @@ - (void)setUpKeyboardAnimationVsyncClient:
});
};

_keyboardAnimationVSyncClient = [[VSyncClient alloc] initWithTaskRunner:[self.engine uiTaskRunner]
_keyboardAnimationVSyncClient = [[VSyncClient alloc] initWithTaskRunner:self.engine.uiTaskRunner
callback:uiCallback];
_keyboardAnimationVSyncClient.allowPauseAfterVsync = NO;
[_keyboardAnimationVSyncClient await];
Expand Down Expand Up @@ -2105,6 +2107,9 @@ - (void)onAccessibilityStatusChanged:(NSNotification*)notification {
return;
}
fml::WeakPtr<flutter::PlatformView> platformView = self.engine.platformView;
if (!platformView) {
return;
}
int32_t flags = self.accessibilityFlags;
#if TARGET_OS_SIMULATOR
// There doesn't appear to be any way to determine whether the accessibility
Expand Down
4 changes: 2 additions & 2 deletions shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ @implementation VSyncClient {

- (instancetype)initWithTaskRunner:(fml::RefPtr<fml::TaskRunner>)task_runner
callback:(flutter::VsyncWaiter::Callback)callback {
self = [super init];
FML_DCHECK(task_runner);

if (self) {
if (self = [super init]) {
_refreshRate = DisplayLinkManager.displayRefreshRate;
_allowPauseAfterVsync = YES;
_callback = std::move(callback);
Expand Down
Loading