Macos BUGFIX: change strong ref to weak ref #165177
Conversation
The erroneous strong reference to self->engine_ leads to an issue where the engine is deallocated in a background thread, triggering an assert failure or crash in certain scenarios.
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
@knopp PTAL |
| @@ -801,7 +801,7 @@ - (void)viewControllerViewDidLoad:(FlutterViewController*)viewController { | |||
| // We are already on UI thread right now, but have to do the | |||
| // extra hop to main thread. | |||
| [engine->_threadSynchronizer performOnPlatformThread:^{ | |||
| engine->_embedderAPI.OnVsync(_engine, baton, timeNanos, targetTimeNanos); | |||
| engine->_embedderAPI.OnVsync(engine->_engine, baton, timeNanos, targetTimeNanos); | |||
There was a problem hiding this comment.
While to code seems innocuous, I am unable to reason about what this fixes (if anything). If the engine has been collected, won't the check on link 798 fail and short circuit that dereference?
There was a problem hiding this comment.
I see whats happening, you need to do another weak capture in this lambda on line 804 with a weak check just like 798. I don't think this changes anything.
There was a problem hiding this comment.
I see whats happening, you need to do another weak capture in this lambda on line 804 with a weak check just like 798. I don't think this changes anything.
We consider 792 as Block 1 and 803 as Block 2. For Block 2 to capture strong self, the prerequisite is that Block 1 also captures strong self. Therefore, the core issue is not the reference from Block 2 but from Block 1.
There was a problem hiding this comment.
The 'block1' FlutterVSyncWaiter's block will execute on the UI thread. Once this block's strong self reference becomes the last remaining reference, disposing of the block will trigger the engine's deallocation on the UI thread.
There was a problem hiding this comment.
The code captures "self" implicitly:
engine->_embedderAPI.OnVsync(_engine, baton, timeNanos, targetTimeNanos);here engine comes from weakSelf, but _engine is an ivar so it captures self implicitly. The fix is good but it's missing test. I'll see if I can put together one today.
There was a problem hiding this comment.
That said, I don't quite understand how this causes a problem? The FlutterViewController already retains the engine. When FlutterViewController gets disposed, it removes the VSyncWaiter, which deletes the block. So the strong reference is released.
@SilverFruity, can you describe scenario where this retain cycle causes a problem? I'm trying to write a test for it but can't seem to get it to point where it actually causes an issue.
There was a problem hiding this comment.
You may want to review the attached crash stack trace. It appears the FlutterViewController has already lost its reference to the Engine, while only the FlutterVSyncWaiter's block continues to hold the engine reference. Additionally, our developers may have only initialized the Engine without involving the ViewController.
Thread 56 name: io.flutter.ui
Thread 56 (crashed)
0 libdispatch.dylib 0x000000018d743828 _dispatch_assert_queue_fail + 120
1 libdispatch.dylib 0x000000018d743820 _dispatch_assert_queue_fail + 112
2 libdispatch.dylib 0x000000018d7437b0 dispatch_assert_queue$V2 + 196
3 FlutterMacOS 0x0000000146e5b0b8 -[FlutterThreadSynchronizer shutdown] (FlutterThreadSynchronizer.mm :223)
4 FlutterMacOS 0x0000000146e3e258 -[FlutterEngine shutDownEngine] (FlutterEngine.mm :1057)
5 FlutterMacOS 0x0000000146e3a83c -[FlutterEngine dealloc] (FlutterEngine.mm :490)
6 FlutterMacOS 0x0000000146e3bdd8 __destroy_helper_block_ea8_32s40s48w (FlutterEngine.mm :0)
7 libsystem_blocks.dylib 0x000000018d5ed17c _call_dispose_helpers_excp + 48
8 libsystem_blocks.dylib 0x000000018d5ecf48 _Block_release + 252
9 FlutterMacOS 0x0000000146e5c820 -[FlutterVSyncWaiter .cxx_destruct] (FlutterVSyncWaiter.mm :36)
10 libobjc.A.dylib 0x000000018d52364c object_cxxDestructFromClass(objc_object*, objc_class*) + 116
11 libobjc.A.dylib 0x000000018d51ab9c objc_destructInstance + 80
13 FlutterMacOS 0x0000000146e5c774 -[FlutterVSyncWaiter dealloc] (FlutterVSyncWaiter.mm :184)
14 libsystem_blocks.dylib 0x000000018d5ed17c _call_dispose_helpers_excp + 48
15 libsystem_blocks.dylib 0x000000018d5ecf48 _Block_release + 252
16 Foundation 0x000000018eb53ce8 -[_NSTimerBlockTarget dealloc] + 48
17 Foundation 0x000000018eb3bb84 _timerRelease + 68
18 CoreFoundation 0x000000018d9ed2b8 __CFRunLoopDoTimer + 1064
19 CoreFoundation 0x000000018d9ecd94 __CFRunLoopDoTimers + 356
20 CoreFoundation 0x000000018d9d01cc __CFRunLoopRun + 1856
21 CoreFoundation 0x000000018d9cf434 CFRunLoopRunSpecific + 608
22 FlutterMacOS 0x0000000146eab75c fml::MessageLoopDarwin::Run() (message_loop_darwin.mm :51)
23 FlutterMacOS 0x0000000146ea1b14 fml::MessageLoopImpl::DoRun() (message_loop_impl.cc :94)
|
I think we have tests for engine dealloc already, I think we should also have test for this. |
The erroneous strong reference to self->engine_ leads to an issue where the engine is deallocated in a background thread, triggering an assert failure or crash in certain scenarios.
Pre-launch Checklist
///).