[Prerender] Restore request priorities when swapped in

chrome/browser/prerender/prerender_browsertest.cc

Index: chrome/browser/prerender/prerender_browsertest.cc
diff --git a/chrome/browser/prerender/prerender_browsertest.cc b/chrome/browser/prerender/prerender_browsertest.cc
index 22e9b036545db0ec7b7e7eec594f89866049b808..0413c51a8c71ce946f6060ecb82385ea1179e5da 100644
--- a/chrome/browser/prerender/prerender_browsertest.cc
+++ b/chrome/browser/prerender/prerender_browsertest.cc
@@ -143,7 +143,7 @@ using content::WebContentsObserver;
 using net::NetworkChangeNotifier;
 using prerender::test_utils::RequestCounter;
 using prerender::test_utils::CreateCountingInterceptorOnIO;
-using prerender::test_utils::CreateHangingFirstRequestInterceptorOnIO;
+using prerender::test_utils::CreateHangingFirstRequestInterceptor;
 using prerender::test_utils::CreateMockInterceptorOnIO;
 using prerender::test_utils::TestPrerender;
 using prerender::test_utils::TestPrerenderContents;
@@ -539,6 +539,17 @@ 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 {
@@ -1302,10 +1313,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, MAYBE_PrerenderNoCommitNoSwap) {
   base::FilePath file(GetTestPath("prerender_page.html"));
 
   base::RunLoop prerender_start_loop;
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&CreateHangingFirstRequestInterceptorOnIO, kNoCommitUrl, file,
-                 prerender_start_loop.QuitClosure()));
+  CreateHangingFirstRequestInterceptor(
+      kNoCommitUrl, file,
+      GetIOCallbackFromUIClosure(prerender_start_loop.QuitClosure()));
   DisableJavascriptCalls();
   PrerenderTestURL(kNoCommitUrl,
                    FINAL_STATUS_NAVIGATION_UNCOMMITTED,
@@ -1330,10 +1340,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, MAYBE_PrerenderNoCommitNoSwap2) {
   base::FilePath file(GetTestPath("prerender_page.html"));
 
   base::RunLoop prerender_start_loop;
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&CreateHangingFirstRequestInterceptorOnIO, kNoCommitUrl, file,
-                 prerender_start_loop.QuitClosure()));
+  CreateHangingFirstRequestInterceptor(
+      kNoCommitUrl, file,
+      GetIOCallbackFromUIClosure(prerender_start_loop.QuitClosure()));
   DisableJavascriptCalls();
   PrerenderTestURL(CreateClientRedirect(kNoCommitUrl.spec()),
                    FINAL_STATUS_APP_TERMINATING, 1);
@@ -2491,11 +2500,8 @@ 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")));
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&CreateHangingFirstRequestInterceptorOnIO,
-                 hang_url, empty_file,
-                 base::Closure()));
+  CreateHangingFirstRequestInterceptor(
+      hang_url, empty_file, base::Callback<void(net::URLRequest*)>());
 
   set_loader_path("/prerender/prerender_loader_with_unload.html");
   PrerenderTestURL("/prerender/prerender_page.html", FINAL_STATUS_USED, 1);
@@ -3291,6 +3297,130 @@ 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("/prerender/image.jpeg");
+  GURL after_swap_url = embedded_test_server()->GetURL("/prerender/image.png");

[mattcary, 2017/04/14 09:28:46]

nit: if this gets in after cl/2819523002 (which is likely as it's being
submitted now) then there is a new GetURLWithReplacement utility function to use
instead of doing all the replacement text stuff yourself.

[droger, 2017/04/14 11:38:36]

Done.

+  base::StringPairs replacement_text;
+  replacement_text.push_back(
+      std::make_pair("REPLACE_WITH_IMAGE_URL", before_swap_url.spec()));
+  std::string replacement_path;
+  net::test_server::GetFilePathWithReplacements(
+      "/prerender/prerender_with_image.html", replacement_text,
+      &replacement_path);
+
+  // 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, &before_swap_priority));

[mattcary, 2017/04/14 09:28:46]

So before_swap_priority doesn't need to be base::Unretained because it's a basic
type and not a class? The documentation doesn't seem to be clear; I would have
thought that it would need to be unretained regardless.

[droger, 2017/04/14 11:38:36]

Done.

Unretained is often not used in these cases, but probably mostly because:

- Unretained actally does nothing, as opposed to other helpers such as Owned().

- the compiler will only enforce it for classes (and only for the callee object,
it will not even complain for the function parameters).

If you do:
Bind(&MyClass::MyFunction, my_instance, my_param), compilation will fail if you
omit the Unretained(my_instance). It does not check my_param though, and you can
get away without the Unretained there.


I'm not completely sure what is the best though. I made the change because I
could find a couple examples in the codebase, so it's probably a valid thing.

