weblayer: adds support for minimal persistence to Java side

This required some shuffling around to get things to work right.
In particular, prior to this patch I had restore triggered
from the browser_impl.cc's constructor. This proves problematic on
android now as tabs may be created synchronously. When BrowserImpl.java
calls to browser_impl.cc's constructor all the connections (clients)
haven't been set up (and BrowserImpl.java can't call to the native side,
because it's waiting for the return value from creation). This lead to
all sorts of crashes and what not.

The fix is to trigger restore later on, and specifically once everything
has been wired up, which is when setClient() is called.

BUG=1046406
TEST=BrowserFragmentLifecycleTest

Change-Id: Iee2dfcab5ab8b45371099b33b65aa35ffb458ce4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031594Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737341}

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/BrowserFragmentLifecycleTest.java

@@ -51,6 +51,41 @@ public class BrowserFragmentLifecycleTest {
         mActivityTestRule.navigateAndWait(tab, url, false);
     }
 
+    @Test
+    @SmallTest
+    public void restoreAfterRecreate() throws InterruptedException {
+        InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl("about:blank");
+        Tab tab = TestThreadUtils.runOnUiThreadBlockingNoException(() -> activity.getTab());
+
+        String url = "data:text,foo";
+        mActivityTestRule.navigateAndWait(tab, url, false);
+
+        mActivityTestRule.recreateActivity();
+
+        InstrumentationActivity newActivity = mActivityTestRule.getActivity();
+        CountDownLatch latch = new CountDownLatch(1);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            Tab restoredTab = newActivity.getTab();
+            // It's possible the NavigationController hasn't loaded yet, handle either scenario.
+            NavigationController navigationController = restoredTab.getNavigationController();
+            if (navigationController.getNavigationListSize() == 1
+                    && navigationController.getNavigationEntryDisplayUri(0).equals(
+                            Uri.parse(url))) {
+                latch.countDown();
+                return;
+            }
+            navigationController.registerNavigationCallback(new NavigationCallback() {
+                @Override
+                public void onNavigationCompleted(@NonNull Navigation navigation) {
+                    if (navigation.getUri().equals(Uri.parse(url))) {
+                        latch.countDown();
+                    }
+                }
+            });
+        });
+        latch.await();
+    }
+
     // https://crbug.com/1021041
     @Test
     @SmallTest

weblayer/browser/browser_impl.cc

