Commit bda6c898 authored by Josh Karlin's avatar Josh Karlin Committed by Commit Bot

Speed up IsAdScriptInStack by passing ExecutionContext when known

What: Some callers to IsAdScriptInStack know the ExecutionContext that
triggered the event. Those callers should pass the context in to
IsAdScriptInStack as there is a small cost to looking it up.

Why: Some callers (such as core_probes) call frequently and looking up
the ExecutionContext when it's already known only slows things down.

Bug: 851531
Change-Id: Id8eadfc9855cdda0d18df34a4829dd47e31ac64f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090232
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: default avatarDaniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747899}
parent 0e6871ab
......@@ -157,9 +157,9 @@ DispatchEventResult EventDispatcher::Dispatch() {
Document& document = node_->GetDocument();
if (frame) {
// A genuine mouse click cannot be triggered by script so we don't expect
// there are any script in the stack.
// there to be any script in the stack.
DCHECK(!frame->GetAdTracker() ||
!frame->GetAdTracker()->IsAdScriptInStack());
!frame->GetAdTracker()->IsAdScriptInStackSlow());
if (frame->IsAdSubframe()) {
UseCounter::Count(document, WebFeature::kAdClick);
}
......
......@@ -11,6 +11,7 @@
#include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
#include "third_party/blink/renderer/core/core_probe_sink.h"
#include "third_party/blink/renderer/core/dom/document.h"
#include "third_party/blink/renderer/core/execution_context/execution_context.h"
#include "third_party/blink/renderer/core/frame/local_frame.h"
#include "third_party/blink/renderer/core/probe/core_probes.h"
#include "third_party/blink/renderer/platform/loader/fetch/resource_request.h"
......@@ -156,7 +157,7 @@ bool AdTracker::CalculateIfAdSubresource(ExecutionContext* execution_context,
// Check if the document loading the resource is an ad or if any executing
// script is an ad.
known_ad = known_ad || IsKnownAdExecutionContext(execution_context) ||
IsAdScriptInStack();
IsAdScriptInStackForContext(execution_context);
// If it is a script marked as an ad and it's not in an ad context, append it
// to the known ad script set. We don't need to keep track of ad scripts in ad
......@@ -170,12 +171,13 @@ bool AdTracker::CalculateIfAdSubresource(ExecutionContext* execution_context,
return known_ad;
}
void AdTracker::DidCreateAsyncTask(probe::AsyncTaskId* task) {
void AdTracker::DidCreateAsyncTask(probe::AsyncTaskId* task,
ExecutionContext* execution_context) {
DCHECK(task);
if (!async_stack_enabled_)
return;
if (IsAdScriptInStack())
if (IsAdScriptInStackForContext(execution_context))
task->SetAdTask();
}
......@@ -197,11 +199,11 @@ void AdTracker::DidFinishAsyncTask(probe::AsyncTaskId* task) {
running_ad_async_tasks_ -= 1;
}
bool AdTracker::IsAdScriptInStack() {
bool AdTracker::IsAdScriptInStackForContext(
ExecutionContext* execution_context) {
if (num_ads_in_stack_ > 0 || running_ad_async_tasks_ > 0)
return true;
ExecutionContext* execution_context = GetCurrentExecutionContext();
if (!execution_context)
return false;
......@@ -221,6 +223,10 @@ bool AdTracker::IsAdScriptInStack() {
return false;
}
bool AdTracker::IsAdScriptInStackSlow() {
return IsAdScriptInStackForContext(GetCurrentExecutionContext());
}
bool AdTracker::IsKnownAdScript(ExecutionContext* execution_context,
const String& url) {
if (!execution_context)
......
......@@ -60,7 +60,8 @@ class CORE_EXPORT AdTracker : public GarbageCollected<AdTracker> {
// Called when an async task is created. Check at this point for ad script on
// the stack and annotate the task if so.
void DidCreateAsyncTask(probe::AsyncTaskId* task);
void DidCreateAsyncTask(probe::AsyncTaskId* task,
ExecutionContext* execution_context);
// Called when an async task is eventually run.
void DidStartAsyncTask(probe::AsyncTaskId* task);
......@@ -69,8 +70,15 @@ class CORE_EXPORT AdTracker : public GarbageCollected<AdTracker> {
void DidFinishAsyncTask(probe::AsyncTaskId* task);
// Returns true if any script in the pseudo call stack has previously been
// identified as an ad resource.
bool IsAdScriptInStack();
// identified as an ad resource, if |execution_context| is a known ad
// execution context, or if the script at the top of |execution_context|'s
// stack is ad script.
bool IsAdScriptInStackForContext(ExecutionContext* execution_context);
// Determines the current ExecutionContext and then calls
// IsAdScriptInStackForContext with it. If you know the ExecutionContext*,
// call IsAdScriptInStackForContext, as it's faster than looking it up.
bool IsAdScriptInStackSlow();
virtual void Trace(Visitor*);
......
......@@ -96,24 +96,25 @@ class AdTrackerTest : public testing::Test {
if (ad_tracker_)
ad_tracker_->Shutdown();
ad_tracker_ = MakeGarbageCollected<TestAdTracker>(GetFrame());
ad_tracker_->SetExecutionContext(
page_holder_->GetDocument().ToExecutionContext());
ad_tracker_->SetExecutionContext(ExecutionContext());
}
void WillExecuteScript(const String& script_url) {
ad_tracker_->WillExecuteScript(
page_holder_->GetDocument().ToExecutionContext(), String(script_url));
ad_tracker_->WillExecuteScript(ExecutionContext(), String(script_url));
}
ExecutionContext* ExecutionContext() {
return page_holder_->GetDocument().ToExecutionContext();
}
void DidExecuteScript() { ad_tracker_->DidExecuteScript(); }
bool AnyExecutingScriptsTaggedAsAdResource() {
return ad_tracker_->IsAdScriptInStack();
return ad_tracker_->IsAdScriptInStackSlow();
}
void AppendToKnownAdScripts(const String& url) {
ad_tracker_->AppendToKnownAdScripts(
*page_holder_->GetDocument().ToExecutionContext(), url);
ad_tracker_->AppendToKnownAdScripts(*ExecutionContext(), url);
}
Persistent<TestAdTracker> ad_tracker_;
......@@ -251,7 +252,7 @@ TEST_F(AdTrackerTest, AsyncTagging) {
probe::AsyncTaskId async_task;
// Create an async task while ad script is running.
ad_tracker_->DidCreateAsyncTask(&async_task);
ad_tracker_->DidCreateAsyncTask(&async_task, ExecutionContext());
// Finish executing the ad script.
DidExecuteScript();
......@@ -355,12 +356,17 @@ TEST_F(AdTrackerSimTest, ScriptDetectedByContext) {
To<LocalFrame>(GetDocument().GetFrame()->Tree().FirstChild());
EXPECT_TRUE(child_frame->IsAdSubframe());
// Now run unknown script in the child's context. It should be considered an
// ad based on context alone.
ad_tracker_->SetExecutionContext(
child_frame->GetDocument()->ToExecutionContext());
// Run vanilla script in the main frame context. It shouldn't be an ad.
ad_tracker_->SetScriptAtTopOfStack("foo.js");
EXPECT_TRUE(ad_tracker_->IsAdScriptInStack());
EXPECT_FALSE(ad_tracker_->IsAdScriptInStackForContext(
GetDocument().ToExecutionContext()));
// Run the same vanilla script in the child's context. It should be considered
// an ad based on context alone. This also tests IsAdScriptInStack's
// |execution_context| param.
ExecutionContext* execution_context =
child_frame->GetDocument()->ToExecutionContext();
EXPECT_TRUE(ad_tracker_->IsAdScriptInStackForContext(execution_context));
}
TEST_F(AdTrackerSimTest, RedirectToAdUrl) {
......
......@@ -1270,7 +1270,7 @@ void LocalFrame::SetIsAdSubframeIfNecessary() {
bool parent_is_ad = parent->IsAdSubframe();
if (parent_is_ad || ad_tracker_->IsAdScriptInStack()) {
if (parent_is_ad || ad_tracker_->IsAdScriptInStackSlow()) {
SetIsAdSubframe(parent_is_ad ? blink::mojom::AdFrameType::kChildAd
: blink::mojom::AdFrameType::kRootAd);
}
......
......@@ -203,7 +203,7 @@ static void MaybeLogWindowOpen(LocalFrame& opener_frame) {
return;
bool is_ad_subframe = opener_frame.IsAdSubframe();
bool is_ad_script_in_stack = ad_tracker->IsAdScriptInStack();
bool is_ad_script_in_stack = ad_tracker->IsAdScriptInStackSlow();
FromAdState state =
blink::GetFromAdState(is_ad_subframe, is_ad_script_in_stack);
......
......@@ -124,7 +124,7 @@ void AsyncTaskScheduled(ExecutionContext* context,
blink::AdTracker* ad_tracker = AdTracker::FromExecutionContext(context);
if (ad_tracker)
ad_tracker->DidCreateAsyncTask(task);
ad_tracker->DidCreateAsyncTask(task, context);
}
void AsyncTaskScheduledBreakable(ExecutionContext* context,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment