From fcf65a6b5393dc463574136b415065a9c19ec843 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Tue, 10 Dec 2024 09:52:07 -0800 Subject: [PATCH] iOS: Add null checks on shell dereference `FlutterEngine` at the `_shell` unique_ptr ivar it owns have different lifetimes. `_shell` is initialised transitively from `runWithEntrypoint`, and reset in `[FlutterEngine destroyContext]`, which is called transitively from `[FlutterviewController dealloc]` via `[FlutterEngine notifyViewControllerDeallocated]`. As such, all uses of `_shell` should be checked either via an assertion, in cases we know the shell should be non-null, or via a runtime null check in cases where it's expected that it may be null. Specifically, this guards against a crash that can occur if we get a CoreAnimation transaction commit callback for an inflight frame just as we're shutting down the app (or removing the FlutterView in an add-to-app scenario). Example stack trace: ``` 0 Flutter 0x11b28 -[FlutterEngine platformView] + 53 (weak_ptr.h:53) 1 Flutter 0x11994 -[FlutterEngine updateViewportMetrics:] + 186 (ref_ptr.h:186) 2 Flutter 0x1f854 -[FlutterViewController updateViewportMetricsIfNeeded] + 427 (vector:427) 3 Flutter 0x1f9b8 -[FlutterViewController viewDidLayoutSubviews] + 1411 (FlutterViewController.mm:1411) 4 UIKitCore 0x8c864 -[UIView(CALayerDelegate) layoutSublayersOfLayer:] + 2376 5 QuartzCore 0x1fa0c CA::Layer::layout_if_needed(CA::Transaction*) + 516 6 QuartzCore 0x1ae84c CA::Context::commit_transaction(CA::Transaction*, double, double*) + 516 7 QuartzCore 0x2888 CA::Transaction::commit() + 648 ``` Issue: https://github.com/flutter/flutter/issues/159639 --- .../ios/framework/Source/FlutterEngine.mm | 25 +++++++++++++++---- .../ios/framework/Source/FlutterEngineTest.mm | 13 ++++++++++ .../framework/Source/FlutterViewController.mm | 7 +++++- .../ios/framework/Source/vsync_waiter_ios.mm | 4 +-- 4 files changed, 41 insertions(+), 8 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index eff08e6cc53e5..27e6767e411fa 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -320,27 +320,37 @@ - (void)dispatchPointerDataPacket:(std::unique_ptr)p } - (fml::WeakPtr)platformView { - FML_DCHECK(_shell); + if (!_shell) { + return {}; + } return _shell->GetPlatformView(); } - (flutter::PlatformViewIOS*)iosPlatformView { - FML_DCHECK(_shell); + if (!_shell) { + return nullptr; + } return static_cast(_shell->GetPlatformView().get()); } - (fml::RefPtr)platformTaskRunner { - FML_DCHECK(_shell); + if (!_shell) { + return {}; + } return _shell->GetTaskRunners().GetPlatformTaskRunner(); } - (fml::RefPtr)uiTaskRunner { - FML_DCHECK(_shell); + if (!_shell) { + return {}; + } return _shell->GetTaskRunners().GetUITaskRunner(); } - (fml::RefPtr)rasterTaskRunner { - FML_DCHECK(_shell); + if (!_shell) { + return {}; + } return _shell->GetTaskRunners().GetRasterTaskRunner(); } @@ -392,6 +402,9 @@ - (void)sendKeyEvent:(const FlutterKeyEvent&)event } - (void)ensureSemanticsEnabled { + if (!self.iosPlatformView) { + return; + } self.iosPlatformView->SetSemanticsEnabled(true); } @@ -419,6 +432,7 @@ - (void)setViewController:(FlutterViewController*)viewController { } - (void)attachView { + FML_DCHECK(self.iosPlatformView); self.iosPlatformView->attachView(); } @@ -1224,6 +1238,7 @@ - (void)cleanUpConnection:(FlutterBinaryMessengerConnection)connection { #pragma mark - FlutterTextureRegistry - (int64_t)registerTexture:(NSObject*)texture { + FML_DCHECK(self.iosPlatformView); int64_t textureId = self.nextTextureId++; self.iosPlatformView->RegisterExternalTexture(textureId, texture); return textureId; diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm index d82993310d499..70dc2a1badfdf 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm @@ -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 = diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 2fd71c0874b16..09bd0633fcdaf 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -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]; }); @@ -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]; @@ -2105,6 +2107,9 @@ - (void)onAccessibilityStatusChanged:(NSNotification*)notification { return; } fml::WeakPtr 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 diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm index 8552295f3a70c..e73ed315a7ffd 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm @@ -70,9 +70,9 @@ @implementation VSyncClient { - (instancetype)initWithTaskRunner:(fml::RefPtr)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);