Fix an issue that Flutter Engine can't be recycled as strongly referenced by __NSMallocBlock__ #13062
Conversation
…nced by __NSMallocBlock__。
|
This issue is introduced in: 5075172fe |
kangwang1988
requested review from
dnfield and
gaaclarke
and removed request for
gaaclarke
| usingBlock:^(NSNotification* note) { | ||
| [self notifyViewControllerDeallocated]; | ||
| }]; | ||
| - (void)onFlutterVCWillDeallocNotif:(NSNotification *)aNotification { |
There was a problem hiding this comment.
nit: avoid abbreviations - maybe just onFlutterViewControllerWillDealloc?
|
Needs formatting as well. |
This would have caught the regression fixed in flutter/engine#13062 This can't land until that rolls in. @ianh - If this causes more problems for your upcoming sharding PR I'm happy to close it.
dnfield
dismissed
their stale review
@gaaclarke pointed out that the release is in the wrong class. He's going to add some more comments.
There was a problem hiding this comment.
Thanks for the catch but this change is erroneous. There isn't a remove observer to match the add observer. The one you've added is to the wrong class. This should be a 2 line fix. At line 178 add the following
__block FlutterEngine* blockSelf = self;
Then convert the call to:
[blockSelf notifyViewControllerDeallocated];
However, the fix you mentioned would need to set CLANG_ENABLE_OBJC_WEAK flag, or -fobjc-weak flag for the specific .mm file as it's a MRC file. I would rather prefer this fix. |
| _viewController = [viewController getWeakPtr]; | ||
| self.iosPlatformView->SetOwnerViewController(_viewController); | ||
| [self maybeSetupPlatformViewChannels]; | ||
| [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(onFlutterVCWillDeallocNotif:) name:FlutterViewControllerWillDealloc object:viewController]; |
There was a problem hiding this comment.
This isn't removing the observer for the previous viewController.
- (void)setViewController:(FlutterViewController*)viewController {
if (_viewController == viewController) {
return;
}
FML_DCHECK(self.iosPlatformView);
if (_viewController) {
[[NSNotificationCenter defaultCenter] removeObserver:self name:FlutterViewControllerWillDealloc object:_viewController];
}
_viewController = [viewController getWeakPtr];
...| [self notifyViewControllerDeallocated]; | ||
| }]; | ||
| - (void)onFlutterVCWillDeallocNotif:(NSNotification *)aNotification { | ||
| [self notifyViewControllerDeallocated]; |
There was a problem hiding this comment.
Is there a reason to have this new method at all? Can you add a notification parameter to notifyViewControllerDeallocated (assuming this isn't exposed in a header, I'm not looking at the code at the moment)
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(notifyViewControllerDeallocated:) name:FlutterViewControllerWillDealloc object:viewController];- (void)notifyViewControllerDeallocated:(NSNotification *)notification {Also, does this still need to dispatch to the mainQueue if this isn't the main queue? There's no guarantee about what queue the notification will be posted on.
Nope, because we are using |
|
Hey @kangwang1988 I've moved the fix over to #13073. I was having an offline discussion with @jmagman about issues related to this problem you found and it was just easier if I took control so I could work with her on your behalf. Thanks again for finding the issue, we should have this resolved by tomorrow. |
5 participants
Fix an issue that Flutter Engine can't be recycled as strongly referenced by NSMallocBlock。