0

Reland "[Extensions] Regression test: WebRequestAPI ResetURLLoaderFactories bug"

Test needs to be skipped when `kForceWebRequestProxyForTest` is enabled
(for example, in `network_service_linux` trybot).

That's because factories are never reset when this feature is enabled.

See
https://crsrc.org/c/extensions/browser/api/web_request/web_request_api.cc;drc=e1dda381f52e3a553444a434e89139e016675701;l=680

This is a reland of commit e1dda381f5

Original change's description:
> [Extensions] Regression test: WebRequestAPI ResetURLLoaderFactories bug
>
> This change adds a regression test for the referenced bug.
>
> It tests both that enabling the `DeferResetURLLoaderFactories` feature
> flag resolves the issue, and that disabling it causes the issue.
>
> Design doc: http://go/defer-reset-url-loader-factories
>
> Bug: 394523691
> Change-Id: I46671553e4c0631416b76654c476cb6e29dd65ae
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6424484
> Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
> Commit-Queue: Andrea Orru <andreaorru@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1443868}

Bug: 394523691
Change-Id: I21b26c9a60a470be52148a6c11de474b38a4b496
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6438674
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Commit-Queue: Andrea Orru <andreaorru@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1444520}
This commit is contained in:
Andrea Orru
2025-04-08 20:20:19 -07:00
committed by Chromium LUCI CQ
parent 6afbda49d3
commit 61f048644f
9 changed files with 296 additions and 9 deletions

