sessions: enable SessionRestoreTest.NormalAndPopup

This test shuts down the session service part way through,
and then recreates it. The second service ends up with a
different task runner (because each session service has its
own). This means it's possible for the second one to try
reading files that haven't been finished writing. At least
that's my theory as to why this test is flaky.

I couldn't repro this locally, so this is a best guess.

BUG=1154345, 1158715, 1166756
TEST=test only change, SessionRestoreTest.NormalAndPopup

Change-Id: I7bf5ea96134179f607ffcf9d882b6bc7b2277da2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2634013Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844842}

chrome/browser/sessions/session_restore_browsertest.cc

@@ -524,19 +524,30 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTest, MaximizedApps) {
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 // Creates a tabbed browser and popup and makes sure we restore both.
-// Disabled for mac-arm64 bot stabilization: https://crbug.com/1154345
-// Also disabled for Mac flakiness in general: https://crbug.com/1158715
-// Also disabled for cross-platform flakiness in general: https://crbug.com/1166756
-IN_PROC_BROWSER_TEST_F(SessionRestoreTest, DISABLED_NormalAndPopup) {
+// NOTE: If this flakes, please disable and update https://crbug.com/1166756.
+IN_PROC_BROWSER_TEST_F(SessionRestoreTest, NormalAndPopup) {
   // Open a popup.
   Browser* popup = CreateBrowserForPopup(browser()->profile());
   ASSERT_EQ(2u, active_browser_list_->size());
 
+  SessionService* session_service =
+      SessionServiceFactory::GetForProfile(browser()->profile());
+  SessionServiceTestHelper test_helper(session_service);
+  auto backend_task_runner = test_helper.GetBackendTaskRunner();
+  test_helper.ReleaseService();
+
   // Simulate an exit by shutting down the session service. If we don't do this
   // the first window close is treated as though the user closed the window
   // and won't be restored.
   SessionServiceFactory::ShutdownForProfile(browser()->profile());
 
+  // Ensure the session service finishes writing. This is necessary as the
+  // code following this creates a new SessionService, which will use a
+  // different task runner for reading/writing to the same files.
+  base::RunLoop run_loop;
+  backend_task_runner->PostNonNestableTask(FROM_HERE, run_loop.QuitClosure());
+  run_loop.Run();
+
   // Restart and make sure we have two windows.
   CloseBrowserSynchronously(popup);
   QuitBrowserAndRestore(browser(), 1);

chrome/browser/sessions/session_service_test_helper.cc

@@ -122,6 +122,13 @@ void SessionServiceTestHelper::RunTaskOnBackendThread(
   test_helper.RunTaskOnBackendThread(from_here, std::move(task));
 }
 
+scoped_refptr<base::SequencedTaskRunner>
+SessionServiceTestHelper::GetBackendTaskRunner() {
+  return sessions::CommandStorageManagerTestHelper(
+             service_->GetCommandStorageManagerForTest())
+      .GetBackendTaskRunner();
+}
+
 void SessionServiceTestHelper::SetAvailableRange(
     const SessionID& tab_id,
     const std::pair<int, int>& range) {

chrome/browser/sessions/session_service_test_helper.h

@@ -14,12 +14,14 @@
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "components/sessions/core/session_id.h"
 
 class SessionService;
 
 namespace base {
 class Location;
+class SequencedTaskRunner;
 }
 
 namespace sessions {
@@ -86,6 +88,8 @@ class SessionServiceTestHelper {
   void RunTaskOnBackendThread(const base::Location& from_here,
                               base::OnceClosure task);
 
+  scoped_refptr<base::SequencedTaskRunner> GetBackendTaskRunner();
+
   void SetAvailableRange(const SessionID& tab_id,
                          const std::pair<int, int>& range);
   bool GetAvailableRange(const SessionID& tab_id, std::pair<int, int>* range);