Speculative fix for crash in extension_install_event_router.cc:42

- Make `extension_install_event_router_` a unique_ptr in ConnectorsManager
- Delete default copy and move ctors for Extension Install and Browser Crash event routers
- Add a check and a log statement in `ExtensionInstallEventRouter::StartObserving`
  to guard against `extension_registry_` being null

Crash cluster: https://crash.corp.google.com/browse?q=EXISTS+%28SELECT+1+FROM+UNNEST%28CrashedStackTrace.StackFrame%29+WHERE+SourceFileName%3D%27C%3A%5C%5Cb%5C%5Cs%5C%5Cw%5C%5Cir%5C%5Ccache%5C%5Cbuilder%5C%5Csrc%5C%5Cchrome%5C%5Cbrowser%5C%5Centerprise%5C%5Cconnectors%5C%5Creporting%5C%5Cextension_install_event_router.cc%27%29+AND+EXISTS+%28SELECT+1+FROM+UNNEST%28CrashedStackTrace.StackFrame%29+WHERE+SourceLine%3D42%29&stbtiq=&reportid=&index=0

BUG=1400220,1403107

(cherry picked from commit e943c4f)

Change-Id: Ie9283d395207ab5d0578f9fe663ee3dcaa1d601d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4116609
Commit-Queue: Shanthanu Bhardwaj <xanth@google.com>
Reviewed-by: Sebastien Lalancette <seblalancette@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1085946}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4127345
Reviewed-by: Dominique Fauteux-Chapleau <domfc@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5481@{#119}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}

chrome/browser/enterprise/connectors/connectors_manager.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/check.h"
 #include "base/values.h"
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/enterprise/connectors/reporting/browser_crash_event_router.h"
@@ -17,17 +18,19 @@ namespace enterprise_connectors {
 
 ConnectorsManager::ConnectorsManager(
     std::unique_ptr<BrowserCrashEventRouter> browser_crash_event_router,
-    ExtensionInstallEventRouter extension_install_event_router,
+    std::unique_ptr<ExtensionInstallEventRouter> extension_install_event_router,
     PrefService* pref_service,
     const ServiceProviderConfig* config,
     bool observe_prefs)
     : service_provider_config_(config),
       browser_crash_event_router_(std::move(browser_crash_event_router)),
       extension_install_event_router_(
           std::move(extension_install_event_router)) {
+  DCHECK(browser_crash_event_router_) << "Crash event router is null";
+  DCHECK(extension_install_event_router_) << "Extension event router is null";
   if (observe_prefs)
     StartObservingPrefs(pref_service);
-  extension_install_event_router_.StartObserving();
+  extension_install_event_router_->StartObserving();
 }
 
 ConnectorsManager::~ConnectorsManager() = default;

chrome/browser/enterprise/connectors/connectors_manager.h

@@ -36,7 +36,7 @@ class ConnectorsManager {
 
   ConnectorsManager(
       std::unique_ptr<BrowserCrashEventRouter> browser_crash_event_router,
-      ExtensionInstallEventRouter extension_install_router,
+      std::unique_ptr<ExtensionInstallEventRouter> extension_install_router,
       PrefService* pref_service,
       const ServiceProviderConfig* config,
       bool observe_prefs = true);
@@ -129,7 +129,7 @@ class ConnectorsManager {
   std::unique_ptr<BrowserCrashEventRouter> browser_crash_event_router_;
 
   // An observer to report extension install events via the reporting pipeline.
-  ExtensionInstallEventRouter extension_install_event_router_;
+  std::unique_ptr<ExtensionInstallEventRouter> extension_install_event_router_;
 };
 
 }  // namespace enterprise_connectors

chrome/browser/enterprise/connectors/connectors_manager_unittest.cc

@@ -229,9 +229,10 @@ class ConnectorsManagerConnectorPoliciesTest
 };
 
 TEST_P(ConnectorsManagerConnectorPoliciesTest, NormalPref) {
-  ConnectorsManager manager(std::make_unique<BrowserCrashEventRouter>(profile_),
-                            ExtensionInstallEventRouter(profile_),
-                            pref_service(), GetServiceProviderConfig());
+  ConnectorsManager manager(
+      std::make_unique<BrowserCrashEventRouter>(profile_),
+      std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+      GetServiceProviderConfig());
   ASSERT_TRUE(manager.GetAnalysisConnectorsSettingsForTesting().empty());
   ScopedConnectorPref scoped_pref(pref_service(), pref(), pref_value());
   SetUpExpectedAnalysisSettings(pref_value());
@@ -258,9 +259,10 @@ TEST_P(ConnectorsManagerConnectorPoliciesTest, NormalPref) {
 }
 
 TEST_P(ConnectorsManagerConnectorPoliciesTest, EmptyPref) {
-  ConnectorsManager manager(std::make_unique<BrowserCrashEventRouter>(profile_),
-                            ExtensionInstallEventRouter(profile_),
-                            pref_service(), GetServiceProviderConfig());
+  ConnectorsManager manager(
+      std::make_unique<BrowserCrashEventRouter>(profile_),
+      std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+      GetServiceProviderConfig());
   // If the connector's settings list is empty, no analysis settings are ever
   // returned.
   ASSERT_TRUE(manager.GetAnalysisConnectorsSettingsForTesting().empty());
@@ -571,9 +573,10 @@ class ConnectorsManagerConnectorPoliciesSourceDestinationTest
 };
 
 TEST_P(ConnectorsManagerConnectorPoliciesSourceDestinationTest, NormalPref) {
-  ConnectorsManager manager(std::make_unique<BrowserCrashEventRouter>(profile_),
-                            ExtensionInstallEventRouter(profile_),
-                            pref_service(), GetServiceProviderConfig());
+  ConnectorsManager manager(
+      std::make_unique<BrowserCrashEventRouter>(profile_),
+      std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+      GetServiceProviderConfig());
   ASSERT_TRUE(manager.GetAnalysisConnectorsSettingsForTesting().empty());
   ScopedConnectorPref scoped_pref(pref_service(), pref(), pref_value());
   SetUpExpectedAnalysisSettings(pref_value());
@@ -603,9 +606,10 @@ TEST_P(ConnectorsManagerConnectorPoliciesSourceDestinationTest, NormalPref) {
 }
 
 TEST_P(ConnectorsManagerConnectorPoliciesSourceDestinationTest, EmptyPref) {
-  ConnectorsManager manager(std::make_unique<BrowserCrashEventRouter>(profile_),
-                            ExtensionInstallEventRouter(profile_),
-                            pref_service(), GetServiceProviderConfig());
+  ConnectorsManager manager(
+      std::make_unique<BrowserCrashEventRouter>(profile_),
+      std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+      GetServiceProviderConfig());
   // If the connector's settings list is empty, no analysis settings are ever
   // returned.
   ASSERT_TRUE(manager.GetAnalysisConnectorsSettingsForTesting().empty());
@@ -658,9 +662,10 @@ class ConnectorsManagerAnalysisConnectorsTest
 };
 
 TEST_P(ConnectorsManagerAnalysisConnectorsTest, DynamicPolicies) {
-  ConnectorsManager manager(std::make_unique<BrowserCrashEventRouter>(profile_),
-                            ExtensionInstallEventRouter(profile_),
-                            pref_service(), GetServiceProviderConfig());
+  ConnectorsManager manager(
+      std::make_unique<BrowserCrashEventRouter>(profile_),
+      std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+      GetServiceProviderConfig());
   // The cache is initially empty.
   ASSERT_TRUE(manager.GetAnalysisConnectorsSettingsForTesting().empty());
 
@@ -699,9 +704,10 @@ TEST_P(ConnectorsManagerAnalysisConnectorsTest, DynamicPolicies) {
 }
 
 TEST_P(ConnectorsManagerAnalysisConnectorsTest, NamesAndConfigs) {
-  ConnectorsManager manager(std::make_unique<BrowserCrashEventRouter>(profile_),
-                            ExtensionInstallEventRouter(profile_),
-                            pref_service(), GetServiceProviderConfig());
+  ConnectorsManager manager(
+      std::make_unique<BrowserCrashEventRouter>(profile_),
+      std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+      GetServiceProviderConfig());
   ScopedConnectorPref scoped_pref(pref_service(), pref(), pref_value());
 
   auto names = manager.GetAnalysisServiceProviderNames(connector());
@@ -775,9 +781,10 @@ class ConnectorsManagerAnalysisConnectorsSourceDestinationTest
 
 TEST_P(ConnectorsManagerAnalysisConnectorsSourceDestinationTest,
        DynamicPolicies) {
-  ConnectorsManager manager(std::make_unique<BrowserCrashEventRouter>(profile_),
-                            ExtensionInstallEventRouter(profile_),
-                            pref_service(), GetServiceProviderConfig());
+  ConnectorsManager manager(
+      std::make_unique<BrowserCrashEventRouter>(profile_),
+      std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+      GetServiceProviderConfig());
   // The cache is initially empty.
   ASSERT_TRUE(manager.GetAnalysisConnectorsSettingsForTesting().empty());
 
@@ -840,9 +847,10 @@ class ConnectorsManagerReportingTest
 };
 
 TEST_P(ConnectorsManagerReportingTest, DynamicPolicies) {
-  ConnectorsManager manager(std::make_unique<BrowserCrashEventRouter>(profile_),
-                            ExtensionInstallEventRouter(profile_),
-                            pref_service(), GetServiceProviderConfig());
+  ConnectorsManager manager(
+      std::make_unique<BrowserCrashEventRouter>(profile_),
+      std::make_unique<ExtensionInstallEventRouter>(profile_), pref_service(),
+      GetServiceProviderConfig());
   // The cache is initially empty.
   ASSERT_TRUE(manager.GetReportingConnectorsSettingsForTesting().empty());
 

chrome/browser/enterprise/connectors/connectors_service.cc

@@ -598,7 +598,7 @@ KeyedService* ConnectorsServiceFactory::BuildServiceInstanceFor(
   return new ConnectorsService(
       context, std::make_unique<ConnectorsManager>(
                    std::make_unique<BrowserCrashEventRouter>(context),
-                   ExtensionInstallEventRouter(context),
+                   std::make_unique<ExtensionInstallEventRouter>(context),
                    user_prefs::UserPrefs::Get(context),
                    GetServiceProviderConfig(), observe_prefs));
 }

chrome/browser/enterprise/connectors/connectors_service_unittest.cc

@@ -505,11 +505,10 @@ class ConnectorsServiceProfileTypeBrowserTest : public testing::Test {
   }
 
   std::unique_ptr<ConnectorsService> CreateService(Profile* profile) {
-    ExtensionInstallEventRouter router(profile);
-
     auto manager = std::make_unique<ConnectorsManager>(
-        nullptr, router, profile->GetPrefs(), GetServiceProviderConfig(),
-        false);
+        std::make_unique<BrowserCrashEventRouter>(profile),
+        std::make_unique<ExtensionInstallEventRouter>(profile),
+        profile->GetPrefs(), GetServiceProviderConfig(), false);
 
     return std::make_unique<ConnectorsService>(profile, std::move(manager));
   }

chrome/browser/enterprise/connectors/reporting/browser_crash_event_router.h

@@ -24,7 +24,11 @@ class BrowserCrashEventRouter
   // that once the browser launches, OnCloudReportingLaunched() will be called,
   // where we can call ReportCrashes() to report crashes.
   explicit BrowserCrashEventRouter(content::BrowserContext* context);
+
+  BrowserCrashEventRouter(const BrowserCrashEventRouter&) = delete;
   BrowserCrashEventRouter& operator=(const BrowserCrashEventRouter&) = delete;
+  BrowserCrashEventRouter(BrowserCrashEventRouter&&) = delete;
+  BrowserCrashEventRouter& operator=(BrowserCrashEventRouter&&) = delete;
   ~BrowserCrashEventRouter() override;
 
 #if !BUILDFLAG(IS_FUCHSIA)

chrome/browser/enterprise/connectors/reporting/extension_install_event_router.cc

@@ -39,6 +39,10 @@ void ExtensionInstallEventRouter::StartObserving() {
   if (!base::FeatureList::IsEnabled(kExtensionEventsEnabled)) {
     return;
   }
+  if (!extension_registry_) {
+    DLOG(ERROR) << "extension_registry_ is null. Observer not added.";
+    return;
+  }
   extension_registry_->AddObserver(this);
 }
 

chrome/browser/enterprise/connectors/reporting/extension_install_event_router.h

@@ -23,8 +23,12 @@ class ExtensionInstallEventRouter
  public:
   explicit ExtensionInstallEventRouter(content::BrowserContext* context);
 
+  ExtensionInstallEventRouter(const ExtensionInstallEventRouter&) = delete;
   ExtensionInstallEventRouter& operator=(const ExtensionInstallEventRouter&) =
       delete;
+  ExtensionInstallEventRouter(ExtensionInstallEventRouter&&) = delete;
+  ExtensionInstallEventRouter& operator=(ExtensionInstallEventRouter&&) =
+      delete;
 
   ~ExtensionInstallEventRouter() override;
   void OnExtensionInstalled(content::BrowserContext* browser_context,