[Extensions] Change metric emit logic for worker (un)registration.

This change combines a few mild metrics changes together to save on
metrics review churn. The significant changes are:

  * kErrorAbort returned when the browser context is shutting down is
    no longer considered an failure/error for (un)registration This is
    an expected situation.
  * WorkerRegistrationState now only emits *after* registration retries
    have been attempted to avoid multi-counting temporary timeout fails
  * kErrorScriptEvaluateFailed for worker registration is no longer
    considered a failure since it's a syntax error in the developer's
    script
  * !extension during worker registration and the registration not
    succeeding is no longer considered a failure (can happen when
    extension is disabled while registration request is in-flight).
    !extension and successful is considered a failure though since that
    could have implications for future worker registrations for the
    extension
  * !IsCurrentActivation() after worker registration is considered not
    successful since this is an unexpected state

Also makes a no-op change to WorkerRegistrationRetryAttemptsResult to
collapse a redundant if condition.

OBSOLETE_HISTOGRAMS=Reverted incremented histograms to original version since the intent and general logic of the metric is mostly the same as it was. We'll monitor by product version to see differences if needed.

Bug: 346732739
Change-Id: I310f12a648f7dd4d74e61d7d6ad0f6587698df98
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5855267
Auto-Submit: Justin Lulejian <jlulejian@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Commit-Queue: Justin Lulejian <jlulejian@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1354769}

chrome/browser/extensions/service_worker_registration_apitest.cc

@@ -70,17 +70,6 @@ void CheckBooleanHistogramCounts(const char* histogram_name,
                                      /*expected_count=*/false_count);
 }
 
-// Convenience method for checking counts of blink::ServiceWorkerStatusCode
-// emitting histograms.
-void CheckStatusCodeHistogramCounts(const char* histogram_name,
-                                    blink::ServiceWorkerStatusCode status,
-                                    int count,
-                                    base::HistogramTester& histogram_tester) {
-  histogram_tester.ExpectBucketCount(histogram_name,
-                                     /*sample=*/status,
-                                     /*expected_count=*/count);
-}
-
 GURL new_tab_url() {
   return GURL("chrome://newtab");
 }
@@ -104,16 +93,17 @@ class ExtensionRegistrationAndUnregistrationWaiter
   ExtensionRegistrationAndUnregistrationWaiter& operator=(
       const ExtensionRegistrationAndUnregistrationWaiter&) = delete;
 
+  void WaitForWorkerRegistrationAttemptCompleted() {
+    SCOPED_TRACE("Waiting for worker registration attempt to complete");
+    registration_attempt_runloop.Run();
+  }
+
   void WaitForWorkerRegistrationAndUnRegistrationAttemptCompleted() {
     WaitForWorkerRegistrationAttemptCompleted();
     WaitForWorkerUnregistrationAttemptCompleted();
   }
 
  private:
