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

Issue 724323 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug
Proj-Servicification

Blocked on:
issue 706286
issue 724322

Blocking:
issue 598073



Sign in to add a comment

Avoid using associated pointers for URLLoader with network service

Project Member Reported by jam@chromium.org, May 18 2017

Issue description

(summarizing discussion with Ananta and Yuzhu)

When adding support for AppCache:
https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc#newcode89
it turns out that even when not using appcache, if the appcache factory is the one that created the network service, it still has to stay alive as it's the one that implements mojom::URLLoader. It can not pass the URLLoaderRequest pointer to the next factory, as currently URLLoaderFactory is using associated interfaces.

 bug 724322  will fix this because we will stop using URLLoaderFactory objects in the browser and just use C++ to create the URLLoader.

However talking further, it seems the existing URLLoaderFactory interface is not a good fit for the network service. We don't need a separate sync xhr method, as Yuzhu discovered that for safe browsing we will have to implement sync xhr manually through a nested message loop and async calls to CreateLoaderAndStart (see https://docs.google.com/document/d/1AyW5oF3h6QaihAGWYASPn-XPrz2a1P9fv9T8EAuMqY0/edit for more background). The renderer also doesn't need to use associated interfaces, as there's no ordering guarantees with the network service relative to other browser IPCs.

Summary: after 724322 is done, we can create 
interface URLLoaderFactory2 {
  CreateLoaderAndStart(URLLoader& loader,
                       int32 routing_id,
                       int32 request_id,
                       uint32 options,
                       URLRequest request,
                       URLLoaderClient client);
}

that is used by the network service related code.
 

Comment 1 by kinuko@chromium.org, May 22 2017

Cc: yhirano@chromium.org

Comment 2 by jam@chromium.org, May 22 2017

(just to be clear, URLLoaderFactory2 is a placeholder name and isn't intended to be the real one)

Comment 3 by jam@chromium.org, May 24 2017

(to document our sync-up)

Yutaka mentioned that he would like to stop using associated interfaces. There are some test failures with it, so this would be after mojo loading ships.
Cc: tzik@chromium.org

Comment 5 by kinuko@chromium.org, May 24 2017

Regarding the latter part of the issue description:

Sounds like we can probably split the current URLLoaderFactory into two for async one and sync one?  Then we don't need to create a new URLLoaderFactory for async only cases.

Comment 6 by jam@chromium.org, May 24 2017

Blockedon: 706286
If the current URLLoaderFactory would be non-associated, then maybe an extra method that is ignored by network service is ok?

I'm going to mark this bug as duplicate of the other one for now

Comment 7 by jam@chromium.org, May 24 2017

s/duplicate/blocked on

Comment 8 by yzshen@chromium.org, May 24 2017

Components: Internals>Network>Service
Cc: -yhirano@chromium.org
Owner: yhirano@chromium.org
Status: Started (was: Unconfirmed)
Project Member

Comment 10 by bugdroid1@chromium.org, May 29 2017

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

commit f510d0c432ea0330c394e4a1349d6e7ee31c116e
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Mon May 29 15:30:31 2017

Use Independent URLLoader

This CL removes "associated" for URLLoader in
URLLoaderFactory::CreateLoaderAndStart.

The associated keyword was needed in order to keep the ordering guarantee
which the current ChromeIPC provides, but it is harmful from the network
servicification POV.

As all tests pass without the keyword (when kLoadingWithMojo is enabled),
let's stop using the associated interface.

Bug:  706286 ,  724323 
Change-Id: I771ee5ed35ca882cfa77c5e7ce2e862261c3f62d
Reviewed-on: https://chromium-review.googlesource.com/513643
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475356}
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/appcache/appcache_url_loader_factory.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/appcache/appcache_url_loader_factory.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/navigation_url_loader_network_service.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/resource_dispatcher_host_impl.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/resource_message_filter.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/resource_message_filter.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/resource_request_info_impl.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/resource_request_info_impl.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/url_loader_factory_impl.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/url_loader_factory_impl.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/loader/url_loader_factory_impl_unittest.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/browser/webui/web_ui_url_loader_factory.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/child/resource_dispatcher.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/child/resource_dispatcher.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/child/url_loader_client_impl_unittest.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/common/url_loader_factory.mojom
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/network/network_service_url_loader_factory_impl.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/network/network_service_url_loader_factory_impl.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/network/url_loader_impl.cc
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/network/url_loader_impl.h
[modify] https://crrev.com/f510d0c432ea0330c394e4a1349d6e7ee31c116e/content/network/url_loader_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 2 2017

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

