Skip to content

Commit

Permalink
Revert of [Prerender] Restore request priorities when swapped in (pat…
Browse files Browse the repository at this point in the history 
…chset #1 id:1 of https://codereview.chromium.org/2832473002/ )

Reason for revert:
Caused compile failures on Mac and Windows Beta official desktop continuous waterfall.

https://bugs.chromium.org/p/chromium/issues/detail?id=713223

Original issue's description:
> [Prerender] Restore request priorities when swapped in
>
> Requests from prerendered contents have a IDLE priority, in order not to slow
> down visible pages.
> However, when the prerendered contents become visible, the request priorities
> were not reset back to their correct values, leading to bad prerendering
> performance.
>
> In this CL, the priority management for prerender requests is moved to the
> PrerenderResourceThrottle/PrerendereContents.
> The original priorities are stored in the throttle, and restored when the
> prerender contents swaps in.
> A new public function is added to ResourceDispatcherHost to update a request
> priority, and its implementation reuses the existing code that updates the image
> priorities.
>
> The prerender contents keeps a list of all the network resources that were
> started while the prerender is hidden. If this proves to be too large, pruning
> the list when responses are received should be doable.
>
> BUG=705955
>
> Review-Url: https://codereview.chromium.org/2807163002
> Cr-Commit-Position: refs/heads/master@{#464728}
> (cherry picked from commit d3bc614)
>
> Review-Url: https://codereview.chromium.org/2832473002 .
> Cr-Commit-Position: refs/branch-heads/3071@{#48}
> Cr-Branched-From: a106f0a-refs/heads/master@{#464641}
> Committed: https://chromium.googlesource.com/chromium/src/+/d4b0c9f58ec981f98342cb8438da1a69bb489a06

[email protected]
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=705955

Review-Url: https://codereview.chromium.org/2833553002
Cr-Commit-Position: refs/branch-heads/3071@{#58}
Cr-Branched-From: a106f0a-refs/heads/master@{#464641}
  • Loading branch information
@ojanvafai
ojanvafai authored and Commit bot committed Apr 19, 2017
1 parent a3ba228 commit e8cd36e
Show file tree
Hide file tree
Showing 14 changed files with 85 additions and 294 deletions.
21 changes: 21 additions & 0 deletions chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,27 @@ void ChromeResourceDispatcherHostDelegate::RequestBeginning(
safe_browsing_->OnResourceRequest(request);

const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);

// The lowering of request priority causes issues with scheduling, since
// content::ResourceScheduler uses it to delay and throttle requests. This is
// disabled only on Android, as the prerenders are not likely to compete with
// page loads there.
// See https://crbug.com/652746 for details.
// TODO(lizeb,droger): Fix the issue on all platforms.
#if !defined(OS_ANDROID)
bool is_prerendering =
info->GetVisibilityState() == blink::kWebPageVisibilityStatePrerender;
if (is_prerendering) {
// Requests with the IGNORE_LIMITS flag set (i.e., sync XHRs)
// should remain at MAXIMUM_PRIORITY.
if (request->load_flags() & net::LOAD_IGNORE_LIMITS) {
DCHECK_EQ(request->priority(), net::MAXIMUM_PRIORITY);
} else {
request->SetPriority(net::IDLE);
}
}
#endif // OS_ANDROID

ProfileIOData* io_data = ProfileIOData::FromResourceContext(
resource_context);

Expand Down
181 changes: 30 additions & 151 deletions chrome/browser/prerender/prerender_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ using content::WebContentsObserver;
using net::NetworkChangeNotifier;
using prerender::test_utils::RequestCounter;
using prerender::test_utils::CreateCountingInterceptorOnIO;
using prerender::test_utils::CreateHangingFirstRequestInterceptor;
using prerender::test_utils::CreateHangingFirstRequestInterceptorOnIO;
using prerender::test_utils::CreateMockInterceptorOnIO;
using prerender::test_utils::TestPrerender;
using prerender::test_utils::TestPrerenderContents;
Expand All @@ -166,8 +166,6 @@ namespace prerender {

namespace {

const char kPrefetchJpeg[] = "/prerender/image.jpeg";

class FaviconUpdateWatcher : public favicon::FaviconDriverObserver {
public:
explicit FaviconUpdateWatcher(content::WebContents* web_contents)
Expand Down Expand Up @@ -541,17 +539,6 @@ page_load_metrics::PageLoadExtraInfo GenericPageLoadExtraInfo(
dest_url, false /* started_in_foreground */);
}

// Helper function, to allow passing a UI closure to
// CreateHangingFirstRequestInterceptor() instead of a IO callback.
base::Callback<void(net::URLRequest*)> GetIOCallbackFromUIClosure(
base::Closure ui_closure) {
auto lambda = [](base::Closure closure, net::URLRequest*) {
content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
closure);
};
return base::Bind(lambda, ui_closure);
}

} // namespace

class PrerenderBrowserTest : public test_utils::PrerenderInProcessBrowserTest {
Expand Down Expand Up @@ -1315,9 +1302,10 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, MAYBE_PrerenderNoCommitNoSwap) {
base::FilePath file(GetTestPath("prerender_page.html"));

base::RunLoop prerender_start_loop;
CreateHangingFirstRequestInterceptor(
kNoCommitUrl, file,
GetIOCallbackFromUIClosure(prerender_start_loop.QuitClosure()));
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&CreateHangingFirstRequestInterceptorOnIO, kNoCommitUrl, file,
prerender_start_loop.QuitClosure()));
DisableJavascriptCalls();
PrerenderTestURL(kNoCommitUrl,
FINAL_STATUS_NAVIGATION_UNCOMMITTED,
Expand All @@ -1342,9 +1330,10 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, MAYBE_PrerenderNoCommitNoSwap2) {
base::FilePath file(GetTestPath("prerender_page.html"));

base::RunLoop prerender_start_loop;
CreateHangingFirstRequestInterceptor(
kNoCommitUrl, file,
GetIOCallbackFromUIClosure(prerender_start_loop.QuitClosure()));
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&CreateHangingFirstRequestInterceptorOnIO, kNoCommitUrl, file,
prerender_start_loop.QuitClosure()));
DisableJavascriptCalls();
PrerenderTestURL(CreateClientRedirect(kNoCommitUrl.spec()),
FINAL_STATUS_APP_TERMINATING, 1);
Expand Down Expand Up @@ -2120,7 +2109,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderImagePng) {
// Checks that prerendering a JPG works correctly.
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderImageJpeg) {
DisableJavascriptCalls();
PrerenderTestURL(kPrefetchJpeg, FINAL_STATUS_USED, 1);
PrerenderTestURL("/prerender/image.jpeg", FINAL_STATUS_USED, 1);
NavigateToDestURL();
}

Expand Down Expand Up @@ -2192,7 +2181,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderSSLErrorSubresource) {
https_server.SetSSLConfig(net::EmbeddedTestServer::CERT_MISMATCHED_NAME);
https_server.ServeFilesFromSourceDirectory("chrome/test/data");
ASSERT_TRUE(https_server.Start());
GURL https_url = https_server.GetURL(kPrefetchJpeg);
GURL https_url = https_server.GetURL("/prerender/image.jpeg");
base::StringPairs replacement_text;
replacement_text.push_back(
std::make_pair("REPLACE_WITH_IMAGE_URL", https_url.spec()));
Expand Down Expand Up @@ -2304,7 +2293,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
https_server.SetSSLConfig(net::EmbeddedTestServer::CERT_OK, ssl_config);
https_server.ServeFilesFromSourceDirectory("chrome/test/data");
ASSERT_TRUE(https_server.Start());
GURL https_url = https_server.GetURL(kPrefetchJpeg);
GURL https_url = https_server.GetURL("/prerender/image.jpeg");
base::StringPairs replacement_text;
replacement_text.push_back(
std::make_pair("REPLACE_WITH_IMAGE_URL", https_url.spec()));
Expand Down Expand Up @@ -2378,7 +2367,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,

// Ensures that we do not prerender pages which have a malware subresource.
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderSafeBrowsingSubresource) {
GURL image_url = embedded_test_server()->GetURL(kPrefetchJpeg);
GURL image_url = embedded_test_server()->GetURL("/prerender/image.jpeg");
GetFakeSafeBrowsingDatabaseManager()->SetThreatTypeForUrl(
image_url, safe_browsing::SB_THREAT_TYPE_URL_MALWARE);
base::StringPairs replacement_text;
Expand Down Expand Up @@ -2502,8 +2491,11 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderHangingUnload) {
const GURL hang_url("http://unload-url.test");
base::FilePath empty_file = ui_test_utils::GetTestFilePath(
base::FilePath(), base::FilePath(FILE_PATH_LITERAL("empty.html")));
CreateHangingFirstRequestInterceptor(
hang_url, empty_file, base::Callback<void(net::URLRequest*)>());
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&CreateHangingFirstRequestInterceptorOnIO,
hang_url, empty_file,
base::Closure()));

set_loader_path("/prerender/prerender_loader_with_unload.html");
PrerenderTestURL("/prerender/prerender_page.html", FINAL_STATUS_USED, 1);
Expand Down Expand Up @@ -3299,122 +3291,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, AutosigninInPrerenderer) {
EXPECT_EQ(0, done_counter.count());
}

