New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 857577 link

Starred by 4 users
Status: Fixed
Owner:
cduvall@chromium.org
Closed: Jul 20
Cc:
karandeepb@chromium.org
cmumford@chromium.org
█ roc...@chromium.org
█ jcivelli@chromium.org
dxie@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 784576



Sign in to add a comment

Extensions cannot block requests if a request was issued before any extension was installed (with network service only)

Project Member Reported by jcivelli@chromium.org, Jun 28 2018 
This came up as part of changing GaiaAuthFetcher to use SimpleURLLoader.
The test ExtensionWebRequestApiTest.                       WebRequestURLLoaderInterception failed as one of the request made by the browser is not blocked when it should.
This is because when StoragePartitionImpl::GetURLLoaderFactoryForBrowserProcessInternal() the call to WillCreateURLLoaderFactory() does install the WebRequestProxyingURLLoaderFactory (as no extension is set yet). That same SharedURLLoaderFactory is then cached and used bypassing the WebRequestProxyingURLLoaderFactory.

Comment 1 by jam@chromium.org, Jun 28 2018

Blocking: 784576
Cc: roc...@chromium.org jcivelli@chromium.org
Labels: -Pri-3 Proj-Servicification-Canary Proj-Servicification Pri-1
Owner: ----
Per discussion with Jay & Ken, this is also causing the failure of
BackgroundXhrTest.HttpAuth
BackgroundXhrTest.TlsClientAuth

It seems like anything calling StoragPartition::GetURLLoaderFactoryForBrowserProcess or renderers that are holding on to subresource factories won't work correctly if extensions that use webRequest are installed/uninstalled.

Some ideas from the brainstorming:
-perhaps a way to notify places like SP that they need to recreate their factories. however this would also need to be rebroadcast to the clones
-kill the network process and rely on the recovery code to recrease all factories

Comment 2 by jcivelli@chromium.org, Jun 28 2018 

Here is a CL that changes ExtensionWebRequestApiTest.WebRequestURLLoaderInterception to repro:

https://chromium-review.googlesource.com/c/chromium/src/+/1119095/1

Comment 3 by jam@chromium.org, Jun 29 2018 

Also here's more background from Ken: https://bugs.chromium.org/p/chromium/issues/detail?id=784576#c16

another option to solve this: we can tell the network service to destroy all URLLoaderFactory bindings. It would keep all URLLoaderFactories that have URLLoader's in-progress, then delete the URLFs when they have no more URLs. That way the browser & renderer will all update their factories to ones that are intercepted, while not interfering with in-progress network requests.

Comment 4 by jam@chromium.org, Jul 12

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows

Comment 5 by dxie@chromium.org, Jul 12

Components: Internals>Services>Network
Status: Available (was: Untriaged)

Comment 6 by jam@chromium.org, Jul 12

Cc: cmumford@chromium.org karandeepb@chromium.org dxie@chromium.org
 Issue 853733  has been merged into this issue.

Comment 7 by cduvall@chromium.org, Jul 16

Owner: cduvall@chromium.org
Status: Started (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 19 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a76c901a6f20185485697749b84af33a13ba7877

commit a76c901a6f20185485697749b84af33a13ba7877
Author: Clark DuVall <cduvall@chromium.org>
Date: Thu Jul 19 03:30:33 2018

Add FlushAsyncForTesting to InterfacePtr API

This will be used in
URLLoaderFactoryGetter::FlushNetworkInterfaceForTesting to prevent
nested IO message loops.

Hit this in
https://chromium-review.googlesource.com/c/chromium/src/+/1139048.

Bug:  857577 
Change-Id: I1b6a53f39737ff0a6d4681b4256991569b91a2d5
Reviewed-on: https://chromium-review.googlesource.com/1142198
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576350}
[modify] https://crrev.com/a76c901a6f20185485697749b84af33a13ba7877/mojo/public/cpp/bindings/interface_endpoint_client.h
[modify] https://crrev.com/a76c901a6f20185485697749b84af33a13ba7877/mojo/public/cpp/bindings/interface_ptr.h
[modify] https://crrev.com/a76c901a6f20185485697749b84af33a13ba7877/mojo/public/cpp/bindings/lib/control_message_proxy.cc
[modify] https://crrev.com/a76c901a6f20185485697749b84af33a13ba7877/mojo/public/cpp/bindings/lib/control_message_proxy.h
[modify] https://crrev.com/a76c901a6f20185485697749b84af33a13ba7877/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
[modify] https://crrev.com/a76c901a6f20185485697749b84af33a13ba7877/mojo/public/cpp/bindings/lib/interface_ptr_state.h
[modify] https://crrev.com/a76c901a6f20185485697749b84af33a13ba7877/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/574add021ddcc86efd800c490310dce12bfaddec

