Skip to content

Commit

Permalink
Revert "If an extension does a content injection disable bf cache."
Browse files Browse the repository at this point in the history 
This reverts commit c787d2a.

Reason for revert:

Causes flaky failures on some Linux bots, e.g., https://ci.chromium.org/p/chromium/builders/ci/Linux%20Ozone%20Tester%20%28Wayland%29
Example failure:
browser_tests_wayland failed because of:
ExtensionBackForwardCacheContentScriptDisabledBrowserTest.CSSDisallowed

pattern of failures on this bot:
https://screenshot.googleplex.com/87iSx5FiDVGZRmF

Failure error is
../../chrome/browser/extensions/back_forward_cache_browsertest.cc:110: Failure
Value of: delete_observer_rfh_a.deleted()
  Actual: false
Expected: true
Stack trace:
#0 0x563563bb7efc extensions::ExtensionBackForwardCacheContentScriptDisabledBrowserTest_CSSDisallowed_Test::RunTestOnMainThread()
#1 0x563567b440ce content::BrowserTestBase::ProxyRunTestOnMainThreadLoop()
#2 0x5635676da77d ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
#3 0x5635676d9d54 ChromeBrowserMainParts::PreMainMessageLoopRun()
#4 0x56356561f0df content::BrowserMainLoop::PreMainMessageLoopRun()
#5 0x563565a4e6a3 content::StartupTaskRunner::RunAllTasksNow()
#6 0x56356561ed5d content::BrowserMainLoop::CreateStartupTasks()
#7 0x563565621228 content::BrowserMainRunnerImpl::Initialize()
#8 0x56356561d610 content::BrowserMain()
#9 0x563566297393 content::ContentMainRunnerImpl::RunBrowser()
#10 0x563566296f1d content::ContentMainRunnerImpl::Run()
#11 0x5635662944ad content::RunContentProcess()
#12 0x563566294e4d content::ContentMain()
#13 0x563567b43778 content::BrowserTestBase::SetUp()
#14 0x563567587576 InProcessBrowserTest::SetUp()

The same error appears on other bots when this fails too, e.g.,
https://ci.chromium.org/p/chromium/builders/ci/Linux%20Ozone%20Tester%20%28X11%29/29058 
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8850497656556318928/+/steps/browser_tests_x11/0/logs/Deterministic_failure:_ExtensionBackForwardCacheContentScriptDisabledBrowserTest.CSSDisallowed__status_FAILURE_/0

Original change's description:
> If an extension does a content injection disable bf cache.
>
> This code tracks whether a content injection (insertCSS, contentScript,
> executeScript) has occurred for a WebFrame. If so then turn off
> BFCache for the frame.
>
> BUG=1192785
>
> Change-Id: I682a9efb247aae358023e3a591368c84d47001ce
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2787195
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#870582}

Bug: 1192785
Change-Id: Ib122b1d64155d85009038d961306ac620053564b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2816001
Auto-Submit: Mark Pearson <mpearson@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Owners-Override: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#870681}
  • Loading branch information
Mark Pearson authored and Chromium LUCI CQ committed Apr 8, 2021
1 parent 5743c3d commit df7db98
Show file tree
Hide file tree
Showing 23 changed files with 48 additions and 306 deletions.
137 changes: 0 additions & 137 deletions chrome/browser/extensions/back_forward_cache_browsertest.cc
View file

This file was deleted.

3 changes: 1 addition & 2 deletions chrome/renderer/cart/commerce_hint_agent.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,8 +439,7 @@ void CommerceHintAgent::ExtractProducts() {
new JavaScriptRequest(weak_factory_.GetWeakPtr());
main_frame->RequestExecuteScriptInIsolatedWorld(
ISOLATED_WORLD_ID_CHROME_INTERNAL, &source, 1, false,
blink::WebLocalFrame::kAsynchronous, request,
blink::BackForwardCacheAware::kAllow);
blink::WebLocalFrame::kAsynchronous, request);
}

