Fixed resource scheduler to not treat non-blocking js as render-blocking

Prior to https://codereview.chromium.org/462813002 the resource scheduler would exit the critical loading phase as soon as the main parser parsed the body tag: - Blocking JS in the head would keep it in the critical phase - CSS in the head would not (this is the bug that was fixed) - Non-blocking JS or JS discovered by the preload scanner would NOT keep it in the critical phase. After the fix: - Blocking JS in the head still kept it in the critical phase - CSS in the head kept it in the critical phase (fix worked) - Any JS that was started before the body tag was parsed would keep it in the critical phase until that JS finished loading. This inclues non-blocking JS (script-injected while in the head) as well as JS discovered by the preload scanner. This change keeps the CSS fix but also restores the JS behavior so that non-blocking or preload-scanned JS no longer keep the loader in the critical resources phase. The fix was really simple since CSS loads at a higher priority than JS (net::Medium), I just changed the logic to only count Medium+ as render-blocking (waiting for them to finish regardless of the state of the body tag) and returned the logic that unblocks non-render-blocking resources as soon as the body tag was parsed (which now includes JS). Any blocking JS still keeps the main parser blocked so only non-blocking and preloaded JS would be pending or loading when the body tag is parsed. You can see it working here: http://www.webpagetest.org/video/compare.php?tests=141016_WH_e488a05d7b224d7fc362dfa91b7c5ec6,141016_H1_2ecb3d397b3c218ff26dc7bdb9cb0858 Request #13 is a non-blocking script that in teh baseline case keeps images from being loaded but in the fixed case no longer blocks images. The canonical test page for the CSS bug also shows the critical phase still being honored when only css is parsed in the head: http://www.webpagetest.org/video/compare.php?tests=141016_VM_1d25c3ff22f132b4ded97d7d72e5e558,141016_PM_17a2f0282c39f299425ba57c5c5118bf BUG=423853 Review URL: https://codereview.chromium.org/658163002 Cr-Commit-Position: refs/heads/master@{#299959}

Fixed resource scheduler to not treat non-blocking js as render-blocking
Prior to https://codereview.chromium.org/462813002 the resource scheduler would exit the critical loading phase as soon as the main parser parsed the body tag: - Blocking JS in the head would keep it in the critical phase - CSS in the head would not (this is the bug that was fixed) - Non-blocking JS or JS discovered by the preload scanner would NOT keep it in the critical phase. After the fix: - Blocking JS in the head still kept it in the critical phase - CSS in the head kept it in the critical phase (fix worked) - Any JS that was started before the body tag was parsed would keep it in the critical phase until that JS finished loading. This inclues non-blocking JS (script-injected while in the head) as well as JS discovered by the preload scanner. This change keeps the CSS fix but also restores the JS behavior so that non-blocking or preload-scanned JS no longer keep the loader in the critical resources phase. The fix was really simple since CSS loads at a higher priority than JS (net::Medium), I just changed the logic to only count Medium+ as render-blocking (waiting for them to finish regardless of the state of the body tag) and returned the logic that unblocks non-render-blocking resources as soon as the body tag was parsed (which now includes JS). Any blocking JS still keeps the main parser blocked so only non-blocking and preloaded JS would be pending or loading when the body tag is parsed. You can see it working here: http://www.webpagetest.org/video/compare.php?tests=141016_WH_e488a05d7b224d7fc362dfa91b7c5ec6,141016_H1_2ecb3d397b3c218ff26dc7bdb9cb0858 Request #13 is a non-blocking script that in teh baseline case keeps images from being loaded but in the fixed case no longer blocks images. The canonical test page for the CSS bug also shows the critical phase still being honored when only css is parsed in the head: http://www.webpagetest.org/video/compare.php?tests=141016_VM_1d25c3ff22f132b4ded97d7d72e5e558,141016_PM_17a2f0282c39f299425ba57c5c5118bf BUG=423853 Review URL: https://codereview.chromium.org/658163002 Cr-Commit-Position: refs/heads/master@{#299959}
8a7d3a2d · pmeenan · Commit bot · 84afadbb · 8a7d3a2d · 8a7d3a2d
Commit 8a7d3a2d authored Oct 16, 2014 by pmeenan Committed by Commit bot Oct 16, 2014
Showing with 18 additions and 5 deletions

