Reland "Destroy AshTestHelper in AshTestBase::TearDown"

This reverts commit c460691ec4.

Reason for revert: Relanding with fix. Found the UAF.
base::ScopedObservation does not enable raw_ptr check,
so it wasn't caught by CQ.

Original change's description:
> Revert "Destroy AshTestHelper in AshTestBase::TearDown"
>
> This reverts commit 34e658ec6a.
>
> Reason for revert: Caused MSan test failure: https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/49775/test-results?q=ExactID%3Aninja%3A%2F%2Fcomponents%2Fexo%3Aexo_unittests%2FUILockControllerTest.DestroyingWindowCancels+VHash%3A2bb96b2e8deb3632
>
> Original change's description:
> > Destroy AshTestHelper in AshTestBase::TearDown
> >
> > The instance is created in AshTestHelper::SetUp(), so destroy it in
> > AshTestBase::TearDown.
> > This provides a way to run some code after AshTestHelper's destruction
> > in inheriting fixtures in latter CLs.
> >
> > There's two edge cases we cannot handle easily.
> > Screen() instance is updated in ash::Shell()'s tear down.
> >
> > (Testing)ProfileManager needs to be destroyed "during" TearDown
> > to follow the production behavior (specifically, after destroying
> > ash::Shell(), but before SessionManager/UserManager).
> > As a workaround, this CL adds a callback to AshTestBase invoked just
> > before destroying AshTestHelper.
> >
> > BUG=278643115
> > TEST=Tryjob
> >
> > Change-Id: I19fa00ee46fb9b7a494c08af084156b2df0a3dbf
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6224054
> > Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
> > Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1415300}
>
> Bug: 278643115
> Change-Id: Ib33186a515abc76d22e7fd4dc8f419945404ca00
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6228969
> Auto-Submit: Kenichi Ishibashi <bashi@chromium.org>
> Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
> Owners-Override: Kenichi Ishibashi <bashi@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#1415363}

Bug: 278643115
Change-Id: I71221751354b4b85855b8f86b86663e14e0ce497
Cq-Include-Trybots: luci.chromium.try:linux_chromium_chromeos_msan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6228221
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Owners-Override: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1415421}

ash/test/ash_test_base.cc

@@ -15,6 +15,7 @@
 #include "ash/constants/ash_switches.h"
 #include "ash/display/extended_mouse_warp_controller.h"
 #include "ash/display/mouse_cursor_event_filter.h"
+#include "ash/display/screen_ash.h"
 #include "ash/display/screen_orientation_controller_test_api.h"
 #include "ash/display/unified_mouse_warp_controller.h"
 #include "ash/display/window_tree_host_manager.h"
@@ -130,6 +131,11 @@ AshTestBase::AshTestBase(
 }
 
 AshTestBase::~AshTestBase() {
+  // Ensure the next test starts with a null display::Screen.  This must be done
+  // here instead of in TearDown() since some tests test access to the Screen
+  // after the shell shuts down (which they use TearDown() to trigger).
+  ScreenAsh::DeleteScreenForShutdown();
+
   CHECK(setup_called_)
       << "You have overridden SetUp but never called AshTestBase::SetUp";
   CHECK(teardown_called_)
@@ -150,6 +156,10 @@ void AshTestBase::SetUp(std::unique_ptr<TestShellDelegate> delegate) {
 
   init_params_.delegate = std::move(delegate);
   init_params_.local_state = local_state();
+  // AshTestBase destroys the Screen instance at the destructor,
+  // because some of the tests verifies the screen instance
+  // after the ash::Shell destroyed in AshTestHelper::TearDown().
+  init_params_.destroy_screen = false;
 
   // Prepare for a pixel test if having pixel init params.
   std::optional<pixel_test::InitParams> pixel_test_init_params =
@@ -191,6 +201,8 @@ void AshTestBase::TearDown() {
   base::RunLoop().RunUntilIdle();
 
   ash_test_helper_->TearDown();
+  OnHelperWillBeDestroyed();
+  ash_test_helper_.reset();
 
   event_generator_.reset();
   // Some tests set an internal display id,

ash/test/ash_test_base.h

@@ -254,6 +254,9 @@ class AshTestBase : public testing::Test {
       std::list<base::OnceClosure>* tasks,
       bool is_touch);
 
+  // Called when AshTestHelper will be soon destroyed.
+  virtual void OnHelperWillBeDestroyed() {}
+
  protected:
   enum UserSessionBlockReason {
     FIRST_BLOCK_REASON,

ash/test/ash_test_helper.cc

@@ -165,10 +165,12 @@ AshTestHelper::~AshTestHelper() {
 
   SimpleGeolocationProvider::DestroyForTesting();
 
-  // Ensure the next test starts with a null display::Screen.  This must be done
-  // here instead of in TearDown() since some tests test access to the Screen
-  // after the shell shuts down (which they use TearDown() to trigger).
-  ScreenAsh::DeleteScreenForShutdown();
+  if (destroy_screen_) {
+    // Ensure the next test starts with a null display::Screen.  This must be
+    // done here instead of in TearDown() since some tests test access to the
+    // Screen after the shell shuts down (which they use TearDown() to trigger).
+    ScreenAsh::DeleteScreenForShutdown();
+  }
 
   // This should never have a meaningful effect, since either there is no
   // ViewsTestHelperAura instance or the instance is currently in its
@@ -285,6 +287,7 @@ void AshTestHelper::SetUp(InitParams init_params) {
   create_global_cras_audio_handler_ =
       init_params.create_global_cras_audio_handler;
   create_quick_pair_mediator_ = init_params.create_quick_pair_mediator;
+  destroy_screen_ = init_params.destroy_screen;
 
   if (create_global_cras_audio_handler_) {
     // Create `CrasAudioHandler` for testing since `g_browser_process` is not

ash/test/ash_test_helper.h

@@ -100,6 +100,9 @@ class AshTestHelper : public aura::test::AuraTestHelper {
 
     // True to auto create prefs services.
     bool auto_create_prefs_services = true;
+
+    // Whether or not to destroy the screen in the destructor.
+    bool destroy_screen = true;
   };
 
   // Instantiates/destroys an AshTestHelper. This can happen in a
@@ -257,6 +260,8 @@ class AshTestHelper : public aura::test::AuraTestHelper {
   bool create_global_cras_audio_handler_ = true;
   // True if a fake `QuickPairMediator` should be created.
   bool create_quick_pair_mediator_ = true;
+  // True if a screen instance should be destroyed.
+  bool destroy_screen_ = true;
 };
 
 }  // namespace ash

chrome/browser/ui/ash/assistant/assistant_browser_delegate_impl_unittest.cc

@@ -76,6 +76,11 @@ class AssistantBrowserDelegateImplTest : public ChromeAshTestBase {
             std::make_unique<web_app::FakeWebAppDatabaseFactory>());
   }
 
+  void TearDown() override {
+    delegate_.reset();
+    ChromeAshTestBase::TearDown();
+  }
+
  protected:
   Profile* profile() const { return profile_.get(); }
 

chrome/browser/ui/ash/multi_user/multi_profile_support_unittest.cc

@@ -134,6 +134,7 @@ class MultiProfileSupportTest : public ChromeAshTestBase {
   // ChromeAshTestBase:
   void SetUp() override;
   void TearDown() override;
+  void OnHelperWillBeDestroyed() override;
 
  protected:
   void SwitchActiveUser(const AccountId& id) {
@@ -369,11 +370,18 @@ void MultiProfileSupportTest::TearDown() {
 
   ::MultiUserWindowManagerHelper::DeleteInstance();
   ChromeAshTestBase::TearDown();
-  profile_manager_.reset();
+  // ProfileManager instance is destroyed in OnHelperWillBeDestroyed()
+  // invoked inside ChromeAshTestBase::TearDown().
+  EXPECT_FALSE(profile_manager_.get());
   cros_settings_holder_.reset();
   ash::DeviceSettingsService::Shutdown();
 }
 
+void MultiProfileSupportTest::OnHelperWillBeDestroyed() {
+  ChromeAshTestBase::OnHelperWillBeDestroyed();
+  profile_manager_.reset();
+}
+
 std::string MultiProfileSupportTest::GetStatusImpl(bool follow_transients) {
   std::string s;
   for (size_t i = 0; i < windows_.size(); i++) {

chrome/browser/ui/ash/projector/projector_soda_installation_controller_unittest.cc

@@ -88,7 +88,9 @@ class ProjectorSodaInstallationControllerTest : public ChromeAshTestBase {
   void SetUp() override {
     ChromeAshTestBase::SetUp();
 
-    ASSERT_TRUE(testing_profile_manager_.SetUp());
+    testing_profile_manager_ = std::make_unique<TestingProfileManager>(
+        TestingBrowserProcess::GetGlobal());
+    ASSERT_TRUE(testing_profile_manager_->SetUp());
     testing_profile_ = ProfileManager::GetPrimaryUserProfile();
 
     soda_installer_ = std::make_unique<MockSodaInstaller>();
@@ -126,6 +128,14 @@ class ProjectorSodaInstallationControllerTest : public ChromeAshTestBase {
     soda_installer_.reset();
 
     ChromeAshTestBase::TearDown();
+    // ProfileManager is destroyed in OnHelperWillBeDestroyed()
+    // invoked in ChromeAshTestBase::TearDown().
+    EXPECT_FALSE(testing_profile_manager_.get());
+  }
+
+  void OnHelperWillBeDestroyed() override {
+    ChromeAshTestBase::OnHelperWillBeDestroyed();
+    testing_profile_manager_.reset();
   }
 
   MockAppClient& app_client() { return *mock_app_client_; }
@@ -145,8 +155,7 @@ class ProjectorSodaInstallationControllerTest : public ChromeAshTestBase {
  private:
   raw_ptr<Profile, DanglingUntriaged> testing_profile_ = nullptr;
 
-  TestingProfileManager testing_profile_manager_{
-      TestingBrowserProcess::GetGlobal()};
+  std::unique_ptr<TestingProfileManager> testing_profile_manager_;
 
   std::unique_ptr<MockSodaInstaller> soda_installer_;
   std::unique_ptr<MockProjectorClient> mock_client_;

components/exo/ui_lock_controller_unittest.cc

@@ -153,6 +153,7 @@ class UILockControllerTest : public test::ExoTestBase {
   }
 
   void TearDown() override {
+    auth_events_recorder_.reset();
     seat_.reset();
     test::ExoTestBase::TearDown();
   }