FlutterViewController notify will dealloc #12232

gaaclarke · 2019-09-12T00:33:27Z

Made the FlutterViewController's dealloc notification more generic and started turning off semantics when the view controller is remotely deleted.

Relevant issue: flutter/flutter#33173

generic and started turning off semantics when the view controller is remotely deleted.

xster · 2019-09-13T01:16:07Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

  self.iosPlatformView->SetOwnerViewController(_viewController);
  [self maybeSetupPlatformViewChannels];
+
+  self.flutterViewControllerWillDeallocObserver =


This is ok but consistency-wise seems a bit unnecessarily different. We already have _shell->GetPlatformView() all over the place in this file. Can't we just call _shell->GetPlatformView()->SetOwnerViewController(null) on notifyViewControllerDeallocated?

I'm not doing that because that would be changing behavior outside the bounds of the problem I was fixing. I'm just moving this file over to the new mechanism I created so we don't have multiple ways to do the same thing. The only logical change I made here was calling _viewController.reset() in notifyViewControllerDeallocated . I didn't technically have to do that to solve the bug, but it will almost certainly cause a bug in a future to keep a weak pointer to a dealloced object. This will turn that future bug to a null pointer exception which will be easier for someone to fix than calling a method into random memory.

Discussed offline. Moving the direct plumbing to NSNotificationCenter is ok. If we're implicitly saying the precedent is also to promote moving other event plumbing to notification center.

xster · 2019-09-13T01:16:51Z

This needs a test

gaaclarke · 2019-09-13T21:24:13Z

This needs a test

Done. I attempted to add a test for PlatformViewIOS inside of the ios unit tests target, there isn't an existing unit test for this class. It isn't possible to test because PlatformViewIOS makes reference to symbols inside of Flutter.framework that aren't exported so any unit test target that references PlatformViewIOS.h will fail to link. We need to come up with a better solution for testing internal c++ classes.

xster

LGTM.

Would be nice to add an integration test to scenario_apps too.

xster · 2019-09-13T22:03:23Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

    [self destroyContext];
  }
+  _viewController.reset();
 }


This should only happen once right? Should we get rid of the observer here too?

We could. I wouldn't recommend it. It would make more sense inside of the observer's block but it isn't necessary there either since the semantics of the message are that the object is dying, it shouldn't be dying twice.

xster · 2019-09-13T22:04:19Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm


  NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
  [center removeObserver:self name:UIApplicationDidReceiveMemoryWarningNotification object:nil];
+  [_flutterViewControllerWillDeallocObserver release];


I have a feeling if this happened before the view controller got dealloc'ed, it's probably a bug right? I don't think this can happen legitimately. Maybe we should print a message here.

Nope, that wouldn't be a bug. Releasing the observer detaches this object from the notification center for that message, so after that releasing the FlutterViewController will just not notify a FlutterEngine that he's being dealloc'd.

xster · 2019-09-13T22:09:56Z

shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m


+extern NSNotificationName const FlutterViewControllerWillDealloc;
+
+/// OCMock can't be used for FlutterEngine sometimes because it retains arguments to invocations


I didn't understand this comment :D

Also, this is more a comment than a class doc right? Maybe //?

Updated the text to be more clear. I think /// is appropriate since its intention is to be a docstring.

xster · 2019-09-13T22:23:55Z

shell/platform/darwin/ios/platform_view_ios.mm

  if (ios_surface_ || !owner_controller) {
    NotifyDestroyed();
    ios_surface_.reset();
    accessibility_bridge_.reset();


should we get rid of that observer here too?

We are getting rid of the observer a couple lines down, line 55. I don't believe there is any reason why it should be happening higher in the method.

xster · 2019-09-13T22:24:58Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

  self.iosPlatformView->SetOwnerViewController(_viewController);
  [self maybeSetupPlatformViewChannels];
+
+  self.flutterViewControllerWillDeallocObserver =


Discussed offline. Moving the direct plumbing to NSNotificationCenter is ok. If we're implicitly saying the precedent is also to promote moving other event plumbing to notification center.

gaaclarke · 2019-09-14T00:32:46Z

Would be nice to add an integration test to scenario_apps too.

I'll give this a stab on the bus.

git@github.com:flutter/engine.git/compare/00c8941daeae...3f76f55 git log 00c8941..3f76f55 --no-merges --oneline 2019-09-15 iska.kaushik@gmail.com Revert "Roll src/third_party/dart a554c8be6b..d0052c1b31 (22 commits)" (flutter/engine#12290) 2019-09-14 30870216+gaaclarke@users.noreply.github.com FlutterViewController notify will dealloc (flutter/engine#12232) 2019-09-14 bkonyi@google.com Roll src/third_party/dart a554c8be6b..d0052c1b31 (22 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

gaaclarke added 2 commits September 11, 2019 17:27

Made the flutter view controllers dealloc notification more

4610d7e

generic and started turning off semantics when the view controller is remotely deleted.

Cleared up implicit copy.

4dc44a6

googlebot added the cla: yes label Sep 12, 2019

gaaclarke requested a review from chinmaygarde September 12, 2019 00:34

gaaclarke mentioned this pull request Sep 12, 2019

Started making the accessibility bridge retain its view. #12225

Closed

gaaclarke requested a review from xster September 12, 2019 00:37

fixed formatting

f1dff6b

xster reviewed Sep 13, 2019

View reviewed changes

gaaclarke added 2 commits September 13, 2019 13:59

Added test for the dealloc callback.

058d0e6

Updated formatting.

b0ce5d6

xster reviewed Sep 13, 2019

View reviewed changes

update docstring

ca3d5d5

gaaclarke merged commit 5075172 into flutter:master Sep 14, 2019

engine-flutter-autoroll mentioned this pull request Sep 16, 2019

Roll engine 00c8941daeae..3f76f55bc038 (3 commits) flutter/flutter#40537

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 16, 2019

5075172 FlutterViewController notify will dealloc (flutter/engine#12232)

0eb4507


		extern NSNotificationName const FlutterViewControllerWillDealloc;

		/// OCMock can't be used for FlutterEngine sometimes because it retains arguments to invocations

FlutterViewController notify will dealloc #12232

FlutterViewController notify will dealloc #12232

Uh oh!

Conversation

gaaclarke commented Sep 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster commented Sep 13, 2019

Uh oh!

gaaclarke commented Sep 13, 2019

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Sep 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants