Made flutter_view_ atomic #22259
Made flutter_view_ atomic #22259
Conversation
…ronization to avoid a race condition crash.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
|
||
| void FlutterPlatformViewsController::SetFlutterView(UIView* flutter_view) { | ||
| flutter_view_.reset([flutter_view retain]); | ||
| [flutter_view_.exchange([flutter_view retain]) release]; |
There was a problem hiding this comment.
nit: what about a comment that this is retaining the new flutter_view and releasing the previous reference?
There was a problem hiding this comment.
That is what the one line of code says =)
| FML_DCHECK(flutter_view_); | ||
| // Warning: flutter_view_ could be deleted on the platform thread between load | ||
| // and retain. Both are atomic but their composition isn't. | ||
| fml::scoped_nsobject<UIView> flutter_view([flutter_view_.load() retain]); |
There was a problem hiding this comment.
Because we are dealing with multiple threads. The platform thread could delete the view while we are running SubmitFrame on the raster thread. This protects us from that (except in the case it is deleted between load and retain).
There was a problem hiding this comment.
interesting. Do you know what event can cause the platform thread to delete the view? I guess in add-to-app that can happen at any time.
| // `flutter_view_` is set by the platform thread but read on the raster | ||
| // thread. It is possible this is executing on the raster thread before the | ||
| // flutter_view_ has been set. In such case we just submit the frame | ||
| // instead of crashing. The may cause incorrect frames to render |
There was a problem hiding this comment.
This is fine, but I wonder why this is even allowed. The shell setup should ensure that things are wired up on the platform thread before the raster thread starts consuming tasks. Do you know where this is breaking?
There was a problem hiding this comment.
Yes, please see the attached issue. TLDR viewDidAppear starts the raster thread, viewDidLayoutSubviews sets up the platforms view controller. There is no guarantee that viewDidLayoutSubviews is executed before SubmitFrame. We do need a larger fix. I outlined our 3 options in the issue.
There was a problem hiding this comment.
I see. That makes sense. This has nothing to do with the setup in shell after looking at the code.
|
|
||
| FlutterPlatformViewsController::~FlutterPlatformViewsController() = default; | ||
| FlutterPlatformViewsController::~FlutterPlatformViewsController() { | ||
| [flutter_view_.load() release]; |
There was a problem hiding this comment.
Previously the object was contained by a smart pointer, this wasn't needed. Now that we need to make it atomic I had to add the explicit release.
|
|
|
I'm going to close this in lieu of #22275. I think that is a better fix. |
2 participants
Description
I made flutter_view_ atomic to provide a minimal amount of thread synchronization to avoid a race condition crash. This is the simplest, least intrusive thing I could think of that would reduce the risk of the race condition and fix it for me.
The outcome isn't ideal, but it's preferable to crashing. We don't even know how much these situations happen in the wild. The crash was happening for me in my add-to-app prototypes that were probably atypical usage. @cyanglaz says this code is on the chopping block for refactoring so hopefully we can do something more correct then.
Related Issues
flutter/flutter#69449
Tests
Existing tests should cover base behavior. I added an issue to try to implement stress testing that could chase down these race conditions: flutter/flutter#69626
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Reviewer Checklist
Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.