Changed resource scheduler to block until all critical resources actually load.

content/browser/loader/resource_scheduler.cc

Index: content/browser/loader/resource_scheduler.cc
diff --git a/content/browser/loader/resource_scheduler.cc b/content/browser/loader/resource_scheduler.cc
index 85adbdb30e275631b53af88286e75b658e73a16c..c3c813ee919a798dba79ca6dd314ff5a44e432b2 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -128,7 +128,7 @@ class ResourceScheduler::ScheduledResourceRequest
         scheduler_(scheduler),
         priority_(priority),
         fifo_ordering_(0),
-        accounted_as_delayable_request_(false) {
+        category_(NORMAL_REQUEST) {
     TRACE_EVENT_ASYNC_BEGIN1("net", "URLRequest", request_,
                              "url", request->url().spec());
   }
@@ -159,11 +159,11 @@ class ResourceScheduler::ScheduledResourceRequest
   void set_fifo_ordering(uint32 fifo_ordering) {
     fifo_ordering_ = fifo_ordering;
   }
-  bool accounted_as_delayable_request() const {
-    return accounted_as_delayable_request_;
+  RequestCategory category() const {
+    return category_;
   }
-  void set_accounted_as_delayable_request(bool accounted) {
-    accounted_as_delayable_request_ = accounted;
+  void set_category(RequestCategory category) {
+    category_ = category;
   }
 
  private:
@@ -195,11 +195,10 @@ class ResourceScheduler::ScheduledResourceRequest
   net::URLRequest* request_;
   bool ready_;
   bool deferred_;
+  RequestCategory category_;
   ResourceScheduler* scheduler_;
   RequestPriorityParams priority_;
   uint32 fifo_ordering_;
-  // True if the request is delayable in |in_flight_requests_|.
-  bool accounted_as_delayable_request_;
 
   DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest);
 };
@@ -238,6 +237,7 @@ class ResourceScheduler::Client {
         has_body_(false),
         using_spdy_proxy_(false),
         total_delayable_count_(0),
+        total_critical_count_(0),
         throttle_state_(ResourceScheduler::THROTTLED) {
     scheduler_ = scheduler;
   }
@@ -278,7 +278,7 @@ class ResourceScheduler::Client {
     for (RequestSet::iterator it = in_flight_requests_.begin();
          it != in_flight_requests_.end(); ++it) {
       unowned_requests.insert(*it);
-      (*it)->set_accounted_as_delayable_request(false);
+      (*it)->set_category(NORMAL_REQUEST);
     }
     ClearInFlightRequests();
     return unowned_requests;
@@ -374,7 +374,7 @@ class ResourceScheduler::Client {
       DCHECK(ContainsKey(in_flight_requests_, request));
       // The priority and SPDY support may have changed, so update the
       // delayable count.
-      SetRequestDelayable(request, IsDelayableRequest(request));
+      SetRequestCategory(request, GetRequestCategory(request));
       // Request has already started.
       return;
     }
@@ -441,44 +441,56 @@ class ResourceScheduler::Client {
 
   void InsertInFlightRequest(ScheduledResourceRequest* request) {
     in_flight_requests_.insert(request);
-    if (IsDelayableRequest(request))
-      SetRequestDelayable(request, true);
+    SetRequestCategory(request, GetRequestCategory(request));
   }
 
   void EraseInFlightRequest(ScheduledResourceRequest* request) {
     size_t erased = in_flight_requests_.erase(request);
     DCHECK_EQ(1u, erased);
-    SetRequestDelayable(request, false);
+    // Clear any special state that we were tracking for this request.
+    SetRequestCategory(request, NORMAL_REQUEST);
     DCHECK_LE(total_delayable_count_, in_flight_requests_.size());
+    DCHECK_LE(total_critical_count_, in_flight_requests_.size());

[mmenke, 2014/08/13 17:55:11]

Maybe these would get more coverage if at the end of SetRequestCategory instead?
 Could even make these more robust, by actually counting the requests in a
category.  i.e.

DCHECK_EQ(CountRequestsInCategory(DELAYABLE_REQUEST), total_delayable_count_);
and then another one for critical requests as well.

The accounting here isn't that complicated, but I'm paranoid about it
regressing.

[Pat Meenan, 2014/08/13 19:24:55]

Done.

   }
 
   void ClearInFlightRequests() {
     in_flight_requests_.clear();
     total_delayable_count_ = 0;
+    total_critical_count_ = 0;
   }
 
-  bool IsDelayableRequest(ScheduledResourceRequest* request) {
+  void SetRequestCategory(ScheduledResourceRequest* request,
+                          RequestCategory category) {
+    RequestCategory old_category = request->category();
+    if (old_category == category)
+      return;
+
+    if (old_category == DELAYABLE_REQUEST)
+      total_delayable_count_--;
+    if (old_category == CRITICAL_REQUEST)
+      total_critical_count_--;
+
+    if (category == DELAYABLE_REQUEST)
+      total_delayable_count_++;
+    if (category == CRITICAL_REQUEST)
+      total_critical_count_++;
+
+    request->set_category(category);
+  }
+
+  RequestCategory GetRequestCategory(ScheduledResourceRequest* request) {
+    if (!has_body_ && request->url_request()->priority() >= net::LOW)
+      return CRITICAL_REQUEST;

[mmenke, 2014/08/13 17:55:11]

Do critical requests prevent has_body_ from becoming true?  If not, we'll make
them non-critical once has_body_ is true, and they're redirected, since we
re-evaluating their priority on redirect.

[Pat Meenan, 2014/08/13 19:24:55]

No, they do not - good catch.  I changed the logic to not reset existing
critical requests as long as the request priority hasn't dropped to make it a
low (can't think of a case where someone would do that but better safe than
sorry).

     if (request->url_request()->priority() < net::LOW) {
       net::HostPortPair host_port_pair =
           net::HostPortPair::FromURL(request->url_request()->url());
       net::HttpServerProperties& http_server_properties =
           *request->url_request()->context()->http_server_properties();
       if (!http_server_properties.SupportsSpdy(host_port_pair)) {
-        return true;
+        return DELAYABLE_REQUEST;
       }
     }
-    return false;
-  }
-
-  void SetRequestDelayable(ScheduledResourceRequest* request,
-                           bool delayable) {
-    if (request->accounted_as_delayable_request() == delayable)
-      return;
-    if (delayable)
-      total_delayable_count_++;
-    else
-      total_delayable_count_--;
-    request->set_accounted_as_delayable_request(delayable);
+    return NORMAL_REQUEST;
   }
 
   bool ShouldKeepSearching(
@@ -504,7 +516,7 @@ class ResourceScheduler::Client {
 
   // ShouldStartRequest is the main scheduling algorithm.
   //
-  // Requests are categorized into three categories:
+  // Requests are categorized into five categories:
   //
   // 1. Non-delayable requests:
   //   * Synchronous requests.
@@ -515,20 +527,25 @@ class ResourceScheduler::Client {
   // 3. High-priority requests:
   //   * Higher priority requests (>= net::LOW).
   //
-  // 4. Low priority requests
+  // 4. Critical requests:
+  //   * High-priority requests initiated before the renderer has a <body>.

[mmenke, 2014/08/13 17:55:11]

I think it's kinda weird (And very non-intuitive) that "critical" requests can
have lower priority than "high priority" requests.  This can theoretically have
implications for when we start loading delayable requests as well.

[Pat Meenan, 2014/08/13 19:24:55]

Would it be less worrying if I called them render-blocking instead of critical? 
We don't start loading delayable requests until all of the critical requests
have finished (outside of the one-at-a-time gate we allow when we are in the
critical phase).  All high-priority requests started before we hit the body are
critical.

I guess it's theoretically possible for the renderer to give a medium priority
to head resources and then something in the body gets a higher priority which
would cause it to compete with the per-host and connection limits against the
critical request but it won't break anything.  That feels like logic that the
renderer can decide to be smarter about if it really wants to though - I'm not
sure we want to have the extra logic on the net side of the fence.

+  //
+  // 5. Low priority requests
   //
   //  The following rules are followed:
   //
   //  ACTIVE_AND_LOADING and UNTHROTTLED Clients follow these rules:
   //   * Non-delayable, High-priority and SDPY capable requests are issued

[mmenke, 2014/08/13 17:55:11]

nit:  While you're here, SDPY -> SPDY

[Pat Meenan, 2014/08/13 19:24:55]

Done.

-  //     immediately
+  //     immediately.
   //   * If no high priority requests are in flight, start loading low priority
   //     requests.
   //   * Low priority requests are delayable.
-  //   * Once the renderer has a <body>, start loading delayable requests.
+  //   * Allow one delayable request toload at a time while critical requests
+  //     are loading.
+  //   * Once all critical requests have finished loading, start loading
+  //     delayable requests.
   //   * Never exceed 10 delayable requests in flight per client.
   //   * Never exceed 6 delayable requests for a given host.
-  //   * Prior to <body>, allow one delayable request to load at a time.
   //
   //  THROTTLED Clients follow these rules:
   //   * Non-delayable and SPDY-capable requests are issued immediately.
@@ -602,7 +619,8 @@ class ResourceScheduler::Client {
 
     bool have_immediate_requests_in_flight =
         in_flight_requests_.size() > num_delayable_requests_in_flight;
-    if (have_immediate_requests_in_flight && !has_body_ &&
+    if (have_immediate_requests_in_flight &&
+        total_critical_count_ != 0 &&

[mmenke, 2014/08/13 17:55:11]

Shouldn't we check if the current request is critical somewhere?  Or am I
missing something?

[Pat Meenan, 2014/08/13 19:24:55]

I added a comment to the priority >= net::LOW check above which is where
critical requests would get scheduled (basically always requested as long as
they meet the host limit).

[mmenke, 2014/08/15 19:53:40]

Oops - forgot to delete that comment once I figured out what was going on.

         num_delayable_requests_in_flight != 0) {
       return DO_NOT_START_REQUEST_AND_STOP_SEARCHING;
     }
@@ -658,6 +676,8 @@ class ResourceScheduler::Client {
   ResourceScheduler* scheduler_;
   // The number of delayable in-flight requests.
   size_t total_delayable_count_;
+  // The number of critical in-flight requests.
+  size_t total_critical_count_;
   ResourceScheduler::ClientThrottleState throttle_state_;
 };