Enforce exclusivity for activity and fragments attached to the FlutterEngine #21272

xster · 2020-09-18T20:04:23Z

The fix is pretty simple. It just lets the activity and fragment detach themselves when another activity or fragment is attaching to the engine. There's some added ceremony since it's such a central piece that I wanted to make it a bit harder to drift accidentally in the future.

Alternatives considered:

cl/332356164. Statics are leaky.
Creating an object in the FlutterActivityAndFragmentDelegate that tracks other FlutterActivityAndFragmentDelegate. This is conceptually appropriate but we already have a path that's meant to be rigorous in doing exactly that with the control surface mechanism. I wanted to avoid creating yet another parallel bookkeeping track and make a 4th activity<->engine linkage (Non exclusive UI components attached to the FlutterEngine causes event crosstalk flutter#66192) rather than reducing it down to 2.
Just disconnecting directly in the FlutterActivityAndFragmentDelegate. It's a package private implementation detail. Letting activity/fragment being able to somehow customize or be signaled about an eviction seems useful.
Extracting the control surface bookkeeping out of plugins related code. This is probably cleaner. Though that abstraction hasn't been super complete. Considering the control surface implementation FlutterEnginePluginRegistry is a sibling to FlutterEngine. The readability benefit of less indirections seems ok.

Deprecation doc flutter/website#4640

mehmetf · 2020-09-20T13:50:32Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

    super.onDestroy();
-    delegate.onDestroyView();
-    delegate.onDetach();
+    Log.e("meh", "Activity " + this + " onDestroy");


Debugging log.

Oops, thanks!

mehmetf · 2020-09-20T13:52:30Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+      Log.v(TAG, "FlutterActivity " + this + " onDestroy called after release.");
+    }
    lifecycle.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY);
  }


We might need to check the delegate nullness in lifecycle methods below as well (onActivityResult and such). At the very least, let's confirm with an Android expert (perhaps clm) that those methods are never called after onPause().

Ya, sensible. Ping'ed him on the bug.

mehmetf · 2020-09-20T13:54:02Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

+          "FlutterActivityAndFragmentDelegate's getAppComponent should only "
+              + "be queried after onAttach, when the host's activity should always be non-null");
+    }
+    return host.getActivity();


Just return activity.

Do you just mean don't do assertions?

No, I mean you are doing final activity = host.getActivity() above. Why not return the local var?

mehmetf · 2020-09-20T13:57:12Z