@ -37,6 +37,7 @@
#include "chrome/browser/browser_process.h"
#include "chrome/browser/devtools/protocol/devtools_protocol_test_support.h"
#include "chrome/browser/devtools/url_constants.h"
#include "chrome/browser/extensions/chrome_test_extension_loader.h"
#include "chrome/browser/extensions/error_console/error_console.h"
#include "chrome/browser/extensions/error_console/error_console_test_observer.h"
#include "chrome/browser/extensions/extension_apitest.h"
@ -128,6 +129,7 @@
#include "services/network/test/test_url_loader_client.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/common/input/web_input_event.h"
#include "third_party/blink/public/common/service_worker/service_worker_status_code.h"
#include "ui/webui/untrusted_web_ui_browsertest_util.h" // nogncheck
#include "url/origin.h"
@ -2609,7 +2611,7 @@ IN_PROC_BROWSER_TEST_P(ExtensionWebRequestApiTestWithContextType,
content::EvalJs(web_contents, "document.body.textContent.trim();"));
}
// A callback allow waiting for responses to complete with an expected status
// A callback allow waiting for a response to complete with an expected status
// and given content.
auto make_browser_request =
[](network::mojom::URLLoaderFactory* url_loader_factory, const GURL& url,
@ -7564,5 +7566,195 @@ IN_PROC_BROWSER_TEST_F(ManifestV3WebRequestApiTest, RecordUkmOnNavigation) {
}
}
}
// Allows test to wait for the failure of a worker registration.
class WorkerRegistrationFailureObserver
: public ServiceWorkerTaskQueue::TestObserver {
public:
explicit WorkerRegistrationFailureObserver(const ExtensionId extension_id)
: extension_id_(extension_id) {
ServiceWorkerTaskQueue::SetObserverForTest(this);
}
~WorkerRegistrationFailureObserver() override {
ServiceWorkerTaskQueue::SetObserverForTest(nullptr);
}
blink::ServiceWorkerStatusCode WaitForWorkerRegistrationFailure() {
if (!status_code_) {
SCOPED_TRACE("Waiting for worker registration to fail");
failure_loop_.Run();
}
return *status_code_;
}
private:
void OnWorkerRegistrationFailed(
const ExtensionId& extension_id,
blink::ServiceWorkerStatusCode status_code) override {
if (extension_id == extension_id_) {
status_code_ = status_code;
failure_loop_.Quit();
}
}
ExtensionId extension_id_;
base::RunLoop failure_loop_;
std::optional<blink::ServiceWorkerStatusCode> status_code_;
};
// Allows test to wait for the call of `ResetURLLoaderFactories()` in
// WebRequestAPI.
class URLLoaderFactoriesResetWaiter : public WebRequestAPI::TestObserver {
public:
URLLoaderFactoriesResetWaiter() { WebRequestAPI::SetObserverForTest(this); }
~URLLoaderFactoriesResetWaiter() override {
WebRequestAPI::SetObserverForTest(nullptr);
}
URLLoaderFactoriesResetWaiter(const URLLoaderFactoriesResetWaiter&) = delete;
URLLoaderFactoriesResetWaiter& operator=(
const URLLoaderFactoriesResetWaiter&) = delete;
void WaitForResetURLLoaderFactoriesCalled() {
SCOPED_TRACE("Waiting for ResetURLLoaderFactories to be called");
url_loader_factory_reset_runloop_.Run();
}
private:
void OnDidResetURLLoaderFactories() override {
url_loader_factory_reset_runloop_.Quit();
}
base::RunLoop url_loader_factory_reset_runloop_;
};
class ManifestV3WebRequestApiTestWithDeferResetURLLoaderFactories
: public ManifestV3WebRequestApiTest,
public testing::WithParamInterface<bool> {
public:
ManifestV3WebRequestApiTestWithDeferResetURLLoaderFactories() {
feature_list_.InitWithFeatureState(
extensions_features::kDeferResetURLLoaderFactories, GetParam());
}
~ManifestV3WebRequestApiTestWithDeferResetURLLoaderFactories() override =
default;
private:
base::test::ScopedFeatureList feature_list_;
};
// Tests that the call to `ResetURLLoaderFactories()` performed by WebRequestAPI
// doesn't break the registration process of other extensions.
// Regression test for https://crbug.com/394523691.
IN_PROC_BROWSER_TEST_P(
ManifestV3WebRequestApiTestWithDeferResetURLLoaderFactories,
ResetURLLoaderFactoryDoesntBreakRegistration) {
// Skip if the proxy is forced since factories will not be reset in that case.
if (base::FeatureList::IsEnabled(
extensions_features::kForceWebRequestProxyForTest)) {
return;
}
bool feature_enabled = GetParam();
ASSERT_TRUE(StartEmbeddedTestServer());
// A simple extension that sends a message and waits for a response in its
// background script.
const ExtensionId 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"}
})";
static constexpr char kBackgroundJs[] =
R"(chrome.test.sendMessage('will_receive').then(() => {
console.log('received');
}))";
TestExtensionDir extension_dir;
extension_dir.WriteManifest(base::StringPrintf(kManifest, kKey));
extension_dir.WriteFile(FILE_PATH_LITERAL("background.js"), kBackgroundJs);
ServiceWorkerTaskQueue* task_queue = ServiceWorkerTaskQueue::Get(profile());
ASSERT_TRUE(task_queue);
WebRequestAPI* web_request_api =
BrowserContextKeyedAPIFactory<WebRequestAPI>::Get(profile());
ASSERT_TRUE(web_request_api);
// Listen to "will_receive" message from the extension.
ExtensionTestMessageListener will_receive_listener("will_receive",
ReplyBehavior::kWillReply);
// Listen to the completion of the registration storage.
service_worker_test_utils::TestServiceWorkerContextObserver
registration_observer(profile());
// Listen for a failure in the worker registration.
WorkerRegistrationFailureObserver worker_failure_observer(extension_id);
// Asynchronously load the extension so we can wait for a step in the loading
// process in the test.
std::optional<base::UnguessableToken> activation_token;
ChromeTestExtensionLoader(profile()).LoadUnpackedExtensionAsync(
extension_dir.UnpackedPath(),
base::BindLambdaForTesting([&](const Extension* extension) {
ASSERT_TRUE(extension);
activation_token =
task_queue->GetCurrentActivationToken(extension->id());
ASSERT_TRUE(activation_token.has_value());
}));
// ...and wait for the moment right after the worker is requested to start
// during the registration process.
registration_observer.WaitForStartWorkerMessageSent();
URLLoaderFactoriesResetWaiter url_loader_factories_reset_waiter;
// Simulate the effect of loading an extension with WebRequestAPI permissions.
// In other words, make sure we're proxying for the current profile.
// This will cause WebRequestAPI to attempt calling
// `ResetURLLoaderFactories()`. Because the extension is still in the early
// phases of starting its worker here, this would break its registration
// before it has a chance of being completed and stored.
// Instead, the call to `ResetURLLoaderFactories()` will be deferred.
// NOTE: We simulate the call to `ResetURLLoaderFactories()` rather
// than loading an extension with WebRequestAPI permissions, as that
// would take too long and won't trigger the bug in all cases.
web_request_api->ForceProxyForTesting();
if (feature_enabled) {
// DeferResetURLLoaderFactories feature enabled: expect successful
// execution. Check that the worker is still running and functional.
registration_observer.WaitForWorkerStarted();
std::optional<WorkerId> worker_id = GetWorkerIdForExtension(extension_id);
EXPECT_TRUE(worker_id);
SCOPED_TRACE(
"Waiting for extension background to signal that it can send messages");
ASSERT_TRUE(will_receive_listener.WaitUntilSatisfied());
will_receive_listener.Reply("go");
// Check `ResetURLLoaderFactories()` is called after registration is stored.
registration_observer.WaitForRegistrationStored();
url_loader_factories_reset_waiter.WaitForResetURLLoaderFactoriesCalled();
} else {
// DeferResetURLLoaderFactories feature disabled: expect worker registration
// to fail. We have observed that the registration can fail with either
// `kErrorStartWorkerFailed` or `kErrorNetwork` depending on when exactly
// it's interrupted.
auto status_code =
worker_failure_observer.WaitForWorkerRegistrationFailure();
EXPECT_NE(status_code, blink::ServiceWorkerStatusCode::kOk);
}
}
INSTANTIATE_TEST_SUITE_P(
All,
ManifestV3WebRequestApiTestWithDeferResetURLLoaderFactories,
testing::Bool());
#endif // !BUILDFLAG(IS_ANDROID)
} // namespace extensions

