Skip to content

Commit

Permalink
Revert "[bfcache] Pause PausableScriptExecutor for frozen context"
Browse files Browse the repository at this point in the history
This reverts commit 22c0064.

Reason for revert: Breaking builds in linux-chromeos-chrome
- https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/26886

Original change's description:
> [bfcache] Pause PausableScriptExecutor for frozen context
>
> 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}

Bug: 1379227
Change-Id: Iea4ac2d21e47313b0a05156f889f54c9ceeda659
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4003758
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Martin Rodriguez <rodmartin@google.com>
Reviewed-by: Mohammed Abdon <mohammedabdon@chromium.org>
Commit-Queue: Renato Silva <rrsilva@google.com>
Owners-Override: Renato Silva <rrsilva@google.com>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067446}
  • Loading branch information
Renato Silva authored and Chromium LUCI CQ committed Nov 4, 2022
1 parent 408c4fb commit 3e288cb
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 23 deletions.
25 changes: 10 additions & 15 deletions chrome/browser/extensions/back_forward_cache_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,6 @@ 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 @@ -998,20 +994,19 @@ IN_PROC_BROWSER_TEST_F(ExtensionBackForwardCacheBrowserTest,
content::RenderFrameHostWrapper rfh_a(
ui_test_utils::NavigateToURL(browser(), url_a));

// 2) Navigate to B. Ensure that |rfh_a| is in back/forward cache.
// 2) Navigate to B.
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url_b));
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(

// 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(
"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,13 +3,10 @@
// found in the LICENSE file.

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

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::IsContextFrozenOrPaused() const {
bool ExecutionContext::IsLoadDeferred() 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 IsContextFrozenOrPaused() const;
bool IsLoadDeferred() 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->IsContextFrozenOrPaused()) {
if (!context->IsContextPaused()) {
ExecuteAndDestroySelf();
return;
}
Expand Down

0 comments on commit 3e288cb

Please sign in to comment.