Skip to content

Commit

Permalink
Send the full ActivationState computation to the renderer
Browse files Browse the repository at this point in the history
As of this patch, the renderer will not need to compute ActivationState
by using ancestor URLs. This allows for correct activation decisions
in OOPIF frames.

BUG=637415
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2776533003
Cr-Commit-Position: refs/heads/master@{#462180}
  • Loading branch information
csharrison authored and Commit bot committed Apr 5, 2017
1 parent 3334893 commit 27c6839
Show file tree
Hide file tree
Showing 11 changed files with 145 additions and 209 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,10 @@ class ContentSubresourceFilterDriverFactoryTest
SubresourceFilterMsg_ActivateForNextCommittedLoad::ID);
ASSERT_EQ(expect_activation, !!message);
if (expect_activation) {
std::tuple<ActivationLevel, bool> args;
std::tuple<ActivationState> args;
SubresourceFilterMsg_ActivateForNextCommittedLoad::Read(message, &args);
EXPECT_NE(ActivationLevel::DISABLED, std::get<0>(args));
ActivationLevel level = std::get<0>(args).activation_level;
EXPECT_NE(ActivationLevel::DISABLED, level);
}
render_process_host->sink().ClearMessages();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ void ContentSubresourceFilterThrottleManager::ReadyToCommitNavigation(
content::RenderFrameHost* frame_host =
navigation_handle->GetRenderFrameHost();
frame_host->Send(new SubresourceFilterMsg_ActivateForNextCommittedLoad(
frame_host->GetRoutingID(), filter->activation_state().activation_level,
filter->activation_state().measure_performance));
frame_host->GetRoutingID(), filter->activation_state()));
}