content/browser/loader/resource_scheduler.cc content/browser/loader/resource_scheduler.cc +6 -5

content/browser/loader/resource_scheduler_unittest.cc content/browser/loader/resource_scheduler_unittest.cc +12 -0

No files found.
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -511,11 +511,11 @@ class ResourceScheduler::Client {
    // If a request is already marked as layout-blocking make sure to keep the
    // classification across redirects unless the priority was lowered.
    if (request->classification() == LAYOUT_BLOCKING_REQUEST &&
-        request->url_request()->priority() >= net::LOW) {
+        request->url_request()->priority() > net::LOW) {
      return LAYOUT_BLOCKING_REQUEST;
    }
-    if (!has_body_ && request->url_request()->priority() >= net::LOW)
+    if (!has_body_ && request->url_request()->priority() > net::LOW)
      return LAYOUT_BLOCKING_REQUEST;
    if (request->url_request()->priority() < net::LOW) {
@@ -566,7 +566,8 @@ class ResourceScheduler::Client {
  //   * Higher priority requests (>= net::LOW).
  //
  // 4. Layout-blocking requests:
-  //   * High-priority requests initiated before the renderer has a <body>.
+  //   * High-priority requests (> net::LOW) initiated before the renderer has
+  //     a <body>.
  //
  // 5. Low priority requests
  //
@@ -577,7 +578,7 @@ class ResourceScheduler::Client {
  //     immediately.
  //   * Low priority requests are delayable.
  //   * Allow one delayable request to load at a time while layout-blocking
-  //     requests are loading.
+  //     requests are loading or the body tag has not yet been parsed.
  //   * If no high priority or layout-blocking requests are in flight, start
  //     loading delayable requests.
  //   * Never exceed 10 delayable requests in flight per client.
@@ -656,7 +657,7 @@ class ResourceScheduler::Client {
    bool have_immediate_requests_in_flight =
        in_flight_requests_.size() > in_flight_delayable_count_;
    if (have_immediate_requests_in_flight &&
-        total_layout_blocking_count_ != 0 &&
+        (!has_body_ || total_layout_blocking_count_ != 0) &&
        in_flight_delayable_count_ != 0) {
      return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
    }

--- a/content/browser/loader/resource_scheduler_unittest.cc
+++ b/content/browser/loader/resource_scheduler_unittest.cc
@@ -327,6 +327,18 @@ TEST_F(ResourceSchedulerTest, OneLowLoadsUntilCriticalComplete) {
  EXPECT_TRUE(low2->started());
 }
+TEST_F(ResourceSchedulerTest, LowDoesNotBlockCriticalComplete) {
+  scoped_ptr<TestRequest> low(NewRequest("http://host/low", net::LOW));
+  scoped_ptr<TestRequest> lowest(NewRequest("http://host/lowest", net::LOWEST));
+  scoped_ptr<TestRequest> lowest2(
+      NewRequest("http://host/lowest", net::LOWEST));
+  EXPECT_TRUE(low->started());
+  EXPECT_TRUE(lowest->started());
+  EXPECT_FALSE(lowest2->started());
+  scheduler_.OnWillInsertBody(kChildId, kRouteId);
+  EXPECT_TRUE(lowest2->started());
+}
 TEST_F(ResourceSchedulerTest, OneLowLoadsUntilBodyInsertedExceptSpdy) {
  http_server_properties_.SetSupportsSpdy(
      net::HostPortPair("spdyhost", 443), true);