-  void WaitForWorkerRegistrationAttemptCompleted() {
-    SCOPED_TRACE("Waiting for worker registration attempt to complete");
-    registration_attempt_runloop.Run();
-  }
   void WaitForWorkerUnregistrationAttemptCompleted() {
     SCOPED_TRACE("Waiting for worker unregistration attempt to complete");
     unregistration_attempt_runloop.Run();
@@ -193,6 +183,56 @@ class ServiceWorkerRegistrationApiTest : public ExtensionApiTest {
 // and other tests in that file that should go here so that it's less
 // monolithic.
 
+// Tests that when an extension has invalid syntax in it's background worker
+// script the registration is not considered a failure due to it being user
+// error.
+IN_PROC_BROWSER_TEST_F(ServiceWorkerRegistrationApiTest,
+                       InvalidWorkerSyntaxFailsWorkerRegistration) {
+  const ExtensionId test_extension_id("iegclhlplifhodhkoafiokenjoapiobj");
+  static constexpr const char kKey[] =
+      "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAjzv7dI7Ygyh67VHE1DdidudpYf8P"
+      "Ffv8iucWvzO+3xpF/Dm5xNo7aQhPNiEaNfHwJQ7lsp4gc+C+4bbaVewBFspTruoSJhZc5uEf"
+      "qxwovJwN+v1/SUFXTXQmQBv6gs0qZB4gBbl4caNQBlqrFwAMNisnu1V6UROna8rOJQ90D7Nv"
+      "7TCwoVPKBfVshpFjdDOTeBg4iLctO3S/06QYqaTDrwVceSyHkVkvzBY6tc6mnYX0RZu78J9i"
+      "L8bdqwfllOhs69cqoHHgrLdI6JdOyiuh6pBP6vxMlzSKWJ3YTNjaQTPwfOYaLMuzdl0v+Ydz"
+      "afIzV9zwe4Xiskk+5JNGt8b2rQIDAQAB";
+  static constexpr char kManifest[] =
+      R"({
+           "name": "TestExtension",
+           "manifest_version": 3,
+           "version": "0.1",
+           "key": "%s",
+           "background": {"service_worker": "background.js"}
+         })";
+  TestExtensionDir extension_dir;
+  extension_dir.WriteManifest(base::StringPrintf(kManifest, kKey));
+  extension_dir.WriteFile(FILE_PATH_LITERAL("background.js"),
+                          "invalid_js_syntax;");
+
+  base::HistogramTester histogram_tester;
+  ExtensionRegistrationAndUnregistrationWaiter registration_waiter(
+      test_extension_id);
+
+  ChromeTestExtensionLoader(profile()).LoadUnpackedExtensionAsync(
+      extension_dir.UnpackedPath(), base::DoNothing());
+
+  {
+    SCOPED_TRACE("waiting for extension registration to finish");
+    registration_waiter.WaitForWorkerRegistrationAttemptCompleted();
+  }
+
+  CheckBooleanHistogramCounts(
+      "Extensions.ServiceWorkerBackground.WorkerRegistrationState",
+      /*true_count=*/1, /*false_count=*/0, histogram_tester);
+  histogram_tester.ExpectTotalCount(
+      "Extensions.ServiceWorkerBackground.Registration_FailStatus",
+      /*expected_count=*/0);
+  histogram_tester.ExpectBucketCount(
+      "Extensions.ServiceWorkerBackground.Registration_FailStatus",
+      /*sample=*/blink::ServiceWorkerStatusCode::kErrorScriptEvaluateFailed,
+      /*expected_count=*/0);
+}
+
 // Tests that a service worker registration is properly stored after extension
 // installation, both at the content layer and in the cached state in the
 // extensions layer.
@@ -329,33 +369,27 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerRegistrationApiTest,
   extension_registration_waiter
       .WaitForWorkerRegistrationAndUnRegistrationAttemptCompleted();
 
-  // Registration fails because we didn't allow the registration to complete
-  // before we disabled the extension. We can't check the status code because
-  // it's not consistent. I've seen
-  // blink::ServiceWorkerStatusCode::kErrorDisallowed and
-  // blink::ServiceWorkerStatusCode::kErrorNetwork in testing.
+  // Registration considered success because we didn't allow the registration to
+  // complete before we disabled the extension. We can't check the failure
+  // status code because it's not consistent. I've seen
+  // blink::ServiceWorkerStatusCode::kErrorDisallowed, kErrorNetwork, and
+  // kErrorNotFound in manual testing.
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerRegistrationState2",
-      /*true_count=*/0, /*false_count=*/1, histogram_tester);
-
-  // Unregistration fails with not found (expectedly) because the registration
-  // above did not complete.
-  // TODO(crbug.com/346732739): This status in this test example should not be
-  // considered a failure, but will first need to create logic to check for
-  // previous successful registration before we can fix that.
-  CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerRegistrationState",
+      /*true_count=*/1, /*false_count=*/0, histogram_tester);
+
+  // Unregistration "succeeds"  because the registration above did not complete.
+  CheckBooleanHistogramCounts(
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
-  // Unregistering a non-existent worker registration is not considered a
-  // failure.
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus3",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus",
       /*expected_count=*/0);
 
   // Confirm the activation token doesn't remain after disabling the
   // failed-to-register extension.
   activation_token = task_queue->GetCurrentActivationToken(test_extension_id);
-  ASSERT_FALSE(activation_token.has_value());
+  EXPECT_FALSE(activation_token.has_value());
 }
 
 // Tests updating an extension and installing it immediately while it has an
