sessions: move ownership of SessionService from TestHelper to Test

SessionServiceTestHelper no longer owns SessionService.
Having SessionServiceTestHelper own the SessionService is
not at all expected and lead to awkward code.

BUG=none
TEST=none

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

chrome/browser/error_reporting/webui_js_error_reporting_browsertest.cc

@@ -249,10 +249,8 @@ IN_PROC_BROWSER_TEST_F(WebUIJSErrorReportingTest,
     mock_processor.reset();
     mock_processor =
         std::make_unique<ScopedMockChromeJsErrorReportProcessor>(endpoint);
-    SessionServiceTestHelper helper(
-        SessionServiceFactory::GetForProfileForSessionRestore(profile));
+    SessionServiceTestHelper helper(profile);
     helper.SetForceBrowserNotAliveWithNoWindows(true);
-    helper.ReleaseService();
     chrome::NewEmptyWindow(profile);
 
     // ScopedKeepAlive goes out of scope, so the new browser will return to

chrome/browser/media/media_engagement_browsertest.cc

@@ -782,10 +782,8 @@ class MediaEngagementSessionRestoreBrowserTest
     SessionStartupPref::SetStartupPref(
         profile, SessionStartupPref(SessionStartupPref::LAST));
 #if BUILDFLAG(IS_CHROMEOS_ASH)
-    SessionServiceTestHelper helper(
-        SessionServiceFactory::GetForProfile(profile));
+    SessionServiceTestHelper helper(profile);
     helper.SetForceBrowserNotAliveWithNoWindows(true);
-    helper.ReleaseService();
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
     std::unique_ptr<ScopedKeepAlive> keep_alive(new ScopedKeepAlive(

chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc

@@ -1937,10 +1937,8 @@ class SessionRestorePageLoadMetricsBrowserTest
     SessionStartupPref::SetStartupPref(
         profile, SessionStartupPref(SessionStartupPref::LAST));
 #if BUILDFLAG(IS_CHROMEOS_ASH)
-    SessionServiceTestHelper helper(
-        SessionServiceFactory::GetForProfile(profile));
+    SessionServiceTestHelper helper(profile);
     helper.SetForceBrowserNotAliveWithNoWindows(true);
-    helper.ReleaseService();
 #endif
 
     std::unique_ptr<ScopedKeepAlive> keep_alive(new ScopedKeepAlive(

chrome/browser/sessions/better_session_restore_browsertest.cc

@@ -154,10 +154,8 @@ class BetterSessionRestoreTest : public InProcessBrowserTest {
 
  protected:
   void SetUpOnMainThread() override {
-    SessionServiceTestHelper helper(
-        SessionServiceFactory::GetForProfile(browser()->profile()));
+    SessionServiceTestHelper helper(browser()->profile());
     helper.SetForceBrowserNotAliveWithNoWindows(true);
-    helper.ReleaseService();
 #if BUILDFLAG(ENABLE_BACKGROUND_MODE)
     g_browser_process->set_background_mode_manager_for_test(
         std::unique_ptr<BackgroundModeManager>(new FakeBackgroundModeManager));
@@ -291,11 +289,8 @@ class BetterSessionRestoreTest : public InProcessBrowserTest {
     else
       CloseBrowserSynchronously(browser);
 
-    SessionServiceTestHelper helper;
-    helper.SetService(
-        SessionServiceFactory::GetForProfileForSessionRestore(profile));
+    SessionServiceTestHelper helper(profile);
     helper.SetForceBrowserNotAliveWithNoWindows(true);
-    helper.ReleaseService();
 
     // Create a new window, which may trigger session restore.
     size_t count = BrowserList::GetInstance()->size();

chrome/browser/sessions/session_restore_browsertest.cc

@@ -121,10 +121,8 @@ class SessionRestoreTest : public InProcessBrowserTest {
     if (strcmp(test_info->name(), "NoSessionRestoreNewWindowChromeOS") != 0) {
       // Undo the effect of kBrowserAliveWithNoWindows in defaults.cc so that we
       // can get these test to work without quitting.
-      SessionServiceTestHelper helper(
-          SessionServiceFactory::GetForProfile(browser()->profile()));
+      SessionServiceTestHelper helper(browser()->profile());
       helper.SetForceBrowserNotAliveWithNoWindows(true);
-      helper.ReleaseService();
     }
 #endif
   }
@@ -164,10 +162,8 @@ class SessionRestoreTest : public InProcessBrowserTest {
 
     // Ensure the session service factory is started, even if it was explicitly
     // shut down.
-    SessionServiceTestHelper helper(
-        SessionServiceFactory::GetForProfileForSessionRestore(profile));
+    SessionServiceTestHelper helper(profile);
     helper.SetForceBrowserNotAliveWithNoWindows(true);
-    helper.ReleaseService();
 
     // Create a new window, which should trigger session restore.
     if (url.is_empty()) {
@@ -534,7 +530,6 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTest, NormalAndPopup) {
       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
@@ -1346,10 +1341,8 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTest, CloseSingleTabRestoresNothing) {
 
   // Ensure the session service factory is started, even if it was explicitly
   // shut down.
-  SessionServiceTestHelper helper(
-      SessionServiceFactory::GetForProfileForSessionRestore(profile));
+  SessionServiceTestHelper helper(profile);
   helper.SetForceBrowserNotAliveWithNoWindows(true);
-  helper.ReleaseService();
 
   chrome::NewEmptyWindow(profile);
 
@@ -1386,10 +1379,8 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTest,
 
   // Ensure the session service factory is started, even if it was explicitly
   // shut down.
-  SessionServiceTestHelper helper(
-      SessionServiceFactory::GetForProfileForSessionRestore(profile));
+  SessionServiceTestHelper helper(profile);
   helper.SetForceBrowserNotAliveWithNoWindows(true);
-  helper.ReleaseService();
 
   // Create a new browser by navigating to the test page.
   GURL url = ui_test_utils::GetTestUrl(

chrome/browser/sessions/session_restore_interactive_uitest.cc

@@ -51,10 +51,8 @@ class SessionRestoreInteractiveTest : public InProcessBrowserTest {
 
     // Ensure the session service factory is started, even if it was explicitly
     // shut down.
-    SessionServiceTestHelper helper(
-        SessionServiceFactory::GetForProfileForSessionRestore(profile));
+    SessionServiceTestHelper helper(profile);
     helper.SetForceBrowserNotAliveWithNoWindows(true);
-    helper.ReleaseService();
 
     // Create a new window, which should trigger session restore.
     chrome::NewEmptyWindow(profile);

chrome/browser/sessions/session_restore_observer_browsertest.cc

@@ -116,7 +116,6 @@ class SessionRestoreObserverTest : public InProcessBrowserTest {
     SessionServiceTestHelper helper(
         SessionServiceFactory::GetForProfile(browser()->profile()));
     helper.SetForceBrowserNotAliveWithNoWindows(true);
-    helper.ReleaseService();
 #endif
     ASSERT_TRUE(embedded_test_server()->Start());
   }

chrome/browser/sessions/session_service_test_helper.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/sessions/session_service_test_helper.h"
 
 #include "chrome/browser/sessions/session_service.h"
+#include "chrome/browser/sessions/session_service_factory.h"
 #include "components/sessions/core/command_storage_backend.h"
 #include "components/sessions/core/command_storage_manager_test_helper.h"
 #include "components/sessions/core/serialized_navigation_entry_test_helper.h"
@@ -16,7 +17,12 @@
 
 using base::Time;
 
-SessionServiceTestHelper::SessionServiceTestHelper() {}
+SessionServiceTestHelper::SessionServiceTestHelper()
+    : SessionServiceTestHelper(static_cast<SessionService*>(nullptr)) {}
+
+SessionServiceTestHelper::SessionServiceTestHelper(Profile* profile)
+    : SessionServiceTestHelper(
+          SessionServiceFactory::GetForProfileForSessionRestore(profile)) {}
 
 SessionServiceTestHelper::SessionServiceTestHelper(SessionService* service)
     : service_(service) {}
@@ -27,29 +33,29 @@ void SessionServiceTestHelper::PrepareTabInWindow(const SessionID& window_id,
                                                   const SessionID& tab_id,
                                                   int visual_index,
                                                   bool select) {
-  service()->SetTabWindow(window_id, tab_id);
-  service()->SetTabIndexInWindow(window_id, tab_id, visual_index);
+  service_->SetTabWindow(window_id, tab_id);
+  service_->SetTabIndexInWindow(window_id, tab_id, visual_index);
   if (select)
-    service()->SetSelectedTabInWindow(window_id, visual_index);
+    service_->SetSelectedTabInWindow(window_id, visual_index);
 }
 
 void SessionServiceTestHelper::SetTabExtensionAppID(
     const SessionID& window_id,
     const SessionID& tab_id,
     const std::string& extension_app_id) {
-  service()->SetTabExtensionAppID(window_id, tab_id, extension_app_id);
+  service_->SetTabExtensionAppID(window_id, tab_id, extension_app_id);
 }
 
 void SessionServiceTestHelper::SetTabUserAgentOverride(
     const SessionID& window_id,
     const SessionID& tab_id,
     const sessions::SerializedUserAgentOverride& user_agent_override) {
-  service()->SetTabUserAgentOverride(window_id, tab_id, user_agent_override);
+  service_->SetTabUserAgentOverride(window_id, tab_id, user_agent_override);
 }
 
 void SessionServiceTestHelper::SetForceBrowserNotAliveWithNoWindows(
     bool force_browser_not_alive_with_no_windows) {
-  service()->force_browser_not_alive_with_no_windows_ =
+  service_->force_browser_not_alive_with_no_windows_ =
       force_browser_not_alive_with_no_windows;
 }
 
@@ -62,7 +68,7 @@ void SessionServiceTestHelper::ReadWindows(
   std::vector<std::unique_ptr<sessions::SessionCommand>> read_commands =
       test_helper.ReadLastSessionCommands();
   RestoreSessionFromCommands(read_commands, windows, active_window_id);
-  service()->RemoveUnusedRestoreWindows(windows);
+  service_->RemoveUnusedRestoreWindows(windows);
 }
 
 void SessionServiceTestHelper::AssertTabEquals(
@@ -105,15 +111,11 @@ void SessionServiceTestHelper::AssertSingleWindowWithSingleTab(
 }
 
 void SessionServiceTestHelper::SetService(SessionService* service) {
-  service_.reset(service);
+  service_ = service;
   // Execute IO tasks posted by the SessionService.
   content::RunAllTasksUntilIdle();
 }
 
-SessionService* SessionServiceTestHelper::ReleaseService() {
-  return service_.release();
-}
-
 void SessionServiceTestHelper::RunTaskOnBackendThread(
     const base::Location& from_here,
     base::OnceClosure task) {

chrome/browser/sessions/session_service_test_helper.h

@@ -13,10 +13,10 @@
 #include <vector>
 
 #include "base/callback_forward.h"
-#include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "components/sessions/core/session_id.h"
 
+class Profile;
 class SessionService;
 
 namespace base {
@@ -36,7 +36,10 @@ struct SessionWindow;
 class SessionServiceTestHelper {
  public:
   SessionServiceTestHelper();
+  explicit SessionServiceTestHelper(Profile* profile);
   explicit SessionServiceTestHelper(SessionService* service);
+  SessionServiceTestHelper(const SessionServiceTestHelper&) = delete;
+  SessionServiceTestHelper& operator=(const SessionServiceTestHelper&) = delete;
   ~SessionServiceTestHelper();
 
   void PrepareTabInWindow(const SessionID& window_id,
@@ -82,8 +85,7 @@ class SessionServiceTestHelper {
       size_t nav_count);
 
   void SetService(SessionService* service);
-  SessionService* ReleaseService();
-  SessionService* service() { return service_.get(); }
+  SessionService* service() { return service_; }
 
   void RunTaskOnBackendThread(const base::Location& from_here,
                               base::OnceClosure task);
@@ -100,9 +102,7 @@ class SessionServiceTestHelper {
   void SetIsOnlyOneTabLeft(bool is_only_one_tab_left);
 
  private:
-  std::unique_ptr<SessionService> service_;
-
-  DISALLOW_COPY_AND_ASSIGN(SessionServiceTestHelper);
+  SessionService* service_;
 };
 
 #endif  // CHROME_BROWSER_SESSIONS_SESSION_SERVICE_TEST_HELPER_H_

chrome/browser/sessions/session_service_unittest.cc

@@ -65,10 +65,10 @@ class SessionServiceTest : public BrowserWithTestWindowTest {
     BrowserWithTestWindowTest::SetUp();
 
     Profile* profile = browser()->profile();
-    SessionService* session_service = new SessionService(profile);
+    session_service_ = std::make_unique<SessionService>(profile);
     path_ = profile->GetPath();
 
-    helper_.SetService(session_service);
+    helper_.SetService(session_service_.get());
 
     service()->SetWindowType(window_id, Browser::TYPE_NORMAL);
     service()->SetWindowBounds(window_id,
@@ -78,10 +78,17 @@ class SessionServiceTest : public BrowserWithTestWindowTest {
   }
 
   void TearDown() override {
-    helper_.SetService(nullptr);
+    DestroySessionService();
     BrowserWithTestWindowTest::TearDown();
   }
 
+  void DestroySessionService() {
+    // Destroy the SessionService first as it may post tasks.
+    session_service_.reset();
+    // This flushes tasks.
+    helper_.SetService(nullptr);
+  }
+
   void UpdateNavigation(
       const SessionID& window_id,
       const SessionID& tab_id,
@@ -107,11 +114,10 @@ class SessionServiceTest : public BrowserWithTestWindowTest {
   void ReadWindows(
       std::vector<std::unique_ptr<sessions::SessionWindow>>* windows,
       SessionID* active_window_id) {
-    // Forces closing the file.
-    helper_.SetService(nullptr);
+    DestroySessionService();
 
-    SessionService* session_service = new SessionService(path_);
-    helper_.SetService(session_service);
+    session_service_ = std::make_unique<SessionService>(path_);
+    helper_.SetService(session_service_.get());
 
     SessionID* non_null_active_window_id = active_window_id;
     SessionID dummy_active_window_id = SessionID::InvalidValue();
@@ -184,6 +190,7 @@ class SessionServiceTest : public BrowserWithTestWindowTest {
   base::ScopedTempDir temp_dir_;
   base::FilePath path_;
 
+  std::unique_ptr<SessionService> session_service_;
   SessionServiceTestHelper helper_;
 };
 
@@ -1327,7 +1334,9 @@ TEST_F(SessionServiceTest, GetSessionsAndDestroy) {
       base::BindOnce(&SimulateWaitForTesting, base::Unretained(&flag)));
   service()->GetLastSession(base::BindOnce(&OnGotPreviousSession));
   helper_.RunTaskOnBackendThread(FROM_HERE, run_loop.QuitClosure());
-  delete helper_.ReleaseService();
+  // Don't use DestroySessionService() as that runs the MessageLoop, which
+  // this test needs to control.
+  session_service_.reset();
   flag.Set();
   run_loop.Run();
 }

chrome/browser/ui/views/frame/browser_non_client_frame_view_chromeos_browsertest.cc

@@ -438,10 +438,8 @@ IN_PROC_BROWSER_TEST_P(BrowserNonClientFrameViewChromeOSTest,
   Profile* profile = browser()->profile();
   SessionStartupPref::SetStartupPref(profile, pref);
 
-  SessionServiceTestHelper helper(
-      SessionServiceFactory::GetForProfile(profile));
+  SessionServiceTestHelper helper(profile);
   helper.SetForceBrowserNotAliveWithNoWindows(true);
-  helper.ReleaseService();
 
   // Do not exit from test when last browser is closed.
   ScopedKeepAlive keep_alive(KeepAliveOrigin::SESSION_RESTORE,