commit c9ad960e2f22c27c7a984ec5bdb14c612737c5ad
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Fri Jun 02 10:41:23 2017

Revert "Use Independent URLLoader"

This reverts commit f510d0c432ea0330c394e4a1349d6e7ee31c116e.

Reason for revert: Performance regression: See  crbug.com/728467 

Original change's description:
> Use Independent URLLoader
> 
> This CL removes "associated" for URLLoader in
> URLLoaderFactory::CreateLoaderAndStart.
> 
> The associated keyword was needed in order to keep the ordering guarantee
> which the current ChromeIPC provides, but it is harmful from the network
> servicification POV.
> 
> As all tests pass without the keyword (when kLoadingWithMojo is enabled),
> let's stop using the associated interface.
> 
> Bug:  706286 ,  724323 
> Change-Id: I771ee5ed35ca882cfa77c5e7ce2e862261c3f62d
> Reviewed-on: https://chromium-review.googlesource.com/513643
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#475356}

TBR=dcheng@chromium.org,kinuko@chromium.org,jam@chromium.org,yhirano@chromium.org
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug:  706286 ,  724323 ,  728467 

Change-Id: If0a91fe994b2b784020b69499a5206a41596075c
Reviewed-on: https://chromium-review.googlesource.com/522184
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476614}
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/appcache/appcache_url_loader_factory.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/appcache/appcache_url_loader_factory.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/navigation_url_loader_network_service.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_dispatcher_host_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_message_filter.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_message_filter.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_request_info_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/resource_request_info_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/url_loader_factory_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/url_loader_factory_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/loader/url_loader_factory_impl_unittest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/browser/webui/web_ui_url_loader_factory.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/child/throttling_url_loader.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/child/throttling_url_loader_unittest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/child/url_loader_client_impl_unittest.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/common/url_loader_factory.mojom
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/network_service_url_loader_factory_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/network_service_url_loader_factory_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/url_loader_impl.cc
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/url_loader_impl.h
[modify] https://crrev.com/c9ad960e2f22c27c7a984ec5bdb14c612737c5ad/content/network/url_loader_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 25 2017

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

commit 4f87e237dabe3d4025c52a43885a469a7e872cd2
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Tue Jul 25 08:13:52 2017

Use Independent URLLoader

This is a reland of
https://chromium.googlesource.com/chromium/src/+/f510d0c432ea0330c394e4a1349d6e7ee31c116e.

This CL removes "associated" for URLLoader in
URLLoaderFactory::CreateLoaderAndStart.

The associated keyword was needed in order to keep the ordering guarantee
which the current ChromeIPC provides, but it is harmful from the network
servicification POV.

As all tests pass without the keyword (when kLoadingWithMojo is enabled),
let's stop using the associated interface.

Bug:  706286 ,  724323 
Change-Id: I2662ea948850e5b10b4278740beb1286fbeb6003
Reviewed-on: https://chromium-review.googlesource.com/578700
Reviewed-by: Eric Roman <eroman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489254}
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/chrome/browser/net/network_context_configuration_browsertest.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/appcache/appcache_subresource_url_factory.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/appcache/appcache_subresource_url_factory.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/appcache/appcache_url_loader_job.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/appcache/appcache_url_loader_job.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/blob_storage/blob_url_loader_factory.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/blob_storage/blob_url_loader_factory.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/blob_storage/blob_url_unittest.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/resource_dispatcher_host_impl.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/resource_message_filter.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/resource_message_filter.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/resource_request_info_impl.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/resource_request_info_impl.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/url_loader_factory_impl.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/url_loader_factory_impl.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/loader/url_loader_factory_impl_unittest.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/browser/webui/web_ui_url_loader_factory.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/child/url_loader_client_impl_unittest.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/common/throttling_url_loader.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/common/throttling_url_loader.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/common/throttling_url_loader_unittest.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/network/network_service_url_loader_factory_impl.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/network/network_service_url_loader_factory_impl.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/network/url_loader_impl.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/network/url_loader_impl.h
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/network/url_loader_unittest.cc
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/public/common/url_loader_factory.mojom
[modify] https://crrev.com/4f87e237dabe3d4025c52a43885a469a7e872cd2/content/renderer/renderer_blink_platform_impl.cc

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.

Sign in to add a comment