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

Issue 826082 link

Starred by 1 user
Status: Fixed
Owner:
█ jcivelli@chromium.org
Last visit > 30 days ago
Closed: Apr 2018
Cc:
mfo...@chromium.org
gov...@chromium.org
clamy@chromium.org
creis@chromium.org
lukasza@chromium.org
█ cco3@chromium.org
alex...@chromium.org
arthurso...@chromium.org
█ nasko@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

WebContentsObserver::ResourceLoadComplete() does not report the IP address for frames

Project Member Reported by jcivelli@chromium.org, Mar 27 2018 
The ResourceLoadInfo provided to the WebContentsObserver::ResourceLoadComplete() notification does not include the |ip| field (it's null).
This is because ResourceDispatcher::ContinueForNavigation passes a default network::ResourceResponseHead() to URLLoaderClientImpl::OnReceiveResponse.
The frame request being triggered from the browser, the actual ResourceResponseHead is stashed in the request and retrieved in the WebURLLoaderImpl.
We should stash that ResourceResponseHead in a place where the ResourceDispatcher can access it so it can pass it to URLLoaderClientImpl::OnReceiveResponse and the code path is the same for subresource and frames.

Comment 1 by jcivelli@chromium.org, Mar 27 2018 

It does not report mime type for frames for the same reason.

Comment 2 by cco3@chromium.org, Mar 28 2018

Cc: cco3@chromium.org
And now network_accessed
https://chromium-review.googlesource.com/c/chromium/src/+/963313
I imagine this list will keep growing.

Comment 3 by jcivelli@chromium.org, Mar 30 2018 

Related problem with url in case of redirects for the main frame.
The navigation is initiated from the browser and the WebURLLoader gets a stream param containing the redirects that it replays when the request completes. However the ResourceDispatcher never sees the redirects and does not update the final URL.
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 6 2018 

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

commit a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd
Author: Jay Civelli <jcivelli@google.com>
Date: Fri Apr 06 15:53:58 2018

Move replaying of redirects and response started to ResourceDispatcher

Moving the replaying of redirects and response started notifications for
frames from WebURLLoaderImpl to the ResourceDispatcher. This allows to
provide the actual ResourceResponseHead to
URLLoaderClientImpl::OnReceiveResponse and fixes the bug where IP
address and MIME type would not be reported in ResourceLoadComplete
notifications for frames.

Bug:  826082 
Change-Id: I02c90300e0399045e646764cf6a266dd58c0b0b7
Reviewed-on: https://chromium-review.googlesource.com/982733
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548789}
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/common/navigation_params.h
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/BUILD.gn
[add] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/navigation_response_override_parameters.cc
[add] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/navigation_response_override_parameters.h
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/request_extra_data.h
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/resource_dispatcher_unittest.cc
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/url_loader_client_impl_unittest.cc
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/url_response_body_consumer_unittest.cc
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/a9d4f3475db7300488ccf1c4d2f0cfb7ce9ef1fd/content/renderer/render_frame_impl.cc

Comment 5 by jcivelli@chromium.org, Apr 9 2018

Status: Fixed (was: Untriaged)

Comment 6 by creis@chromium.org, Apr 10 2018

Cc: arthurso...@chromium.org mfo...@chromium.org gov...@chromium.org alex...@chromium.org nasko@chromium.org lukasza@chromium.org clamy@chromium.org
Components: UI>Browser>Navigation
We think r548789 might be causing a huge spike in renderer kills in issue 831082, when run with --disable-features=NavigationMojoResponse.

We might want to revert this as the safest option.  Jay, would you be able to help?

Comment 7 by creis@chromium.org, Apr 10 2018

Cc: creis@chromium.org
(The revert would also need to be merged to the Dev channel branch.)
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 14 2018 

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

commit 916f33589c6e4656f25cb846cbb572191be4fb6a
Author: Jay Civelli <jcivelli@google.com>
Date: Sat Apr 14 04:45:50 2018

Move replaying of redirects and response started to ResourceDispatcher

Relanding "Move replaying of redirects and response started to
ResourceDispatcher". It was breaking when a renderer would access
cookies after a redirect, when the NavigationMojoResponse was disabled.
The non NavigationMojoResponse code path has been removed, so this
should now work.
Also addressing a crasher when replaying redirects for deferred
responses.

Original description:
Moving the replaying of redirects and response started notifications for
frames from WebURLLoaderImpl to the ResourceDispatcher. This allows to
provide the actual ResourceResponseHead to
URLLoaderClientImpl::OnReceiveResponse and fixes the bug where IP
address and MIME type would not be reported in ResourceLoadComplete
notifications for frames.

Tbr: kinuko@chromium.org, clamy@chromium.org
Bug:  826082 
Change-Id: I250fa02e8f82fe4db5dec01ca4d736551464dbc4
Reviewed-on: https://chromium-review.googlesource.com/1007625
Commit-Queue: Jay Civelli <jcivelli@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550879}
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/common/navigation_params.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/BUILD.gn
[add] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/navigation_response_override_parameters.cc
[add] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/navigation_response_override_parameters.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/request_extra_data.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/resource_dispatcher_unittest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/url_loader_client_impl_unittest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/url_response_body_consumer_unittest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/render_frame_impl.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/916f33589c6e4656f25cb846cbb572191be4fb6a

commit 916f33589c6e4656f25cb846cbb572191be4fb6a
Author: Jay Civelli <jcivelli@google.com>
Date: Sat Apr 14 04:45:50 2018

Move replaying of redirects and response started to ResourceDispatcher

Relanding "Move replaying of redirects and response started to
ResourceDispatcher". It was breaking when a renderer would access
cookies after a redirect, when the NavigationMojoResponse was disabled.
The non NavigationMojoResponse code path has been removed, so this
should now work.
Also addressing a crasher when replaying redirects for deferred
responses.

Original description:
Moving the replaying of redirects and response started notifications for
frames from WebURLLoaderImpl to the ResourceDispatcher. This allows to
provide the actual ResourceResponseHead to
URLLoaderClientImpl::OnReceiveResponse and fixes the bug where IP
address and MIME type would not be reported in ResourceLoadComplete
notifications for frames.

Tbr: kinuko@chromium.org, clamy@chromium.org
Bug:  826082 
Change-Id: I250fa02e8f82fe4db5dec01ca4d736551464dbc4
Reviewed-on: https://chromium-review.googlesource.com/1007625
Commit-Queue: Jay Civelli <jcivelli@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550879}
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/common/navigation_params.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/BUILD.gn
[add] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/navigation_response_override_parameters.cc
[add] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/navigation_response_override_parameters.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/request_extra_data.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/resource_dispatcher_unittest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/sync_load_context.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/url_loader_client_impl_unittest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/url_response_body_consumer_unittest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/916f33589c6e4656f25cb846cbb572191be4fb6a/content/renderer/render_frame_impl.cc
►

Sign in to add a comment