// Checks that the requests from a prerender are IDLE priority before the swap
// (except on Android), but normal priority after the swap.
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, ResourcePriority) {
GURL before_swap_url = embedded_test_server()->GetURL(kPrefetchJpeg);
GURL after_swap_url = embedded_test_server()->GetURL("/prerender/image.png");
GURL main_page_url =
GetURLWithReplacement("/prerender/prerender_with_image.html",
"REPLACE_WITH_IMAGE_URL", kPrefetchJpeg);

// Setup request interceptors for subresources.
auto get_priority_lambda = [](net::RequestPriority* out_priority,
net::URLRequest* request) {
*out_priority = request->priority();
};
RequestCounter before_swap_counter;
net::RequestPriority before_swap_priority = net::THROTTLED;
InterceptRequestAndCount(
before_swap_url, &before_swap_counter,
base::Bind(get_priority_lambda, base::Unretained(&before_swap_priority)));
RequestCounter after_swap_counter;
net::RequestPriority after_swap_priority = net::THROTTLED;
InterceptRequestAndCount(
after_swap_url, &after_swap_counter,
base::Bind(get_priority_lambda, base::Unretained(&after_swap_priority)));

// Start the prerender.
PrerenderTestURL(main_page_url, FINAL_STATUS_USED, 1);

// Check priority before swap.
before_swap_counter.WaitForCount(1);
#if defined(OS_ANDROID)
EXPECT_GT(before_swap_priority, net::IDLE);
#else
EXPECT_EQ(net::IDLE, before_swap_priority);
#endif

// Swap.
NavigateToDestURL();

// Check priority after swap.
GetActiveWebContents()->GetMainFrame()->ExecuteJavaScriptForTests(
base::ASCIIToUTF16(
"var img=new Image(); img.src='/prerender/image.png'"));
after_swap_counter.WaitForCount(1);
EXPECT_NE(net::IDLE, after_swap_priority);
}

// Checks that a request started before the swap gets its original priority back
// after the swap.
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, ResourcePriorityOverlappingSwap) {
GURL image_url = embedded_test_server()->GetURL(kPrefetchJpeg);
GURL main_page_url =
GetURLWithReplacement("/prerender/prerender_with_image.html",
"REPLACE_WITH_IMAGE_URL", kPrefetchJpeg);

// Setup request interceptors for subresources.
net::URLRequest* url_request = nullptr;
net::RequestPriority priority = net::THROTTLED;
base::RunLoop wait_loop;
auto io_lambda = [](net::URLRequest** out_request,
net::RequestPriority* out_priority, base::Closure closure,
net::URLRequest* request) {
if (out_request)
*out_request = request;
content::BrowserThread::PostTask(
content::BrowserThread::UI, FROM_HERE,
base::Bind(
[](net::RequestPriority priority,
net::RequestPriority* out_priority, base::Closure closure) {
*out_priority = priority;
closure.Run();
},
request->priority(), base::Unretained(out_priority), closure));
};

CreateHangingFirstRequestInterceptor(
image_url, base::FilePath(),
base::Bind(io_lambda, base::Unretained(&url_request),
base::Unretained(&priority), wait_loop.QuitClosure()));

// The prerender will hang on the image resource, can't run the usual checks.
DisableLoadEventCheck();
DisableJavascriptCalls();
// Start the prerender.
PrerenderTestURL(main_page_url, FINAL_STATUS_USED, 0);

// Check priority before swap.
#if defined(OS_ANDROID)
if (priority <= net::IDLE)
wait_loop.Run();
EXPECT_GT(priority, net::IDLE);
#else
if (priority != net::IDLE)
wait_loop.Run();
EXPECT_EQ(net::IDLE, priority);
#endif

// Swap. Cannot use NavigateToDestURL, because it waits for the load to
// complete, but the resource is still hung.
current_browser()->OpenURL(content::OpenURLParams(
dest_url(), Referrer(), WindowOpenDisposition::CURRENT_TAB,
ui::PAGE_TRANSITION_TYPED, false));

// Check priority after swap. The test may timeout in case of failure.
priority = net::THROTTLED;
do {
base::RunLoop loop;
content::BrowserThread::PostTask(
content::BrowserThread::IO, FROM_HERE,
base::Bind(io_lambda, nullptr, base::Unretained(&priority),
loop.QuitClosure(), base::Unretained(url_request)));
loop.Run();
} while (priority <= net::IDLE);
EXPECT_GT(priority, net::IDLE);
}

IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, FirstContentfulPaintTimingSimple) {
GetPrerenderManager()->DisablePageLoadMetricsObserverForTesting();
base::SimpleTestTickClock* clock = OverridePrerenderManagerTimeTicks();
Expand Down Expand Up @@ -3447,9 +3323,10 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, FirstContentfulPaintTimingReuse) {

GURL url = embedded_test_server()->GetURL("/prerender/prerender_page.html");
base::RunLoop hanging_request_waiter;
CreateHangingFirstRequestInterceptor(
url, GetTestPath("prerender_page.html"),
GetIOCallbackFromUIClosure(hanging_request_waiter.QuitClosure()));
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
base::Bind(&CreateHangingFirstRequestInterceptorOnIO,
url, GetTestPath("prerender_page.html"),
hanging_request_waiter.QuitClosure()));
// As this load will be canceled, it is not waited for, and hence no
// javascript is executed.
DisableJavascriptCalls();
Expand Down Expand Up @@ -3534,9 +3411,10 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
base::FilePath(FILE_PATH_LITERAL("prerender/prerender_page.html")));

base::RunLoop prerender_start_loop;
CreateHangingFirstRequestInterceptor(
url, url_file,
GetIOCallbackFromUIClosure(prerender_start_loop.QuitClosure()));
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&CreateHangingFirstRequestInterceptorOnIO, url, url_file,
prerender_start_loop.QuitClosure()));
// As this load is uncommitted, it is not waited for, and hence no
// javascript is executed.
DisableJavascriptCalls();
Expand Down Expand Up @@ -3653,9 +3531,10 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
base::FilePath(FILE_PATH_LITERAL("prerender/prerender_page.html")));