shell/platform/android/io/flutter/embedding/engine/FlutterEnginePluginRegistry.java

  // -------- Start ActivityControlSurface -------
  private boolean isAttachedToActivity() {
-    return activity != null;
+    return activity != null || exclusiveActivity != null;


What's the difference between activity and exclusiveActivity? As far as plugins are concerned, isn't there always one activity? It would be dangerous to have different plugins be attached to different activity instances.

There is only one. This is just extra fluff since I didn't want to make the deprecation too hard breaking.

mehmetf · 2020-09-20T13:57:33Z

shell/platform/android/io/flutter/embedding/engine/FlutterEnginePluginRegistry.java

-    // If we were already attached to an Android component, detach from it.
-    detachFromAndroidComponent();
+    if (this.exclusiveActivity != null) {
+      this.exclusiveActivity.detachFromFlutterEngine();


It is odd that activity/engine relationship is being managed in plugin registry..

Ya, that's the last piece I noted in the PR summary above. I think the place to put it is right (e.g. consolidating the place where we attach/detach, (e.g. if a person makes a custom activity, there should be burden to attach it to the engine in just one API instead of 3 or 4)).

The name is not right. The easiest solution is to just name this FlutterEngineConnectionRegistry since this file here is sibling here with the engine rather than in the plugins package. For super maintenance, we can also split this into 2 files to make it shorter than 800 LOC. But I don't think we need to do that right now.

I've renamed the package private class FlutterEngineConnectionRegistry to reflect its role.

mehmetf · 2020-09-20T14:01:17Z

shell/platform/android/io/flutter/embedding/engine/plugins/activity/ActivityControlSurface.java

+   *     avoid situations where multiple activities are driving the FlutterEngine simultaneously.
+   *     See https://github.com/flutter/flutter/issues/66192.
   */
+  @Deprecated


I am surprised this is public API?

I don't have any proof that anyone's using it but it is public API. The theoretical usage I think would be fore someone who make a custom activity that holds a FlutterView. This public interface is the APIs that lets that FlutterView's owner tell that FlutterView's FlutterEngine's plugins when an activity is available / unavailable.

That's why there's that activity/exclusiveActivity thing above. It's just transitional so we have a not-too-rough breaking change.

That's fair.

So, there will be an announcement and a deprecation phase so we can clean it up?

Yes, writing website PR at the moment.

… activity/fragment

mehmetf · 2020-09-22T03:42:31Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

  protected void onNewIntent(@NonNull Intent intent) {
    // TODO(mattcarroll): change G3 lint rule that forces us to call super
    super.onNewIntent(intent);
+    ensureAlive();


I don't think this is needed since these methods would not normally be called by user code. They are entirely controlled by Android lifecycle itself and Android guarantees not calling these methods once activity is destroyed.

ensureAlive type thing made some sense in delegate because delegate could be used in many objects (including fragment and activity) it had to assert its own internal state.

makes sense. Thanks

mehmetf

LGTM

…e FlutterEngine (flutter/engine#21272)

…e FlutterEngine (#21272)" This reverts commit 88e5377.

…e FlutterEngine (flutter/engine#21272)

flutter-dashboard bot added the platform-android label Sep 18, 2020

googlebot added the cla: yes label Sep 18, 2020

xster added 5 commits September 19, 2020 23:46

Let FlutterActivity subclasses detach from engine

6924873

Enforce exclusivity for activity and fragments attached to FlutterEngine

c9eaae6

docs

501f886

add test

2c21857

format

436ecd6

xster force-pushed the activity-overlap branch from 284be07 to 436ecd6 Compare September 20, 2020 09:23

xster changed the title ~~Let FlutterActivity subclasses detach from engine~~ Enforce exclusivity for activity and fragments attached to the FlutterEngine Sep 20, 2020

xster requested review from jason-simmons and mehmetf September 20, 2020 09:35

missed a license file

f3e00aa

mehmetf reviewed Sep 20, 2020

View reviewed changes

xster added 3 commits September 20, 2020 11:43

review

1f42415

add some more explicit errors than null reference for using destroyed…

a8d3e4b

… activity/fragment

rename FlutterEnginePluginRegistry to FlutterEngineConectionRegistry

4b9483a

xster mentioned this pull request Sep 21, 2020

Add deprecation page for ActivityControlSurface API flutter/website#4640

Merged

autoformat

9537f33

mehmetf reviewed Sep 22, 2020

View reviewed changes

make events droppable

7281a80

mehmetf approved these changes Sep 22, 2020

View reviewed changes

autoformat

88442f5

xster merged commit 88e5377 into flutter:master Sep 22, 2020

xster deleted the activity-overlap branch September 22, 2020 17:34

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 88e53777d1aa (34 revisions) flutter/flutter#66371

Closed

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

88e5377 Enforce exclusivity for activity and fragments attached to th…

1484f5f

…e FlutterEngine (flutter/engine#21272)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to c0aa47e7fe60 (35 revisions) flutter/flutter#66378

Closed

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

88e5377 Enforce exclusivity for activity and fragments attached to th…

4cc481d

…e FlutterEngine (flutter/engine#21272)

zanderso added a commit that referenced this pull request Sep 22, 2020

Revert "Enforce exclusivity for activity and fragments attached to th…

0916d5f

…e FlutterEngine (#21272)" This reverts commit 88e5377.

zanderso mentioned this pull request Sep 22, 2020

Revert "Enforce exclusivity for activity and fragments attached to the FlutterEngine" #21334

Closed

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 84d50d71d20b (36 revisions) flutter/flutter#66388

Closed

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

88e5377 Enforce exclusivity for activity and fragments attached to th…

0a61d3b

…e FlutterEngine (flutter/engine#21272)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 69e24105951e (40 revisions) flutter/flutter#66396

Closed

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

88e5377 Enforce exclusivity for activity and fragments attached to th…

bb56531

…e FlutterEngine (flutter/engine#21272)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to c62d6652992b (44 revisions) flutter/flutter#66398

Merged

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

88e5377 Enforce exclusivity for activity and fragments attached to th…

f3a3a28

…e FlutterEngine (flutter/engine#21272)

xster mentioned this pull request Oct 12, 2020

Single cached engine can't work with multi FlutterFragment flutter/flutter#49950

Open

Enforce exclusivity for activity and fragments attached to the FlutterEngine #21272

Enforce exclusivity for activity and fragments attached to the FlutterEngine #21272

Uh oh!

Conversation

xster commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

xster Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

mehmetf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xster commented Sep 18, 2020 •

edited

Loading

xster Sep 20, 2020 •

edited

Loading