-
Notifications
You must be signed in to change notification settings - Fork 6k
Allow FlutterViewController to be released when not initialized with an engine #6879
Conversation
ca1892f
to
95bc229
Compare
95bc229
to
044ed26
Compare
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
This now appears to be leak free when releasing the view controller - although I believe we still have some artifacts around from the Dart VM, but I think Chinmay is working on that separately. |
/cc @goderbauer since this now involves some changes to accessibility bridge, although they're all memory related. |
The Accessibility Bridge retain cycles that this fixes are causing the crash noted in flutter/flutter#24028 |
Oops, I didn't mean to give approval (I only meant to remove my earlier disapproval). |
_settingsChannel.reset(); | ||
|
||
_shell.reset(); | ||
_threadHost.Reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit brittle to me, since anyone adding new members probably will need to remember to update this method.
One alternative would be to wrap all of these members within another struct
, and then you can just clobber that struct
with a default-initialized one.
If this were pure C++, I'd even consider doing something like *this = FlutterEngine();
(and explicitly saving/restoring members we want to preserve), but I don't know how that'd work with Objective-C++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - I'll clean this up...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved most of these to a method with a comment - I also found a couple that weren't necessary in there. The basic problem is that MessageChannels get the Engine pointer and retain it - and we don't have a nice, non-breaking way to deal with that via a WeakPtr or something because it's part of the public Objective-C API. I know this approach still isn't quite ideal, but hopefully it's better.
[super viewDidDisappear:animated]; | ||
} | ||
|
||
- (void)dealloc { | ||
[[NSNotificationCenter defaultCenter] removeObserver:self]; | ||
if (_engineIsOwnedByMe) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't fully digested this, but my initial impression is that this feels clunky. My instinct is that we shouldn't need to keep track of this explicitly with proper ref-counting and strong/weak references.
That is, could FlutterViewController
always own its reference to the FlutterEngine
, everything else that doesn't need to keep the engine alive could use weak references, and anybody that does want to keep the engine alive could be responsible for retaining their own reference to the FlutterEngine
?
(Is it important that we be able to shut down the engine without deallocating it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that we end up with a cyclic reference in the case where FlutterViewController
initializes its own engine. Basically, it creates the engine (and it needs a strong reference to the engine), and then tells the engine to use it (weakly). When some external action that dismisses the view controller happens, it doesn't currently notify the engine that it can be dismissed - and so it never gets deallocated.
Just notifying the engine that it's not visible isn't enough either, because the engine currently assumes that once it's started it should never stop unless deallocated - but we end up with a circle of references that prevents that from happening. Basically, we should be treating a deallocating of the view controller as something that needs to shut down the engine IFF the engine is owned by the view controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, a FlutterEngine
is built such that it will keep running whether it has a FlutterViewController
or not. But in this case, we want to tell it to stop running once the FlutterViewController
is gone.
Stated that way, I'll try to see if I can alternatively add a parameter or initializer to FlutterEngine
that would ask whether it should be allowed to run headless or not.
…ed with an engine (flutter/engine#6879)
…ed with an engine (flutter/engine#6879)
…ed with an engine (flutter/engine#6879)
…ed with an engine (flutter/engine#6879)
…ed with an engine (flutter/engine#6879)
@dnfield thanks :) is this available on flutter master channel? not sure how your build works, but we're still having this issue, when on master channel. Borkos-iMac:flutter-comment-wall btomic$ flutter doctor -v [✓] Android toolchain - develop for Android devices (Android SDK version 28.0.3) [✓] iOS toolchain - develop for iOS devices (Xcode 10.1) [✓] Android Studio (version 3.0) [✓] Connected device (1 available) • No issues found! edit: |
Yes, it usually takes about a day to get the engine rolled in. Should be soon |
…ed with an engine (flutter/engine#6879)
…ed with an engine (flutter/engine#6879)
…ed with an engine (flutter/engine#6879)
…ed with an engine (flutter/engine#6879)
…ed with an engine (flutter/engine#6879)
This landed in master as of flutter/flutter#26596 |
Tested. Works as expected. Thank you very much ;) |
@@ -348,6 +376,11 @@ - (BOOL)createShell:(NSString*)entrypoint libraryURI:(NSString*)libraryURI { | |||
FML_LOG(ERROR) << "Could not start a shell FlutterEngine with entrypoint: " | |||
<< entrypoint.UTF8String; | |||
} else { | |||
[self setupChannels]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random blast from the past: didn't we do this on init already? Why do we need to do it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it was because the channels could get reset between there and here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would it though. Looking at the code, it only happens on destroyContext and based on the doc, the object is in an unusable state until it's deallocated. This second setupChannels seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember now and I can't see a code path where this happens. I think we can probably kill one of these - I suspec tthe one in the initializer is wrong because actually using a channel before the engine is launched would be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG, thanks for double checking
Fixes flutter/flutter#20984
If the FlutterViewController creates the engine itself, we have to make sure we don't create a cyclic reference that can't be removed.