@ -93,9 +93,7 @@ namespace web_request = api::web_request;
namespace {
BASE_FEATURE(kDeferResetURLLoaderFactories,
"DeferResetURLLoaderFactories",
base::FEATURE_ENABLED_BY_DEFAULT);
WebRequestAPI::TestObserver* g_test_observer = nullptr;
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
@ -344,7 +342,8 @@ WebRequestAPI::WebRequestAPI(content::BrowserContext* context)
// We only have to observe ServiceWorkerTaskQueue and react to
// `OnAllRegistrationsStored` if we need to defer calls to
// `ResetURLLoaderFactories` to after registration storage.
if (base::FeatureList::IsEnabled(kDeferResetURLLoaderFactories)) {
if (base::FeatureList::IsEnabled(
extensions_features::kDeferResetURLLoaderFactories)) {
service_worker_task_queue_observation_.Observe(
ServiceWorkerTaskQueue::Get(browser_context_));
}
@ -373,6 +372,15 @@ WebRequestAPI::GetFactoryInstance() {
return g_factory.Pointer();
}
// static
void WebRequestAPI::SetObserverForTest(TestObserver* observer) {
g_test_observer = observer;
}
WebRequestAPI::TestObserver::TestObserver() = default;
WebRequestAPI::TestObserver::~TestObserver() = default;
void WebRequestAPI::OnListenerRemoved(const EventListenerInfo& details) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
@ -715,10 +723,18 @@ bool WebRequestAPI::HasExtraHeadersListenerForTesting() {
->HasAnyExtraHeadersListener(browser_context_);
}
void WebRequestAPI::ResetURLLoaderFactories() {
browser_context_->GetDefaultStoragePartition()->ResetURLLoaderFactories();
if (g_test_observer) {
g_test_observer->OnDidResetURLLoaderFactories();
}
}
void WebRequestAPI::UpdateMayHaveProxies() {
bool may_have_proxies = MayHaveProxies();
if (!may_have_proxies_ && may_have_proxies) {
if (base::FeatureList::IsEnabled(kDeferResetURLLoaderFactories) &&
if (base::FeatureList::IsEnabled(
extensions_features::kDeferResetURLLoaderFactories) &&
has_pending_worker_registrations_) {
// If any service worker registration is still in flight (started, but not
// stored), it's not safe to call `ResetURLLoaderFactories`, since it
@ -731,7 +747,7 @@ void WebRequestAPI::UpdateMayHaveProxies() {
// can be reset safely.
} else {
// Otherwise, we can safely call it now.
browser_context_->GetDefaultStoragePartition()->ResetURLLoaderFactories();
ResetURLLoaderFactories();
}
}
may_have_proxies_ = may_have_proxies;
@ -761,7 +777,7 @@ void WebRequestAPI::OnAllRegistrationsStored() {
service_worker_context_observation_.RemoveAllObservations();
// ...and reset the URLLoaderFactories if we haven't done it already.
if (deferred_reset_url_loader_factories_) {
browser_context_->GetDefaultStoragePartition()->ResetURLLoaderFactories();
ResetURLLoaderFactories();
deferred_reset_url_loader_factories_ = false;
}
}

@ -199,6 +199,19 @@ class WebRequestAPI : public BrowserContextKeyedAPI,
static BrowserContextKeyedAPIFactory<WebRequestAPI>* GetFactoryInstance();
void Shutdown() override;
class TestObserver {
public:
TestObserver();
TestObserver(const TestObserver&) = delete;
TestObserver& operator=(const TestObserver&) = delete;
virtual ~TestObserver();
// Called when `ResetURLLoaderFactories()` has been performed.
virtual void OnDidResetURLLoaderFactories() {}
};
static void SetObserverForTest(TestObserver* observer);
// EventRouter::Observer overrides:
void OnListenerRemoved(const EventListenerInfo& details) override;
@ -308,6 +321,8 @@ class WebRequestAPI : public BrowserContextKeyedAPI,
// URLLoaderFactories if so.
void UpdateMayHaveProxies();
void ResetURLLoaderFactories();
// ExtensionRegistryObserver implementation.
void OnExtensionLoaded(content::BrowserContext* browser_context,
const Extension* extension) override;

@ -732,6 +732,10 @@ void ServiceWorkerTaskQueue::DidRegisterServiceWorker(
const bool success = IsWorkerRegistrationSuccess(status_code);
base::UmaHistogramBoolean(
"Extensions.ServiceWorkerBackground.WorkerRegistrationState2", success);
if (!success && g_test_observer) {
g_test_observer->OnWorkerRegistrationFailed(context_id.extension_id,
status_code);
}
ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context_);
const ExtensionId& extension_id = context_id.extension_id;

@ -348,6 +348,10 @@ class ServiceWorkerTaskQueue
// it ultimately succeeds or fails).
virtual void RequestedWorkerStart(const ExtensionId& extension_id) {}
virtual void OnWorkerRegistrationFailed(
const ExtensionId& extension_id,
blink::ServiceWorkerStatusCode status_code) {}
virtual void DidStartWorkerFail(
const ExtensionId& extension_id,
size_t num_pending_tasks,

@ -12,6 +12,7 @@
#include "content/public/browser/storage_partition.h"
#include "extensions/common/constants.h"
#include "extensions/common/extension.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/mojom/service_worker/service_worker_database.mojom-forward.h"
namespace extensions {
@ -45,6 +46,7 @@ TestServiceWorkerContextObserver::TestServiceWorkerContextObserver(
: extension_scope_(GetScopeForExtensionID(std::move(extension_id))),
context_(context) {
scoped_observation_.Observe(context_);
scoped_sync_observation_.Observe(context_);
}
TestServiceWorkerContextObserver::TestServiceWorkerContextObserver(
@ -53,6 +55,7 @@ TestServiceWorkerContextObserver::TestServiceWorkerContextObserver(
: extension_scope_(GetScopeForExtensionID(std::move(extension_id))),
context_(GetServiceWorkerContext(browser_context)) {
scoped_observation_.Observe(context_);
scoped_sync_observation_.Observe(context_);
}
TestServiceWorkerContextObserver::~TestServiceWorkerContextObserver() = default;
@ -62,13 +65,26 @@ void TestServiceWorkerContextObserver::WaitForRegistrationStored() {
return;
}
SCOPED_TRACE("Waiting for worker registration to be stored");
base::RunLoop run_loop;
stored_quit_closure_ = run_loop.QuitClosure();
run_loop.Run();
}
int64_t TestServiceWorkerContextObserver::WaitForStartWorkerMessageSent() {
if (!start_message_sent_version_id_) {
SCOPED_TRACE("Waiting for StartWorker message to be sent");
base::RunLoop run_loop;
start_message_sent_quit_closure_ = run_loop.QuitClosure();
run_loop.Run();
}
return *start_message_sent_version_id_;
}
int64_t TestServiceWorkerContextObserver::WaitForWorkerStarted() {
if (!running_version_id_) {
SCOPED_TRACE("Waiting for worker to be started");
base::RunLoop run_loop;
started_quit_closure_ = run_loop.QuitClosure();
run_loop.Run();
@ -84,6 +100,7 @@ int64_t TestServiceWorkerContextObserver::WaitForWorkerStopped() {
return *stopped_version_id_;
}
SCOPED_TRACE("Waiting for worker to be stopped");
base::RunLoop run_loop;
stopped_quit_closure_ = run_loop.QuitClosure();
run_loop.Run();
@ -93,6 +110,7 @@ int64_t TestServiceWorkerContextObserver::WaitForWorkerStopped() {
int64_t TestServiceWorkerContextObserver::WaitForWorkerActivated() {
if (!activated_version_id_) {
SCOPED_TRACE("Waiting for worker to be activated");
base::RunLoop run_loop;
activated_quit_closure_ = run_loop.QuitClosure();
run_loop.Run();
@ -123,6 +141,19 @@ void TestServiceWorkerContextObserver::OnRegistrationStored(
}
}
void TestServiceWorkerContextObserver::OnStartWorkerMessageSent(
int64_t version_id,
const GURL& scope) {
if (extension_scope_ && extension_scope_ != scope) {
return;
}
start_message_sent_version_id_ = version_id;
if (start_message_sent_quit_closure_) {
std::move(start_message_sent_quit_closure_).Run();
}
}
void TestServiceWorkerContextObserver::OnVersionStartedRunning(
int64_t version_id,
const content::ServiceWorkerRunningInfo& running_info) {
@ -161,6 +192,7 @@ void TestServiceWorkerContextObserver::OnVersionActivated(int64_t version_id,
void TestServiceWorkerContextObserver::OnDestruct(
content::ServiceWorkerContext* context) {
scoped_observation_.Reset();
scoped_sync_observation_.Reset();
context_ = nullptr;
}

@ -39,7 +39,8 @@ content::ServiceWorkerContext* GetServiceWorkerContext(
// Note: This class only works well when there is a *single* service worker
// being registered. We could extend this to track multiple workers.
class TestServiceWorkerContextObserver
: public content::ServiceWorkerContextObserver {
: public content::ServiceWorkerContextObserver,
public content::ServiceWorkerContextObserverSynchronous {
public:
explicit TestServiceWorkerContextObserver(
content::ServiceWorkerContext* context,
@ -58,6 +59,11 @@ class TestServiceWorkerContextObserver
// scope to be stored.
void WaitForRegistrationStored();
// Wait for OnStartWorkerMessageSent event is triggered, so that the observer
// captures the version ID of the service worker that is about to be started.
// Returns the version ID.
int64_t WaitForStartWorkerMessageSent();
// Wait for OnVersionStartedRunning event is triggered, so that the observer
// captures the running service worker version ID. Returns the version ID.
int64_t WaitForWorkerStarted();
@ -89,6 +95,9 @@ class TestServiceWorkerContextObserver
void OnVersionActivated(int64_t version_id, const GURL& scope) override;
void OnDestruct(content::ServiceWorkerContext* context) override;
// ServiceWorkerContextObserverSynchronous:
void OnStartWorkerMessageSent(int64_t version_id, const GURL& scope) override;
using RegistrationsMap = std::map<GURL, int>;
RegistrationsMap registrations_completed_map_;
@ -96,6 +105,7 @@ class TestServiceWorkerContextObserver
// Multiple events may come in so we must wait for the specific event
// to be triggered.
base::OnceClosure activated_quit_closure_;
base::OnceClosure start_message_sent_quit_closure_;
base::OnceClosure started_quit_closure_;
base::OnceClosure stored_quit_closure_;
base::OnceClosure stopped_quit_closure_;
@ -104,6 +114,7 @@ class TestServiceWorkerContextObserver
std::optional<bool> registration_stored_;
std::optional<int64_t> activated_version_id_;
std::optional<int64_t> start_message_sent_version_id_;
std::optional<int64_t> running_version_id_;
std::optional<int64_t> stopped_version_id_;
@ -112,6 +123,10 @@ class TestServiceWorkerContextObserver
base::ScopedObservation<content::ServiceWorkerContext,
content::ServiceWorkerContextObserver>
scoped_observation_{this};
base::ScopedObservation<content::ServiceWorkerContext,
content::ServiceWorkerContextObserverSynchronous>
scoped_sync_observation_{this};
};
// Observes ProcessManager::UnregisterServiceWorker.

@ -64,6 +64,10 @@ BASE_FEATURE(kCheckingNoExtensionIdInExtensionIpcs,
"EMF_NO_EXTENSION_ID_FOR_EXTENSION_SOURCE",
base::FEATURE_ENABLED_BY_DEFAULT);
BASE_FEATURE(kDeferResetURLLoaderFactories,
"DeferResetURLLoaderFactories",
base::FEATURE_ENABLED_BY_DEFAULT);
BASE_FEATURE(kEnableWebHidInWebView,
"EnableWebHidInWebView",
base::FEATURE_ENABLED_BY_DEFAULT);

@ -85,6 +85,11 @@ BASE_DECLARE_FEATURE(kAllowWithholdingExtensionPermissionsOnInstall);
// extension).
BASE_DECLARE_FEATURE(kCheckingNoExtensionIdInExtensionIpcs);
// If enabled, defers the execution of WebRequestAPI call of
// `ResetURLLoaderFactories()` to when there's no extension service worker
// registrations in flight, to avoid disrupting the worker(s) registration(s).
BASE_DECLARE_FEATURE(kDeferResetURLLoaderFactories);
// If enabled, <webview>s will be allowed to request permission from an
// embedding Chrome App to request access to Human Interface Devices.
BASE_DECLARE_FEATURE(kEnableWebHidInWebView);