weblayer: adds a persistent id for tabs

This way the embedder has a way to persist state with a tab. Without this the embedder has no way to identify what tab the data was associated with. BUG=1056710 TEST=BrowserFragmentLifecycleTests Change-Id: I944e538ba0110f5d3434d5dfd8aa76fe2fdd08e5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2093691 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Commit-Position: refs/heads/master@{#748314}
qweri0p · Mar 9, 2020 · b2e0cf1 · b2e0cf1
1 parent e615715
commit b2e0cf1
Show file tree

Hide file tree

Showing 14 changed files with 199 additions and 55 deletions.
diff --git a/components/sessions/core/session_service_commands.cc b/components/sessions/core/session_service_commands.cc
@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/containers/flat_set.h"
+#include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "base/pickle.h"
 #include "base/token.h"
@@ -64,6 +65,7 @@ static const SessionCommand::id_type kCommandTabNavigationPathPruned = 24;
 static const SessionCommand::id_type kCommandSetTabGroup = 25;
 static const SessionCommand::id_type kCommandSetTabGroupMetadata = 26;
 static const SessionCommand::id_type kCommandSetTabGroupMetadata2 = 27;
+static const SessionCommand::id_type kCommandSetTabGuid = 28;
 
 namespace {
 
@@ -778,9 +780,21 @@ bool CreateTabsAndWindows(
         break;
       }
 
+      case kCommandSetTabGuid: {
+        std::unique_ptr<base::Pickle> pickle(command->PayloadAsPickle());
+        base::PickleIterator it(*pickle);
+        SessionID::id_type tab_id = -1;
+        std::string guid;
+        if (!it.ReadInt(&tab_id) || !it.ReadString(&guid) ||
+            !base::IsValidGUID(guid)) {
+          DVLOG(1) << "Failed reading command " << command->id();
+          return true;
+        }
+        GetTab(SessionID::FromSerializedValue(tab_id), tabs)->guid = guid;
+        break;
+      }
+
       default:
-        // TODO(skuhne): This might call back into a callback handler to extend
-        // the command set for specific implementations.
         DVLOG(1) << "Failed reading an unknown command " << command->id();
         return true;
     }
@@ -986,6 +1000,15 @@ std::unique_ptr<SessionCommand> CreateSetWindowAppNameCommand(
                                        app_name);
 }
 
