[Extensions] Increase download priority for external component extensions

External component extensions are loaded through the external provider
mechanism. These external extensions, by default, have a lower fetch
priority. For external policy extensions, we increase the priority. Do
the same for external component extensions, and add a test.

Bug: 965686

Change-Id: Ied58e08f403d574c4b6e8f7bdc484db90f926018
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1627783
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664844}

chrome/browser/extensions/extension_service.cc

@@ -1122,10 +1122,12 @@ void ExtensionService::OnAllExternalProvidersReady() {
             : base::BindOnce(
                   [](base::RepeatingClosure callback) { callback.Run(); },
                   external_updates_finished_callback_);
-    // We have to mark policy-forced extensions with foreground fetch priority,
-    // otherwise their installation may be throttled by bandwidth limits.
-    // See https://crbug.com/904600.
-    if (pending_extension_manager_.HasPendingExtensionFromPolicy()) {
+    // We have to mark high-priority extensions (such as policy-forced
+    // extensions or external component extensions) with foreground fetch
+    // priority; otherwise their installation may be throttled by bandwidth
+    // limits.
+    // See https://crbug.com/904600 and https://crbug.com/965686.
+    if (pending_extension_manager_.HasHighPriorityPendingExtension()) {
       params.fetch_priority = ManifestFetchData::FOREGROUND;
     }
     updater()->CheckNow(std::move(params));

chrome/browser/extensions/extension_service_unittest.cc

@@ -7420,12 +7420,16 @@ TEST_F(ExtensionServiceTest, PluginManagerCrash) {
   service()->BlockAllExtensions();
 }
 
+class ExternalExtensionPriorityTest
+    : public ExtensionServiceTest,
+      public testing::WithParamInterface<Manifest::Location> {};
+
 // Policy-forced extensions should be fetched with FOREGROUND priority,
 // otherwise they may be throttled (web store sends “noupdate” response to
 // reduce load), which is OK for updates, but not for a new install. This is
 // a regression test for problems described in https://crbug.com/904600 and
 // https://crbug.com/917700.
-TEST_F(ExtensionServiceTest, PolicyForegroundFetch) {
+TEST_P(ExternalExtensionPriorityTest, PolicyForegroundFetch) {
   ExtensionUpdater::ScopedSkipScheduledCheckForTest skip_scheduled_checks;
   ExtensionServiceInitParams params = CreateDefaultInitParams();
   params.autoupdate_enabled = true;
@@ -7440,9 +7444,9 @@ TEST_F(ExtensionServiceTest, PolicyForegroundFetch) {
 
   GURL update_url(extension_urls::kChromeWebstoreUpdateURL);
   service()->OnExternalExtensionUpdateUrlFound(
-      ExternalInstallInfoUpdateUrl(
-          all_zero /* extension_id */, "" /* install_parameter */, update_url,
-          Manifest::EXTERNAL_POLICY_DOWNLOAD /* download_location */,
+      ExternalInstallInfoUpdateUrl(all_zero /* extension_id */,
+                                   "" /* install_parameter */, update_url,
+                                   GetParam() /* download_location */,
                                    Extension::NO_FLAGS /* creation_flag */,
                                    true /* mark_acknowledged */),
       true /* is_initial_load */);
@@ -7458,10 +7462,19 @@ TEST_F(ExtensionServiceTest, PolicyForegroundFetch) {
   std::string header;
   EXPECT_TRUE(pending_request->request.headers.GetHeader(
       "X-Goog-Update-Interactivity", &header));
-  EXPECT_EQ(header, "fg");
+  bool is_high_priority = GetParam() == Manifest::EXTERNAL_POLICY_DOWNLOAD ||
+                          GetParam() == Manifest::EXTERNAL_COMPONENT;
+  const char* expected_header = is_high_priority ? "fg" : "bg";
+  EXPECT_EQ(expected_header, header);
 
   // Destroy updater's downloader as it uses |helper|.
   service()->updater()->SetExtensionDownloaderForTesting(nullptr);
 }
 
+INSTANTIATE_TEST_SUITE_P(,
+                         ExternalExtensionPriorityTest,
+                         testing::Values(Manifest::EXTERNAL_POLICY_DOWNLOAD,
+                                         Manifest::EXTERNAL_COMPONENT,
+                                         Manifest::EXTERNAL_PREF_DOWNLOAD));
+
 }  // namespace extensions

chrome/browser/extensions/pending_extension_manager.cc

@@ -95,12 +95,13 @@ bool PendingExtensionManager::HasPendingExtensionFromSync() const {
   return false;
 }
 
-bool PendingExtensionManager::HasPendingExtensionFromPolicy() const {
-  return std::find_if(pending_extension_list_.begin(),
-                      pending_extension_list_.end(),
+bool PendingExtensionManager::HasHighPriorityPendingExtension() const {
+  return std::find_if(
+             pending_extension_list_.begin(), pending_extension_list_.end(),
              [](const PendingExtensionInfo& info) {
                return info.install_source() ==
-                               Manifest::EXTERNAL_POLICY_DOWNLOAD;
+                          Manifest::EXTERNAL_POLICY_DOWNLOAD ||
+                      info.install_source() == Manifest::EXTERNAL_COMPONENT;
              }) != pending_extension_list_.end();
 }
 

chrome/browser/extensions/pending_extension_manager.h

@@ -69,8 +69,9 @@ class PendingExtensionManager {
   // Whether there is pending extension install from sync.
   bool HasPendingExtensionFromSync() const;
 
-  // Whether there is pending extension install from policy.
-  bool HasPendingExtensionFromPolicy() const;
+  // Whether there is a high-priority pending extension (one from either policy
+  // or an external component extension).
+  bool HasHighPriorityPendingExtension() const;
 
   // Notifies the manager that we are reinstalling the policy force-installed
   // extension with |id| because we detected corruption in the current copy.