[google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin #3213

math1man · 2020-10-27T23:48:31Z

Description

Overhaul lifecycle management in GoogleMapsPlugin.

GoogleMapController is now uniformly driven by implementing DefaultLifecycleObserver. That observer is registered to a lifecycle passed via the new LifecycleProvider interface, which has 3 implementations:

For v2 plugin registration, GoogleMapsPlugin implements the interface and holds the lifecycle in a field, which is itself controlled by ActivityAware methods.
For v1 plugin registration, if the activity implements LifecycleOwner, it's lifecycle is used directly.
For v1 plugin registration, if the activity does not implement LifecycleOwner, a proxy lifecycle is created via ActivityLifecycleCallbacks.

I'm suspicious that there may still be a bug where a GoogleMapController instance can be shared across multiple activities and therefore leak the activity and lifecycle.

Related Issues

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.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

…fecycleObserver. That observer is registered to a lifecycle passed via the new LifecycleProvider interface, which has 3 implementations: 1. For v2 plugin registration, GoogleMapsPlugin implements the interface and holds the lifecycle in a field, which is itself controlled by ActivityAware methods. 2. For v1 plugin registration, if the activity implements LifecycleOwner, it's lifecycle is used directly. 3. For v1 plugin registration, if the activity does not implement LifecycleOwner, a proxy lifecycle is created via ActivityLifecycleCallbacks. I'm suspicious that there may still be a bug where a GoogleMapController instance can be shared across multiple activities and therefore leak the activity and lifecycle.

…Event() rather than moveToState().

amirh

This is great! thanks!

Left a few minor comments.

amirh · 2020-10-28T20:33:34Z

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md

@@ -1,3 +1,13 @@
+## 1.1.0


This doesn't introduce new features so should be a patch bump (1.0.5)

amirh · 2020-10-28T20:41:02Z

...oogle_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapsPlugin.java

-    pluginBinding = binding;
+    binding
+        .getPlatformViewRegistry()
+        .registerViewFactory(VIEW_TYPE, new GoogleMapFactory(binding.getBinaryMessenger(), this));


What do you think about creating a separate LifecycleProvider object which is just the lifecycle provider and passing that here (instead of having GoogleMapsPlugin implement LifecycleProvider). My worry is that future developers adding members to GoogleMapsPlugins will miss the fact that a reference to it is being retained by GoogleMapFactory...

Makes sense, done

amirh · 2020-10-28T20:47:31Z

...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

-      getApplication().registerActivityLifecycleCallbacks(this);
-    }
+  void init() {
+    lifecycleProvider.getLifecycle().addObserver(this);


Not sure so double checking - will we get the initial notification for the current state?

Yes, from addObserver's documentation:

The given observer will be brought to the current state of the LifecycleOwner. For example, if the LifecycleOwner is in State#STARTED state, the given observer will receive Event#ON_CREATE, Event#ON_START events

amirh · 2020-10-28T20:54:35Z

...le_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

-    getApplication().unregisterActivityLifecycleCallbacks(this);
+    Lifecycle lifecycle = lifecycleProvider.getLifecycle();
+    if (lifecycle != null) {
+      lifecycle.removeObserver(this);


(not sure) do we need to protect against calling removeObserver twice? (here and from onDestroy)

Nope, operation is idempotent

amirh · 2020-10-28T20:55:40Z

packages/google_maps_flutter/google_maps_flutter/pubspec.yaml

@@ -1,7 +1,7 @@
 name: google_maps_flutter
 description: A Flutter plugin for integrating Google Maps in iOS and Android applications.
 homepage: https://github.com/flutter/plugins/tree/master/packages/google_maps_flutter/google_maps_flutter
-version: 1.0.4
+version: 1.1.0


don't forget to update the version here as well

amirh · 2020-10-28T20:58:14Z

...oogle_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapsPlugin.java

-  public void onStop(@NonNull LifecycleOwner owner) {
-    state.set(CREATED);
-  }
+  private static final class ProxyLifecycleProvider


nit: Can you add some comments here describing that this is used make a lifecycle our of the application lifecycle calls?

amirh · 2020-10-28T20:58:58Z

...oogle_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapsPlugin.java

+                  }));
+    } else {
+      ProxyLifecycleProvider proxyLifecycleProvider = new ProxyLifecycleProvider(activity);
+      activity.getApplication().registerActivityLifecycleCallbacks(proxyLifecycleProvider);


optional nit: I'd consider moving this into ProxyLifecycleProvider to keep all the hackery in one class.

…ycleCallbacks registration inside its constructor.

…ead create an anonymous inner class.

amirh · 2020-10-28T21:41:25Z

...oogle_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapsPlugin.java

+            VIEW_TYPE,
+            new GoogleMapFactory(
+                binding.getBinaryMessenger(),
+                new LifecycleProvider() {


Is this going to keep an implicit reference to the outer class?

It actually keeps an explicit reference to the outer class, because lifecycle is a field in the outer class. I don't think that's avoidable because only the plugin itself can implement ActivityAware.

I'm probably missing something obvious... what fails if:

GoogleMapsPlugin holds a reference to the provider (an implementation with a lifecycle field) and sets the lifecycle when attached/detached to the activity, and we pass that provider reference to GoogleMapFactory, won't we avoid a reference from GoogleMapFactory to GoogleMapsPlugin in this case?

I could do that, but it doesn't really matter and ends up being more boiler plate. My assumption is that the scope of GoogleMapsPlugin is greater than the scope of GoogleMapFactory, so it doesn't matter if GoogleMapFactory transitively captures the GooglemapsPlugin instance. If that assumption is violated, all sorts of worse things happen than this particular leak.

I guess my question is what problem are you trying to solve?

You are correct, thanks for pushing back and keeping the code simple!

amirh

LGTM

amirh · 2020-10-29T03:11:54Z

"Windows Plugins" was fixed and the re-run is green (the presubmit indicator is stale).

* master: (48 commits) [video_player] Add toString() to Caption (flutter#3233) [google_maps_flutter_web] Show one InfoWindow at a time. (flutter#3224) [in_app_purchase] Bump version (flutter#3227) [google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin (flutter#3213) [in_app_purchase] Remove the custom analysis options, fix failing lints. (flutter#3220) [video_player]Fixes Playing video from asset on Android (flutter#3123) [camera] Added documentation about video not working correctly on Android emulators (flutter#3180) Revert "update api" update api [wifi_info_flutter] Method channel name fixed for android (flutter#3207) [share] Fix bug on iPad where `origin` is null and enable XCUITests in the repo (flutter#3210) [google_maps_flutter] Clean up google_maps_flutter plugin (flutter#3206) Exclude generated_plugin_registrant.cc (flutter#3198) broaden the constraint on package:vm_service (flutter#3208) Remove unnecessary work around from test in prep for vm_service migration (flutter#3209) Add windows directory to examples (flutter#3149) [video_player] Upgrade ExoPlayer (flutter#3204) [android_alarm_manager] Removed deprecated display1 (flutter#3200) [Connectivity] wifi removal (flutter#3173) [wifi_info_flutter] make it ready for initial 1.0.0 release (flutter#3191) ...

slightfoot · 2020-11-04T22:43:29Z

@math1man Is there a reason you didn't use this? https://github.com/flutter/plugins/tree/master/packages/flutter_plugin_android_lifecycle

math1man · 2020-11-04T22:49:47Z

@math1man Is there a reason you didn't use this? https://github.com/flutter/plugins/tree/master/packages/flutter_plugin_android_lifecycle

I do exactly that here.

slightfoot · 2020-11-05T12:49:10Z

@math1man Is there a reason you didn't use this? https://github.com/flutter/plugins/tree/master/packages/flutter_plugin_android_lifecycle

I do exactly that here.

My point was your suppose to import that package and use its adapter.

math1man · 2020-11-05T17:14:41Z

Lines 90-93 of GoogleMapsPlugin are this:

  @Override
  public void onAttachedToActivity(ActivityPluginBinding binding) {
    lifecycle = FlutterLifecycleAdapter.getActivityLifecycle(binding);
  }

Could you elaborate on what you mean?

…in (flutter#3213) GoogleMapController is now uniformly driven by implementing DefaultLifecycleObserver. That observer is registered to a lifecycle passed via the new LifecycleProvider interface, which has 3 implementations: 1. For v2 plugin registration, GoogleMapsPlugin implements the interface and holds the lifecycle in a field, which is itself controlled by ActivityAware methods. 2. For v1 plugin registration, if the activity implements LifecycleOwner, it's lifecycle is used directly. 3. For v1 plugin registration, if the activity does not implement LifecycleOwner, a proxy lifecycle is created via ActivityLifecycleCallbacks.

google-cla bot added the cla: yes label Oct 27, 2020

math1man added 2 commits October 27, 2020 16:50

Update CHANGELOG.md and version

e934a18

Merge remote-tracking branch 'origin/master'

9bdc386

math1man changed the title ~~GoogleMapController is now uniformly driven by implementing DefaultLi…~~ [google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin Oct 28, 2020

math1man added 4 commits October 28, 2020 09:50

Fix formatting issues

96cf26b

Update GoogleMapControllerTest

62476dc

Merge remote-tracking branch 'origin/master'

a8e02a4

Remove extraneous import

b920973

math1man marked this pull request as ready for review October 28, 2020 18:36

math1man requested a review from cyanglaz as a code owner October 28, 2020 18:36

amirh mentioned this pull request Oct 28, 2020

Windows Plugins presubmit is failing on flutter/plugins flutter/flutter#69232

Closed

math1man added 2 commits October 28, 2020 12:49

Update GoogleMapsPlugin.ProxyLifecycleProvider to use handleLifecycle…

ea016b8

…Event() rather than moveToState().

Merge remote-tracking branch 'origin/master'

d6c9014

amirh suggested changes Oct 28, 2020

View reviewed changes

math1man added 5 commits October 28, 2020 14:08

Update version from 1.1.0 to 1.0.5

5f479f0

Merge remote-tracking branch 'origin/master'

2c182a9

Add javadoc for ProxyLifecycleProvider and move initial ActivityLifec…

039e3ce

…ycleCallbacks registration inside its constructor.

Merge remote-tracking branch 'origin/master'

85e592e

Remove LifecycleProvider as an interface of GoogleMapsPlugin and inst…

df2345d

…ead create an anonymous inner class.

amirh reviewed Oct 28, 2020

View reviewed changes

amirh approved these changes Oct 28, 2020

View reviewed changes

amirh merged commit 8a613b9 into flutter:master Oct 29, 2020

slightfoot mentioned this pull request Nov 4, 2020

[google_maps_flutter] App crashes when using maps in release mode. flutter/flutter#69820

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin #3213

[google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin #3213

math1man commented Oct 27, 2020

amirh left a comment

amirh Oct 28, 2020

math1man Oct 28, 2020

amirh Oct 28, 2020

math1man Oct 28, 2020

amirh Oct 28, 2020

math1man Oct 28, 2020

amirh Oct 28, 2020

math1man Oct 28, 2020

amirh Oct 28, 2020

math1man Oct 28, 2020

amirh Oct 28, 2020

math1man Oct 28, 2020

amirh Oct 28, 2020

math1man Oct 28, 2020

amirh Oct 28, 2020

math1man Oct 28, 2020 •

edited

Loading

amirh Oct 28, 2020

math1man Oct 28, 2020

amirh Oct 28, 2020

amirh left a comment

amirh commented Oct 29, 2020

slightfoot commented Nov 4, 2020

math1man commented Nov 4, 2020 •

edited

Loading

slightfoot commented Nov 5, 2020

math1man commented Nov 5, 2020

[google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin #3213

[google_maps_flutter] Overhaul lifecycle management in GoogleMapsPlugin #3213

Conversation

math1man commented Oct 27, 2020

Description

Related Issues

Checklist

Breaking Change

amirh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

math1man Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirh left a comment

Choose a reason for hiding this comment

amirh commented Oct 29, 2020

slightfoot commented Nov 4, 2020

math1man commented Nov 4, 2020 • edited Loading

slightfoot commented Nov 5, 2020

math1man commented Nov 5, 2020

math1man Oct 28, 2020 •

edited

Loading

math1man commented Nov 4, 2020 •

edited

Loading