+  RequestCounter after_swap_counter;
+  net::RequestPriority after_swap_priority = net::THROTTLED;
+  InterceptRequestAndCount(
+      after_swap_url, &after_swap_counter,
+      base::Bind(get_priority_lambda, &after_swap_priority));
+
+  // Start the prerender.
+  PrerenderTestURL(replacement_path, FINAL_STATUS_USED, 1);
+
+  // Check priority before swap.
+  before_swap_counter.WaitForCount(1);
+#if defined(OS_ANDROID)
+  EXPECT_NE(net::IDLE, before_swap_priority);

[mattcary, 2017/04/14 09:28:46]

why not kDefaultImagePriority, as below?

[droger, 2017/04/14 11:38:36]

Done.

+#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("/prerender/image.jpeg");
+  base::StringPairs replacement_text;
+  replacement_text.push_back(
+      std::make_pair("REPLACE_WITH_IMAGE_URL", image_url.spec()));
+  std::string replacement_path;
+  net::test_server::GetFilePathWithReplacements(
+      "/prerender/prerender_with_image.html", replacement_text,
+      &replacement_path);
+
+  // Setup request interceptors for subresources.
+  net::URLRequest* url_request = nullptr;
+  net::RequestPriority priority = net::THROTTLED;
+  base::RunLoop wait_loop;
+  const net::RequestPriority kDefaultImagePriority = net::MEDIUM;
+  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(), out_priority, closure));
+  };
+
+  CreateHangingFirstRequestInterceptor(
+      image_url, base::FilePath(),
+      base::Bind(io_lambda, &url_request, &priority, wait_loop.QuitClosure()));
+
+  // The prerender will hang on the image resource, can't run the usual checks.
+  DisableLoadEventCheck();
+  DisableJavascriptCalls();
+  // Start the prerender.
+  PrerenderTestURL(replacement_path, FINAL_STATUS_USED, 0);
+
+  // Check priority before swap.
+  net::RequestPriority expected_priority =
+#if defined(OS_ANDROID)
+      kDefaultImagePriority;
+#else
+      net::IDLE;
+#endif
+  if (priority != expected_priority)
+    wait_loop.Run();
+  EXPECT_EQ(expected_priority, priority);
+
+  // 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.

[mattcary, 2017/04/14 09:28:46]

Clarify that the test will timeout if the priority isn't changed as expected.

[droger, 2017/04/14 11:38:36]

Done, and changed the test a bit so that it should be harder to timeout. It will
now timeout if the priority is never updated. No longer timeout if the priority
is updated to a wrong value.

+  priority = net::THROTTLED;
+  do {
+    base::RunLoop loop;
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(io_lambda, nullptr, &priority, loop.QuitClosure(),
+                   url_request));
+    loop.Run();
+  } while (priority != kDefaultImagePriority);
+}
+
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, FirstContentfulPaintTimingSimple) {
   GetPrerenderManager()->DisablePageLoadMetricsObserverForTesting();
   base::SimpleTestTickClock* clock = OverridePrerenderManagerTimeTicks();
@@ -3323,10 +3453,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, FirstContentfulPaintTimingReuse) {
 
   GURL url = embedded_test_server()->GetURL("/prerender/prerender_page.html");
   base::RunLoop hanging_request_waiter;
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                          base::Bind(&CreateHangingFirstRequestInterceptorOnIO,
-                                     url, GetTestPath("prerender_page.html"),
-                                     hanging_request_waiter.QuitClosure()));
+  CreateHangingFirstRequestInterceptor(
+      url, GetTestPath("prerender_page.html"),
+      GetIOCallbackFromUIClosure(hanging_request_waiter.QuitClosure()));
   // As this load will be canceled, it is not waited for, and hence no
   // javascript is executed.
   DisableJavascriptCalls();
@@ -3411,10 +3540,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
       base::FilePath(FILE_PATH_LITERAL("prerender/prerender_page.html")));
 
   base::RunLoop prerender_start_loop;
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&CreateHangingFirstRequestInterceptorOnIO, url, url_file,
-                 prerender_start_loop.QuitClosure()));
+  CreateHangingFirstRequestInterceptor(
+      url, url_file,
+      GetIOCallbackFromUIClosure(prerender_start_loop.QuitClosure()));
   // As this load is uncommitted, it is not waited for, and hence no
   // javascript is executed.
   DisableJavascriptCalls();
@@ -3531,10 +3659,9 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
       base::FilePath(FILE_PATH_LITERAL("prerender/prerender_page.html")));
 
   base::RunLoop prerender_start_loop;
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&CreateHangingFirstRequestInterceptorOnIO, url, url_file,
-                 prerender_start_loop.QuitClosure()));
+  CreateHangingFirstRequestInterceptor(
+      url, url_file,
+      GetIOCallbackFromUIClosure(prerender_start_loop.QuitClosure()));
   // As this load is uncommitted, it is not waited for, and hence no
   // javascript is executed.
   DisableJavascriptCalls();