Engine v1.12.13-hotfixes has bug when using AddToApp #52455
Comments
|
@xster please take a look |
kangwang1988
added
a: existing-apps
|
CC @dnfield |
|
@gaaclarke can you take a look? |
|
Does this bug still exist on master? |
|
I am afraid it still exist |
|
Hm. Both the Android and the iOS embedding believe they can return nullptr in That should only be valid if there really is no View to work with. But in this case, there should be. I haven't traced this all the way through, but it seems like we might not be calling /cc @chinmaygarde @amirh @cyanglaz @gaaclarke probably has more recent context on this than I do as well but those people might be interested in the platform view/rasterizer aspects. |
xujim
changed the title
We need to check if surface is nullptr before invoke the setup as well
|
stacktrace: |
|
The following assert can fail because --- a/shell/common/platform_view.cc
+++ b/shell/common/platform_view.cc
@@ -74,6 +74,7 @@ void PlatformView::NotifyCreated() {
latch.Signal();
});
latch.Wait();
+ assert(surface);
delegate_.OnPlatformViewCreated(std::move(surface));
}Here is the code where PlatformViewIOS returns nullptr: std::unique_ptr<Surface> PlatformViewIOS::CreateRenderingSurface() {
FML_DCHECK(task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread());
std::lock_guard<std::mutex> guard(ios_surface_mutex_);
if (!ios_surface_) {
FML_DLOG(INFO) << "Could not CreateRenderingSurface, this PlatformViewIOS "
"has no ViewController.";
return nullptr;
}
return ios_surface_->CreateGPUSurface();
}callstack: If you put a breakpoint on So this means the FlutterViewController for the PlatformView is getting set to nil just before it is getting shown on screen and |
--- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
+++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
@@ -570,6 +570,8 @@ static void sendFakeTouchEvent(FlutterEngine* engine,
- (void)viewWillAppear:(BOOL)animated {
TRACE_EVENT0("flutter", "viewWillAppear");
+ fml::WeakPtr<flutter::PlatformViewIOS> weakPlatformView = [_engine.get() platformView];
+ assert(weakPlatformView->GetOwnerViewController());
// Send platform settings to Flutter, e.g., platform brightness.
[self onUserSettingsChanged:nil];The following assert will trip before the crash happens. This seems to be a problem with presenting a FlutterViewController that has been previously covered up, like in the case of a UINavigationController stack pop. |
gaaclarke
moved this from Awaiting triage
to Engineer reviewed
in Mobile - iOS engine review
|
Ok, I've looked at the code that reproduces this more closely. The following change fixes the crash: |
|
For the record, here is the patch that can make this not crash (but it behaves in a weird way because of the shared engine still): |
|
@xujim @kangwang1988, was there a reason why flutter_boost needed to have multiple FlutterViewControllers attached to the same FlutterEngine at the same time rather than detaching the bottom one first before attaching the top one? Also, please add a flutter_boost test to our testing repository at https://github.com/flutter/tests which runs with each framework PR. This way, we'll make sure we don't break you guys via something in the engine like this (or even by naming another widget Action). See https://cs.opensource.google/flutter/engine/+/master:testing/scenario_app/ios/Scenarios/ScenariosTests for examples of driving tests from the platform side. |
|
@gaaclarke @xster Thanks. Currently, concerns on the memory consumption, flutter boost create only one engine and have multiple view controller attach it in turn。However, they are NOT attached to same engine at the same time. It is in turn, that is mean, we only attach the top VC to engine, and the previous VC(covered up) will be detached as soon as the new one is set as OwnerViewController to engine. When the VC is covered, we will try to call surfaceUpdated:NO to tear down the surface, and call surfaceUpdated:YES to setup the new surface for the top VC vice versa. |
|
@xujim what you're describing sounds reasonable to me. Does it work if you manually set the engine.viewController to nil or to the top vc before the bottom vc gets covered? Seems like that's what https://github.com/xujim/testaccessibility/compare/master...foxsofter:fix_crash?expand=1#diff-a6425004106bb459a3f91283fe144fd2 is doing right? This bug is essentially asking us to make |
|
Posterity: from chatting with @gaaclarke documentation and ergonomics around this may all end up bundled in the #37644 work as well. |
|
@xster , thanks, fix_crash works for my example, but it needs to hook the UINavigationController, which is not so nice. I finally figured out a workaround, which is better for me——try not call surfaceUpdated:NO for my FlutterViewController. This way, we can make sure the flutter engine holding an ios_surface_ until it is reset by SetOwnerViewController as soon as another FlutterVC is attached to FlutterEngine. |
|
The cons of my solution is that need to override the viewDidDisappear of FlutterViewController, which will lose the TRACE_EVENT0. So, I do believe @gaaclarke 's words that it is not supported flow by flutter:( //in this way, we can call FlutterViewController's parent in FLBFlutterViewContainer
@interface FlutterViewController (bridgeToviewDidDisappear)
- (void)flushOngoingTouches;
- (void)override_viewDidDisappear:(BOOL)animated;
@end
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wincomplete-implementation"
@implementation FlutterViewController (bridgeToviewDidDisappear)
- (void)bridge_viewDidDisappear:(BOOL)animated{
// TRACE_EVENT0("flutter", "viewDidDisappear");
[self flushOngoingTouches];
[super viewDidDisappear:animated];
}
@end
//my VC inherited from FlutterViewController
@implementation FLBFlutterViewContainer
- (void)viewDidDisappear:(BOOL)animated
{
[BoostMessageChannel didDisappearPageContainer:^(NSNumber *result) {}
pageName:_name
params:_params
uniqueId:self.uniqueIDString];
if (FLUTTER_VC.beingPresented || self.beingDismissed /*|| ![self.uniqueIDString isEqualToString:[(FLBFlutterViewContainer*)FLUTTER_VC uniqueIDString]]*/)
{
[FLUTTER_APP resume];
[(FLBFlutterViewContainer*)FLUTTER_VC surfaceUpdated:YES];
}
//call super's super implementation
[self bridge_viewDidDisappear:animated];
}
@end
|
|
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
hi, I found engine v1.12.13-hotfixes is not ok with the add to app. When I pushed multiple FlutterViewController and pop back, the app will crash due to accessing the nulled surface in Rasterizer::Setup. Here is my mini example to reproduce the problem: https://github.com/xujim/testaccessibility
The steps to reproduce the crash
Here is my add to app example to reproduce the crash bug:
https://github.com/xujim/testaccessibility
With this example installed, you just need to "Open Flutter Page", and again and again, then pop back the VC, finally, it crashed.
The Root Cause Analysis
It is caused by the sequence of Setup and Teardown of Rasterizer in the addToApps circumstance.
In the addToApps, we need to push and pop multiple VC, it will lead to setup and teardown of surface more than one time. And the setup and teardown tasks are queued by GPU thread.
However, note that the surface is held by on Engine, and the underlying memory is shared by the tasks(setup and teardown task) in the GPU thread queue. This will cause a situation, that a setup task need to access surface which has just been teardown in the last teardown task. Just as following diagram:
Change the Rasterizer::Setup function this way can avoid the crash, even though it might be not the best fix:
My Flutter Version
The text was updated successfully, but these errors were encountered: