Skip to content

Commit

Permalink
[bfcache] Pause PausableScriptExecutor for frozen context
Browse files Browse the repository at this point in the history
This CL makes PausableScriptExecutor pause and post tasks to execute
scripts instead of synchronously running, when the context is frozen.

We saw a big increase of JavaScript execution in bfcache that caused eviction. This was coming from extensions[1].

The users of PausableScriptExecutor are JSInjection and Extensions, and it makes sense to respect freezing for both of the cases.

[1]Design doc: https://docs.google.com/document/d/1uf5KOSHzVlh1zSE_PeCD1IxP_Fu-E5rS7jsLmpzCDVo/edit#

Bug: 1379227
Change-Id: If3b92ad9f8311190c4238d4c6d2d9f7291b7921f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3995288
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Yuzu Saijo <yuzus@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067406}
  • Loading branch information
Yuzu Saijo authored and Chromium LUCI CQ committed Nov 4, 2022
1 parent 8bc1ba7 commit 22c0064
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 15 deletions.
25 changes: 15 additions & 10 deletions chrome/browser/extensions/back_forward_cache_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,10 @@ class ExtensionBackForwardCacheBrowserTest : public ExtensionBrowserTest {
EXPECT_NE(u"fail", title);
}

content::WebContents* web_contents() const {
return browser()->tab_strip_model()->GetActiveWebContents();
}

protected:
base::HistogramTester histogram_tester_;

Expand Down Expand Up @@ -994,19 +998,20 @@ IN_PROC_BROWSER_TEST_F(ExtensionBackForwardCacheBrowserTest,
content::RenderFrameHostWrapper rfh_a(
ui_test_utils::NavigateToURL(browser(), url_a));

// 2) Navigate to B.
// 2) Navigate to B. Ensure that |rfh_a| is in back/forward cache.
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url_b));

// Expect that `rfh_a` is destroyed as loading page B will causes a storage
// event which is sent to all listeners. Since `rfh_a` is a listener but is in
// the back forward cache it gets evicted.
EXPECT_TRUE(rfh_a.WaitUntilRenderFrameDeleted());

// Validate also that the eviction reason is `kJavascriptExecution` due
// to the extension processing a callback while in the back forward cache.
EXPECT_EQ(1, histogram_tester_.GetBucketCount(
EXPECT_EQ(rfh_a->GetLifecycleState(),
content::RenderFrameHost::LifecycleState::kInBackForwardCache);
// Validate that the eviction due to JavaScript execution has not happened.
EXPECT_EQ(0, histogram_tester_.GetBucketCount(
"BackForwardCache.Eviction.Renderer",
blink::mojom::RendererEvictionReason::kJavaScriptExecution));

// 3) Navigate back to A and make sure that the callback is called after
// restore.
ASSERT_TRUE(content::HistoryGoBack(web_contents()));
EXPECT_EQ("called",
EvalJs(rfh_a.get(), "document.getElementById('callback').value;"));
}

// Test that allows all extensions but disables bfcache in the presence of a few
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
// found in the LICENSE file.

chrome.storage.onChanged.addListener((changes, area) => {
// do nothing
let input = document.createElement("input");
input.id = "callback"
input.value += "called";
document.documentElement.appendChild(input);
});

if (window.location.host.indexOf('b.com') != -1) {
var options = {test: true};
chrome.storage.sync.set({options});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ LoaderFreezeMode ExecutionContext::GetLoaderFreezeMode() const {
return LoaderFreezeMode::kNone;
}

bool ExecutionContext::IsLoadDeferred() const {
bool ExecutionContext::IsContextFrozenOrPaused() const {
return lifecycle_state_ == mojom::blink::FrameLifecycleState::kPaused ||
lifecycle_state_ == mojom::blink::FrameLifecycleState::kFrozen;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ class CORE_EXPORT ExecutionContext : public Supplementable<ExecutionContext>,
mojom::FrameLifecycleState ContextPauseState() const {
return lifecycle_state_;
}
bool IsLoadDeferred() const;
bool IsContextFrozenOrPaused() const;

// Gets the next id in a circular sequence from 1 to 2^31-1.
int CircularSequentialID();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ PausableScriptExecutor::~PausableScriptExecutor() = default;
void PausableScriptExecutor::Run() {
ExecutionContext* context = GetExecutionContext();
DCHECK(context);
if (!context->IsContextPaused()) {
if (!context->IsContextFrozenOrPaused()) {
ExecuteAndDestroySelf();
return;
}
Expand Down

0 comments on commit 22c0064

Please sign in to comment.