Increase fetch priority for policy-forced extensions
When fetch extensions from chrome web store, there is a header which
indicated pritority (or “interactivity”) of extension fetch. Usually it
is high (FOREGROUND) when user manually clicked “Add to Chrome” in the
store, and low (BACKGROUND) otherwise. Background fetches could be
throttled on a store server-side (eg. due to bandwidth limit), which is
perfectly OK for background updates, but is harmful for new installs
(which can occur not only because of user click, but also because of
extension forcelist policy). Therefore we have to set priority to
foreground for policy-based extension downloads.
Bug: 904600, 917700
Change-Id: I25ac15e71398f833261cd0cb1116785fcf7a241d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1569019
Commit-Queue: Oleg Davydov <[email protected]>
Reviewed-by: Sergey Poromov <[email protected]>
Reviewed-by: Devlin <[email protected]>
Cr-Commit-Position: refs/heads/master@{#651691}diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 306d95c..6d8ac09 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -92,6 +92,7 @@
#include "extensions/browser/update_observer.h"
#include "extensions/browser/updater/extension_cache.h"
#include "extensions/browser/updater/extension_downloader.h"
+#include "extensions/browser/updater/manifest_fetch_data.h"
#include "extensions/common/extension_messages.h"
#include "extensions/common/extension_urls.h"
#include "extensions/common/feature_switch.h"
@@ -1119,6 +1120,12 @@
: 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()) {
+ params.fetch_priority = ManifestFetchData::FOREGROUND;
+ }
updater()->CheckNow(std::move(params));
}
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 9f5130d13..6419a76 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -65,6 +65,7 @@
#include "chrome/browser/extensions/pending_extension_manager.h"
#include "chrome/browser/extensions/permissions_test_util.h"
#include "chrome/browser/extensions/permissions_updater.h"
+#include "chrome/browser/extensions/plugin_manager.h"
#include "chrome/browser/extensions/test_blacklist.h"
#include "chrome/browser/extensions/test_extension_system.h"
#include "chrome/browser/extensions/unpacked_installer.h"
@@ -97,7 +98,6 @@
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/storage_partition.h"
#include "content/public/common/content_constants.h"
-#include "chrome/browser/extensions/plugin_manager.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "content/public/test/test_utils.h"
#include "extensions/browser/disable_reason.h"
@@ -115,10 +115,13 @@
#include "extensions/browser/test_extension_registry_observer.h"
#include "extensions/browser/test_management_policy.h"
#include "extensions/browser/uninstall_reason.h"
+#include "extensions/browser/updater/extension_downloader_test_helper.h"
+#include "extensions/browser/updater/null_extension_cache.h"
#include "extensions/common/extension.h"
#include "extensions/common/extension_builder.h"
#include "extensions/common/extension_l10n_util.h"
#include "extensions/common/extension_resource.h"
+#include "extensions/common/extension_urls.h"
#include "extensions/common/file_util.h"
#include "extensions/common/manifest_constants.h"
#include "extensions/common/manifest_handlers/background_info.h"
@@ -7419,4 +7422,48 @@
service()->BlockAllExtensions();
}
+// 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) {
+ ExtensionUpdater::ScopedSkipScheduledCheckForTest skip_scheduled_checks;
+ ExtensionServiceInitParams params = CreateDefaultInitParams();
+ params.autoupdate_enabled = true;
+ InitializeExtensionService(params);
+
+ ExtensionDownloaderTestHelper helper;
+ NullExtensionCache extension_cache;
+ service()->updater()->SetExtensionDownloaderForTesting(
+ helper.CreateDownloader());
+ service()->updater()->SetExtensionCacheForTesting(&extension_cache);
+ service()->updater()->Start();
+
+ GURL update_url(extension_urls::kChromeWebstoreUpdateURL);
+ service()->OnExternalExtensionUpdateUrlFound(
+ ExternalInstallInfoUpdateUrl(
+ all_zero /* extension_id */, "" /* install_parameter */, update_url,
+ Manifest::EXTERNAL_POLICY_DOWNLOAD /* download_location */,
+ Extension::NO_FLAGS /* creation_flag */,
+ true /* mark_acknowledged */),
+ true /* is_initial_load */);
+
+ MockExternalProvider provider(nullptr, Manifest::EXTERNAL_POLICY_DOWNLOAD);
+ service()->OnExternalProviderReady(&provider);
+
+ content::RunAllTasksUntilIdle();
+
+ EXPECT_EQ(helper.test_url_loader_factory().NumPending(), 1);
+ network::TestURLLoaderFactory::PendingRequest* pending_request =
+ helper.test_url_loader_factory().GetPendingRequest(0);
+ std::string header;
+ EXPECT_TRUE(pending_request->request.headers.GetHeader(
+ "X-Goog-Update-Interactivity", &header));
+ EXPECT_EQ(header, "fg");
+
+ // Destroy updater's downloader as it uses |helper|.
+ service()->updater()->SetExtensionDownloaderForTesting(nullptr);
+}
+
} // namespace extensions
diff --git a/chrome/browser/extensions/pending_extension_manager.cc b/chrome/browser/extensions/pending_extension_manager.cc
index eba191f..3d35a7aa 100644
--- a/chrome/browser/extensions/pending_extension_manager.cc
+++ b/chrome/browser/extensions/pending_extension_manager.cc
@@ -95,6 +95,15 @@
return false;
}
+bool PendingExtensionManager::HasPendingExtensionFromPolicy() const {
+ return std::find_if(pending_extension_list_.begin(),
+ pending_extension_list_.end(),
+ [](const PendingExtensionInfo& info) {
+ return info.install_source() ==
+ Manifest::EXTERNAL_POLICY_DOWNLOAD;
+ }) != pending_extension_list_.end();
+}
+
void PendingExtensionManager::ExpectPolicyReinstallForCorruption(
const ExtensionId& id) {
if (base::ContainsKey(expected_policy_reinstalls_, id))
diff --git a/chrome/browser/extensions/pending_extension_manager.h b/chrome/browser/extensions/pending_extension_manager.h
index 3cfc583..cbadf7d 100644
--- a/chrome/browser/extensions/pending_extension_manager.h
+++ b/chrome/browser/extensions/pending_extension_manager.h
@@ -69,6 +69,9 @@
// Whether there is pending extension install from sync.
bool HasPendingExtensionFromSync() const;
+ // Whether there is pending extension install from policy.
+ bool HasPendingExtensionFromPolicy() const;
+
// Notifies the manager that we are reinstalling the policy force-installed
// extension with |id| because we detected corruption in the current copy.
void ExpectPolicyReinstallForCorruption(const ExtensionId& id);
diff --git a/chrome/browser/extensions/updater/extension_updater.cc b/chrome/browser/extensions/updater/extension_updater.cc
index 1bb1117..b13c16b 100644
--- a/chrome/browser/extensions/updater/extension_updater.cc
+++ b/chrome/browser/extensions/updater/extension_updater.cc
@@ -243,6 +243,11 @@
extension_cache_ = extension_cache;
}
+void ExtensionUpdater::SetExtensionDownloaderForTesting(
+ std::unique_ptr<ExtensionDownloader> downloader) {
+ downloader_.swap(downloader);
+}
+
void ExtensionUpdater::DoCheckSoon() {
DCHECK(will_check_soon_);
CheckNow(CheckParams());
diff --git a/chrome/browser/extensions/updater/extension_updater.h b/chrome/browser/extensions/updater/extension_updater.h
index 546f8b1..f2a7f294 100644
--- a/chrome/browser/extensions/updater/extension_updater.h
+++ b/chrome/browser/extensions/updater/extension_updater.h
@@ -138,6 +138,10 @@
// Overrides the extension cache with |extension_cache| for testing.
void SetExtensionCacheForTesting(ExtensionCache* extension_cache);
+ // Overrides the extension downloader with |downloader| for testing.
+ void SetExtensionDownloaderForTesting(
+ std::unique_ptr<ExtensionDownloader> downloader);
+
private:
friend class ExtensionUpdaterTest;
friend class ExtensionUpdaterFileHandler;