commit 574add021ddcc86efd800c490310dce12bfaddec
Author: Clark DuVall <cduvall@chromium.org>
Date: Fri Jul 20 23:37:01 2018

Reset URLLoaderFactory bindings for web request proxy

Previously, if a request was made before any web request listeners or
rules were added, the URLLoaderFactory would not be proxied through the
browser process to run the WebRequest code. Now, if it is detected that
WebRequest listeners/rules are added, we reset the bindings so they will
be recreated and proxied through the browser process.

Observers were added for a few classes so WebRequestAPI can listen to
rule changes.

Changes in URLLoaderFactoryGetter were needed due to crbug.com/613371.

Bug:  857577 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I7805be86512545b496e30b9693374981fdc2633e
Reviewed-on: https://chromium-review.googlesource.com/1139048
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577036}
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/chrome/browser/extensions/api/web_request/web_request_apitest.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/chrome/browser/extensions/background_xhr_browsertest.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/chrome/test/data/extensions/background_xhr/background.js
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/chrome/test/data/extensions/background_xhr/test_http_auth.js
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/chrome/test/data/extensions/background_xhr/test_tls_client_auth.js
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/content/browser/storage_partition_impl.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/content/browser/url_loader_factory_getter.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/content/browser/url_loader_factory_getter.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/declarative/rules_cache_delegate.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/declarative/rules_cache_delegate.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/declarative/rules_registry_service.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/declarative/rules_registry_service.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/declarative_net_request/rules_monitor_service.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/declarative_net_request/rules_monitor_service.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/web_request/web_request_api.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/services/network/cors/cors_url_loader_factory.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/services/network/cors/cors_url_loader_factory.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/services/network/network_context.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/services/network/network_context.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/services/network/test/test_network_context.h
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/services/network/test/test_url_loader_factory_unittest.cc
[modify] https://crrev.com/574add021ddcc86efd800c490310dce12bfaddec/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Comment 10 by cduvall@chromium.org, Jul 20

Status: Fixed (was: Started)
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 23 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/353108ea6461c59370af55fb019f361c0cdd7e63

commit 353108ea6461c59370af55fb019f361c0cdd7e63
Author: Timothy Loh <timloh@chromium.org>
Date: Mon Jul 23 12:14:56 2018

Revert "Reset URLLoaderFactory bindings for web request proxy"

This reverts commit 574add021ddcc86efd800c490310dce12bfaddec.

Reason for revert: network_service_browser_tests / DevToolsFrontendInWebRequestApiTest.HiddenRequests times out most of the time

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=network_service_browser_tests&tests=DevToolsFrontendInWebRequestApiTest.HiddenRequests

Original change's description:
> Reset URLLoaderFactory bindings for web request proxy
> 
> Previously, if a request was made before any web request listeners or
> rules were added, the URLLoaderFactory would not be proxied through the
> browser process to run the WebRequest code. Now, if it is detected that
> WebRequest listeners/rules are added, we reset the bindings so they will
> be recreated and proxied through the browser process.
> 
> Observers were added for a few classes so WebRequestAPI can listen to
> rule changes.
> 
> Changes in URLLoaderFactoryGetter were needed due to crbug.com/613371.
> 
> Bug:  857577 
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: I7805be86512545b496e30b9693374981fdc2633e
> Reviewed-on: https://chromium-review.googlesource.com/1139048
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577036}

