Add additional stages into InstallationReporter::Stage

We see in statistics that for some reason force-installed extensions stuck in
CREATED stage. Generally it should never happen, since these extensions
should immediately be moved to PENDING stage (or reported as failed).
Unfortunately, we weren't able to reproduce it locally. To have the
problem more localized we need to split CREATED->PENDING transition into
substages.

Bug: 989526
Change-Id: I4a3f1601dea67c0a75cec1c1ebcf07536ef8704c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1729252Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: Oleg Davydov <burunduk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688452}

chrome/browser/extensions/extension_management.cc

@@ -506,6 +506,17 @@ void ExtensionManagement::OnExtensionPrefChanged() {
 }
 
 void ExtensionManagement::NotifyExtensionManagementPrefChanged() {
+  for (const auto& entry : settings_by_id_) {
+    if (entry.second->installation_mode == INSTALLATION_FORCED) {
+      InstallationReporter::ReportInstallationStage(
+          profile_, entry.first,
+          InstallationReporter::Stage::NOTIFIED_FROM_MANAGEMENT);
+    } else {
+      InstallationReporter::ReportInstallationStage(
+          profile_, entry.first,
+          InstallationReporter::Stage::NOTIFIED_FROM_MANAGEMENT_NOT_FORCED);
+    }
+  }
   for (auto& observer : observer_list_)
     observer.OnExtensionManagementSettingsChanged();
 }

chrome/browser/extensions/external_policy_loader.cc