CommerceHintAgent::JavaScriptRequest::JavaScriptRequest(
Expand Down
1 change: 0 additions & 1 deletion chrome/test/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2044,7 +2044,6 @@ if (!is_android) {
"../browser/extensions/app_process_apitest.cc",
"../browser/extensions/app_window_overrides_browsertest.cc",
"../browser/extensions/autoplay_browsertest.cc",
"../browser/extensions/back_forward_cache_browsertest.cc",
"../browser/extensions/background_header_browsertest.cc",
"../browser/extensions/background_page_apitest.cc",
"../browser/extensions/background_scripts_apitest.cc",
Expand Down
5 changes: 0 additions & 5 deletions chrome/test/data/extensions/back_forward_cache/content_css/color.css
View file

This file was deleted.

14 changes: 0 additions & 14 deletions chrome/test/data/extensions/back_forward_cache/content_css/manifest.json
View file

This file was deleted.

5 changes: 0 additions & 5 deletions chrome/test/data/extensions/back_forward_cache/content_script/change_page_title.js
View file

This file was deleted.

14 changes: 0 additions & 14 deletions chrome/test/data/extensions/back_forward_cache/content_script/manifest.json
View file

This file was deleted.

19 changes: 9 additions & 10 deletions components/translate/content/renderer/per_frame_translate_agent.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,7 @@ void PerFrameTranslateAgent::ExecuteScript(const std::string& script) {
return;

WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
local_frame->ExecuteScriptInIsolatedWorld(
world_id_, source, blink::BackForwardCacheAware::kAllow);
local_frame->ExecuteScriptInIsolatedWorld(world_id_, source);
}

bool PerFrameTranslateAgent::ExecuteScriptAndGetBoolResult(
Expand All @@ -215,8 +214,8 @@ bool PerFrameTranslateAgent::ExecuteScriptAndGetBoolResult(
v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
v8::Local<v8::Value> result =
local_frame->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id_, source, blink::BackForwardCacheAware::kAllow);
local_frame->ExecuteScriptInIsolatedWorldAndReturnValue(world_id_,
source);
DCHECK(result->IsBoolean());

return result.As<v8::Boolean>()->Value();
Expand All @@ -233,8 +232,8 @@ std::string PerFrameTranslateAgent::ExecuteScriptAndGetStringResult(
v8::HandleScope handle_scope(isolate);
WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
v8::Local<v8::Value> result =
local_frame->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id_, source, blink::BackForwardCacheAware::kAllow);
local_frame->ExecuteScriptInIsolatedWorldAndReturnValue(world_id_,
source);
DCHECK(result->IsString());

v8::Local<v8::String> v8_str = result.As<v8::String>();
Expand All @@ -254,8 +253,8 @@ double PerFrameTranslateAgent::ExecuteScriptAndGetDoubleResult(
v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
v8::Local<v8::Value> result =
local_frame->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id_, source, blink::BackForwardCacheAware::kAllow);
local_frame->ExecuteScriptInIsolatedWorldAndReturnValue(world_id_,
source);
DCHECK(result->IsNumber());

return result.As<v8::Number>()->Value();
Expand All @@ -271,8 +270,8 @@ int64_t PerFrameTranslateAgent::ExecuteScriptAndGetIntegerResult(
v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
v8::Local<v8::Value> result =
local_frame->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id_, source, blink::BackForwardCacheAware::kAllow);
local_frame->ExecuteScriptInIsolatedWorldAndReturnValue(world_id_,
source);
DCHECK(result->IsNumber());

return result.As<v8::Integer>()->Value();
Expand Down
15 changes: 5 additions & 10 deletions components/translate/content/renderer/translate_agent.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,7 @@ void TranslateAgent::ExecuteScript(const std::string& script) {
return;

WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
main_frame->ExecuteScriptInIsolatedWorld(
world_id_, source, blink::BackForwardCacheAware::kAllow);
main_frame->ExecuteScriptInIsolatedWorld(world_id_, source);
}

bool TranslateAgent::ExecuteScriptAndGetBoolResult(const std::string& script,
Expand All @@ -263,8 +262,7 @@ bool TranslateAgent::ExecuteScriptAndGetBoolResult(const std::string& script,
v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
v8::Local<v8::Value> result =
main_frame->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id_, source, blink::BackForwardCacheAware::kAllow);
main_frame->ExecuteScriptInIsolatedWorldAndReturnValue(world_id_, source);
if (result.IsEmpty() || !result->IsBoolean()) {
NOTREACHED();
return fallback;
Expand All @@ -283,8 +281,7 @@ std::string TranslateAgent::ExecuteScriptAndGetStringResult(
v8::HandleScope handle_scope(isolate);
WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
v8::Local<v8::Value> result =
main_frame->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id_, source, blink::BackForwardCacheAware::kAllow);
main_frame->ExecuteScriptInIsolatedWorldAndReturnValue(world_id_, source);
if (result.IsEmpty() || !result->IsString()) {
NOTREACHED();
return std::string();
Expand All @@ -309,8 +306,7 @@ double TranslateAgent::ExecuteScriptAndGetDoubleResult(
v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
v8::Local<v8::Value> result =
main_frame->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id_, source, blink::BackForwardCacheAware::kAllow);
main_frame->ExecuteScriptInIsolatedWorldAndReturnValue(world_id_, source);
if (result.IsEmpty() || !result->IsNumber()) {
NOTREACHED();
return 0.0;
Expand All @@ -328,8 +324,7 @@ int64_t TranslateAgent::ExecuteScriptAndGetIntegerResult(
v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
WebScriptSource source = WebScriptSource(WebString::FromASCII(script));
v8::Local<v8::Value> result =
main_frame->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id_, source, blink::BackForwardCacheAware::kAllow);
main_frame->ExecuteScriptInIsolatedWorldAndReturnValue(world_id_, source);
if (result.IsEmpty() || !result->IsNumber()) {
NOTREACHED();
return 0;
Expand Down
13 changes: 0 additions & 13 deletions content/browser/renderer_host/back_forward_cache_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,6 @@ bool IsGeolocationSupported() {
return geolocation_supported.Get();
}

bool IsContentInjectionSupported() {
if (!IsBackForwardCacheEnabled())
return false;
static constexpr base::FeatureParam<bool> content_injection_supported(
&features::kBackForwardCache, "content_injection_supported", false);
return content_injection_supported.Get();
}

bool IsFileSystemSupported() {
if (!IsBackForwardCacheEnabled())
return false;
Expand Down Expand Up @@ -212,11 +204,6 @@ uint64_t GetDisallowedFeatures(RenderFrameHostImpl* rfh,
WebSchedulerTrackedFeature::kRequestedGeolocationPermission);
}

if (!IsContentInjectionSupported()) {
result |= FeatureToBit(WebSchedulerTrackedFeature::kIsolatedWorldScript) |
FeatureToBit(WebSchedulerTrackedFeature::kInjectedStyleSheet);
}

if (!IgnoresOutstandingNetworkRequestForTesting()) {
result |=
FeatureToBit(
Expand Down
6 changes: 2 additions & 4 deletions content/renderer/render_frame_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2446,8 +2446,7 @@ void RenderFrameImpl::JavaScriptExecuteRequestForTests(
WebScriptSource(WebString::FromUTF16(javascript)));
} else {
result = frame_->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id, WebScriptSource(WebString::FromUTF16(javascript)),
blink::BackForwardCacheAware::kAllow);
world_id, WebScriptSource(WebString::FromUTF16(javascript)));
}

if (!weak_this)
Expand Down Expand Up @@ -2482,8 +2481,7 @@ void RenderFrameImpl::JavaScriptExecuteRequestInIsolatedWorld(
JavaScriptIsolatedWorldRequest* request = new JavaScriptIsolatedWorldRequest(
weak_factory_.GetWeakPtr(), wants_result, std::move(callback));
frame_->RequestExecuteScriptInIsolatedWorld(
world_id, &script, 1, false, WebLocalFrame::kSynchronous, request,
blink::BackForwardCacheAware::kAllow);
world_id, &script, 1, false, WebLocalFrame::kSynchronous, request);
}

RenderFrameImpl::JavaScriptIsolatedWorldRequest::JavaScriptIsolatedWorldRequest(
Expand Down
7 changes: 3 additions & 4 deletions content/web_test/renderer/test_runner.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1075,8 +1075,8 @@ TestRunnerBindings::EvaluateScriptInIsolatedWorldAndReturnValue(
return {};

blink::WebScriptSource source = blink::WebString::FromUTF8(script);
return GetWebFrame()->ExecuteScriptInIsolatedWorldAndReturnValue(
world_id, source, blink::BackForwardCacheAware::kAllow);
return GetWebFrame()->ExecuteScriptInIsolatedWorldAndReturnValue(world_id,
source);
}

void TestRunnerBindings::EvaluateScriptInIsolatedWorld(
Expand All @@ -1086,8 +1086,7 @@ void TestRunnerBindings::EvaluateScriptInIsolatedWorld(
return;

blink::WebScriptSource source = blink::WebString::FromUTF8(script);
GetWebFrame()->ExecuteScriptInIsolatedWorld(
world_id, source, blink::BackForwardCacheAware::kAllow);
GetWebFrame()->ExecuteScriptInIsolatedWorld(world_id, source);
}

void TestRunnerBindings::SetIsolatedWorldInfo(
Expand Down
8 changes: 3 additions & 5 deletions extensions/renderer/script_injection.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,7 @@ void ScriptInjection::InjectJs(std::set<std::string>* executing_scripts,

render_frame_->GetWebFrame()->RequestExecuteScriptInIsolatedWorld(
world_id, &sources.front(), sources.size(), is_user_gesture,
execution_option, callback.release(),
blink::BackForwardCacheAware::kPossiblyDisallow);
execution_option, callback.release());
}

void ScriptInjection::OnJsInjectionCompleted(
Expand Down Expand Up @@ -437,9 +436,8 @@ void ScriptInjection::InjectOrRemoveCss(
} else {
DCHECK(adding_css);
for (const blink::WebString& css : css_sources)
web_frame->GetDocument().InsertStyleSheet(
css, &style_sheet_key, blink_css_origin,
blink::BackForwardCacheAware::kPossiblyDisallow);
web_frame->GetDocument().InsertStyleSheet(css, &style_sheet_key,
blink_css_origin);
}
}

Expand Down
Loading

0 comments on commit df7db98

Please sign in to comment.