void ContentSubresourceFilterThrottleManager::DidFinishNavigation(
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "base/time/time.h"
#include "components/subresource_filter/core/common/activation_level.h"
#include "components/subresource_filter/core/common/activation_state.h"
#include "components/subresource_filter/core/common/document_load_statistics.h"
#include "content/public/common/common_param_traits_macros.h"
#include "ipc/ipc_message.h"
Expand All @@ -18,6 +19,13 @@
IPC_ENUM_TRAITS_MAX_VALUE(subresource_filter::ActivationLevel,
subresource_filter::ActivationLevel::LAST);

IPC_STRUCT_TRAITS_BEGIN(subresource_filter::ActivationState)
IPC_STRUCT_TRAITS_MEMBER(activation_level)
IPC_STRUCT_TRAITS_MEMBER(filtering_disabled_for_document)
IPC_STRUCT_TRAITS_MEMBER(generic_blocking_rules_disabled)
IPC_STRUCT_TRAITS_MEMBER(measure_performance)
IPC_STRUCT_TRAITS_END()

IPC_STRUCT_TRAITS_BEGIN(subresource_filter::DocumentLoadStatistics)
IPC_STRUCT_TRAITS_MEMBER(num_loads_total)
IPC_STRUCT_TRAITS_MEMBER(num_loads_evaluated)
Expand All @@ -38,7 +46,7 @@ IPC_MESSAGE_CONTROL1(SubresourceFilterMsg_SetRulesetForProcess,
IPC::PlatformFileForTransit /* ruleset_file */);

// Instructs the renderer to activate subresource filtering at the specified
// |activation_level| for the document load committed next in a frame.
// |activation_state| for the document load committed next in a frame.
//
// Without browser-side navigation, the message must arrive just before the
// provisional load is committed on the renderer side. In practice, it is often
Expand All @@ -49,9 +57,8 @@ IPC_MESSAGE_CONTROL1(SubresourceFilterMsg_SetRulesetForProcess,
// FrameMsg_CommitNavigation.
//
// If no message arrives, the default behavior is ActivationLevel::DISABLED.
IPC_MESSAGE_ROUTED2(SubresourceFilterMsg_ActivateForNextCommittedLoad,
subresource_filter::ActivationLevel /* activation_level */,
bool /* measure_performance */);
IPC_MESSAGE_ROUTED1(SubresourceFilterMsg_ActivateForNextCommittedLoad,
subresource_filter::ActivationState /* activation_state */);

// ----------------------------------------------------------------------------
// Messages sent from the renderer to the browser.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,8 @@ SubresourceFilterAgent::SubresourceFilterAgent(

SubresourceFilterAgent::~SubresourceFilterAgent() = default;

std::vector<GURL> SubresourceFilterAgent::GetAncestorDocumentURLs() {
std::vector<GURL> urls;
// As a temporary workaround for --isolate-extensions, ignore the ancestor
// hierarchy after crossing an extension/non-extension boundary. This,
// however, will not be a satisfactory solution for OOPIF in general.
// See: https://crbug.com/637415.
blink::WebFrame* frame = render_frame()->GetWebFrame();
do {
urls.push_back(frame->document().url());
frame = frame->parent();
} while (frame && frame->isWebLocalFrame());
return urls;
GURL SubresourceFilterAgent::GetDocumentURL() {
return render_frame()->GetWebFrame()->document().url();
}

void SubresourceFilterAgent::SetSubresourceFilterForCommittedLoad(
Expand All @@ -70,20 +60,20 @@ void SubresourceFilterAgent::SendDocumentLoadStatistics(
}

void SubresourceFilterAgent::OnActivateForNextCommittedLoad(
ActivationLevel activation_level,
bool measure_performance) {
activation_level_for_next_commit_ = activation_level;
measure_performance_for_next_commit_ = measure_performance;
ActivationState activation_state) {
activation_state_for_next_commit_ = activation_state;
}

void SubresourceFilterAgent::RecordHistogramsOnLoadCommitted() {
// Note: ActivationLevel used to be called ActivationState, the legacy name is
// kept for the histogram.
ActivationLevel activation_level =
activation_state_for_next_commit_.activation_level;
UMA_HISTOGRAM_ENUMERATION("SubresourceFilter.DocumentLoad.ActivationState",
static_cast<int>(activation_level_for_next_commit_),
static_cast<int>(activation_level),
static_cast<int>(ActivationLevel::LAST) + 1);

if (activation_level_for_next_commit_ != ActivationLevel::DISABLED) {
if (activation_level != ActivationLevel::DISABLED) {
UMA_HISTOGRAM_BOOLEAN("SubresourceFilter.DocumentLoad.RulesetIsAvailable",
ruleset_dealer_->IsRulesetFileAvailable());
}
Expand Down Expand Up @@ -133,8 +123,8 @@ void SubresourceFilterAgent::RecordHistogramsOnLoadFinished() {
}

void SubresourceFilterAgent::ResetActivatonStateForNextCommit() {
activation_level_for_next_commit_ = ActivationLevel::DISABLED;
measure_performance_for_next_commit_ = false;
activation_state_for_next_commit_ =
ActivationState(ActivationLevel::DISABLED);
}

void SubresourceFilterAgent::OnDestruct() {
Expand All @@ -149,11 +139,13 @@ void SubresourceFilterAgent::DidCommitProvisionalLoad(

filter_for_last_committed_load_.reset();

std::vector<GURL> ancestor_document_urls = GetAncestorDocumentURLs();
if (ancestor_document_urls.front().SchemeIsHTTPOrHTTPS() ||
ancestor_document_urls.front().SchemeIsFile()) {
// TODO(csharrison): Use WebURL and WebSecurityOrigin for efficiency here,
// which require changes to the unit tests.
const GURL& url = GetDocumentURL();
if (url.SchemeIsHTTPOrHTTPS() || url.SchemeIsFile()) {
RecordHistogramsOnLoadCommitted();
if (activation_level_for_next_commit_ != ActivationLevel::DISABLED &&
if (activation_state_for_next_commit_.activation_level !=
ActivationLevel::DISABLED &&
ruleset_dealer_->IsRulesetFileAvailable()) {
base::OnceClosure first_disallowed_load_callback(
base::BindOnce(&SubresourceFilterAgent::
Expand All @@ -162,13 +154,8 @@ void SubresourceFilterAgent::DidCommitProvisionalLoad(

auto ruleset = ruleset_dealer_->GetRuleset();
DCHECK(ruleset);
ActivationState activation_state =
ComputeActivationState(activation_level_for_next_commit_,
measure_performance_for_next_commit_,
ancestor_document_urls, ruleset.get());
DCHECK(!ancestor_document_urls.empty());
auto filter = base::MakeUnique<WebDocumentSubresourceFilterImpl>(
url::Origin(ancestor_document_urls[0]), activation_state,
url::Origin(url), activation_state_for_next_commit_,
std::move(ruleset), std::move(first_disallowed_load_callback));

filter_for_last_committed_load_ = filter->AsWeakPtr();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "components/subresource_filter/core/common/activation_level.h"
#include "components/subresource_filter/core/common/activation_state.h"
#include "content/public/renderer/render_frame_observer.h"
#include "url/gurl.h"

Expand Down Expand Up @@ -40,10 +40,8 @@ class SubresourceFilterAgent
protected:
// Below methods are protected virtual so they can be mocked out in tests.

// Returns the URLs of documents loaded into nested frames starting with the
// current frame and ending with the main frame. The returned array is
// guaranteed to have at least one element.
virtual std::vector<GURL> GetAncestorDocumentURLs();
// Returns the URL of the currently committed document.
virtual GURL GetDocumentURL();

// Injects the provided subresource |filter| into the DocumentLoader
// orchestrating the most recently committed load.
Expand All @@ -59,8 +57,7 @@ class SubresourceFilterAgent
const DocumentLoadStatistics& statistics);

private:
void OnActivateForNextCommittedLoad(ActivationLevel activation_level,
bool measure_performance);
void OnActivateForNextCommittedLoad(ActivationState activation_state);
void RecordHistogramsOnLoadCommitted();
void RecordHistogramsOnLoadFinished();
void ResetActivatonStateForNextCommit();
Expand All @@ -76,8 +73,7 @@ class SubresourceFilterAgent
// Owned by the ChromeContentRendererClient and outlives us.
UnverifiedRulesetDealer* ruleset_dealer_;

ActivationLevel activation_level_for_next_commit_ = ActivationLevel::DISABLED;
bool measure_performance_for_next_commit_ = false;
ActivationState activation_state_for_next_commit_;

base::WeakPtr<WebDocumentSubresourceFilterImpl>
filter_for_last_committed_load_;
Expand Down
Loading

0 comments on commit 27c6839

Please sign in to comment.