base::RunLoop prerender_start_loop;
CreateHangingFirstRequestInterceptor(
url, url_file,
GetIOCallbackFromUIClosure(prerender_start_loop.QuitClosure()));
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&CreateHangingFirstRequestInterceptorOnIO, url, url_file,
prerender_start_loop.QuitClosure()));
// As this load is uncommitted, it is not waited for, and hence no
// javascript is executed.
DisableJavascriptCalls();
Expand Down
18 changes: 4 additions & 14 deletions chrome/browser/prerender/prerender_contents.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,7 @@ const char* const kValidHttpMethodsForPrerendering[] = {
};

void ResumeThrottles(
std::vector<base::WeakPtr<PrerenderResourceThrottle>> throttles,
std::vector<base::WeakPtr<PrerenderResourceThrottle>> idle_resources) {
for (auto resource : idle_resources) {
if (resource)
resource->ResetResourcePriority();
}
std::vector<base::WeakPtr<PrerenderResourceThrottle> > throttles) {
for (size_t i = 0; i < throttles.size(); i++) {
if (throttles[i])
throttles[i]->ResumeHandler();
Expand Down Expand Up @@ -762,10 +757,10 @@ void PrerenderContents::PrepareForUse() {
NotifyPrerenderStop();

BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&ResumeThrottles, resource_throttles_, idle_resources_));
BrowserThread::IO,
FROM_HERE,
base::Bind(&ResumeThrottles, resource_throttles_));
resource_throttles_.clear();
idle_resources_.clear();
}

void PrerenderContents::CancelPrerenderForPrinting() {
Expand All @@ -783,11 +778,6 @@ void PrerenderContents::AddResourceThrottle(
resource_throttles_.push_back(throttle);
}

void PrerenderContents::AddIdleResource(
const base::WeakPtr<PrerenderResourceThrottle>& throttle) {
idle_resources_.push_back(throttle);
}

void PrerenderContents::AddNetworkBytes(int64_t bytes) {
network_bytes_ += bytes;
for (Observer& observer : observer_list_)
Expand Down
8 changes: 0 additions & 8 deletions chrome/browser/prerender/prerender_contents.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,12 +235,6 @@ class PrerenderContents : public content::NotificationObserver,
void AddResourceThrottle(
const base::WeakPtr<PrerenderResourceThrottle>& throttle);

// Called when a PrerenderResourceThrottle changes a resource priority to
// net::IDLE. The resources are reset back to their original priorities when
// the prerender contents is swapped in.
void AddIdleResource(
const base::WeakPtr<PrerenderResourceThrottle>& throttle);

// Increments the number of bytes fetched over the network for this prerender.
void AddNetworkBytes(int64_t bytes);

Expand Down Expand Up @@ -378,8 +372,6 @@ class PrerenderContents : public content::NotificationObserver,
// Resources that are throttled, pending a prerender use. Can only access a
// throttle on the IO thread.
std::vector<base::WeakPtr<PrerenderResourceThrottle> > resource_throttles_;
// Resources for which the priority was lowered to net::IDLE.
std::vector<base::WeakPtr<PrerenderResourceThrottle>> idle_resources_;

// A running tally of the number of bytes this prerender has caused to be
// transferred over the network for resources. Updated with AddNetworkBytes.
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,10 @@ IN_PROC_BROWSER_TEST_F(NoStatePrefetchBrowserTest, PrefetchSimultaneous) {
base::FilePath first_path = ui_test_utils::GetTestFilePath(
base::FilePath(), base::FilePath().AppendASCII(kPrefetchPage));

test_utils::CreateHangingFirstRequestInterceptor(
first_url, first_path, base::Callback<void(net::URLRequest*)>());
content::BrowserThread::PostTask(
content::BrowserThread::IO, FROM_HERE,
base::Bind(&test_utils::CreateHangingFirstRequestInterceptorOnIO,
first_url, first_path, base::Closure()));

// Start the first prefetch directly instead of via PrefetchFromFile for the
// first prefetch to avoid the wait on prerender stop.
Expand Down
Loading

0 comments on commit e8cd36e

Please sign in to comment.