@@ -1068,23 +1102,23 @@ IN_PROC_BROWSER_TEST_F(
   // unregisters v1 of the worker), then we unregister again because v1 of the
   // extension is loaded.
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState",
       /*true_count=*/2, /*false_count=*/0, *v2_update_histogram_tester_);
   CheckBooleanHistogramCounts(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "DeactivateExtension2",
+      "DeactivateExtension",
       /*true_count=*/1, /*false_count=*/0, *v2_update_histogram_tester_);
   CheckBooleanHistogramCounts(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "AddExtension2",
+      "AddExtension",
       /*true_count=*/1, /*false_count=*/0, *v2_update_histogram_tester_);
 
   // Then v2 extension worker is registered.
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerRegistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerRegistrationState",
       /*true_count=*/1, /*false_count=*/0, *v2_update_histogram_tester_);
   v2_update_histogram_tester_->ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.Registration_FailStatus2",
+      "Extensions.ServiceWorkerBackground.Registration_FailStatus",
       /*expected_count=*/0);
 }
 
@@ -1141,13 +1175,13 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerRegistrationInstallMetricBrowserTest,
   ASSERT_NO_FATAL_FAILURE(InstallMv2OrMv3Extension());
 
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerRegistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerRegistrationState",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState",
       /*expected_count=*/0);
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.Registration_FailStatus2",
+      "Extensions.ServiceWorkerBackground.Registration_FailStatus",
       /*expected_count=*/0);
 }
 
@@ -1209,28 +1243,28 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerRegistrationInstallMetricBrowserTest,
 
   // Expected unregistration metrics for disable.
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   CheckBooleanHistogramCounts(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "DeactivateExtension2",
+      "DeactivateExtension",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus3",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus",
       /*expected_count=*/0);
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-      "DeactivateExtension3",
+      "DeactivateExtension",
       /*expected_count=*/0);
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-      "AddExtension3",
+      "AddExtension",
       /*expected_count=*/0);
 
   // We didn't update the extension.
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "AddExtension2",
+      "AddExtension",
       /*expected_count=*/0);
 }
 
@@ -1274,34 +1308,34 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerRegistrationRestartMetricBrowserTest,
   // Expected unregistration and registration metrics for disable and then
   // enable for restart.
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   CheckBooleanHistogramCounts(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "DeactivateExtension2",
+      "DeactivateExtension",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus3",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus",
       /*expected_count=*/0);
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-      "DeactivateExtension3",
+      "DeactivateExtension",
       /*expected_count=*/0);
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-      "AddExtension3",
+      "AddExtension",
       /*expected_count=*/0);
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerRegistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerRegistrationState",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
 
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.Registration_FailStatus2",
+      "Extensions.ServiceWorkerBackground.Registration_FailStatus",
       /*expected_count=*/0);
   // We didn't update the extension.
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "AddExtension2",
+      "AddExtension",
       /*expected_count=*/0);
 }
 
@@ -1427,38 +1461,28 @@ IN_PROC_BROWSER_TEST_P(MV2BackgroundsToMV3WorkerRegistrationMetricBrowserTest,
   // When updating from an MV2 worker we try to unregister the previous worker
   // version first.
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2",
-      /*true_count=*/0, /*false_count=*/1, histogram_tester);
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState",
+      /*true_count=*/1, /*false_count=*/0, histogram_tester);
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "DeactivateExtension2",
+      "DeactivateExtension",
       /*expected_count=*/0);
   // We unsuccessfully try to unregister it again to handle workers that are
   // registered via the web API. This is an expected failure.
   CheckBooleanHistogramCounts(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "AddExtension2",
-      /*true_count=*/0, /*false_count=*/1, histogram_tester);
-  CheckStatusCodeHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus3",
-      /*status=*/blink::ServiceWorkerStatusCode::kErrorNotFound, /*count=*/1,
-      histogram_tester);
-  CheckStatusCodeHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-      "AddExtension3",
-      /*status=*/blink::ServiceWorkerStatusCode::kErrorNotFound, /*count=*/1,
-      histogram_tester);
+      "AddExtension",
+      /*true_count=*/1, /*false_count=*/0, histogram_tester);
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-      "DeactivateExtension3",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus4",
       /*expected_count=*/0);
 
   // Then the new worker registration is registered.
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerRegistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerRegistrationState",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.Registration_FailStatus2",
+      "Extensions.ServiceWorkerBackground.Registration_FailStatus",
       /*expected_count=*/0);
 }
 