TBR=kinuko@chromium.org,rockot@chromium.org,cduvall@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  857577 
Change-Id: I1c70daaa63107ac1527755b3d94e0fe61cb49ae8
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1146541
Reviewed-by: Timothy Loh <timloh@chromium.org>
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577153}
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/chrome/browser/extensions/api/web_request/web_request_apitest.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/chrome/browser/extensions/background_xhr_browsertest.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/chrome/test/data/extensions/background_xhr/background.js
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/chrome/test/data/extensions/background_xhr/test_http_auth.js
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/chrome/test/data/extensions/background_xhr/test_tls_client_auth.js
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/content/browser/storage_partition_impl.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/content/browser/url_loader_factory_getter.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/content/browser/url_loader_factory_getter.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/declarative/rules_cache_delegate.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/declarative/rules_cache_delegate.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/declarative/rules_registry_service.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/declarative/rules_registry_service.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/declarative_net_request/rules_monitor_service.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/declarative_net_request/rules_monitor_service.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/web_request/web_request_api.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/services/network/cors/cors_url_loader_factory.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/services/network/cors/cors_url_loader_factory.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/services/network/network_context.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/services/network/network_context.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/services/network/test/test_network_context.h
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/services/network/test/test_url_loader_factory_unittest.cc
[modify] https://crrev.com/353108ea6461c59370af55fb019f361c0cdd7e63/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 23 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16be25475132a83e3311ef17c417c40b90781494

commit 16be25475132a83e3311ef17c417c40b90781494
Author: Clark DuVall <cduvall@chromium.org>
Date: Mon Jul 23 22:42:42 2018

Reland "Reset URLLoaderFactory bindings for web request proxy"

This is a reland of 574add021ddcc86efd800c490310dce12bfaddec

Added DevToolsFrontendInWebRequestApiTest.HiddenRequests to the filter
file, will fix it in a follow up.

Original change's description:
> Reset URLLoaderFactory bindings for web request proxy
>
> Previously, if a request was made before any web request listeners or
> rules were added, the URLLoaderFactory would not be proxied through the
> browser process to run the WebRequest code. Now, if it is detected that
> WebRequest listeners/rules are added, we reset the bindings so they will
> be recreated and proxied through the browser process.
>
> Observers were added for a few classes so WebRequestAPI can listen to
> rule changes.
>
> Changes in URLLoaderFactoryGetter were needed due to crbug.com/613371.
>
> Bug:  857577 
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: I7805be86512545b496e30b9693374981fdc2633e
> Reviewed-on: https://chromium-review.googlesource.com/1139048
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577036}

TBR=rockot@chromium.org,kinuko@chromium.org

Bug:  857577 
Change-Id: I83f266cfcd572ccbde36405d7cff501f92122b2d
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1147042
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577310}
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/chrome/browser/extensions/api/web_request/web_request_apitest.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/chrome/browser/extensions/background_xhr_browsertest.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/chrome/test/data/extensions/background_xhr/background.js
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/chrome/test/data/extensions/background_xhr/test_http_auth.js
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/chrome/test/data/extensions/background_xhr/test_tls_client_auth.js
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/content/browser/storage_partition_impl.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/content/browser/url_loader_factory_getter.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/content/browser/url_loader_factory_getter.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/declarative/rules_cache_delegate.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/declarative/rules_cache_delegate.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/declarative/rules_registry_service.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/declarative/rules_registry_service.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/declarative_net_request/rules_monitor_service.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/declarative_net_request/rules_monitor_service.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/web_request/web_request_api.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/services/network/cors/cors_url_loader_factory.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/services/network/cors/cors_url_loader_factory.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/services/network/network_context.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/services/network/network_context.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/services/network/test/test_network_context.h
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/services/network/test/test_url_loader_factory_unittest.cc
[modify] https://crrev.com/16be25475132a83e3311ef17c417c40b90781494/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
►

Sign in to add a comment