[accessibility] BrowserMainLoop owns BrowserAccessibilityStateImpl

Make the process-wide BrowserAccessibilityStateImpl a member of
BrowserMainLoop rather than a self-contained singleton that outlives the
browser process. This provides deterministic construction and makes it
easier to allow the content embedder to customize the instance.

For the sake of tests:
- content::UnitTestTestSuite owns an instance for use by unit tests.
- content::TestContentClientInitializer owns an instance for use by unit
  tests that run outside of an ordinary content unit test.

Bug: 1470199
AX-Relnotes: n/a.
Change-Id: I33b8606f2113ce6a4cc4f24f4fce1131b84f956d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4742506
Reviewed-by: Avi Drissman <[email protected]>
Reviewed-by: Benjamin Beaudry <[email protected]>
Reviewed-by: Jacques Newman <[email protected]>
Commit-Queue: Greg Thompson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1186389}

content/browser/accessibility/browser_accessibility_state_impl.cc

@@ -6,12 +6,13 @@
 
 #include <stddef.h>
 
+#include "base/check.h"
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
 #include "base/functional/bind.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/no_destructor.h"
 #include "base/notreached.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/thread_pool.h"
@@ -28,6 +29,10 @@
 
 namespace content {
 
+namespace {
+
+BrowserAccessibilityStateImpl* g_instance = nullptr;
+
 // Auto-disable accessibility if this many seconds elapse with user input
 // events but no accessibility API usage.
 constexpr int kAutoDisableAccessibilityTimeSecs = 30;
@@ -66,24 +71,35 @@ void RecordNewAccessibilityModeFlags(
 // Update the accessibility histogram 45 seconds after initialization.
 static const int ACCESSIBILITY_HISTOGRAM_DELAY_SECS = 45;
 
+}  // namespace
+
 // static
 BrowserAccessibilityState* BrowserAccessibilityState::GetInstance() {
   return BrowserAccessibilityStateImpl::GetInstance();
 }
 
+// static
+BrowserAccessibilityStateImpl* BrowserAccessibilityStateImpl::GetInstance() {
+  CHECK(g_instance);
+  return g_instance;
+}
+
 // On Android, Mac, Lacros, and Windows there are platform-specific subclasses.
 #if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_WIN) && !BUILDFLAG(IS_MAC) && \
     !BUILDFLAG(IS_CHROMEOS_LACROS)
 // static
-BrowserAccessibilityStateImpl* BrowserAccessibilityStateImpl::GetInstance() {
-  static base::NoDestructor<BrowserAccessibilityStateImpl> instance;
-  return &*instance;
+std::unique_ptr<BrowserAccessibilityStateImpl>
+BrowserAccessibilityStateImpl::Create() {
+  return base::WrapUnique(new BrowserAccessibilityStateImpl());
 }
 #endif
 
 BrowserAccessibilityStateImpl::BrowserAccessibilityStateImpl()
     : BrowserAccessibilityState(),
       histogram_delay_(base::Seconds(ACCESSIBILITY_HISTOGRAM_DELAY_SECS)) {
+  DCHECK_EQ(g_instance, nullptr);
+  g_instance = this;
+
   bool disallow_changes = false;
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableRendererAccessibility)) {
@@ -150,6 +166,9 @@ void BrowserAccessibilityStateImpl::InitBackgroundTasks() {
 }
 
 BrowserAccessibilityStateImpl::~BrowserAccessibilityStateImpl() {
+  DCHECK_EQ(g_instance, this);
+  g_instance = nullptr;
+
   // Remove ourselves from the AXMode global observer list.
   ui::AXPlatformNode::RemoveAXModeObserver(this);
 }

content/browser/accessibility/browser_accessibility_state_impl.h

@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_ACCESSIBILITY_BROWSER_ACCESSIBILITY_STATE_IMPL_H_
 #define CONTENT_BROWSER_ACCESSIBILITY_BROWSER_ACCESSIBILITY_STATE_IMPL_H_
 
+#include <memory>
 #include <vector>
 
 #include "base/time/time.h"
@@ -40,16 +41,19 @@ class CONTENT_EXPORT BrowserAccessibilityStateImpl
     : public BrowserAccessibilityState,
       public ui::AXModeObserver {
  public:
-  BrowserAccessibilityStateImpl();
-
   BrowserAccessibilityStateImpl(const BrowserAccessibilityStateImpl&) = delete;
   BrowserAccessibilityStateImpl& operator=(
       const BrowserAccessibilityStateImpl&) = delete;
 
   ~BrowserAccessibilityStateImpl() override;
 
+  // Returns the single process-wide instance.
   static BrowserAccessibilityStateImpl* GetInstance();
 
+  // Returns a new instance. Only one instance may be live in the process at any
+  // time.
+  static std::unique_ptr<BrowserAccessibilityStateImpl> Create();
+
   // This needs to be called explicitly by content::BrowserMainLoop during
   // initialization, in order to schedule tasks that need to be done, but
   // don't need to block the main thread.
@@ -121,6 +125,8 @@ class CONTENT_EXPORT BrowserAccessibilityStateImpl
   void DisallowAXModeChanges();
 
  protected:
+  BrowserAccessibilityStateImpl();
+
   // Called a short while after startup to allow time for the accessibility
   // state to be determined. Updates histograms with the current state.
   // Two variants - one for things that must be run on the UI thread, and

content/browser/accessibility/browser_accessibility_state_impl_android.cc

@@ -4,10 +4,11 @@
 
 #include "content/browser/accessibility/browser_accessibility_state_impl_android.h"
 
+#include <memory>
+
 #include "base/containers/contains.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/no_destructor.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "ui/accessibility/android/accessibility_state.h"
@@ -455,15 +456,10 @@ void BrowserAccessibilityStateImplAndroid::SetImageLabelsModeForProfile(
   }
 }
 
-//
-// BrowserAccessibilityStateImpl::GetInstance implementation that constructs
-// this class instead of the base class.
-//
-
 // static
-BrowserAccessibilityStateImpl* BrowserAccessibilityStateImpl::GetInstance() {
-  static base::NoDestructor<BrowserAccessibilityStateImplAndroid> instance;
-  return &*instance;
+std::unique_ptr<BrowserAccessibilityStateImpl>
+BrowserAccessibilityStateImpl::Create() {
+  return std::make_unique<BrowserAccessibilityStateImplAndroid>();
 }
 
 }  // namespace content

content/browser/accessibility/browser_accessibility_state_impl_lacros.cc

@@ -4,7 +4,7 @@
 
 #include "content/browser/accessibility/browser_accessibility_state_impl_lacros.h"
 
-#include "base/no_destructor.h"
+#include <memory>
 
 namespace content {
 
@@ -27,9 +27,9 @@ void BrowserAccessibilityStateImplLacros::OnSpokenFeedbackPrefChanged(
 }
 
 // static
-BrowserAccessibilityStateImpl* BrowserAccessibilityStateImpl::GetInstance() {
-  static base::NoDestructor<BrowserAccessibilityStateImplLacros> instance;
-  return &*instance;
+std::unique_ptr<BrowserAccessibilityStateImpl>
+BrowserAccessibilityStateImpl::Create() {
+  return std::make_unique<BrowserAccessibilityStateImplLacros>();
 }
 
 }  // namespace content

content/browser/accessibility/browser_accessibility_state_impl_mac.mm

@@ -6,8 +6,9 @@
 
 #import <Cocoa/Cocoa.h>
 
+#include <memory>
+
 #include "base/metrics/histogram_macros.h"
-#include "base/no_destructor.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "ui/gfx/animation/animation.h"
@@ -76,15 +77,10 @@ void SetupAccessibilityDisplayOptionsNotifier() {
                         mode.has_mode(ui::AXMode::kScreenReader));
 }
 
-//
-// BrowserAccessibilityStateImpl::GetInstance implementation that constructs
-// this class instead of the base class.
-//
-
 // static
-BrowserAccessibilityStateImpl* BrowserAccessibilityStateImpl::GetInstance() {
-  static base::NoDestructor<BrowserAccessibilityStateImplMac> instance;
-  return &*instance;
+std::unique_ptr<BrowserAccessibilityStateImpl>
+BrowserAccessibilityStateImpl::Create() {
+  return std::make_unique<BrowserAccessibilityStateImplMac>();
 }
 
 }  // namespace content

content/browser/accessibility/browser_accessibility_state_impl_win.cc

@@ -13,7 +13,6 @@
 
 #include "base/files/file_path.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/no_destructor.h"
 #include "base/strings/string_util.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "ui/accessibility/accessibility_features.h"
@@ -155,7 +154,6 @@ void OnWndProc(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) {
 class BrowserAccessibilityStateImplWin : public BrowserAccessibilityStateImpl {
  public:
   BrowserAccessibilityStateImplWin();
-  ~BrowserAccessibilityStateImplWin() override {}
 
  protected:
   void UpdateHistogramsOnOtherThread() override;
@@ -268,15 +266,10 @@ void BrowserAccessibilityStateImplWin::UpdateUniqueUserHistograms() {
   UMA_HISTOGRAM_BOOLEAN("Accessibility.WinZoomText.EveryReport", g_zoomtext);
 }
 
-//
-// BrowserAccessibilityStateImpl::GetInstance implementation that constructs
-// this class instead of the base class.
-//
-
 // static
-BrowserAccessibilityStateImpl* BrowserAccessibilityStateImpl::GetInstance() {
-  static base::NoDestructor<BrowserAccessibilityStateImplWin> instance;
-  return &*instance;
+std::unique_ptr<BrowserAccessibilityStateImpl>
+BrowserAccessibilityStateImpl::Create() {
+  return std::make_unique<BrowserAccessibilityStateImplWin>();
 }
 
 }  // namespace content

content/browser/browser_main_loop.cc

@@ -754,7 +754,8 @@ void BrowserMainLoop::PostCreateMainMessageLoop() {
   {
     TRACE_EVENT0("startup",
                  "BrowserMainLoop::Subsystem:BrowserAccessibilityStateImpl");
-    BrowserAccessibilityStateImpl::GetInstance()->InitBackgroundTasks();
+    browser_accessibility_state_ = BrowserAccessibilityStateImpl::Create();
+    browser_accessibility_state_->InitBackgroundTasks();
   }
 }
 

content/browser/browser_main_loop.h

@@ -86,6 +86,7 @@ class HostFrameSinkManager;
 }  // namespace viz
 
 namespace content {
+class BrowserAccessibilityStateImpl;
 class BrowserMainParts;
 class BackgroundTracingManager;
 class BrowserOnlineStateObserver;
@@ -337,6 +338,7 @@ class CONTENT_EXPORT BrowserMainLoop {
   // Android implementation of ScreenOrientationDelegate
   std::unique_ptr<ScreenOrientationDelegate> screen_orientation_delegate_;
 #endif
+  std::unique_ptr<BrowserAccessibilityStateImpl> browser_accessibility_state_;
 
   // Destroy |parts_| before above members (except the ones that are explicitly
   // reset() on shutdown) but after |main_thread_| and services below.

content/public/test/test_content_client_initializer.cc

@@ -5,6 +5,7 @@
 #include "content/public/test/test_content_client_initializer.h"
 
 #include "build/build_config.h"
+#include "content/browser/accessibility/browser_accessibility_state_impl.h"
 #include "content/browser/notification_service_impl.h"
 #include "content/public/browser/network_service_instance.h"
 #include "content/public/common/content_client.h"
@@ -30,9 +31,12 @@ TestContentClientInitializer::TestContentClientInitializer() {
 
   content_browser_client_ = std::make_unique<TestContentBrowserClient>();
   content::SetBrowserClientForTesting(content_browser_client_.get());
+
+  browser_accessibility_state_ = BrowserAccessibilityStateImpl::Create();
 }
 
 TestContentClientInitializer::~TestContentClientInitializer() {
+  browser_accessibility_state_.reset();
   test_render_view_host_factory_.reset();
   rph_factory_.reset();
   notification_service_.reset();

content/public/test/test_content_client_initializer.h

@@ -13,6 +13,7 @@ class TestNetworkConnectionTracker;
 
 namespace content {
 
+class BrowserAccessibilityStateImpl;
 class ContentClient;
 class MockAgentSchedulingGroupHostFactory;
 class MockRenderProcessHostFactory;
@@ -50,6 +51,7 @@ class TestContentClientInitializer {
   std::unique_ptr<MockRenderProcessHostFactory> rph_factory_;
   std::unique_ptr<MockAgentSchedulingGroupHostFactory> asgh_factory_;
   std::unique_ptr<TestRenderViewHostFactory> test_render_view_host_factory_;
+  std::unique_ptr<BrowserAccessibilityStateImpl> browser_accessibility_state_;
 };
 
 }  // namespace content

content/public/test/unittest_test_suite.cc

@@ -16,6 +16,7 @@
 #include "components/breadcrumbs/core/breadcrumb_manager.h"
 #include "components/breadcrumbs/core/crash_reporter_breadcrumb_observer.h"
 #include "content/app/mojo/mojo_init.h"
+#include "content/browser/accessibility/browser_accessibility_state_impl.h"
 #include "content/browser/network_service_instance_impl.h"
 #include "content/browser/notification_service_impl.h"
 #include "content/browser/storage_partition_impl.h"
@@ -150,6 +151,7 @@ UnitTestTestSuite::UnitTestTestSuite(
 
   DCHECK(test_suite);
   test_host_resolver_ = std::make_unique<TestHostResolver>();
+  browser_accessibility_state_ = BrowserAccessibilityStateImpl::Create();
 }
 
 UnitTestTestSuite::~UnitTestTestSuite() = default;

content/public/test/unittest_test_suite.h

@@ -18,6 +18,7 @@ class TestSuite;
 }
 
 namespace content {
+class BrowserAccessibilityStateImpl;
 class ContentBrowserClient;
 class ContentClient;
 class ContentUtilityClient;
@@ -78,6 +79,8 @@ class UnitTestTestSuite {
 
   std::unique_ptr<TestHostResolver> test_host_resolver_;
 
+  std::unique_ptr<BrowserAccessibilityStateImpl> browser_accessibility_state_;
+
   base::RepeatingCallback<std::unique_ptr<ContentClients>()> create_clients_;
 
   base::test::ScopedFeatureList scoped_feature_list_;