[Sync] Send tombstone for the last closed tab in window.

If the last tab is closed, it won't be removed from the "Tabs from other
devices" list. This CL fixes it by sending the latest state even if
there are no tabs left in window.

After current CL the window is considered syncable if it is not about to
be closed (regardless of the number of tabs).

Bug: 1039234
Change-Id: I15c61b38a6bbd53ea158b216644cd27655f8c4dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485261Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#826844}

chrome/browser/sync/test/integration/session_hierarchy_match_checker.cc

@@ -14,6 +14,9 @@ SessionHierarchyMatchChecker::SessionHierarchyMatchChecker(
 
 bool SessionHierarchyMatchChecker::IsExitConditionSatisfied(std::ostream* os) {
   *os << "Waiting for matching sessions hierarchy to be reflected in fake "
-         "server";
-  return verifier_.VerifySessions(sessions_hierarchy_);
+         "server. ";
+  testing::AssertionResult result =
+      verifier_.VerifySessions(sessions_hierarchy_);
+  *os << result.message();
+  return result;
 }

chrome/browser/sync/test/integration/sessions_helper.cc

@@ -360,10 +360,7 @@ bool NavigationEquals(const sessions::SerializedNavigationEntry& expected,
   return true;
 }
 
-namespace {
-
-template <typename T1, typename T2>
-bool WindowsMatchImpl(const T1& win1, const T2& win2) {
+bool WindowsMatch(const ScopedWindowMap& win1, const ScopedWindowMap& win2) {
   sessions::SessionTab* client0_tab;
   sessions::SessionTab* client1_tab;
   if (win1.size() != win2.size()) {
@@ -404,16 +401,6 @@ bool WindowsMatchImpl(const T1& win1, const T2& win2) {
   return true;
 }
 
-}  // namespace
-
-bool WindowsMatch(const ScopedWindowMap& win1, const ScopedWindowMap& win2) {
-  return WindowsMatchImpl(win1, win2);
-}
-
-bool WindowsMatch(const SessionWindowMap& win1, const ScopedWindowMap& win2) {
-  return WindowsMatchImpl(win1, win2);
-}
-
 void DeleteForeignSession(int browser_index, std::string session_tag) {
   SessionSyncServiceFactory::GetInstance()
       ->GetForProfile(test()->GetProfile(browser_index))
@@ -441,9 +428,13 @@ bool ForeignSessionsMatchChecker::IsExitConditionSatisfied(std::ostream* os) {
   DCHECK(foreign_local_sessions);
 
   SyncedSessionVector sessions;
-  if (!GetSessionData(profile_index_, &sessions)) {
-    *os << "Cannot get foreign sessions on profile " << profile_index_ << ".";
-    return false;
+  GetSessionData(profile_index_, &sessions);
+
+  if (foreign_local_sessions->windows.empty() && sessions.empty()) {
+    // The case when the remote session has deleted all tabs. In this case if
+    // there is no local windows and remote sessions, then it is considered to
+    // match.
+    return true;
   }
 
   for (const sync_sessions::SyncedSession* remote_session : sessions) {

chrome/browser/sync/test/integration/sessions_helper.h

@@ -67,7 +67,6 @@ bool NavigationEquals(const sessions::SerializedNavigationEntry& expected,
 //    4. actual tab navigations contents
 // - false otherwise.
 bool WindowsMatch(const ScopedWindowMap& win1, const ScopedWindowMap& win2);
-bool WindowsMatch(const SessionWindowMap& win1, const ScopedWindowMap& win2);
 
 // Retrieves the foreign sessions for a particular profile and compares them
 // with a reference SessionWindow list.
@@ -151,8 +150,8 @@ class ForeignSessionsMatchChecker : public MultiClientStatusChangeChecker {
   bool IsExitConditionSatisfied(std::ostream* os) override;
 
  private:
-  int profile_index_;
-  int foreign_profile_index_;
+  const int profile_index_;
+  const int foreign_profile_index_;
 };
 
 }  // namespace sessions_helper

chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc

@@ -458,6 +458,21 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NavigateThenCloseTab) {
           .Wait());
 }
 
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
+                       ShouldDeleteLastClosedTab) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(CheckInitialState(0));
+
+  ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
+  ASSERT_TRUE(OpenTab(0, GURL(kURL2)));
+  WaitForHierarchyOnServer(SessionsHierarchy({{kURL1, kURL2}}));
+
+  CloseTab(/*index=*/0, /*tab_index=*/0);
+  WaitForHierarchyOnServer(SessionsHierarchy({{kURL2}}));
+  CloseTab(/*index=*/0, /*tab_index=*/0);
+  WaitForHierarchyOnServer(SessionsHierarchy());
+}
+
 class SingleClientSessionsWithDeferRecyclingSyncTest
     : public SingleClientSessionsSyncTest {
  public:

chrome/browser/sync/test/integration/sync_test.cc

@@ -98,6 +98,8 @@
 #else
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
@@ -278,6 +280,29 @@ bool SyncTest::FakeInstanceIDDriver::ExistsInstanceID(
   return fake_instance_ids_.count(app_id);
 }
 
+#if !defined(OS_ANDROID)
+class SyncTest::ClosedBrowserObserver : public BrowserListObserver {
+ public:
+  using OnBrowserRemovedCallback =
+      base::RepeatingCallback<void(Browser* browser)>;
+
+  explicit ClosedBrowserObserver(OnBrowserRemovedCallback callback)
+      : browser_remove_callback_(std::move(callback)) {
+    BrowserList::AddObserver(this);
+  }
+
+  ~ClosedBrowserObserver() override { BrowserList::RemoveObserver(this); }
+
+  // BrowserListObserver overrides.
+  void OnBrowserRemoved(Browser* browser) override {
+    browser_remove_callback_.Run(browser);
+  }
+
+ private:
+  OnBrowserRemovedCallback browser_remove_callback_;
+};
+#endif
+
 SyncTest::SyncTest(TestType test_type)
     : test_type_(test_type),
       test_construction_time_(base::Time::Now()),
@@ -295,6 +320,10 @@ SyncTest::SyncTest(TestType test_type)
       break;
     }
   }
+#if !defined(OS_ANDROID)
+  browser_list_observer_ = std::make_unique<ClosedBrowserObserver>(
+      base::BindRepeating(&SyncTest::OnBrowserRemoved, base::Unretained(this)));
+#endif
 }
 
 SyncTest::~SyncTest() = default;
@@ -523,7 +552,7 @@ Browser* SyncTest::GetBrowser(int index) {
   Browser* browser = browsers_[index];
   DCHECK(browser);
 
-  return browsers_[index];
+  return browser;
 }
 
 Browser* SyncTest::AddBrowser(int profile_index) {
@@ -534,6 +563,14 @@ Browser* SyncTest::AddBrowser(int profile_index) {
 
   return browsers_[browsers_.size() - 1];
 }
+
+void SyncTest::OnBrowserRemoved(Browser* browser) {
+  for (size_t i = 0; i < browsers_.size(); ++i) {
+    if (browsers_[i] == browser) {
+      browsers_[i] = nullptr;
+    }
+  }
+}
 #endif
 
 ProfileSyncServiceHarness* SyncTest::GetClient(int index) {
@@ -971,12 +1008,16 @@ void SyncTest::TearDownOnMainThread() {
   // Closing all browsers created by this test. The calls here block until
   // they are closed. Other browsers created outside SyncTest setup should be
   // closed by the creator of that browser.
-  size_t init_browser_count = chrome::GetTotalBrowserCount();
-  for (size_t i = 0; i < browsers_.size(); ++i) {
-    CloseBrowserSynchronously(browsers_[i]);
+  const size_t initial_total_browser_count = chrome::GetTotalBrowserCount();
+  size_t closed_browser_count = 0;
+  for (Browser* browser : browsers_) {
+    if (browser) {
+      CloseBrowserSynchronously(browser);
+      closed_browser_count++;
+    }
   }
   ASSERT_EQ(chrome::GetTotalBrowserCount(),
-            init_browser_count - browsers_.size());
+            initial_total_browser_count - closed_browser_count);
 #endif
 
   if (fake_server_.get()) {
@@ -1112,7 +1153,9 @@ void SyncTest::ResetSyncForPrimaryAccount() {
     ASSERT_TRUE(SyncDisabledChecker(GetSyncService(0)).Wait());
 
 #if !defined(OS_ANDROID)
-    CloseBrowserSynchronously(browsers_[0]);
+    if (browsers_[0]) {
+      CloseBrowserSynchronously(browsers_[0]);
+    }
 #endif
 
     // After reset, this client will disable sync. It may log some messages

chrome/browser/sync/test/integration/sync_test.h

@@ -184,7 +184,7 @@ class SyncTest : public PlatformBrowserTest {
 
 #if !defined(OS_ANDROID)
   // Returns a pointer to a particular browser. Callee owns the object
-  // and manages its lifetime.
+  // and manages its lifetime. The called browser must not be closed before.
   Browser* GetBrowser(int index);
 
   // Adds a new browser belonging to the profile at |profile_index|, and appends
@@ -372,6 +372,14 @@ class SyncTest : public PlatformBrowserTest {
       std::vector<syncer::FCMHandler*>* sync_invalidations_fcm_handlers,
       content::BrowserContext* context);
 
+#if !defined(OS_ANDROID)
+  // Called when the |browser| was removed externally. This just marks the
+  // |browser| in the |browsers_| list as nullptr to keep indexes in |browsers_|
+  // and |profiles_| in sync. It is used when the |browser| is removed within a
+  // test (e.g. when the last tab is closed for the |browser|).
+  void OnBrowserRemoved(Browser* browser);
+#endif
+
   // Helper to Profile::CreateProfile that handles path creation. It creates
   // a profile then registers it as a testing profile.
   Profile* MakeTestProfile(base::FilePath profile_path, int index);
@@ -466,6 +474,9 @@ class SyncTest : public PlatformBrowserTest {
   // managed by BrowserList, so we don't use a std::vector<std::unique_ptr<>>
   // here.
   std::vector<Browser*> browsers_;
+
+  class ClosedBrowserObserver;
+  std::unique_ptr<ClosedBrowserObserver> browser_list_observer_;
 #endif
 
   // Collection of sync clients used by a test. A sync client is associated

chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc

@@ -28,6 +28,7 @@ using sessions_helper::ForeignSessionsMatchChecker;
 using sessions_helper::GetLocalWindows;
 using sessions_helper::GetSessionData;
 using sessions_helper::NavigateTab;
+using sessions_helper::OpenMultipleTabs;
 using sessions_helper::OpenTab;
 using sessions_helper::OpenTabAtIndex;
 using sessions_helper::ScopedWindowMap;
@@ -217,4 +218,25 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest,
   EXPECT_THAT(GetFakeServer()->GetCommittedHistoryURLs(), IsEmpty());
 }
 
+IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, ShouldSyncAllClosedTabs) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+
+  ASSERT_TRUE(CheckInitialState(0));
+  ASSERT_TRUE(CheckInitialState(1));
+
+  ASSERT_TRUE(OpenMultipleTabs(0, {GURL(kURL1), GURL(kURL2)}));
+
+  ASSERT_TRUE(
+      WaitForForeignSessionsToSync(/*local_index=*/0, /*non_local_index=*/1));
+
+  // Close all tabs and wait for syncing.
+  CloseTab(/*index=*/0, /*tab_index=*/0);
+  ASSERT_TRUE(
+      WaitForForeignSessionsToSync(/*local_index=*/0, /*non_local_index=*/1));
+
+  CloseTab(/*index=*/0, /*tab_index=*/0);
+  EXPECT_TRUE(
+      WaitForForeignSessionsToSync(/*local_index=*/0, /*non_local_index=*/1));
+}
+
 }  // namespace

chrome/browser/ui/sync/browser_synced_window_delegate.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/sync/browser_synced_tab_delegate.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "components/sync_sessions/switches.h"
 
 BrowserSyncedWindowDelegate::BrowserSyncedWindowDelegate(Browser* browser)
     : browser_(browser) {}
@@ -67,5 +68,12 @@ bool BrowserSyncedWindowDelegate::IsSessionRestoreInProgress() const {
 }
 
 bool BrowserSyncedWindowDelegate::ShouldSync() const {
-  return IsTypeNormal() || IsTypePopup();
+  if (!IsTypeNormal() && !IsTypePopup()) {
+    return false;
+  }
+
+  // Do not sync windows which are about to be closed.
+  return !browser_->IsAttemptingToCloseBrowser() ||
+         !base::FeatureList::IsEnabled(
+             switches::kSyncConsiderEmptyWindowsSyncable);
 }

components/sync_sessions/BUILD.gn

@@ -27,6 +27,8 @@ static_library("sync_sessions") {
     "session_sync_service_impl.h",
     "sessions_global_id_mapper.cc",
     "sessions_global_id_mapper.h",
+    "switches.cc",
+    "switches.h",
     "sync_sessions_client.cc",
     "sync_sessions_client.h",
     "synced_session.cc",

components/sync_sessions/local_session_event_handler_impl.cc

@@ -15,6 +15,7 @@
 #include "build/build_config.h"
 #include "components/sync/model/sync_change.h"
 #include "components/sync/protocol/sync.pb.h"
+#include "components/sync_sessions/switches.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 #include "components/sync_sessions/synced_session_tracker.h"
 #include "components/sync_sessions/synced_tab_delegate.h"
@@ -44,8 +45,12 @@ bool IsSessionRestoreInProgress(SyncSessionsClient* sessions_client) {
 }
 
 bool IsWindowSyncable(const SyncedWindowDelegate& window_delegate) {
-  return window_delegate.ShouldSync() && window_delegate.GetTabCount() &&
-         window_delegate.HasWindow();
+  // TODO(crbug.com/1039234): remove the feature toggle once the logic is rolled
+  // out.
+  return window_delegate.ShouldSync() && window_delegate.HasWindow() &&
+         (window_delegate.GetTabCount() ||
+          base::FeatureList::IsEnabled(
+              switches::kSyncConsiderEmptyWindowsSyncable));
 }
 
 // On Android, it's possible to not have any tabbed windows when only custom
@@ -157,12 +162,16 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
                               window_delegate->GetTabCount());
     }
 
-    // Make sure the window has tabs and a viewable window. The viewable
-    // window check is necessary because, for example, when a browser is
-    // closed the destructor is not necessarily run immediately. This means
+    // Make sure the window is viewable and is not about to be closed. The
+    // viewable window check is necessary because, for example, when a browser
+    // is closed the destructor is not necessarily run immediately. This means
     // its possible for us to get a handle to a browser that is about to be
-    // removed. If the tab count is 0 or the window is null, the browser is
-    // about to be deleted, so we ignore it.
+    // removed. If the window is null, the browser is about to be deleted, so we
+    // ignore it. There is no check for having open tabs anymore. This is needed
+    // to handle a case when the last tab is closed (on Andorid it doesn't mean
+    // that the window is about to be removed). Instead, there is a check if the
+    // window is about to be closed. If the window is last for the profile, the
+    // latest state will be kept.
     if (!IsWindowSyncable(*window_delegate)) {
       continue;
     }

components/sync_sessions/switches.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/sync_sessions/switches.h"
+
+namespace switches {
+
+// Enables syncing Sessions data type in case when the window doesn't have open
+// tabs anymore.
+const base::Feature kSyncConsiderEmptyWindowsSyncable{
+    "SyncConsiderEmptyWindowsSyncable", base::FEATURE_ENABLED_BY_DEFAULT};
+
+}  // namespace switches

components/sync_sessions/switches.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_SYNC_SESSIONS_SWITCHES_H_
+#define COMPONENTS_SYNC_SESSIONS_SWITCHES_H_
+
+#include "base/feature_list.h"
+
+namespace switches {
+
+extern const base::Feature kSyncConsiderEmptyWindowsSyncable;
+
+}  // namespace switches
+
+#endif  // COMPONENTS_SYNC_SESSIONS_SWITCHES_H_