Skip to content

GitLab

T
tangled
Commit 381033f4 authored by Findit's avatar Findit
Browse files
Options

Revert "[bfcache] Don't cache pages that are still loading." 

This reverts commit fdd7489f.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 680383 as the
culprit for flakes in the build cycles as shown on:
https://analysis.chromium.org/p/chromium/flake-portal/analysis/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vZmRkNzQ4OWYxZjk1YTk4NDM3MWMwM2UxMThmZjE3YTQ2MGM2NTlmOAw

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Mac%20ASan%2064%20Tests%20%281%29/55141

Sample Failed Step: content_browsertests

Sample Flaky Test: BackForwardCacheBrowserTest.LoadingSubframeDoesNotPreventCaching

Original change's description:
> [bfcache] Don't cache pages that are still loading.
> 
> To avoid the complexity of caching and restoring still-loading pages,
> for now simply don't cache pages that haven't finished loading.
> 
> Long term this logic will probably need to be extended to be more
> complicated/permissive, to increase back-forward cache coverage.
> 
> Change-Id: Ic06d9b3d425d3df95b8d09bea1561c000a96acc4
> Bug: 976697
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692414
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
> Reviewed-by: Alexander Timin <altimin@chromium.org>
> Commit-Queue: Lowell Manners <lowell@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#680383}


Change-Id: Iedaebe0c40be92b34e2ad8d6ea650eb06044e2f2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 976697
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715263
Cr-Commit-Position: refs/heads/master@{#680565}
parent 4f6001ec
Hide whitespace changes
Inline Side-by-side
Showing with 1 addition and 101 deletions
content/browser/back_forward_cache_browsertest.cc
View file @ 381033f4
......@@ -4,7 +4,6 @@
#include "base/command_line.h"
#include "base/test/scoped_feature_list.h"
#include "build/build_config.h"
#include "content/browser/frame_host/back_forward_cache.h"
#include "content/browser/frame_host/frame_tree_node.h"
#include "content/browser/web_contents/web_contents_impl.h"
......@@ -17,7 +16,6 @@
#include "content/shell/browser/shell.h"
#include "content/test/content_browser_test_utils_internal.h"
#include "net/dns/mock_host_resolver.h"
#include "net/test/embedded_test_server/controllable_http_response.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "third_party/blink/public/common/scheduler/web_scheduler_tracked_feature.h"
......@@ -646,95 +644,4 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
}
}
IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
DoesNotCacheIfMainFrameStillLoading) {
net::test_server::ControllableHttpResponse response(embedded_test_server(),
"/main_document");
ASSERT_TRUE(embedded_test_server()->Start());
// 1) Navigate to a page that doesn't finish loading.
GURL url(embedded_test_server()->GetURL("a.com", "/main_document"));
TestNavigationManager navigation_manager(shell()->web_contents(), url);
shell()->LoadURL(url);
// The navigation starts.
EXPECT_TRUE(navigation_manager.WaitForRequestStart());
navigation_manager.ResumeNavigation();
// The server sends the first part of the response and waits.
response.WaitForRequest();
response.Send(
"HTTP/1.1 200 OK\r\n"
"Content-Type: text/html; charset=utf-8\r\n"
"\r\n"
"<html><body> ... ");
// The navigation finishes while the body is still loading.
navigation_manager.WaitForNavigationFinished();
RenderFrameDeletedObserver delete_rfh_a(current_frame_host());
// 2) Navigate away.
shell()->LoadURL(embedded_test_server()->GetURL("b.com", "/title1.html"));
// The page was still loading when we navigated away, so it shouldn't have
// been cached.
delete_rfh_a.WaitUntilDeleted();
}
IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
DoesNotCacheIfImageStillLoading) {
ASSERT_TRUE(embedded_test_server()->Start());
// 1) Navigate to a page with an image that loads forever.
GURL url(embedded_test_server()->GetURL("a.com",
"/infinitely_loading_image.html"));
TestNavigationManager navigation_manager(shell()->web_contents(), url);
shell()->LoadURL(url);
// The navigation finishes while the image is still loading.
navigation_manager.WaitForNavigationFinished();
RenderFrameDeletedObserver delete_rfh_a(current_frame_host());
// 2) Navigate away.
shell()->LoadURL(embedded_test_server()->GetURL("b.com", "/title1.html"));
// The page was still loading when we navigated away, so it shouldn't have
// been cached.
delete_rfh_a.WaitUntilDeleted();
}
// Test is flaky on Android (https://crbug.com/986742).
#if defined(OS_ANDROID)
#define MAYBE_LoadingSubframeDoesNotPreventCaching \
DISABLED_LoadingSubframeDoesNotPreventCaching
#else
#define MAYBE_LoadingSubframeDoesNotPreventCaching \
LoadingSubframeDoesNotPreventCaching
#endif
IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
MAYBE_LoadingSubframeDoesNotPreventCaching) {
// Note: This test is only documenting current behavior. Not trying to say it
// should work this way...
ASSERT_TRUE(embedded_test_server()->Start());
// 1) Navigate to a page with an iframe that loads forever.
GURL url(embedded_test_server()->GetURL("a.com",
"/infinitely_loading_iframe.html"));
TestNavigationManager navigation_manager(shell()->web_contents(), url);
shell()->LoadURL(url);
// The navigation finishes while the iframe is still loading.
navigation_manager.WaitForNavigationFinished();
RenderFrameHostImpl* rfh_a = current_frame_host();
RenderFrameDeletedObserver delete_rfh_a(rfh_a);
// 2) Navigate away.
shell()->LoadURL(embedded_test_server()->GetURL("b.com", "/title1.html"));
EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
// The page with the infinitely loading iframe was cached.
EXPECT_FALSE(delete_rfh_a.deleted());
EXPECT_TRUE(rfh_a->is_in_back_forward_cache());
}
} // namespace content
content/browser/frame_host/back_forward_cache.cc
View file @ 381033f4
......@@ -94,13 +94,6 @@ bool BackForwardCache::CanStoreDocument(RenderFrameHostImpl* rfh) {
if (!IsBackForwardCacheEnabled())
return false;
// Note that we check is_loading on the rfh directly, rather than calling
// FrameTreeNode::IsLoading. This is because FrameTreeNode is already
// loading the new page being navigated to.
// TODO(lowell): Consider also checking whether any subframes are loading.
if (rfh->is_loading())
return false;
// Don't enable BackForwardCache if the page has any disallowed features.
// TODO(lowell): Handle races involving scheduler_tracked_features.
// One solution could be to listen for changes to scheduler_tracked_features
......
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