@@ -1561,36 +1585,36 @@ IN_PROC_BROWSER_TEST_P(WorkerBackgroundToWorkerBackgroundRegistrationMetricTest,
   // redundantly again (but curiously it succeeds) before adding the new version
   // of the extension.
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState",
       /*true_count=*/2, /*false_count=*/0, histogram_tester);
   // And it's unregistered due to the MV2 service worker being deactivated.
   CheckBooleanHistogramCounts(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "DeactivateExtension2",
+      "DeactivateExtension",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   // We redundantly attempt to unregister it again to handle workers that are
   // registered via the web API.
   CheckBooleanHistogramCounts(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "AddExtension2",
+      "AddExtension",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus3",
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus",
       /*expected_count=*/0);
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-      "DeactivateExtension3",
+      "DeactivateExtension",
       /*expected_count=*/0);
   histogram_tester.ExpectTotalCount(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-      "AddExtension3",
+      "AddExtension",
       /*expected_count=*/0);
 
   CheckBooleanHistogramCounts(
-      "Extensions.ServiceWorkerBackground.WorkerRegistrationState2",
+      "Extensions.ServiceWorkerBackground.WorkerRegistrationState",
       /*true_count=*/1, /*false_count=*/0, histogram_tester);
   histogram_tester.ExpectTotalCount(
-      "Extensions.ServiceWorkerBackground.Registration_FailStatus2",
+      "Extensions.ServiceWorkerBackground.Registration_FailStatus",
       /*expected_count=*/0);
 }
 

extensions/browser/extension_registrar.cc

