New issue
Advanced search Search tips

Issue 739777 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[subresource_filter] Ensure activation is propagated to about:blank / data: / about:srcdoc frames

Project Member Reported by csharrison@chromium.org, Jul 6 2017

Issue description

It is relatively common to load resources in these types of subframes. Previously, the component explicitly ignored them since their loads do not align well with our navigation/browser oriented architecture.

My proposal is to just inherit activation state from the parent.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 18 2017

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

commit cc908983b2ef3869295a8658110b67a03da27b94
Author: Charles Harrison <csharrison@chromium.org>
Date: Tue Jul 18 22:56:33 2017

[subresource_filter] Properly handle blank/data/srdoc navigation activation

It is relatively common to load resources in subframes that navigate
but do not trigger ReadyToCommitNavigation in the browser process,
or go through the standard NavigationThrottle pipeline.

Previously, the component explicitly ignored them since their loads do
not align well with our navigation/browser oriented architecture.

This CL simply forwards the parent frame's activation state into the
child, which the child uses any time it navigates to one of these
special URLs. 

Bug:  739777 
Change-Id: I052ce6df3efead9cb78f309211f6ababa8ac3944
Reviewed-on: https://chromium-review.googlesource.com/559732
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487645}
[modify] https://crrev.com/cc908983b2ef3869295a8658110b67a03da27b94/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
[add] https://crrev.com/cc908983b2ef3869295a8658110b67a03da27b94/chrome/test/data/subresource_filter/frame_set_special_urls.html
[delete] https://crrev.com/44094e255a6f73633b4e4703c5121df37e894676/chrome/test/data/subresource_filter/frame_set_sync_loads.html
[modify] https://crrev.com/cc908983b2ef3869295a8658110b67a03da27b94/components/subresource_filter/content/renderer/subresource_filter_agent.cc
[modify] https://crrev.com/cc908983b2ef3869295a8658110b67a03da27b94/components/subresource_filter/content/renderer/subresource_filter_agent.h
[modify] https://crrev.com/cc908983b2ef3869295a8658110b67a03da27b94/components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
[modify] https://crrev.com/cc908983b2ef3869295a8658110b67a03da27b94/components/subresource_filter/content/renderer/web_document_subresource_filter_impl.cc
[modify] https://crrev.com/cc908983b2ef3869295a8658110b67a03da27b94/components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h
[modify] https://crrev.com/cc908983b2ef3869295a8658110b67a03da27b94/components/subresource_filter/core/common/document_subresource_filter.h

Cc: csharrison@chromium.org
Labels: -Pri-3 Pri-2
Owner: shivanisha@chromium.org
Moving ownership to Shivani
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 31 2017

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

commit c0cd953bf9d47d073c12eb223a6324253370acdb
Author: Shivani Sharma <shivanisha@chromium.org>
Date: Thu Aug 31 16:58:00 2017

[subresource filter] Adds support for PlzNavigate for special urls handling.

Since special urls like about:blank, about:srcdoc etc. do not lead to
creation of navigation throttles, subresource filter's code treats 
such sub-frames specially in the renderer code. This CL will align the
triggering condition for these special urls with how PlzNavigate decides
a URL to be special.

This CL does the following:
- Moves ShouldUseParentNavigation to content/common so that it could be
used by browser side code.
- Branches ShouldUseParentNavigation to use ShouldMakeNetworkRequestForURL
for PlzNavigate. This will have the functional change that data url will go
through navigation throttles. This also required moving
ShouldMakeNetworkRequestForURL to content/public/common.
- Makes sure DidCommitProvisionalLoad at renderer side does not early
return for PlzNavigate Data url scenario by modifying the early return
condition.

Bug:  739777 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I8aa2b9e197084d9929e037973f17a67df3850885
Reviewed-on: https://chromium-review.googlesource.com/623829
Commit-Queue: Shivani Sharma <shivanisha@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498891}
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/chrome/browser/subresource_filter/subresource_filter_special_subframe_navigations_browsertest.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/components/subresource_filter/content/common/BUILD.gn
[add] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/components/subresource_filter/content/common/subresource_filter_utils.cc
[add] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/components/subresource_filter/content/common/subresource_filter_utils.h
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/components/subresource_filter/content/renderer/subresource_filter_agent.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/components/subresource_filter/content/renderer/subresource_filter_agent.h
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/common/navigation_params.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/common/navigation_params.h
[delete] https://crrev.com/05159ccd6ee65078f160cb1fba0ba69b8a9a3c7c/content/common/navigation_params_unittest.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/public/common/url_utils.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/public/common/url_utils.h
[add] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/public/common/url_utils_unittest.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/test/BUILD.gn
[modify] https://crrev.com/c0cd953bf9d47d073c12eb223a6324253370acdb/content/test/test_render_frame_host.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 31 2017

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

commit 56b005876fad056c540dfbe8933a09c2ce9e7c0f
Author: Shivani Sharma <shivanisha@chromium.org>
Date: Thu Aug 31 19:44:59 2017

[subresource filter] Persists browser side activation state for special url sub-frames.

Special url schemes that do not require a network request, do not go through
creating navigation throttles. Such iframes therefore do not get to compute their
activation state. For renderer initiated navigations, in renderer they inherit the 
activation state of their parent in DidCommitProvisionalLoad so that any sub-resources 
they load can be checked for blocking.
This CL does the same for them in the browser process in DidFinishNavigation so that
any of their normal-url child iframes can be activated as well. 

The impact of this CL is that it will increase the coverage of subresource filtering
to include child frames of special url frames.

Bug:  739777 
Change-Id: Id83a76f9a956257441d5d62b869494fc70833148
Reviewed-on: https://chromium-review.googlesource.com/624415
Commit-Queue: Shivani Sharma <shivanisha@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498957}
[modify] https://crrev.com/56b005876fad056c540dfbe8933a09c2ce9e7c0f/chrome/browser/subresource_filter/subresource_filter_special_subframe_navigations_browsertest.cc
[modify] https://crrev.com/56b005876fad056c540dfbe8933a09c2ce9e7c0f/chrome/test/data/subresource_filter/frame_set_special_urls.html
[modify] https://crrev.com/56b005876fad056c540dfbe8933a09c2ce9e7c0f/components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc
[modify] https://crrev.com/56b005876fad056c540dfbe8933a09c2ce9e7c0f/components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h

Status: Fixed (was: Started)

Sign in to add a comment