@@ -8,12 +8,14 @@
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
+#include "chrome/browser/extensions/forced_extensions/installation_reporter.h"
 
 namespace extensions {
 
-ExternalPolicyLoader::ExternalPolicyLoader(ExtensionManagement* settings,
+ExternalPolicyLoader::ExternalPolicyLoader(Profile* profile,
+                                           ExtensionManagement* settings,
                                            InstallationType type)
-    : settings_(settings), type_(type) {
+    : profile_(profile), settings_(settings), type_(type) {
   settings_->AddObserver(this);
 }
 
@@ -39,6 +41,11 @@ void ExternalPolicyLoader::StartLoading() {
   switch (type_) {
     case FORCED:
       prefs = settings_->GetForceInstallList();
+      for (const auto& it : prefs->DictItems()) {
+        InstallationReporter::ReportInstallationStage(
+            profile_, it.first,
+            InstallationReporter::Stage::SEEN_BY_POLICY_LOADER);
+      }
       break;
     case RECOMMENDED:
       prefs = settings_->GetRecommendedInstallList();

chrome/browser/extensions/external_policy_loader.h

@@ -12,6 +12,8 @@
 #include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/extensions/external_loader.h"
 
+class Profile;
+
 namespace base {
 class DictionaryValue;
 }
@@ -32,7 +34,9 @@ class ExternalPolicyLoader : public ExternalLoader,
     RECOMMENDED
   };
 
-  ExternalPolicyLoader(ExtensionManagement* settings, InstallationType type);
+  ExternalPolicyLoader(Profile* profile,
+                       ExtensionManagement* settings,
+                       InstallationType type);
 
   // ExtensionManagement::Observer implementation
   void OnExtensionManagementSettingsChanged() override;
@@ -50,6 +54,7 @@ class ExternalPolicyLoader : public ExternalLoader,
 
   ~ExternalPolicyLoader() override;
 
+  Profile* profile_;
   ExtensionManagement* settings_;
   InstallationType type_;
 

chrome/browser/extensions/external_policy_loader_unittest.cc

@@ -57,12 +57,11 @@ class MockExternalPolicyProviderVisitor
     provider_.reset(new ExternalProviderImpl(
         this,
         new ExternalPolicyLoader(
+            profile_.get(),
             ExtensionManagementFactory::GetForBrowserContext(profile_.get()),
             ExternalPolicyLoader::FORCED),
-        profile_.get(),
-        Manifest::INVALID_LOCATION,
-        Manifest::EXTERNAL_POLICY_DOWNLOAD,
-        Extension::NO_FLAGS));
+        profile_.get(), Manifest::INVALID_LOCATION,
+        Manifest::EXTERNAL_POLICY_DOWNLOAD, Extension::NO_FLAGS));
 
     // Extensions will be removed from this list as they visited,
     // so it should be emptied by the end.

chrome/browser/extensions/external_provider_impl.cc

@@ -145,6 +145,12 @@ void ExternalProviderImpl::SetPrefs(
   // away while |loader_| was working on the FILE thread.
   if (!service_) return;
 
+  for (const auto& it : prefs->DictItems()) {
+    InstallationReporter::ReportInstallationStage(
+        profile_, it.first,
+        InstallationReporter::Stage::SEEN_BY_EXTERNAL_PROVIDER);
+  }
+
   prefs_ = std::move(prefs);
   ready_ = true;  // Queries for extensions are allowed from this point.
 
@@ -588,7 +594,7 @@ void ExternalProviderImpl::CreateExternalProviders(
     // path will have type |TYPE_LOGIN_SCREE_EXTENSION| with limited API
     // capabilities.
     external_loader = new ExternalPolicyLoader(
-        ExtensionManagementFactory::GetForBrowserContext(profile),
+        profile, ExtensionManagementFactory::GetForBrowserContext(profile),
         ExternalPolicyLoader::FORCED);
     auto signin_profile_provider = std::make_unique<ExternalProviderImpl>(
         service, external_loader, profile, crx_location,
@@ -622,18 +628,18 @@ void ExternalProviderImpl::CreateExternalProviders(
     }
   } else {
     external_loader = new ExternalPolicyLoader(
-        ExtensionManagementFactory::GetForBrowserContext(profile),
+        profile, ExtensionManagementFactory::GetForBrowserContext(profile),
         ExternalPolicyLoader::FORCED);
     external_recommended_loader = new ExternalPolicyLoader(
-        ExtensionManagementFactory::GetForBrowserContext(profile),
+        profile, ExtensionManagementFactory::GetForBrowserContext(profile),
         ExternalPolicyLoader::RECOMMENDED);
   }
 #else
   external_loader = new ExternalPolicyLoader(
-      ExtensionManagementFactory::GetForBrowserContext(profile),
+      profile, ExtensionManagementFactory::GetForBrowserContext(profile),
       ExternalPolicyLoader::FORCED);
   external_recommended_loader = new ExternalPolicyLoader(
-      ExtensionManagementFactory::GetForBrowserContext(profile),
+      profile, ExtensionManagementFactory::GetForBrowserContext(profile),
       ExternalPolicyLoader::RECOMMENDED);
 #endif
 

chrome/browser/extensions/forced_extensions/installation_reporter.h

@@ -31,6 +31,26 @@ class InstallationReporter {
     // ExtensionManagement::settings_by_id_.
     CREATED = 0,
 
+    // TODO(crbug.com/989526): stages from NOTIFIED_FROM_MANAGEMENT to
+    // SEEN_BY_EXTERNAL_PROVIDER are temporary ones for investigation. Remove
+    // then after investigation will complete and we'll be confident in
+    // extension handling between CREATED and PENDING.
+
+    // ExtensionManagement class is about to pass extension with
+    // INSTALLATION_FORCED mode to its observers.
+    NOTIFIED_FROM_MANAGEMENT = 5,
+
+    // ExtensionManagement class is about to pass extension with other mode to
+    // its observers.
+    NOTIFIED_FROM_MANAGEMENT_NOT_FORCED = 6,
+
+    // ExternalPolicyLoader with FORCED type fetches extension from
+    // ExtensionManagement.
+    SEEN_BY_POLICY_LOADER = 7,
+
+    // ExternalProviderImpl receives extension.
+    SEEN_BY_EXTERNAL_PROVIDER = 8,
+
     // Extension added to PendingExtensionManager.
     PENDING = 1,
 
@@ -45,7 +65,7 @@ class InstallationReporter {
 
     // Magic constant used by the histogram macros.
     // Always update it to the max value.
-    kMaxValue = COMPLETE,
+    kMaxValue = SEEN_BY_EXTERNAL_PROVIDER,
   };
 
   // Enum used for UMA. Do NOT reorder or remove entries. Don't forget to

tools/metrics/histograms/enums.xml

@@ -20391,6 +20391,10 @@ Called by update_net_error_codes.py.-->
   <int value="2" label="DOWNLOADING"/>
   <int value="3" label="INSTALLING"/>
   <int value="4" label="COMPLETE"/>
+  <int value="5" label="NOTIFIED_FROM_MANAGEMENT"/>
+  <int value="6" label="NOTIFIED_FROM_MANAGEMENT_NOT_FORCED"/>
+  <int value="7" label="SEEN_BY_POLICY_LOADER"/>
+  <int value="8" label="SEEN_BY_EXTERNAL_PROVIDER"/>
 </enum>
 
 <enum name="ExtensionInstallCause">