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

Issue 791049 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

URLLoader::ProceedWithResponse has to be removed.

Project Member Reported by arthurso...@chromium.org, Dec 1 2017

Issue description

The following patch will introduce URLLoader::ProceedWithResponse()
https://chromium-review.googlesource.com/c/chromium/src/+/753384/6
This is temporary and must be removed in the future.

It is used when the NavigationMojoResponse feature is enabled.

The goal is to be able to pause/resume a loading on the IO thread while the NavigationThrottle::WillProcessResponse() checks are done on the IU thread.

There are a few reason for this. One of them is the way the ResourceHandler works.
The MojoAsyncResourceHandler is behind the InterceptingResourceHandler. If the loading is not paused by the MojoAsyncResourceHandler, the loading continue and the InterceptingResourceHandler could receive OnResponseComplete() while checks are done on the UI thread. In this case the InterceptingResourceHandler can intercept and continue a deferred or blocked navigation.

This will not be needed in the future and must be removed.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 21 2017

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

commit 2e1524a72495925cf6720e92260321c1121cd073
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu Dec 21 15:47:07 2017

Pause/Resume navigation in WillProcessResponse.

Ensure that the request is properly paused in the net stack while the
navigation code is performing the WillProcessResponse checks, and only
restart it when Resume is called.

This is part 3 of the implementation plan.
Design doc: https://goo.gl/Rrrc7n

Bug:  705744 , 791049
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I4166aa3eb50b934238bf352378df257c2026da6a
Reviewed-on: https://chromium-review.googlesource.com/753384
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525697}
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/appcache/appcache_subresource_url_factory.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/appcache/appcache_url_loader_job.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/appcache/appcache_url_loader_job.h
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/blob_storage/blob_url_loader_factory.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/file_url_loader_factory.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/service_worker/service_worker_script_url_loader.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/service_worker/service_worker_script_url_loader.h
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/service_worker/service_worker_url_loader_job.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/browser/service_worker/service_worker_url_loader_job.h
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/common/throttling_url_loader_unittest.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/network/url_loader.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/network/url_loader.h
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/public/common/simple_url_loader_unittest.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/public/common/url_loader.mojom
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/renderer/loader/cors_url_loader.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/renderer/loader/cors_url_loader.h
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/2e1524a72495925cf6720e92260321c1121cd073/content/renderer/service_worker/service_worker_subresource_loader.h

Comment 2 by cco3@chromium.org, Jan 23 2018

In the old path, eventually ChromeResourceDispatcherHostDelegate::RequestComplete would be called.
https://cs.chromium.org/chromium/src/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc?l=873

This would trigger (when appropriate) page_load_metrics::MetricsWebContentsObserver::OnRequestComplete

However, when ProceedWithResponse is bypassed altogether, the page load metrics aren't recorded, and this causes some of the tests to be broken for the NetworkService:
https://cs.chromium.org/chromium/src/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter?l=98
(See PageLoadMetricsBrowserTest)

arthursonzogni@, do you have a suggestion for how to get the PageLoadMetrics working properly?

Comment 3 by cco3@chromium.org, Jan 23 2018

Cc: cco3@chromium.org
Summary: URLLoader::ProceedWithResponse has to be removed. (was: NetworkService: URLLoader::ProceedWithResponse has to be removed.)
comment 2:
URLLoader::ProceedWithResponse() will be used with the feature "NavigationMojoResponse" (disabled by default).
So it is currently not used (it has never been used in the past) and it will not be used with the NetworkService either.
BTW, your tests are also working fine with "NavigationMojoResponse" enabled.

This bug was about URLLoader::ProceedWithResponse(), but I think you are talking about NavigationURLLoader::ProceedWithResponse(), right?

With the NetworkService, AFAIU the ResourceDispatcherHost(Delegate)/Resource{Loader,Handler} are no more used.
I took a look, page_load_metrics::MetricsWebContentsObserver::OnRequestComplete is is called by a ResourceDispatcherHostDelegate.
So that's why it is no more called.

I guess the ones implementing the NetworkService need to find a way to replace everything that was done in a ResourceDispatcherHostDelegate.
I don't have any "good" suggestions to give.

Maybe a passthrough NavigationThrottle can be added at the end and observe NavigationThrottle::WillProcessResponse?

Sign in to add a comment