Wire up channel for restoration data #18042

goderbauer · 2020-04-29T22:50:11Z

Android provides the engine with the restoration data in the Bundle given to the onCreate callback. The framework can retrieve this data via the new channel. When the framework has new restoration data to store it can send it over the channel to the engine. The engine will hold onto it until Android asks for the data by invoking the onSaveInstanceState callback.

cbracken · 2020-04-30T18:58:41Z

/cc @jason-simmons and @GaryQian who may have thoughts on this approach.

goderbauer · 2020-04-30T23:34:04Z

/cc @xster, who may also have thoughts on this.

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

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

goderbauer · 2020-05-04T23:25:52Z

I changed this PR to accommodate pre-cached engines (or other engines that have a different lifecycle then the activity):

To properly support pre-warmed engines that run dart code before they are attached to an activity and thus before we have a chance to provide them with restoration data, the RestorationChannel can be configured to delay responding to a framework request for the current restoration data until the engine has provided said data (controlled via the willProvideRestorationData on the engine).
The activity/fragment can enable/disable state restoration by overriding shouldRestoreAndSaveState. Cached engines have state restoration turned off by default, but this can be overridden via a setting in the intent that launched the activity.

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

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

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

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

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

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

goderbauer · 2020-06-10T17:26:45Z

@xster This one is ready for another review. (I still need to add tests, though)

xster

Generally LGTM. Just some nits to make sure we leave more breadcrumbs for future maintainers.

xster · 2020-06-12T02:00:23Z

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

@@ -949,6 +950,17 @@ public void onFlutterUiNoLongerDisplayed() {
    // no-op
  }

