Allow syncing custom tabs without tabbed windows

This logically reverts the precaution introduced in
https://chromium-review.googlesource.com/843574 which prevented sync IDs
from being vended when the browser itself was not running (no tabbed
activity/window), effectively resulting in a partial unlaunch session
sync for Custom Tabs.

The reason to do this was UMA metric Sync.SesssionsDuplicateSyncId
reporting many sync ID conflicts, now investigated in depth in
crbug.com/843554. The suspicion was that duplicate IDs were produced
by a race condition due to using two separate persistence databases (one
for sync, one for session restore).

We recently reworked the SyncedTabDelegate API to migrate away from sync
IDs, which renders the above concern moot and makes it straightforward
(as proposed here) to start syncing Custom Tabs even when no tabbed
window exists.

Bug: 840722,853459
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I8a27e68faa79271a6058f00c3dd5355ef635f1a4
Reviewed-on: https://chromium-review.googlesource.com/1092536
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568785}

components/sync_sessions/local_session_event_handler_impl.cc

@@ -46,6 +46,20 @@ bool IsWindowSyncable(const SyncedWindowDelegate& window_delegate) {
          window_delegate.HasWindow();
 }
 
+// On Android, it's possible to not have any tabbed windows when only custom
+// tabs are currently open. This means that there is tab data that will be
+// restored later, but we cannot access it.
+bool ScanForTabbedWindow(SyncedWindowDelegatesGetter* delegates_getter) {
+  for (const auto& window_iter_pair :
+       delegates_getter->GetSyncedWindowDelegates()) {
+    const SyncedWindowDelegate* window_delegate = window_iter_pair.second;
+    if (window_delegate->IsTypeTabbed() && IsWindowSyncable(*window_delegate)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace
 
 LocalSessionEventHandlerImpl::WriteBatch::WriteBatch() = default;
@@ -77,7 +91,7 @@ LocalSessionEventHandlerImpl::~LocalSessionEventHandlerImpl() {}
 
 void LocalSessionEventHandlerImpl::OnSessionRestoreComplete() {
   std::unique_ptr<WriteBatch> batch = delegate_->CreateLocalSessionWriteBatch();
-  AssociateWindows(RELOAD_TABS, ScanForTabbedWindow(), batch.get());
+  AssociateWindows(RELOAD_TABS, batch.get());
   batch->Commit();
 }
 
@@ -88,9 +102,12 @@ LocalSessionEventHandlerImpl::GetTabSpecificsFromDelegateForTest(
 }
 
 void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
-                                                    bool has_tabbed_window,
                                                     WriteBatch* batch) {
   DCHECK(!IsSessionRestoreInProgress(sessions_client_));
+
+  const bool has_tabbed_window =
+      ScanForTabbedWindow(sessions_client_->GetSyncedWindowDelegatesGetter());
+
   // Note that |current_session| is a pointer owned by |session_tracker_|.
   // |session_tracker_| will continue to update |current_session| under
   // the hood so care must be taken accessing it. In particular, invoking
@@ -113,8 +130,6 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
     session_tracker_->ResetSessionTracking(current_session_tag_);
     current_session->modified_time = base::Time::Now();
   } else {
-    // TODO(mastiz): Investigate if this whole code block should be
-    // conditioned to RELOAD_TABS.
     DVLOG(1) << "Found no tabbed windows. Reloading "
              << current_session->windows.size()
              << " windows from previous session.";
@@ -164,7 +179,7 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
         // changed after a session restore.
         AssociateRestoredPlaceholderTab(*synced_tab, tab_id, window_id, batch);
       } else if (RELOAD_TABS == option) {
-        AssociateTab(synced_tab, has_tabbed_window, batch);
+        AssociateTab(synced_tab, batch);
       }
 
       // If the tab was syncable, it would have been added to the tracker
@@ -222,7 +237,6 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
 
 void LocalSessionEventHandlerImpl::AssociateTab(
     SyncedTabDelegate* const tab_delegate,
-    bool has_tabbed_window,
     WriteBatch* batch) {
   DCHECK(!tab_delegate->IsPlaceholderTab());
 
@@ -241,29 +255,19 @@ void LocalSessionEventHandlerImpl::AssociateTab(
   }
 
   SessionID tab_id = tab_delegate->GetSessionId();
-  DVLOG(1) << "Syncing tab " << tab_id.id() << " from window "
-           << tab_delegate->GetWindowId().id();
-
   int tab_node_id =
       session_tracker_->LookupTabNodeFromTabId(current_session_tag_, tab_id);
 
-  if (tab_node_id != TabNodePool::kInvalidTabNodeID) {
-    // Already associated, so do nothing.
-  } else if (has_tabbed_window) {
+  if (tab_node_id == TabNodePool::kInvalidTabNodeID) {
     // Allocate a new (or reused) sync node for this tab.
     tab_node_id = session_tracker_->AssociateLocalTabWithFreeTabNode(tab_id);
     DCHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id)
         << "https://crbug.com/639009";
-  } else {
-    // Only allowed to allocate sync ids when we have native data, which is only
-    // true when we have a tabbed window. Without a sync id we cannot sync this
-    // data, the tracker cannot even really track it. So don't do any more work.
-    // This effectively breaks syncing custom tabs when the native browser isn't
-    // fully loaded. Ideally this is fixed by saving tab data and sync data
-    // atomically, see https://crbug.com/681921.
-    return;
   }
 
+  DVLOG(1) << "Syncing tab " << tab_id << " from window "
+           << tab_delegate->GetWindowId() << " using tab node " << tab_node_id;
+
   // Get the previously synced url.
   sessions::SessionTab* session_tab =
       session_tracker_->GetTab(current_session_tag_, tab_id);
@@ -359,14 +363,13 @@ void LocalSessionEventHandlerImpl::OnLocalTabModified(
       modified_tab->GetCurrentEntryIndex(), &current);
   delegate_->TrackLocalNavigationId(current.timestamp(), current.unique_id());
 
-  bool found_tabbed_window = ScanForTabbedWindow();
   std::unique_ptr<WriteBatch> batch = delegate_->CreateLocalSessionWriteBatch();
-  AssociateTab(modified_tab, found_tabbed_window, batch.get());
+  AssociateTab(modified_tab, batch.get());
   // Note, we always associate windows because it's possible a tab became
   // "interesting" by going to a valid URL, in which case it needs to be added
   // to the window's tab information. Similarly, if a tab became
   // "uninteresting", we remove it from the window's tab information.
-  AssociateWindows(DONT_RELOAD_TABS, found_tabbed_window, batch.get());
+  AssociateWindows(DONT_RELOAD_TABS, batch.get());
   batch->Commit();
 }
 
@@ -478,16 +481,4 @@ sync_pb::SessionTab LocalSessionEventHandlerImpl::GetTabSpecificsFromDelegate(
   return specifics;
 }
 
-bool LocalSessionEventHandlerImpl::ScanForTabbedWindow() {
-  for (const auto& window_iter_pair :
-       sessions_client_->GetSyncedWindowDelegatesGetter()
-           ->GetSyncedWindowDelegates()) {
-    const SyncedWindowDelegate* window_delegate = window_iter_pair.second;
-    if (window_delegate->IsTypeTabbed() && IsWindowSyncable(*window_delegate)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 }  // namespace sync_sessions

components/sync_sessions/local_session_event_handler_impl.h

@@ -80,23 +80,20 @@ class LocalSessionEventHandlerImpl : public LocalSessionEventHandler {
   enum ReloadTabsOption { RELOAD_TABS, DONT_RELOAD_TABS };
 
   void AssociateWindows(ReloadTabsOption option,
-                        bool has_tabbed_window,
                         WriteBatch* batch);
 
   // Loads and reassociates the local tab referenced in |tab|.
   // |batch| must not be null. This function will append necessary
-  // changes for processing later. Will only assign a new sync id if there is
-  // a tabbed window, which results in failure for tabs without sync ids yet.
+  // changes for processing later.
   void AssociateTab(SyncedTabDelegate* const tab,
-                    bool has_tabbed_window,
                     WriteBatch* batch);
 
   // It's possible that when we associate windows, tabs aren't all loaded
   // into memory yet (e.g on android) and we don't have a WebContents. In this
   // case we can't do a full association, but we still want to update tab IDs
   // as they may have changed after a session was restored.  This method
-  // compares new_tab_id and new_window_id against the previously persisted tab
-  // ID and window ID (from our TabNodePool) and updates them if either differs.
+  // new_window_id against the previously persisted window ID (from our
+  // TabNodePool) and updates it.
   void AssociateRestoredPlaceholderTab(const SyncedTabDelegate& tab_delegate,
                                        SessionID tab_id,
                                        SessionID new_window_id,
@@ -112,12 +109,6 @@ class LocalSessionEventHandlerImpl : public LocalSessionEventHandler {
   // Update |tab_specifics| with the corresponding task ids.
   void WriteTasksIntoSpecifics(sync_pb::SessionTab* tab_specifics);
 
-  // On Android, it's possible to not have any tabbed windows when only custom
-  // tabs are currently open. This means that there is tab data that will be
-  // restored later, but we cannot access it. This method is an elaborate way to
-  // check if we're currently in that state or not.
-  bool ScanForTabbedWindow();
-
   // Injected dependencies (not owned).
   Delegate* const delegate_;
   SyncSessionsClient* const sessions_client_;

components/sync_sessions/local_session_event_handler_impl_unittest.cc

@@ -528,6 +528,35 @@ TEST_F(LocalSessionEventHandlerImplTest, PropagateNewTab) {
   AddTab(kWindowId1, kBar1, kTabId2);
 }
 
+TEST_F(LocalSessionEventHandlerImplTest, PropagateNewCustomTab) {
+  InitHandler();
+
+  // Tab creation triggers an update event due to the tab parented notification,
+  // so the event handler issues two commits as well (one for tab creation, one
+  // for tab update). During the first update, however, the tab is not syncable
+  // and is hence skipped.
+  auto tab_create_mock_batch = std::make_unique<StrictMock<MockWriteBatch>>();
+  EXPECT_CALL(*tab_create_mock_batch,
+              Put(Pointee(MatchesHeader(kSessionTag, {}, {}))));
+  EXPECT_CALL(*tab_create_mock_batch, Commit());
+
+  auto navigation_mock_batch = std::make_unique<StrictMock<MockWriteBatch>>();
+  EXPECT_CALL(
+      *navigation_mock_batch,
+      Put(Pointee(MatchesHeader(kSessionTag, {kWindowId1}, {kTabId1}))));
+  EXPECT_CALL(*navigation_mock_batch,
+              Put(Pointee(MatchesTab(kSessionTag, kWindowId1, kTabId1,
+                                     /*tab_node_id=*/0, /*urls=*/{kFoo1}))));
+  EXPECT_CALL(*navigation_mock_batch, Commit());
+
+  EXPECT_CALL(mock_delegate_, CreateLocalSessionWriteBatch())
+      .WillOnce(Return(ByMove(std::move(tab_create_mock_batch))))
+      .WillOnce(Return(ByMove(std::move(navigation_mock_batch))));
+
+  AddWindow(kWindowId1, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
+  AddTab(kWindowId1, kFoo1, kTabId1);
+}
+
 TEST_F(LocalSessionEventHandlerImplTest, PropagateNewWindow) {
   AddWindow(kWindowId1);
   AddTab(kWindowId1, kFoo1, kTabId1);

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -758,17 +758,18 @@ TEST_F(SessionSyncBridgeTest, ShouldPreserveTabbedDataIfCustomTabOnlyFound) {
   InitializeBridge();
   StartSyncing();
 
-  // The previous session should be preserved. The transient window cannot be
-  // synced because we do not have enough local data to ensure that we wouldn't
-  // vend the same sync ID if our persistent storage didn't match upon the last
-  // shutdown.
-  EXPECT_THAT(GetAllData(),
-              UnorderedElementsAre(
-                  Pair(_, EntityDataHasSpecifics(
-                              MatchesHeader(kLocalSessionTag, _, _))),
-                  Pair(_, EntityDataHasSpecifics(MatchesTab(
-                              kLocalSessionTag, _, _,
-                              /*tab_node_id=*/0, {"http://foo.com/"})))));
+  // The previous session should be preserved, together with the new custom tab.
+  EXPECT_THAT(
+      GetAllData(),
+      UnorderedElementsAre(
+          Pair(_,
+               EntityDataHasSpecifics(MatchesHeader(kLocalSessionTag, _, _))),
+          Pair(_, EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, _, _,
+                                                    /*tab_node_id=*/0,
+                                                    {"http://foo.com/"}))),
+          Pair(_, EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, _, _,
+                                                    /*tab_node_id=*/1,
+                                                    {"http://bar.com/"})))));
 }
 
 // Ensure that tabbed windows from a previous session are preserved and combined
@@ -815,19 +816,24 @@ TEST_F(SessionSyncBridgeTest, ShouldPreserveTabbedDataIfNewCustomTabAlsoFound) {
 }
 
 // Ensure that, in a scenario without prior sync data, encountering a custom
-// tab only ( no tabbed window) does not vend new sync IDs.
-TEST_F(SessionSyncBridgeTest, ShouldIgnoreIfCustomTabOnlyOnStartup) {
+// tab only (no tabbed window) starts syncing that tab.
+TEST_F(SessionSyncBridgeTest, ShouldAssociateIfCustomTabOnlyOnStartup) {
   const int kWindowId = 1000001;
+  const int kTabId = 1000002;
 
   AddWindow(kWindowId, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
-  AddTab(kWindowId, "http://foo.com/");
+  AddTab(kWindowId, "http://foo.com/", kTabId);
 
   InitializeBridge();
   StartSyncing();
 
   EXPECT_THAT(GetAllData(),
-              UnorderedElementsAre(Pair(_, EntityDataHasSpecifics(MatchesHeader(
-                                               kLocalSessionTag, _, _)))));
+              UnorderedElementsAre(
+                  Pair(_, EntityDataHasSpecifics(MatchesHeader(
+                              kLocalSessionTag, {kWindowId}, {kTabId}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId, kTabId,
+                              /*tab_node_id=*/0, {"http://foo.com/"})))));
 }
 
 // Ensure that all tabs are exposed in a scenario where only a custom tab
@@ -840,45 +846,35 @@ TEST_F(SessionSyncBridgeTest, ShouldExposeTabbedWindowAfterCustomTabOnly) {
   const int kTabId2 = 1000004;
 
   AddWindow(kWindowId1, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
-  TestSyncedTabDelegate* custom_tab =
-      AddTab(kWindowId1, "http://foo.com/", kTabId1);
+  AddTab(kWindowId1, "http://foo.com/", kTabId1);
 
   InitializeBridge();
   StartSyncing();
 
   ASSERT_THAT(GetAllData(),
-              UnorderedElementsAre(Pair(_, EntityDataHasSpecifics(MatchesHeader(
-                                               kLocalSessionTag, _, _)))));
+              UnorderedElementsAre(
+                  Pair(_, EntityDataHasSpecifics(MatchesHeader(
+                              kLocalSessionTag, {kWindowId1}, {kTabId1}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId1, kTabId1,
+                              /*tab_node_id=*/0, {"http://foo.com/"})))));
 
   // Load the actual tabbed window, now that we're syncing.
   AddWindow(kWindowId2);
   AddTab(kWindowId2, "http://bar.com/", kTabId2);
 
-  // The local change should be created and tracked correctly. This doesn't
-  // actually start syncing the custom tab yet, because the tab itself isn't
-  // associated yet.
-  EXPECT_THAT(GetAllData(),
-              UnorderedElementsAre(
-                  Pair(_, EntityDataHasSpecifics(MatchesHeader(
-                              kLocalSessionTag, {kWindowId2}, {kTabId2}))),
-                  Pair(_, EntityDataHasSpecifics(MatchesTab(
-                              kLocalSessionTag, kWindowId2, kTabId2,
-                              /*tab_node_id=*/0, {"http://bar.com/"})))));
-
-  // Now trigger OnLocalTabModified() for the custom tab again, it should sync.
-  custom_tab->Navigate("http://baz.com/");
+  // The local change should be created and tracked correctly.
   EXPECT_THAT(GetAllData(),
               UnorderedElementsAre(
                   Pair(_, EntityDataHasSpecifics(MatchesHeader(
                               kLocalSessionTag, {kWindowId1, kWindowId2},
                               {kTabId1, kTabId2}))),
-                  Pair(_, EntityDataHasSpecifics(MatchesTab(
-                              kLocalSessionTag, kWindowId2, kTabId2,
-                              /*tab_node_id=*/0, {"http://bar.com/"}))),
                   Pair(_, EntityDataHasSpecifics(MatchesTab(
                               kLocalSessionTag, kWindowId1, kTabId1,
-                              /*tab_node_id=*/1,
-                              {"http://foo.com/", "http://baz.com/"})))));
+                              /*tab_node_id=*/0, {"http://foo.com/"}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId2, kTabId2,
+                              /*tab_node_id=*/1, {"http://bar.com/"})))));
 }
 
 TEST_F(SessionSyncBridgeTest, ShouldDisableSyncAndReenable) {

components/sync_sessions/sessions_sync_manager_unittest.cc

@@ -485,36 +485,13 @@ TEST_F(SessionsSyncManagerTest, PreserveTabbedDataCustomTab) {
   manager()->StopSyncing(syncer::SESSIONS);
   ResetWindows();
   window = AddWindow(sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
-  TestSyncedTabDelegate* custom_tab = AddTab(window->GetSessionId(), kBar1);
+  AddTab(window->GetSessionId(), kBar1);
   InitWithSyncDataTakeOutput(ConvertToRemote(in), &out);
 
-  // The previous session should be preserved. The transient window cannot be
-  // synced because we do not have enough local data to ensure that we wouldn't
-  // vend the same sync id if our persistent storage didn't match upon the last
-  // shutdown.
-  ASSERT_TRUE(ChangeTypeMatches(out, {SyncChange::ACTION_UPDATE}));
-  VerifyLocalHeaderChange(out[0], 1, 1);
-  out.clear();
-
-  // Now re-create local data and modify it.
-  TestSyncedWindowDelegate* alive_again = AddWindow();
-  alive_again->OverrideTabAt(0, tab);
-  tab->Navigate(kBaz1);
-
-  // The local change should be created and tracked correctly. This doesn't
-  // actually start syncing the custom tab yet, because the tab itself isn't
-  // associated yet.
-  ASSERT_TRUE(ChangeTypeMatches(
-      out, {SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE}));
-  VerifyLocalTabChange(out[0], 3, kBaz1);
-  VerifyLocalHeaderChange(out[1], 1, 1);
-  out.clear();
-
-  // Now trigger OnLocalTabModified() for the custom tab again, it should sync.
-  custom_tab->Navigate(kBar2);
+  // The previous session should be preserved, together with the new custom tab.
   ASSERT_TRUE(ChangeTypeMatches(
       out, {SyncChange::ACTION_ADD, SyncChange::ACTION_UPDATE}));
-  VerifyLocalTabChange(out[0], 2, kBar2);
+  VerifyLocalTabChange(out[0], 1, kBar1);
   VerifyLocalHeaderChange(out[1], 2, 2);
 }