+std::unique_ptr<SessionCommand> CreateSetTabGuidCommand(
+    const SessionID& tab_id,
+    const std::string& guid) {
+  base::Pickle pickle;
+  pickle.WriteInt(tab_id.id());
+  pickle.WriteString(guid);
+  return std::make_unique<SessionCommand>(kCommandSetTabGuid, pickle);
+}
+
 bool ReplacePendingCommand(CommandStorageManager* command_storage_manager,
                            std::unique_ptr<SessionCommand>* command) {
   // We optimize page navigations, which can happen quite frequently and

diff --git a/components/sessions/core/session_service_commands.h b/components/sessions/core/session_service_commands.h
@@ -27,7 +27,6 @@ class SessionCommand;
 
 // The following functions create sequentialized change commands which are
 // used to reconstruct the current/previous session state.
-// It is up to the caller to delete the returned SessionCommand* object.
 SESSIONS_EXPORT std::unique_ptr<SessionCommand>
 CreateSetSelectedTabInWindowCommand(const SessionID& window_id, int index);
 SESSIONS_EXPORT std::unique_ptr<SessionCommand> CreateSetTabWindowCommand(
@@ -89,6 +88,10 @@ SESSIONS_EXPORT std::unique_ptr<SessionCommand> CreateSetWindowWorkspaceCommand(
     const SessionID& window_id,
     const std::string& workspace);
 
+SESSIONS_EXPORT std::unique_ptr<SessionCommand> CreateSetTabGuidCommand(
+    const SessionID& tab_id,
+    const std::string& guid);
+
 // Searches for a pending command using |command_storage_manager| that can be
 // replaced with |command|. If one is found, pending command is removed, the
 // command is added to the pending commands (taken ownership) and true is

diff --git a/components/sessions/core/session_types.h b/components/sessions/core/session_types.h
@@ -97,6 +97,9 @@ struct SESSIONS_EXPORT SessionTab {
   // For reassociating sessionStorage.
   std::string session_storage_persistent_id;
 
+  // guid associated with the tab, may be empty.
+  std::string guid;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(SessionTab);
 };

diff --git a/...rowser/android/javatests/src/org/chromium/weblayer/test/BrowserFragmentLifecycleTest.java b/...rowser/android/javatests/src/org/chromium/weblayer/test/BrowserFragmentLifecycleTest.java
@@ -37,7 +37,6 @@ public class BrowserFragmentLifecycleTest {
     public void successfullyLoadsUrlAfterRecreation() {
         InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl("about:blank");
         Tab tab = TestThreadUtils.runOnUiThreadBlockingNoException(() -> activity.getTab());
-
         String url = "data:text,foo";
         mActivityTestRule.navigateAndWait(tab, url, false);
 
@@ -51,37 +50,19 @@ public void successfullyLoadsUrlAfterRecreation() {
 
     @Test
     @SmallTest
-    public void restoreAfterRecreate() {
+    public void restoreAfterRecreate() throws Throwable {
         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();
-        BoundedCountDownLatch latch = new BoundedCountDownLatch(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.timedAwait();
+        waitForTabToFinishRestore(TestThreadUtils.runOnUiThreadBlocking(() -> {
+            return mActivityTestRule.getActivity().getTab();
+        }),
+                url);
     }
 
     // https://crbug.com/1021041
@@ -107,16 +88,8 @@ public void onReadyToCommitNavigation(@NonNull Navigation navigation) {
         latch.timedAwait();
     }
 
-    private void restoresPreviousSession(Bundle extras) {
-        extras.putString(InstrumentationActivity.EXTRA_PERSISTENCE_ID, "x");
-        final String url = mActivityTestRule.getTestDataURL("simple_page.html");
-        InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(url, extras);
-
-        mActivityTestRule.recreateActivity();
-
-        InstrumentationActivity newActivity = mActivityTestRule.getActivity();
-        Tab tab = TestThreadUtils.runOnUiThreadBlockingNoException(() -> newActivity.getTab());
-        Assert.assertNotNull(tab);
+    // Waits for |tab| to finish loadding |url. This is intended to be called after restore.
+    private void waitForTabToFinishRestore(Tab tab, String url) {
         BoundedCountDownLatch latch = new BoundedCountDownLatch(1);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             // It's possible the NavigationController hasn't loaded yet, handle either scenario.
@@ -139,18 +112,77 @@ public void onNavigationCompleted(@NonNull Navigation navigation) {
         latch.timedAwait();
     }
 
+    // Recreates the activity and waits for the first tab to be restored. |extras| is the Bundle
+    // used to launch the shell.
+    private void restoresPreviousSession(Bundle extras) {
+        extras.putString(InstrumentationActivity.EXTRA_PERSISTENCE_ID, "x");
+        final String url = mActivityTestRule.getTestDataURL("simple_page.html");
+        mActivityTestRule.launchShellWithUrl(url, extras);
+        mActivityTestRule.recreateActivity();
+
+        InstrumentationActivity newActivity = mActivityTestRule.getActivity();
+        Assert.assertNotNull(newActivity);
+        Tab tab = TestThreadUtils.runOnUiThreadBlockingNoException(() -> newActivity.getTab());
+        Assert.assertNotNull(tab);
+        waitForTabToFinishRestore(tab, url);
+    }
+
     @Test
     @SmallTest
-    public void restoresPreviousSession() throws InterruptedException {
+    public void restoresPreviousSession() throws Throwable {
         restoresPreviousSession(new Bundle());
     }
 
     @Test
     @SmallTest
-    public void restoresPreviousSessionIncognito() throws InterruptedException {
+    public void restoresPreviousSessionIncognito() throws Throwable {
         Bundle extras = new Bundle();
         // This forces incognito.
         extras.putString(InstrumentationActivity.EXTRA_PROFILE_NAME, null);
         restoresPreviousSession(extras);
     }
+
+    @Test
+    @SmallTest
+    public void restoresTabGuid() throws Throwable {
+        Bundle extras = new Bundle();
+        extras.putString(InstrumentationActivity.EXTRA_PERSISTENCE_ID, "x");
+        final String url = mActivityTestRule.getTestDataURL("simple_page.html");
+        InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(url, extras);
+        final String initialTabId = TestThreadUtils.runOnUiThreadBlocking(
+                () -> { return mActivityTestRule.getActivity().getTab().getGuid(); });
+        Assert.assertNotNull(initialTabId);
+        Assert.assertFalse(initialTabId.isEmpty());
+
+        mActivityTestRule.recreateActivity();
+
+        InstrumentationActivity newActivity = mActivityTestRule.getActivity();
+        Tab tab = TestThreadUtils.runOnUiThreadBlockingNoException(() -> newActivity.getTab());
+        Assert.assertNotNull(tab);
+        waitForTabToFinishRestore(tab, url);
+        final String restoredTabId =
+                TestThreadUtils.runOnUiThreadBlockingNoException(() -> { return tab.getGuid(); });
+        Assert.assertEquals(initialTabId, restoredTabId);
+    }
+
+    @Test
+    @SmallTest
+    public void restoreTabGuidAfterRecreate() throws Throwable {
+        InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl("about:blank");
+        final Tab tab = TestThreadUtils.runOnUiThreadBlockingNoException(() -> activity.getTab());
+        final String initialTabId = TestThreadUtils.runOnUiThreadBlocking(
+                () -> { return mActivityTestRule.getActivity().getTab().getGuid(); });
+        String url = "data:text,foo";
+        mActivityTestRule.navigateAndWait(tab, url, false);
+
+        mActivityTestRule.recreateActivity();
+
+        InstrumentationActivity newActivity = mActivityTestRule.getActivity();
+        final Tab restoredTab =
+                TestThreadUtils.runOnUiThreadBlockingNoException(() -> newActivity.getTab());
+        waitForTabToFinishRestore(restoredTab, url);
+        final String restoredTabId = TestThreadUtils.runOnUiThreadBlockingNoException(
+                () -> { return restoredTab.getGuid(); });
+        Assert.assertEquals(initialTabId, restoredTabId);
+    }
 }
diff --git a/weblayer/browser/browser_impl.cc b/weblayer/browser/browser_impl.cc
@@ -59,9 +59,10 @@ BrowserImpl::~BrowserImpl() {
 }
 
 TabImpl* BrowserImpl::CreateTabForSessionRestore(
-    std::unique_ptr<content::WebContents> web_contents) {
+    std::unique_ptr<content::WebContents> web_contents,
+    const std::string& guid) {
   std::unique_ptr<TabImpl> tab =
-      std::make_unique<TabImpl>(profile_, std::move(web_contents));
+      std::make_unique<TabImpl>(profile_, std::move(web_contents), guid);
 #if defined(OS_ANDROID)
   Java_BrowserImpl_createTabForSessionRestore(
       AttachCurrentThread(), java_impl_, reinterpret_cast<jlong>(tab.get()));

diff --git a/weblayer/browser/browser_impl.h b/weblayer/browser/browser_impl.h
@@ -43,7 +43,8 @@ class BrowserImpl : public Browser {
   // Creates and adds a Tab from session restore. The returned tab is owned by
   // this Browser.
   TabImpl* CreateTabForSessionRestore(
-      std::unique_ptr<content::WebContents> web_contents);
+      std::unique_ptr<content::WebContents> web_contents,
+      const std::string& guid);
 
 #if defined(OS_ANDROID)
   void AddTab(JNIEnv* env,

diff --git a/weblayer/browser/java/org/chromium/weblayer_private/TabImpl.java b/weblayer/browser/java/org/chromium/weblayer_private/TabImpl.java
@@ -433,6 +433,11 @@ public boolean dismissTransientUi() {
         return false;
     }
 
+    @Override
+    public String getGuid() {
+        return TabImplJni.get().getGuid(mNativeTab);
+    }
+
     @CalledByNative
     private static RectF createRectF(float x, float y, float right, float bottom) {
         return new RectF(x, y, right, bottom);
@@ -575,5 +580,6 @@ void setTopControlsContainerView(
         void executeScript(long nativeTabImpl, String script, boolean useSeparateIsolate,
                 Callback<String> callback);
         void updateBrowserControlsState(long nativeTabImpl, int newConstraint);
+        String getGuid(long nativeTabImpl);
     }
 }
diff --git a/weblayer/browser/java/org/chromium/weblayer_private/interfaces/ITab.aidl b/weblayer/browser/java/org/chromium/weblayer_private/interfaces/ITab.aidl
@@ -38,5 +38,8 @@ interface ITab {
 
   void dismissTabModalOverlay() = 10;
   void dispatchBeforeUnloadAndClose() = 11;
+
   boolean dismissTransientUi() = 12;
+
+  String getGuid() = 13;
 }
diff --git a/weblayer/browser/persistence/browser_persistence_common.cc b/weblayer/browser/persistence/browser_persistence_common.cc
@@ -76,7 +76,8 @@ void ProcessRestoreCommands(
                                           content::RestoreType::CURRENT_SESSION,
                                           &entries);
     DCHECK(entries.empty());
-    TabImpl* tab = browser->CreateTabForSessionRestore(std::move(web_contents));
+    TabImpl* tab = browser->CreateTabForSessionRestore(std::move(web_contents),
+                                                       session_tab.guid);
 
     if (!had_tabs && i == (windows[0])->selected_tab_index)
       browser->SetActiveTab(tab);
@@ -97,7 +98,8 @@ void RestoreBrowserState(
 
   if (browser->GetTabs().empty()) {
     // Nothing to restore, or restore failed. Create a default tab.
-    browser->SetActiveTab(browser->CreateTabForSessionRestore(nullptr));
+    browser->SetActiveTab(
+        browser->CreateTabForSessionRestore(nullptr, std::string()));
   }
 }
 
@@ -107,25 +109,27 @@ BuildCommandsForTabConfiguration(const SessionID& browser_session_id,
                                  int index_in_browser) {
   DCHECK(tab);
   std::vector<std::unique_ptr<sessions::SessionCommand>> result;
-  const SessionID& session_id = GetSessionIDForTab(tab);
+  const SessionID& tab_id = GetSessionIDForTab(tab);
   result.push_back(
-      sessions::CreateSetTabWindowCommand(browser_session_id, session_id));
+      sessions::CreateSetTabWindowCommand(browser_session_id, tab_id));
 
   result.push_back(sessions::CreateLastActiveTimeCommand(
-      session_id, tab->web_contents()->GetLastActiveTime()));
+      tab_id, tab->web_contents()->GetLastActiveTime()));
 
   const std::string& ua_override = tab->web_contents()->GetUserAgentOverride();
   if (!ua_override.empty()) {
-    result.push_back(sessions::CreateSetTabUserAgentOverrideCommand(
-        session_id, ua_override));
+    result.push_back(
+        sessions::CreateSetTabUserAgentOverrideCommand(tab_id, ua_override));
   }
   if (index_in_browser != -1) {
-    result.push_back(sessions::CreateSetTabIndexInWindowCommand(
-        session_id, index_in_browser));
+    result.push_back(
+        sessions::CreateSetTabIndexInWindowCommand(tab_id, index_in_browser));
   }
 
   result.push_back(sessions::CreateSetSelectedNavigationIndexCommand(
-      session_id, tab->web_contents()->GetController().GetCurrentEntryIndex()));
+      tab_id, tab->web_contents()->GetController().GetCurrentEntryIndex()));
+
+  result.push_back(sessions::CreateSetTabGuidCommand(tab_id, tab->GetGuid()));
 
   return result;
 }

diff --git a/weblayer/browser/persistence/browser_persister_browsertest.cc b/weblayer/browser/persistence/browser_persister_browsertest.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind_helpers.h"
 #include "base/files/file_path.h"
+#include "base/guid.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "build/build_config.h"
@@ -203,6 +204,32 @@ IN_PROC_BROWSER_TEST_F(BrowserPersisterTest, SingleTab) {
                    ->GetNavigationListSize());
 }
 
+IN_PROC_BROWSER_TEST_F(BrowserPersisterTest, RestoresGuid) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  std::unique_ptr<BrowserImpl> browser = CreateBrowser(GetProfile(), "x");
+  Tab* tab = browser->AddTab(Tab::Create(GetProfile()));
+  const std::string original_guid = tab->GetGuid();
+  EXPECT_FALSE(original_guid.empty());
+  EXPECT_TRUE(base::IsValidGUID(original_guid));
+  const GURL url = embedded_test_server()->GetURL("/simple_page.html");
+  NavigateAndWaitForCompletion(url, tab);
+  ShutdownBrowserPersisterAndWait(browser.get());
+  tab = nullptr;
+  browser.reset();
+
+  browser = CreateBrowser(GetProfile(), "x");
+  // Should be no tabs while waiting for restore.
+  EXPECT_TRUE(browser->GetTabs().empty());
+  // Wait for the restore and navigation to complete.
+  BrowserNavigationObserverImpl::WaitForNewTabToCompleteNavigation(
+      browser.get(), url);
+
+  ASSERT_EQ(1u, browser->GetTabs().size());
+  EXPECT_EQ(browser->GetTabs()[0], browser->GetActiveTab());
+  EXPECT_EQ(original_guid, browser->GetTabs()[0]->GetGuid());
+}
+
 IN_PROC_BROWSER_TEST_F(BrowserPersisterTest, TwoTabs) {
   ASSERT_TRUE(embedded_test_server()->Start());