@@ -45,16 +45,6 @@ BASE_FEATURE(kExtensionUpdatesImmediatelyUnregisterWorker,
              "ExtensionUpdatesImmediatelyUnregisterWorker",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
-// TODO(crbug.com/346732739): Combine with
-// ServiceWorkerTaskQueue::IsWorkerUnregistrationSuccess() when the logic is the
-// same.
-// Worker unregistrations can fail in expected and unexpected ways, this
-// determines if the unregistration can be accepted as successful from the
-// extension's perspective.
-bool IsWorkerUnregistrationSuccess(blink::ServiceWorkerStatusCode status) {
-  return status == blink::ServiceWorkerStatusCode::kOk;
-}
-
 }  // namespace
 
 ExtensionRegistrar::ExtensionRegistrar(content::BrowserContext* browser_context,
@@ -544,6 +534,9 @@ void ExtensionRegistrar::UnregisterServiceWorkerWithRootScope(
   content::ServiceWorkerContext* context =
       util::GetServiceWorkerContextForExtensionId(new_extension->id(),
                                                   browser_context_);
+  bool worker_previously_registered =
+      ServiceWorkerTaskQueue::Get(browser_context_)
+          ->IsWorkerRegistered(new_extension->id());
   // Even though the unregistration process for a service worker is
   // asynchronous, we begin the process before the new extension is added, so
   // the old worker will be unregistered before the new one is registered.
@@ -553,25 +546,30 @@ void ExtensionRegistrar::UnregisterServiceWorkerWithRootScope(
         new_extension->url(),
         blink::StorageKey::CreateFirstParty(new_extension->origin()),
         base::BindOnce(&ExtensionRegistrar::NotifyServiceWorkerUnregistered,
-                       weak_factory_.GetWeakPtr(), new_extension->id()));
+                       weak_factory_.GetWeakPtr(), new_extension->id(),
+                       worker_previously_registered));
   } else {
     context->UnregisterServiceWorker(
         new_extension->url(),
         blink::StorageKey::CreateFirstParty(new_extension->origin()),
         base::BindOnce(&ExtensionRegistrar::NotifyServiceWorkerUnregistered,
-                       weak_factory_.GetWeakPtr(), new_extension->id()));
+                       weak_factory_.GetWeakPtr(), new_extension->id(),
+                       worker_previously_registered));
   }
 }
 
 void ExtensionRegistrar::NotifyServiceWorkerUnregistered(
     const ExtensionId& extension_id,
+    bool worker_previously_registered,
     blink::ServiceWorkerStatusCode status) {
-  bool success = IsWorkerUnregistrationSuccess(status);
+  bool success =
+      ServiceWorkerTaskQueue::Get(browser_context_)
+          ->IsWorkerUnregistrationSuccess(status, worker_previously_registered);
   base::UmaHistogramBoolean(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2", success);
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState", success);
   base::UmaHistogramBoolean(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "AddExtension2",
+      "AddExtension",
       success);
 
   if (!success) {
@@ -579,11 +577,11 @@ void ExtensionRegistrar::NotifyServiceWorkerUnregistered(
     LOG(ERROR) << "Failed to unregister service worker for extension "
                << extension_id;
     base::UmaHistogramEnumeration(
-        "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus3",
+        "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus",
         status);
     base::UmaHistogramEnumeration(
         "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-        "AddExtension3",
+        "AddExtension",
         status);
   }
 }

extensions/browser/extension_registrar.h

@@ -171,6 +171,7 @@ class ExtensionRegistrar : public ProcessManagerObserver {
   // root scope.
   void UnregisterServiceWorkerWithRootScope(const Extension* extension);
   void NotifyServiceWorkerUnregistered(const ExtensionId& extension_id,
+                                       bool worker_previously_registered,
                                        blink::ServiceWorkerStatusCode status);
 
   // Given an extension that was disabled for reloading, completes the reload

extensions/browser/service_worker/service_worker_task_queue.cc

@@ -67,32 +67,6 @@ ServiceWorkerTaskQueue::TestObserver* g_test_observer = nullptr;
 // Prevent check on multiple workers per extension for testing purposes.
 bool g_allow_multiple_workers_per_extension = false;
 
-// Worker unregistrations can fail in expected and unexpected ways, this
-// determines if the unregistration can be accepted as successful from the
-// extension's perspective.
-bool IsWorkerUnregistrationSuccess(blink::ServiceWorkerStatusCode status,
-                                   bool worker_previously_registered) {
-  if (status == blink::ServiceWorkerStatusCode::kOk) {
-    return true;
-  }
-
-  if (status != blink::ServiceWorkerStatusCode::kErrorNotFound) {
-    return false;
-  }
-
-  // If worker was not successfully registered before then a not found error
-  // is expected, but can occur because unregistration requests do not check
-  // current registration status.
-  return !worker_previously_registered;
-}
-
-// Worker registrations can fail in expected and unexpected ways, this
-// determines if the registration can be accepted as successful from the
-// extension's perspective.
-bool IsWorkerRegistrationSuccess(blink::ServiceWorkerStatusCode status) {
-  return status == blink::ServiceWorkerStatusCode::kOk;
-}
-
 }  // namespace
 
 ServiceWorkerTaskQueue::ServiceWorkerTaskQueue(BrowserContext* browser_context)
@@ -690,12 +664,23 @@ void ServiceWorkerTaskQueue::DidRegisterServiceWorker(
   const Extension* extension =
       registry->enabled_extensions().GetByID(extension_id);
   if (!extension) {
+    // No extension and failed registration can expectedly happen if an
+    // extension is deactivated when worker activation/registration request is
+    // in-flight. But if registration was successful then that could interfere
+    // with future worker registrations for the extension.
+    base::UmaHistogramBoolean(
+        "Extensions.ServiceWorkerBackground.WorkerRegistrationState", !success);
     if (g_test_observer) {
       g_test_observer->OnWorkerRegistered(context_id.extension_id);
     }
     return;
   }
   if (!IsCurrentActivation(extension_id, context_id.token)) {
+    // TODO(crbug.com/346732739): This shouldn't be happening since we seem to
+    // always remove extension from enabled extension before we delete the
+    // extension activation token, but lets confirm that.
+    base::UmaHistogramBoolean(
+        "Extensions.ServiceWorkerBackground.WorkerRegistrationState", false);
     if (g_test_observer) {
       g_test_observer->OnWorkerRegistered(context_id.extension_id);
     }
@@ -713,7 +698,7 @@ void ServiceWorkerTaskQueue::DidRegisterServiceWorker(
       // TODO(crbug.com/346732739): Create a test for this if it is feasible.
       base::UmaHistogramEnumeration(
           "Extensions.ServiceWorkerBackground.RegistrationMismatchMitigated_"
-          "FailStatus2",
+          "FailStatus",
           status_code);
     }
     if (g_test_observer) {
@@ -732,29 +717,30 @@ void ServiceWorkerTaskQueue::DidRegisterServiceWorker(
     return;
   }
 
-  // We aren't retrying anymore so record success and emit metrics.
+  // We aren't retrying anymore so emit metrics specifically about the retries.
   if (reason == RegistrationReason::RE_REGISTER_ON_TIMEOUT) {
-    if (success) {
-      base::UmaHistogramBoolean(
-          "Extensions.ServiceWorkerBackground."
-          "WorkerRegistrationRetryAttemptsResult",
-          true);
-    } else {
-      // We've exhausted all retry attempts or hit a status code on retry other
-      // than blink::ServiceWorkerStatusCode::kErrorTimeout that we will not
-      // retry.
-      base::UmaHistogramBoolean(
-          "Extensions.ServiceWorkerBackground."
-          "WorkerRegistrationRetryAttemptsResult",
-          false);
-    }
+    base::UmaHistogramBoolean(
+        "Extensions.ServiceWorkerBackground."
+        "WorkerRegistrationRetryAttemptsResult",
+        success);
     worker_reregistration_attempts_.erase(context_id.token);
   }
 
+  // After retries are exhausted, emit the ultimate end result.
+  base::UmaHistogramBoolean(
+      "Extensions.ServiceWorkerBackground.WorkerRegistrationState", success);
+
   if (!success) {
     base::UmaHistogramEnumeration(
-        "Extensions.ServiceWorkerBackground.Registration_FailStatus2",
+        "Extensions.ServiceWorkerBackground.Registration_FailStatus",
         status_code);
+  }
+
+  if (!success ||
+      // Still show script evaluate error to developer so that it can be fixed,
+      // despite it not being considered an internal failure.
+      status_code ==
+          blink::ServiceWorkerStatusCode::kErrorScriptEvaluateFailed) {
     std::string msg = base::StringPrintf(
         "Service worker registration failed. Status code: %d",
         static_cast<int>(status_code));
@@ -803,10 +789,10 @@ void ServiceWorkerTaskQueue::DidUnregisterServiceWorker(
       IsWorkerUnregistrationSuccess(status, worker_previously_registered);
 
   base::UmaHistogramBoolean(
-      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState2", success);
+      "Extensions.ServiceWorkerBackground.WorkerUnregistrationState", success);
   base::UmaHistogramBoolean(
       "Extensions.ServiceWorkerBackground.WorkerUnregistrationState_"
-      "DeactivateExtension2",
+      "DeactivateExtension",
       success);
 
   // TODO(crbug.com/346732739): Handle this better than just logging an error
@@ -815,11 +801,11 @@ void ServiceWorkerTaskQueue::DidUnregisterServiceWorker(
     LOG(ERROR) << "Failed to unregister service worker for extension id: "
                << extension_id << " error status was: " << (int)status;
     base::UmaHistogramEnumeration(
-        "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus3",
+        "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus",
         status);
     base::UmaHistogramEnumeration(
         "Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus_"
-        "DeactivateExtension3",
+        "DeactivateExtension",
         status);
   }
 
@@ -828,6 +814,22 @@ void ServiceWorkerTaskQueue::DidUnregisterServiceWorker(
   }
 }
 
+bool ServiceWorkerTaskQueue::IsWorkerRegistrationSuccess(
+    blink::ServiceWorkerStatusCode status) {
+  switch (status) {
+    case blink::ServiceWorkerStatusCode::kOk:
+      return true;
+    case blink::ServiceWorkerStatusCode::kErrorAbort:
+      return browser_context_shutting_down_;
+    case blink::ServiceWorkerStatusCode::kErrorScriptEvaluateFailed:
+      // Developer script syntax errors are considered user errors.
+      return true;
+    default:
+      // All other registration failures are unexpected.
+      return false;
+  }
+}
+
 base::Version ServiceWorkerTaskQueue::RetrieveRegisteredServiceWorkerVersion(
     const ExtensionId& extension_id) {
   if (browser_context_->IsOffTheRecord()) {
@@ -1015,6 +1017,38 @@ void ServiceWorkerTaskQueue::OnStopped(
   worker_state->worker_id_.reset();
 }
 
+bool ServiceWorkerTaskQueue::IsWorkerUnregistrationSuccess(
+    blink::ServiceWorkerStatusCode status,
+    bool worker_previously_registered) {
+  switch (status) {
+    case blink::ServiceWorkerStatusCode::kOk:
+      return true;
+    case blink::ServiceWorkerStatusCode::kErrorNotFound:
+      return !worker_previously_registered;
+    case blink::ServiceWorkerStatusCode::kErrorAbort:
+      return browser_context_shutting_down_;
+    default:
+      // All other unregistration failures are unexpected.
+      return false;
+  }
+}
+
+bool ServiceWorkerTaskQueue::IsWorkerRegistered(
+    const ExtensionId extension_id) {
+  // TODO(crbug.com/346732739): Key worker_registered_ by extension_id so that
+  // this check isn't necessary anymore.
+  std::optional<base::UnguessableToken> activation_token =
+      GetCurrentActivationToken(extension_id);
+  if (!activation_token) {
+    // This implies that a request to register the worker hasn't been sent yet,
+    // or a worker unregistration has, at least, been sent.
+    return false;
+  }
+  const SequencedContextId context_id = {extension_id, browser_context_,
+                                         *activation_token};
+  return base::Contains(worker_registered_, context_id);
+}
+
 size_t ServiceWorkerTaskQueue::GetNumPendingTasksForTest(
     const LazyContextId& lazy_context_id) {
   auto activation_token =

extensions/browser/service_worker/service_worker_task_queue.h

@@ -291,6 +291,19 @@ class ServiceWorkerTaskQueue
   void OnStopped(int64_t version_id,
                  const content::ServiceWorkerRunningInfo& worker_info) override;
 
+  // Worker unregistrations can fail in expected and unexpected ways, this
+  // determines if the unregistration can be accepted as successful from the
+  // extension's perspective. When there was a record of worker registration
+  // prior to unregistering, `worker_previously_registered` should be set to
+  // true. Used in metrics.
+  bool IsWorkerUnregistrationSuccess(blink::ServiceWorkerStatusCode status_code,
+                                     bool worker_previously_registered);
+  // Whether this class is aware of a worker being registered. Note: This does
+  // not verify that the registration exists in the service worker layer, so it
+  // may not be 100% accurate (if there are bugs in registration tracking logic
+  // in this class). Used in metrics.
+  bool IsWorkerRegistered(const ExtensionId extension_id);
+
   class TestObserver {
    public:
     TestObserver();
@@ -394,6 +407,11 @@ class ServiceWorkerTaskQueue
       bool worker_previously_registered,
       blink::ServiceWorkerStatusCode status);
 
+  // Worker registrations can fail in expected and unexpected ways, this
+  // determines if the registration can be accepted as successful from the
+  // extension's perspective.
+  bool IsWorkerRegistrationSuccess(blink::ServiceWorkerStatusCode status);
+
   void DidStartWorkerForScope(const SequencedContextId& context_id,
                               base::Time start_time,
                               int64_t version_id,

tools/metrics/histograms/metadata/extensions/histograms.xml

@@ -4561,17 +4561,15 @@ This is emitted for two scenarios when a worker may be unregistered:
   </summary>
 </histogram>
 
-<histogram name="Extensions.ServiceWorkerBackground.Registration_FailStatus2"
+<histogram name="Extensions.ServiceWorkerBackground.Registration_FailStatus"
     enum="ServiceWorkerStatusCode" expires_after="2025-02-15">
   <owner>jlulejian@chromium.org</owner>
   <owner>extensions-core@chromium.org</owner>
   <summary>
     Tracks when a background service worker fails to register what is the
     ServiceWorkerStatusCode that occurred. Logged each time a service worker
-    fails to register.
-
-    Note: incremented Aug, 2024 due to the implementation of worker registration
-    retries which could change the number of emits of this metric.
+    fails to register. Some failures are expected and are not counted by this
+    metric.
   </summary>
 </histogram>
 
@@ -4606,7 +4604,7 @@ This is emitted for two scenarios when a worker may be unregistered:
 </histogram>
 
 <histogram
-    name="Extensions.ServiceWorkerBackground.RegistrationMismatchMitigated_FailStatus2"
+    name="Extensions.ServiceWorkerBackground.RegistrationMismatchMitigated_FailStatus"
     enum="ServiceWorkerStatusCode" expires_after="2025-01-15">
   <owner>jlulejian@chromium.org</owner>
   <owner>extensions-core@chromium.org</owner>
@@ -4618,9 +4616,6 @@ This is emitted for two scenarios when a worker may be unregistered:
 
     Emitted when
     Extensions.ServiceWorkerBackground.RegistrationMismatchMitigated is false.
-
-    Note: incremented Aug, 2024 due to the implementation of worker registration
-    retries which could increase the number of emits of this metric.
   </summary>
 </histogram>
 
@@ -4772,16 +4767,18 @@ This is emitted for two scenarios when a worker may be unregistered:
   </summary>
 </histogram>
 
-<histogram name="Extensions.ServiceWorkerBackground.WorkerRegistrationState2"
+<histogram name="Extensions.ServiceWorkerBackground.WorkerRegistrationState"
     enum="Boolean" expires_after="2025-02-23">
   <owner>jlulejian@chromium.org</owner>
   <owner>extensions-core@chromium.org</owner>
   <summary>
-    Tracks whether background service worker registration succeeded or not. It
-    is emitted whenever an extension is enabled (including updates).
+    Tracks whether background service worker registration ultimately succeeded
+    or unexpectedly not. It can be emitted whenever an extension is enabled
+    (including updates).
 
-    Note: incremented Aug, 2024 due to the implementation of worker registration
-    retries which will likely increase the number of emits of this metric.
+    If there are attempts to retry registration, those are not emitted. See
+    Extensions.ServiceWorkerBackground.WorkerRegistrationRetryAttemptsResult for
+    a metric on retry attempts.
   </summary>
 </histogram>
 
@@ -4800,15 +4797,15 @@ This is emitted for two scenarios when a worker may be unregistered:
 </histogram>
 
 <histogram
-    name="Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus{UnregistrationType}3"
+    name="Extensions.ServiceWorkerBackground.WorkerUnregistrationFailureStatus{UnregistrationType}"
     enum="ServiceWorkerStatusCode" expires_after="2025-03-25">
   <owner>jlulejian@chromium.org</owner>
   <owner>extensions-core@chromium.org</owner>
   <summary>
     The ServiceWorkerStatusCode when unregistering a worker for an extension
-    fails.
+    fails unexpectedly.
 
-    Emitted when Extensions.ServiceWorkerBackground.WorkerRegistrationState is
+    Emitted when Extensions.ServiceWorkerBackground.WorkerUnregistrationState is
     false.
 
     It is emitted when this failure happens in two scenarios when a worker may
@@ -4824,23 +4821,18 @@ This is emitted for two scenarios when a worker may be unregistered:
     #2 can occur independently outside of an extension update.
 
     {UnregistrationType}
-
-    Note: This metric was incremented Aug, 2024 due to the #2 case no longer
-    emitting the not found error if there was no previous worker registered. The
-    logic for #2 does not check if a worker was previously registered before
-    attempting to unregister it so the error is not considered a failure.
   </summary>
   <token key="UnregistrationType" variants="UnregistrationType"/>
 </histogram>
 
 <histogram
-    name="Extensions.ServiceWorkerBackground.WorkerUnregistrationState{UnregistrationType}2"
+    name="Extensions.ServiceWorkerBackground.WorkerUnregistrationState{UnregistrationType}"
     enum="Boolean" expires_after="2025-01-15">
   <owner>jlulejian@chromium.org</owner>
   <owner>extensions-core@chromium.org</owner>
   <summary>
-    The success (true) or failure (false) of attempting to unregister a worker
-    for an extension.
+    The success (true) or unexpected failure (false) of attempting to unregister
+    a worker for an extension.
 
     This is emitted for two scenarios when a worker may be unregistered:
 
@@ -4854,12 +4846,6 @@ This is emitted for two scenarios when a worker may be unregistered:
     #2 can occur independently outside of an extension update.
 
     {UnregistrationType}
-
-    Note: This metric was incremented Aug, 2024 due to the #2 case emitting true
-    if the error status is not found and there was no previous worker registered
-    (instead of the previous false). The logic for #2 does not check if a worker
-    was previously registered before attempting to unregister it so the error is
-    considered an unregistration success.
   </summary>
   <token key="UnregistrationType" variants="UnregistrationType"/>
 </histogram>