Repository navigation
Fix SIGSEGV in VSyncClient on ProMotion devices when engine shell is nil - #184639
realmeylisdev wants to merge 7 commits into
Conversation
…nil (flutter#183900) On iOS 26 with the implicit engine pattern, `viewDidLoad` can fire before the engine shell is fully initialized. `createTouchRateCorrectionVSyncClientIfNeeded` passes `self.engine.platformTaskRunner` (an empty `fml::RefPtr` when `_shell` is nil) to VSyncClient, which dereferences the null pointer at `task_runner->PostTask(...)`. Only hits ProMotion devices because ≤60Hz returns early before reaching the VSyncClient creation. Defense-in-depth null checks at three levels: - VSyncClient `initWithTaskRunner:callback:` returns nil for null task_runner - `createTouchRateCorrectionVSyncClientIfNeeded` guards platformTaskRunner - `setUpKeyboardAnimationVsyncClient:` guards uiTaskRunner Includes 3 new unit tests covering the null task runner paths.
github-actions
Bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces null checks for platform and UI task runners in FlutterViewController and VSyncClient to prevent crashes when the engine lacks a shell. It also includes unit tests to verify these scenarios. Feedback suggests removing a conflicting FML_DCHECK that would cause debug builds to crash despite the new null handling, and adding stopMocking calls to the new tests to ensure proper mock cleanup and isolation.
| - (instancetype)initWithTaskRunner:(fml::RefPtr<fml::TaskRunner>)task_runner | ||
| callback:(flutter::VsyncWaiter::Callback)callback { | ||
| FML_DCHECK(task_runner); | ||
| if (!task_runner) { |
There was a problem hiding this comment.
The FML_DCHECK(task_runner) on line 73 will cause debug builds and the new unit test testInitWithNullTaskRunnerReturnsNil to crash when a null task runner is provided. Since this PR explicitly adds support for handling a null task runner by returning nil, the FML_DCHECK should be removed to allow the code to function as intended in debug environments.
| - (void)testCreateTouchRateCorrectionVSyncClientDoesNotCrashWhenEngineHasNoShell { | ||
| id mockDisplayLinkManager = [OCMockObject mockForClass:[DisplayLinkManager class]]; | ||
| double maxFrameRate = 120; | ||
| [[[mockDisplayLinkManager stub] andReturnValue:@(maxFrameRate)] displayRefreshRate]; | ||
| // Create engine without running it — shell will be nil, so platformTaskRunner returns empty RefPtr. | ||
| FlutterEngine* engine = [[FlutterEngine alloc] init]; | ||
| FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engine | ||
| nibName:nil | ||
| bundle:nil]; | ||
| // Should not crash even though engine has no shell. | ||
| [viewController createTouchRateCorrectionVSyncClientIfNeeded]; | ||
| XCTAssertNil(viewController.touchRateCorrectionVSyncClient); | ||
| } |
There was a problem hiding this comment.
When mocking a class with OCMock, it is important to call stopMocking on the mock object at the end of the test. This ensures that the class is restored to its original state and prevents the mock behavior from leaking into other tests.
- (void)testCreateTouchRateCorrectionVSyncClientDoesNotCrashWhenEngineHasNoShell {
id mockDisplayLinkManager = [OCMockObject mockForClass:[DisplayLinkManager class]];
double maxFrameRate = 120;
[[[mockDisplayLinkManager stub] andReturnValue:@(maxFrameRate)] displayRefreshRate];
// Create engine without running it — shell will be nil, so platformTaskRunner returns empty RefPtr.
FlutterEngine* engine = [[FlutterEngine alloc] init];
FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engine
nibName:nil
bundle:nil];
// Should not crash even though engine has no shell.
[viewController createTouchRateCorrectionVSyncClientIfNeeded];
XCTAssertNil(viewController.touchRateCorrectionVSyncClient);
[mockDisplayLinkManager stopMocking];
}
References
- Code should follow the guidance described in the writing effective tests guide, which includes proper cleanup of mocks to ensure test isolation. (link)
…ng to test - Remove FML_DCHECK(task_runner) that would crash debug builds before the null check could return nil - Add [mockDisplayLinkManager stopMocking] to prevent mock leakage
|
@realmeylisdev from triage: can you address the failing checks? |
|
From triage: test are failing and it's not addressed, so we are closing it. Thanks |
Summary
Fixes #183900
viewDidLoadfires before the engine shell is initialized.createTouchRateCorrectionVSyncClientIfNeededpasses an emptyfml::RefPtrtoVSyncClient initWithTaskRunner:callback:, which dereferences the null pointer attask_runner->PostTask(...). Only affects ProMotion devices (>60Hz) because ≤60Hz returns early.VSyncClient initWithTaskRunner:callback:returns nil for null task_runner,createTouchRateCorrectionVSyncClientIfNeededguardsplatformTaskRunner, andsetUpKeyboardAnimationVsyncClient:guardsuiTaskRunner.Files changed
vsync_waiter_ios.mminitWithTaskRunner:callback:FlutterViewController.mmcreateTouchRateCorrectionVSyncClientIfNeededandsetUpKeyboardAnimationVsyncClient:VsyncWaiterIosTest.mmtestInitWithNullTaskRunnerReturnsNilFlutterViewControllerTest.mmTest plan