@@ -30,25 +30,12 @@ namespace weblayer {
 std::unique_ptr<Browser> Browser::Create(
     Profile* profile,
     const PersistenceInfo* persistence_info) {
-  return std::make_unique<BrowserImpl>(static_cast<ProfileImpl*>(profile),
-                                       persistence_info);
-}
-
-#if defined(OS_ANDROID)
-BrowserImpl::BrowserImpl(ProfileImpl* profile,
-                         const PersistenceInfo* persistence_info,
-                         const base::android::JavaParamRef<jobject>& java_impl)
-    : BrowserImpl(profile, persistence_info) {
-  java_impl_ = java_impl;
-}
-#endif
-
-BrowserImpl::BrowserImpl(ProfileImpl* profile,
-                         const PersistenceInfo* persistence_info)
-    : profile_(profile),
-      persistence_id_(persistence_info ? persistence_info->id : std::string()) {
+  // BrowserImpl's constructor is private.
+  auto browser =
+      base::WrapUnique(new BrowserImpl(static_cast<ProfileImpl*>(profile)));
   if (persistence_info)
-    RestoreStateIfNecessary(*persistence_info);
+    browser->RestoreStateIfNecessary(*persistence_info);
+  return browser;
 }
 
 BrowserImpl::~BrowserImpl() {
@@ -165,6 +152,36 @@ BrowserImpl::GetMinimalPersistenceState(
   return base::android::ToJavaByteArray(env, &(state.front()), state.size());
 }
 
+void BrowserImpl::RestoreStateIfNecessary(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& caller,
+    const base::android::JavaParamRef<jstring>& j_persistence_id,
+    const base::android::JavaParamRef<jbyteArray>& j_persistence_crypto_key,
+    const base::android::JavaParamRef<jbyteArray>&
+        j_minimal_persistence_state) {
+  Browser::PersistenceInfo persistence_info;
+  Browser::PersistenceInfo* persistence_info_ptr = nullptr;
+
+  if (j_persistence_id.obj()) {
+    const std::string persistence_id =
+        base::android::ConvertJavaStringToUTF8(j_persistence_id);
+    if (!persistence_id.empty()) {
+      persistence_info.id = persistence_id;
+      if (j_persistence_crypto_key.obj()) {
+        base::android::JavaByteArrayToByteVector(
+            env, j_persistence_crypto_key, &(persistence_info.last_crypto_key));
+      }
+      persistence_info_ptr = &persistence_info;
+    }
+  } else if (j_minimal_persistence_state.obj()) {
+    base::android::JavaByteArrayToByteVector(env, j_minimal_persistence_state,
+                                             &(persistence_info.minimal_state));
+    persistence_info_ptr = &persistence_info;
+  }
+  if (persistence_info_ptr)
+    RestoreStateIfNecessary(*persistence_info_ptr);
+}
+
 #endif
 
 std::vector<uint8_t> BrowserImpl::GetMinimalPersistenceState(
@@ -268,6 +285,19 @@ void BrowserImpl::RemoveObserver(BrowserObserver* observer) {
   browser_observers_.RemoveObserver(observer);
 }
 
+BrowserImpl::BrowserImpl(ProfileImpl* profile) : profile_(profile) {}
+
+void BrowserImpl::RestoreStateIfNecessary(
+    const PersistenceInfo& persistence_info) {
+  persistence_id_ = persistence_info.id;
+  if (!persistence_id_.empty()) {
+    session_service_ = std::make_unique<SessionService>(
+        GetSessionServiceDataPath(), this, persistence_info.last_crypto_key);
+  } else if (!persistence_info.minimal_state.empty()) {
+    RestoreMinimalState(this, persistence_info.minimal_state);
+  }
+}
+
 base::FilePath BrowserImpl::GetSessionServiceDataPath() {
   base::FilePath base_path;
   if (profile_->GetBrowserContext()->IsOffTheRecord()) {
@@ -281,41 +311,30 @@ base::FilePath BrowserImpl::GetSessionServiceDataPath() {
   return base_path.AppendASCII("State" + encoded_name);
 }
 
-void BrowserImpl::RestoreStateIfNecessary(
-    const PersistenceInfo& persistence_info) {
-  if (!persistence_info.id.empty()) {
-    session_service_ = std::make_unique<SessionService>(
-        GetSessionServiceDataPath(), this, persistence_info.last_crypto_key);
-  } else if (!persistence_info.minimal_state.empty()) {
-    RestoreMinimalState(this, persistence_info.minimal_state);
-  }
+#if defined(OS_ANDROID)
+// This function is friended. JNI_BrowserImpl_CreateBrowser can not be
+// friended, as it requires browser_impl.h to include BrowserImpl_jni.h, which
+// is problematic (meaning not really supported and generates compile errors).
+BrowserImpl* CreateBrowserForAndroid(
+    ProfileImpl* profile,
+    const base::android::JavaParamRef<jobject>& java_impl) {
+  BrowserImpl* browser = new BrowserImpl(profile);
+  browser->java_impl_ = java_impl;
+  return browser;
 }
 
-#if defined(OS_ANDROID)
 static jlong JNI_BrowserImpl_CreateBrowser(
     JNIEnv* env,
     jlong profile,
-    const base::android::JavaParamRef<jstring>& j_persistence_id,
-    const base::android::JavaParamRef<jbyteArray>& j_persistence_crypto_key,
     const base::android::JavaParamRef<jobject>& java_impl) {
-  Browser::PersistenceInfo persistence_info;
-  Browser::PersistenceInfo* persistence_info_ptr = nullptr;
-
-  if (j_persistence_id.obj()) {
-    const std::string persistence_id =
-        base::android::ConvertJavaStringToUTF8(j_persistence_id);
-    if (!persistence_id.empty()) {
-      persistence_info.id = persistence_id;
-      if (j_persistence_crypto_key.obj()) {
-        base::android::JavaByteArrayToByteVector(
-            env, j_persistence_crypto_key, &(persistence_info.last_crypto_key));
-      }
-      persistence_info_ptr = &persistence_info;
-    }
-  }
-  return reinterpret_cast<intptr_t>(
-      new BrowserImpl(reinterpret_cast<ProfileImpl*>(profile),
-                      persistence_info_ptr, java_impl));
+  // The android side does not trigger restore from the constructor as at the
+  // time this is called not enough of WebLayer has been wired up. Specifically,
+  // when this is called BrowserImpl.java hasn't obtained the return value so
+  // that it can't call any functions and further the client side hasn't been
+  // fully created, leading to all sort of assertions if Tabs are created
+  // and/or navigations start (which restore may trigger).
+  return reinterpret_cast<intptr_t>(CreateBrowserForAndroid(
+      reinterpret_cast<ProfileImpl*>(profile), java_impl));
 }
 
 static void JNI_BrowserImpl_DeleteBrowser(JNIEnv* env, jlong browser) {

weblayer/browser/browser_impl.h

@@ -32,15 +32,9 @@ class TabImpl;
 
 class BrowserImpl : public Browser {
  public:
-#if defined(OS_ANDROID)
-  BrowserImpl(ProfileImpl* profile,
-              const PersistenceInfo* persistence_info,
-              const base::android::JavaParamRef<jobject>& java_impl);
-#endif
-  BrowserImpl(ProfileImpl* profile, const PersistenceInfo* persistence_info);
-  ~BrowserImpl() override;
   BrowserImpl(const BrowserImpl&) = delete;
   BrowserImpl& operator=(const BrowserImpl&) = delete;
+  ~BrowserImpl() override;
 
   SessionService* session_service() { return session_service_.get(); }
 
@@ -81,6 +75,13 @@ class BrowserImpl : public Browser {
   base::android::ScopedJavaLocalRef<jbyteArray> GetMinimalPersistenceState(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& caller);
+  void RestoreStateIfNecessary(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& caller,
+      const base::android::JavaParamRef<jstring>& j_persistence_id,
+      const base::android::JavaParamRef<jbyteArray>& j_persistence_crypto_key,
+      const base::android::JavaParamRef<jbyteArray>&
+          j_minimal_persistence_state);
 #endif
 
   // Used in tests to specify a non-default max (0 means use the default).
@@ -99,6 +100,16 @@ class BrowserImpl : public Browser {
   void RemoveObserver(BrowserObserver* observer) override;
 
  private:
+  // For creation.
+  friend class Browser;
+#if defined(OS_ANDROID)
+  friend BrowserImpl* CreateBrowserForAndroid(
+      ProfileImpl*,
+      const base::android::JavaParamRef<jobject>&);
+#endif
+
+  explicit BrowserImpl(ProfileImpl* profile);
+
   void RestoreStateIfNecessary(const PersistenceInfo& persistence_info);
 
   // Returns the path used by |session_service_|.
@@ -111,7 +122,7 @@ class BrowserImpl : public Browser {
   ProfileImpl* profile_;
   std::vector<std::unique_ptr<Tab>> tabs_;
   TabImpl* active_tab_ = nullptr;
-  const std::string persistence_id_;
+  std::string persistence_id_;
   std::unique_ptr<SessionService> session_service_;
 };
 

weblayer/browser/java/org/chromium/weblayer_private/BrowserFragmentImpl.java

@@ -54,10 +54,10 @@ public class BrowserFragmentImpl extends RemoteFragmentImpl {
         super.onCreate(savedInstanceState);
         // onCreate() is only called once
         assert mBrowser == null;
-        mBrowser = new BrowserImpl(mProfile, mPersistenceId, savedInstanceState);
         // onCreate() is always called after onAttach(). onAttach() sets |mContext|.
         assert mContext != null;
-        mBrowser.onFragmentAttached(mContext, new FragmentWindowAndroid(mContext, this));
+        mBrowser = new BrowserImpl(mProfile, mPersistenceId, savedInstanceState, mContext,
+                new FragmentWindowAndroid(mContext, this));
     }
 
     @Override

weblayer/browser/java/org/chromium/weblayer_private/BrowserImpl.java

@@ -36,9 +36,15 @@ import java.util.List;
 public class BrowserImpl extends IBrowser.Stub {
     private static final ObserverList<Observer> sLifecycleObservers = new ObserverList<Observer>();
 
+    // Key used to save the crypto key in instance state.
     public static final String SAVED_STATE_SESSION_SERVICE_CRYPTO_KEY =
             "SAVED_STATE_SESSION_SERVICE_CRYPTO_KEY";
 
+    // Key used to save the minimal persistence state in instance state. Only used if a persistence
+    // id was not specified.
+    public static final String SAVED_STATE_MINIMAL_PERSISTENCE_STATE_KEY =
+            "SAVED_STATE_MINIMAL_PERSISTENCE_STATE_KEY";
+
     private long mNativeBrowser;
     private final ProfileImpl mProfile;
     private BrowserViewController mViewController;
@@ -47,6 +53,15 @@ public class BrowserImpl extends IBrowser.Stub {
     private LocaleChangedBroadcastReceiver mLocaleReceiver;
     private boolean mInDestroy;
 
+    // Created in the constructor from saved state and used in setClient().
+    private PersistenceInfo mPersistenceInfo;
+
+    private static final class PersistenceInfo {
+        String mPersistenceId;
+        byte[] mCryptoKey;
+        byte[] mMinimalPersistenceState;
+    };
+
     /**
      * Observer interface that can be implemented to observe when the first
      * fragment requiring WebLayer is attached, and when the last such fragment
@@ -65,13 +80,23 @@ public class BrowserImpl extends IBrowser.Stub {
         sLifecycleObservers.removeObserver(observer);
     }
 
-    public BrowserImpl(ProfileImpl profile, String persistenceId, Bundle savedInstanceState) {
+    public BrowserImpl(ProfileImpl profile, String persistenceId, Bundle savedInstanceState,
+            Context context, FragmentWindowAndroid windowAndroid) {
         mProfile = profile;
-        byte[] cryptoKey = savedInstanceState != null
+
+        mPersistenceInfo = new PersistenceInfo();
+        mPersistenceInfo.mPersistenceId = persistenceId;
+        mPersistenceInfo.mCryptoKey = savedInstanceState != null
                 ? savedInstanceState.getByteArray(SAVED_STATE_SESSION_SERVICE_CRYPTO_KEY)
                 : null;
-        mNativeBrowser = BrowserImplJni.get().createBrowser(
-                profile.getNativeProfile(), persistenceId, cryptoKey, this);
+        mPersistenceInfo.mMinimalPersistenceState =
+                (savedInstanceState != null && (persistenceId == null || persistenceId.isEmpty()))
+                ? savedInstanceState.getByteArray(SAVED_STATE_MINIMAL_PERSISTENCE_STATE_KEY)
+                : null;
+
+        createAttachmentState(context, windowAndroid);
+
+        mNativeBrowser = BrowserImplJni.get().createBrowser(profile.getNativeProfile(), this);
 
         for (Observer observer : sLifecycleObservers) {
             observer.onBrowserCreated();
@@ -87,22 +112,18 @@ public class BrowserImpl extends IBrowser.Stub {
         return mViewController.getContentView();
     }
 
-    public void onFragmentAttached(Context context, FragmentWindowAndroid windowAndroid) {
-        assert mWindowAndroid == null;
+    // Called from constructor and onFragmentAttached() to configure state needed when attached.
+    private void createAttachmentState(Context context, FragmentWindowAndroid windowAndroid) {
         assert mViewController == null;
+        assert mWindowAndroid == null;
         mWindowAndroid = windowAndroid;
         mViewController = new BrowserViewController(context, windowAndroid);
         mLocaleReceiver = new LocaleChangedBroadcastReceiver(context);
+    }
 
-        if (getTabs().size() > 0) {
-            updateAllTabs();
-            mViewController.setActiveTab(getActiveTab());
-        } else if (BrowserImplJni.get().getPersistenceId(mNativeBrowser, this).isEmpty()) {
-            TabImpl tab = new TabImpl(mProfile, windowAndroid);
-            addTab(tab);
-            boolean set_active_result = setActiveTab(tab);
-            assert set_active_result;
-        } // else case is session restore, which will asynchronously create tabs.
+    public void onFragmentAttached(Context context, FragmentWindowAndroid windowAndroid) {
+        createAttachmentState(context, windowAndroid);
+        updateAllTabsAndSetActive();
     }
 
     public void onFragmentDetached() {
@@ -111,13 +132,17 @@ public class BrowserImpl extends IBrowser.Stub {
     }
 
     public void onSaveInstanceState(Bundle outState) {
-        if (mProfile.isIncognito()
-                && !BrowserImplJni.get().getPersistenceId(mNativeBrowser, this).isEmpty()) {
+        boolean hasPersistenceId =
+                !BrowserImplJni.get().getPersistenceId(mNativeBrowser, this).isEmpty();
+        if (mProfile.isIncognito() && hasPersistenceId) {
             // Trigger a save now as saving may generate a new crypto key. This doesn't actually
             // save synchronously, rather triggers a save on a background task runner.
             BrowserImplJni.get().saveSessionServiceIfNecessary(mNativeBrowser, this);
             outState.putByteArray(SAVED_STATE_SESSION_SERVICE_CRYPTO_KEY,
                     BrowserImplJni.get().getSessionServiceCryptoKey(mNativeBrowser, this));
+        } else if (!hasPersistenceId) {
+            outState.putByteArray(SAVED_STATE_MINIMAL_PERSISTENCE_STATE_KEY,
+                    BrowserImplJni.get().getMinimalPersistenceState(mNativeBrowser, this));
         }
     }
 
@@ -247,6 +272,26 @@ public class BrowserImpl extends IBrowser.Stub {
     public void setClient(IBrowserClient client) {
         StrictModeWorkaround.apply();
         mClient = client;
+
+        // This function is called from the client once everything has been setup (meaning all the
+        // client classes have been created and AIDL interfaces established in both directions).
+        // This function is called immediately after the constructor of BrowserImpl from the client.
+        assert mPersistenceInfo != null;
+        PersistenceInfo persistenceInfo = mPersistenceInfo;
+        mPersistenceInfo = null;
+        BrowserImplJni.get().restoreStateIfNecessary(mNativeBrowser, this,
+                persistenceInfo.mPersistenceId, persistenceInfo.mCryptoKey,
+                persistenceInfo.mMinimalPersistenceState);
+
+        if (getTabs().size() > 0) {
+            updateAllTabsAndSetActive();
+        } else if (persistenceInfo.mPersistenceId == null
+                || persistenceInfo.mPersistenceId.isEmpty()) {
+            TabImpl tab = new TabImpl(mProfile, mWindowAndroid);
+            addTab(tab);
+            boolean set_active_result = setActiveTab(tab);
+            assert set_active_result;
+        } // else case is session restore, which will asynchronously create tabs.
     }
 
     @Override
@@ -295,6 +340,13 @@ public class BrowserImpl extends IBrowser.Stub {
         }
     }
 
+    private void updateAllTabsAndSetActive() {
+        if (getTabs().size() > 0) {
+            updateAllTabs();
+            mViewController.setActiveTab(getActiveTab());
+        }
+    }
+
     private void updateAllTabs() {
         for (Object tab : getTabs()) {
             ((TabImpl) tab).updateFromBrowser();
@@ -303,8 +355,7 @@ public class BrowserImpl extends IBrowser.Stub {
 
     @NativeMethods
     interface Natives {
-        long createBrowser(long profile, String persistenceId, byte[] persistenceCryptoKey,
-                BrowserImpl caller);
+        long createBrowser(long profile, BrowserImpl caller);
         void deleteBrowser(long browser);
         void addTab(long nativeBrowserImpl, BrowserImpl browser, long nativeTab);
         void removeTab(long nativeBrowserImpl, BrowserImpl browser, long nativeTab);
@@ -316,5 +367,7 @@ public class BrowserImpl extends IBrowser.Stub {
         void saveSessionServiceIfNecessary(long nativeBrowserImpl, BrowserImpl browser);
         byte[] getSessionServiceCryptoKey(long nativeBrowserImpl, BrowserImpl browser);
         byte[] getMinimalPersistenceState(long nativeBrowserImpl, BrowserImpl browser);
+        void restoreStateIfNecessary(long nativeBrowserImpl, BrowserImpl browser,
+                String persistenceId, byte[] persistenceCryptoKey, byte[] minimalPersistenceState);
     }
 }

weblayer/browser/navigation_controller_impl.cc

@@ -34,7 +34,7 @@ NavigationControllerImpl::~NavigationControllerImpl() = default;
 void NavigationControllerImpl::SetNavigationControllerImpl(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& java_controller) {
-  java_controller_.Reset(env, java_controller);
+  java_controller_ = java_controller;
 }
 
 void NavigationControllerImpl::GoToIndex(

weblayer/browser/navigation_impl.cc

@@ -36,7 +36,7 @@ NavigationImpl::~NavigationImpl() {
 void NavigationImpl::SetJavaNavigation(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& java_navigation) {
-  java_navigation_.Reset(env, java_navigation);
+  java_navigation_ = java_navigation;
 }
 
 ScopedJavaLocalRef<jstring> NavigationImpl::GetUri(

weblayer/browser/persistence/minimal_browser_persister_browsertest.cc

@@ -30,7 +30,7 @@ class MinimalBrowserPersisterTest : public WebLayerBrowserTest {
   void SetUpOnMainThread() override {
     WebLayerBrowserTest::SetUpOnMainThread();
     ASSERT_TRUE(embedded_test_server()->Start());
-    browser_ = std::make_unique<BrowserImpl>(GetProfile(), nullptr);
+    browser_ = Browser::Create(GetProfile(), nullptr);
     tab_ = static_cast<TabImpl*>(browser_->AddTab(Tab::Create(GetProfile())));
     browser_->SetActiveTab(tab_);
   }
@@ -50,16 +50,20 @@ class MinimalBrowserPersisterTest : public WebLayerBrowserTest {
   void RecreateBrowserFromCurrentState(int max_size_in_bytes = 0) {
     Browser::PersistenceInfo persistence_info;
     persistence_info.minimal_state =
-        browser_->GetMinimalPersistenceState(max_size_in_bytes);
+        browser_impl()->GetMinimalPersistenceState(max_size_in_bytes);
     tab_ = nullptr;
-    browser_ = std::make_unique<BrowserImpl>(GetProfile(), &persistence_info);
+    browser_ = Browser::Create(GetProfile(), &persistence_info);
     // There is always at least one tab created (even if restore fails).
     ASSERT_GE(browser_->GetTabs().size(), 1u);
     tab_ = static_cast<TabImpl*>(browser_->GetTabs()[0]);
   }
 
  protected:
-  std::unique_ptr<BrowserImpl> browser_;
+  BrowserImpl* browser_impl() {
+    return static_cast<BrowserImpl*>(browser_.get());
+  }
+
+  std::unique_ptr<Browser> browser_;
   TabImpl* tab_ = nullptr;
 };
 

weblayer/browser/session_service_browsertest.cc

@@ -168,7 +168,9 @@ std::unique_ptr<BrowserImpl> CreateBrowser(ProfileImpl* profile,
                                            const std::string& persistence_id) {
   Browser::PersistenceInfo info;
   info.id = persistence_id;
-  return std::make_unique<BrowserImpl>(profile, &info);
+  auto browser = Browser::Create(profile, &info);
+  return std::unique_ptr<BrowserImpl>(
+      static_cast<BrowserImpl*>(browser.release()));
 }
 
 }  // namespace

weblayer/public/java/org/chromium/weblayer/Browser.java

@@ -39,16 +39,6 @@ public final class Browser {
         } catch (RemoteException e) {
             throw new APICallException(e);
         }
-        try {
-            for (Object tab : impl.getTabs()) {
-                // getTabs() returns List<TabImpl>, which isn't accessible from the client library.
-                ITab iTab = ITab.Stub.asInterface((android.os.IBinder) tab);
-                // Tab's constructor calls registerTab().
-                new Tab(iTab, this);
-            }
-        } catch (RemoteException e) {
-            throw new APICallException(e);
-        }
     }
 
     /**