+  @Override
+  public boolean shouldRestoreAndSaveState() {


Please document this in a doc section in the class doc

xster · 2020-06-12T02:00:40Z

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

@@ -949,6 +950,17 @@ public void onFlutterUiNoLongerDisplayed() {
    // no-op
  }

+  @Override
+  public boolean shouldRestoreAndSaveState() {
+    if (getIntent().hasExtra(EXTRA_ENABLE_STATE_RESTORATION)) {


sprinkle some comments here for while some values were chosen

xster · 2020-06-12T02:04:12Z

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

    ensureAlive();

+    Bundle pluginsBundle = null;
+    byte[] frameworkBundle = null;


wouldn't call it bundle which is a Map<String, Parcelable> (more or less). Call it frameworkState or frameworkBundleData or something.

xster · 2020-06-12T02:05:59Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

+  /** Set the restoration data from which the framework will restore its state. */
+  public void setRestorationData(byte[] data) {
+    engineHasProvidedData = true;
+    if (pendingResult != null) {


if one just jumps into this method here, it's not clear what this is supposed to do. Add breadcrumbs for future maintainer

xster · 2020-06-12T02:09:52Z

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

+      bundle.putByteArray(
+          FRAMEWORK_RESTORATION_BUNDLE_KEY,
+          flutterEngine.getRestorationChannel().getRestorationData());
+    }


I don't really remember. We may have talked about this. Can't we clear the data now that we read this?

If we delete the data, engine and framework are getting out of sync which may lead to strange behavior and will make it harder to reason about restoration state. In general, framework and engine should always agree on what the current restoration state is.

Can you elaborate with a specific example? Perhaps write this down somewhere here for future archeologists too. I get the impression that we'll never run into a case where framework writes, engine reads and deletes, then the framework tries to read again? Is that right?

I gave a longer answer to this on the other comment thread. In reality, the sequence of events that you describe will of course not happen. However, in reality, the only moment where we could delete the engine's copy of the restoration state is after we have handed it to the operating system. At that point, our app is moments away from getting killed anyways (thus freeing up all the memory). So why add the extra complexity of allowing framework and engine restoration state to go out of sync in this situation?

xster · 2020-06-12T02:26:33Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

+      pendingResult.success(data);
+      pendingResult = null;
+      restorationData = data;
+    } else if (frameworkHasRequestedData) {


briefly document when this happens again, or just reference the appropriate classdoc

xster · 2020-06-12T02:28:38Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

+   * <p>This should be called just prior to a hot restart. Otherwise, after the hot restart the
+   * state prior to the hot restart will get restored.
+   */
+  public void clearData() {


Why not do it more frequently? There's some 2 directional complexity here but I still think it should be possible to find logical situations where this data is deterministically not useful no?

Not really, as said elsewhere, the engine and framework should always be kept in sync about the restoration data. If we clear it on one side, we'd also need to clear it on the other. And hot restart is the only one that clears it on both sides.

We could potentially clear it if we know that the engine gets should down right after reading it. But the shutdown will clear it anyways, so the complexity doesn't really seem worth it.

If we clear it on one side, we'd also need to clear it on the other

what does this mean specifically? There's only memory in one place no?

The restoration state is duplicated on the engine and framework side because of the threading issue: The operating system will ask for the restoration state on the platform thread, the state itself is owned by the framework running on the UI thread, though. When the operating system asks for it, we can't reach into the UI thread to retrieve it (see https://docs.google.com/document/d/1KIiq5CdqnSXxQXbZIDy2Ukc-JHFyLak1JR8e2cm3eO4/edit#heading=h.xhk980xrzj8j). Therefore, we must maintain an up-to-date copy of the restoration state in the engine. The two copies are kept in sync via this channel: When the operating system provides restoration state to the engine, it sends it over the framework. When the restoration state in the framework changes, it's send back to the engine (so it can be given to the operating system when it asks for it again).

When we clear the state on one side without telling the other, we break that contract and I don't see a good reason to do it. The only place where we could delete the engine copy is after we've handed it to the operating system. In reality, the OS only asks for it moments before it kills the entire app anyway (and thus frees up the memory). So why introduce the extra complexity of sometimes engine and framework restoration state can be out of sync?

discussed offline. SG

xster · 2020-06-12T02:29:20Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

+        public void onMethodCall(@NonNull MethodCall call, @NonNull MethodChannel.Result result) {
+          final String method = call.method;
+          final Object args = call.arguments;
+          Log.v(TAG, "Received '" + method + "' message.");


Did you want to leave this here? If so, maybe make it less generic?

xster · 2020-06-12T02:31:29Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

+              if (engineHasProvidedData || !waitForRestorationData) {
+                result.success(restorationData);
+              } else {
+                pendingResult = result;


the names are a bit confusing. Can we rename waitForRestorationData to shouldFrameworkWaitForAsyncRestorationData and rename pendingResult to isFrameworkWaitingForRestorationData?

Those names sound even more confusing to me since isFrameworkWaitingForRestorationData sounds like a boolean, which it isn't. Having said that, I renamed pendingResult to pendingFrameworkRequest which should clear up the confusion.

xster · 2020-06-12T02:33:21Z

shell/platform/android/io/flutter/plugin/common/MethodChannel.java

@@ -26,7 +26,7 @@
 * <p>The logical identity of the channel is given by its name. Identically named channels will
 * interfere with each other's communication.
 */
-public final class MethodChannel {


is this to do https://www.baeldung.com/mockito-final#final-class?

Yes, final classes cannot be mocked.

ya that's outside the scope of this PR. Left flutter/flutter#59356. Should be an "easy" fix.

goderbauer · 2020-06-12T19:04:35Z

@xster All comments addressed. PTAL

xster · 2020-06-12T19:21:56Z

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

@@ -63,6 +64,7 @@
 *   <li>Chooses Flutter's initial route.
 *   <li>Renders {@code Activity} transparently, if desired.
 *   <li>Offers hooks for subclasses to provide and configure a {@link FlutterEngine}.
+ *   <li>Save and restore instance state, see {@code shouldRestoreAndSaveState()};


You mean {@link #shouldRestoreAndSaveState()} right?

xster · 2020-06-12T19:26:14Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

+  /** Set the restoration data from which the framework will restore its state. */
+  public void setRestorationData(byte[] data) {
+    engineHasProvidedData = true;
+    if (pendingFrameworkRequest != null) {


ah I misread this to be a bool. Maybe "pendingFrameworkRestorationChannelRequest"?

xster · 2020-06-12T19:28:38Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java

+      pendingFrameworkRequest = null;
+      restorationData = data;
+    } else if (frameworkHasRequestedData) {
+      // If the framework has previously received the engine's restoration data, push the new data


I suppose this means the framework already asked for and have received a restoration data in the past (as opposed to the branch above). When should this happen in a non-add-to-app scenario? Or is this the use at-your-own-risk special case we talked about before? Maybe call that out in the comment for the impetuous future maintainer who might be thinking: "activity gets a bundle onCreate only once, let me delete this redundant handle".

Provided more context.

did you push? I don't see anything

I can see it. Maybe reload? :P

xster · 2020-06-12T19:29:47Z

A few more minor clarifications. Sorry if we already talked about this before. My memory is crapping out these days.

xster · 2020-06-12T20:14:43Z

LGTM

* 141ee78 Roll Dart SDK from 2b917f5b6a0e to 0fab083e1c2b (15 revisions) (flutter/engine#18974) * f5ab179 Call Shell::NotifyLowMemoryWarning on Android Trim and LowMemory events (flutter/engine#18979) * 4dabac9 Roll Fuchsia Linux SDK from 8AiUM... to ThzHh... (flutter/engine#18976) * 50cae02 Reland: Add RAII wrapper for EGLSurface (flutter/engine#18977) * db7f226 Include the memory header as unique_ptr is used in ASCII Trie. (flutter/engine#18978) * b19a17d Implement an EGL resource context for the Linux shell. (flutter/engine#18918) * ca26b75 Make Linux shell plugin constructor descriptions consistent (flutter/engine#18940) * efd1452 Roll Skia from 9c401e7e1ace to f69e40841eb9 (11 revisions) (flutter/engine#18983) * e5845af Put JNI functions under an interface (flutter/engine#18903) * 1ddc6e1 Call destructor and fix check (flutter/engine#18985) * 7f71f1d Roll Fuchsia Mac SDK from oWhyp... to gAD3P... (flutter/engine#18982) * 74291b7 Roll Dart SDK from 0fab083e1c2b to e62b89c56d01 (10 revisions) (flutter/engine#18987) * 2739bbf [web] Provide a hook to disable location strategy (flutter/engine#18969) * 336d000 Roll Skia from f69e40841eb9 to 553496b66f12 (2 revisions) (flutter/engine#18992) * b82769b Roll Skia from 553496b66f12 to 0ad37b87549b (2 revisions) (flutter/engine#18993) * f5ca58b Roll Dart SDK from e62b89c56d01 to b0d2b97d2cd7 (4 revisions) (flutter/engine#18994) * 2a82a08 [dart] Account for compiler api change (flutter/engine#19002) * 369e0a9 Add ui_benchmarks (flutter/engine#18945) * a0a92d6 Roll Skia from 0ad37b87549b to de175abede4d (1 revision) (flutter/engine#18995) * cea5a9c Roll Dart SDK from b0d2b97d2cd7 to f043f9e5f6ea (6 revisions) (flutter/engine#18997) * d417772 Roll Fuchsia Mac SDK from gAD3P... to Wj0yo... (flutter/engine#19001) * 4538622 Roll Skia from de175abede4d to 32d5cfa1f35e (15 revisions) (flutter/engine#19005) * 71fce02 Fix shift-tab not working by adding more GTK->GLFW key mappings. (flutter/engine#18988) * 5ddc122 Fix inverted check in creating resource surface (flutter/engine#18989) * 87d8888 Show warning if method response errors occur and error not handled. (flutter/engine#18946) * 5171fbd Roll Skia from 32d5cfa1f35e to 21bbfc6c2dfe (5 revisions) (flutter/engine#19006) * 4bd6aea Always send key events, even if they're used for text input. (flutter/engine#18991) * 9f898e9 Don't process key events when the text input is not requested (flutter/engine#18990) * 48d7091 Roll buildroot for Windows warning update (flutter/engine#19000) * 5f37029 Roll Dart SDK from f043f9e5f6ea to f0ea02bc51f8 (22 revisions) (flutter/engine#19010) * ac809b4 onBeginFrame JNI (flutter/engine#18866) * e1c622b Make SKSL caching test work on Fuchsia on arm64 (flutter/engine#18572) * 2651beb Exit before pushing a trace event when layer tree holder is empty (flutter/engine#19008) * acf048b Remove log added for local testing (flutter/engine#19012) * 1f3aa23 Roll Dart SDK from f0ea02bc51f8 to 0b64f5488965 (9 revisions) (flutter/engine#19013) * 8dcc95d Roll Fuchsia Mac SDK from Wj0yo... to gR0Zc... (flutter/engine#19015) * f6455fa Roll Dart SDK from 0b64f5488965 to 50836c171e91 (4 revisions) (flutter/engine#19017) * b2fea9d Roll Skia from 21bbfc6c2dfe to 30212b7941d6 (6 revisions) (flutter/engine#19009) * 0065646 Roll Skia from 30212b7941d6 to 3d6bf04366f6 (17 revisions) (flutter/engine#19020) * 0a852d8 Revert "Call Shell::NotifyLowMemoryWarning on Android Trim and LowMemory events (flutter#18979)" (flutter/engine#19023) * 194acdf apply null safety syntax to mobile dart:ui (flutter/engine#18933) * b0a0e0e Roll Skia from 3d6bf04366f6 to 637838d20abd (2 revisions) (flutter/engine#19021) * be499ab Roll Fuchsia Mac SDK from gR0Zc... to H-uAk... (flutter/engine#19022) * 8c24c41 Roll Skia from 637838d20abd to ac16760df463 (1 revision) (flutter/engine#19025) * 7cb7003 onEndFrame JNI (flutter/engine#18867) * e3fdb23 [fuchsia] Add ability to configure separate data and asset dirs (flutter/engine#18858) * 983b6e1 Call Shell::NotifyLowMemory when backgrounded/memory pressure occurs on Android (flutter/engine#19026) * f7d241f Wire up channel for restoration data (flutter/engine#18042) * e84d497 Fix hit testing logic in fuchsia a11y (flutter/engine#19029) * 801559a Revert to last-known-good-rev of Dart SDK (flutter/engine#19031)

googlebot added the cla: yes label Apr 29, 2020

xster self-requested a review April 30, 2020 23:45

xster reviewed Apr 30, 2020

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java Show resolved Hide resolved

xster reviewed Apr 30, 2020

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java Outdated Show resolved Hide resolved

xster reviewed Apr 30, 2020

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java Outdated Show resolved Hide resolved

xster reviewed May 5, 2020

View reviewed changes

goderbauer marked this pull request as ready for review June 10, 2020 17:24

fluttergithubbot added the platform-android label Jun 10, 2020

This comment has been minimized.

Sign in to view

auto-assign bot requested a review from gw280 June 10, 2020 17:25

goderbauer force-pushed the restoration branch from 7fc43ff to 6dd6e3c Compare June 10, 2020 17:26

xster reviewed Jun 12, 2020

View reviewed changes

goderbauer force-pushed the restoration branch from 1685525 to 1da33b5 Compare June 12, 2020 18:51

xster reviewed Jun 12, 2020

View reviewed changes

goderbauer added 7 commits June 12, 2020 13:27

Wire up channel for restoration data

1a06df7

format + add file

c7ee80b

license

b207c19

Make it more configurable

b2a4b80

format

d7459ca

review

95563f6

refactor

91fe70f

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

08b70f6

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to 8b7b244489c1 (86 revisions) flutter/flutter#59634

Closed

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

957301c

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to e3f3fecb18cb (87 revisions) flutter/flutter#59639

Closed

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

1336cda

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to b36c83756f0d (88 revisions) flutter/flutter#59644

Closed

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

0b2b7c8

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to 43ad8347e148 (89 revisions) flutter/flutter#59652

Closed

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

c25f439

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to 137320b5c7a4 (90 revisions) flutter/flutter#59656

Closed

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

951f11f

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to 9dc08393bedd (91 revisions) flutter/flutter#59660

Closed

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

1b525bf

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to ef29ce68aa24 (92 revisions) flutter/flutter#59664

Closed

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

ba92a8f

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to f4a5f9f4486d (93 revisions) flutter/flutter#59665

Closed

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

7356bb8

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

c4ac7ad

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

fdb1195

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

d139bfd

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

2669a58

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

a85935e

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

ce66a7d

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

2782411

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

8ab5eee

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

6f377c4

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

f7d241f Wire up channel for restoration data (flutter/engine#18042)

26483bd

goderbauer mentioned this pull request Jan 7, 2021

State Restoration for iOS #23495

Merged

13 tasks

Wire up channel for restoration data #18042

Wire up channel for restoration data #18042

Conversation

goderbauer commented Apr 29, 2020 • edited Loading

cbracken commented Apr 30, 2020

goderbauer commented Apr 30, 2020

goderbauer commented May 4, 2020

This comment has been minimized.

goderbauer commented Jun 10, 2020

xster 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

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

goderbauer commented Jun 12, 2020

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

xster commented Jun 12, 2020

xster commented Jun 12, 2020

goderbauer commented Apr 29, 2020 •

edited

Loading