Ensure ResourceScheduler starts requests even if the tab is closed.

content/browser/loader/resource_scheduler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <set>
 
 #include "content/browser/loader/resource_scheduler.h"
 
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
@@ skipping 319 matching lines @@
       pending_requests_.Erase(request);
       DCHECK(!ContainsKey(in_flight_requests_, request));
     } else {
       EraseInFlightRequest(request);
 
       // Removing this request may have freed up another to load.
       LoadAnyStartablePendingRequests();
     }
   }
 
-  RequestSet RemoveAllRequests() {
+  RequestSet StartAndRemoveAllRequests() {
+    // First start any pending requests so that they will be moved into
+    // in_flight_requests_. This may exceed the limits
+    // kMaxNumDelayableRequestsPerClient, kMaxNumDelayableRequestsPerHost and
+    // kMaxNumThrottledRequestsPerClient, so this method must not do anything
+    // that depends on those limits before calling ClearInFlightRequests()
+    // below.
+    while (!pending_requests_.IsEmpty()) {
+      ScheduledResourceRequest* request =
+          *pending_requests_.GetNextHighestIterator();
+      pending_requests_.Erase(request);
+      // StartRequest() may modify pending_requests_. TODO(ricea): Does it?
+      StartRequest(request);
+    }
     RequestSet unowned_requests;
     for (RequestSet::iterator it = in_flight_requests_.begin();
          it != in_flight_requests_.end(); ++it) {
       unowned_requests.insert(*it);
       (*it)->set_classification(NORMAL_REQUEST);
     }
     ClearInFlightRequests();
     return unowned_requests;
   }
 
@@ skipping 560 matching lines @@
 
 void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(ContainsKey(client_map_, client_id));
   ClientMap::iterator it = client_map_.find(client_id);
   if (it == client_map_.end())
     return;
 
   Client* client = it->second;
-  // FYI, ResourceDispatcherHost cancels all of the requests after this function
-  // is called. It should end up canceling all of the requests except for a
-  // cross-renderer navigation.
-  RequestSet client_unowned_requests = client->RemoveAllRequests();
+  // ResourceDispatcherHost cancels all requests except for cross-renderer
+  // navigations, async revalidations and detachable requests after
+  // OnClientDeleted() returns.

[mmenke, 2015/06/18 14:49:26]

Wait...So we start a bunch of requests, and then we delete a bunch of them? 
That seems like a bad idea.  If they make it to the socket pools, that results
in either deleting a bunch of idle sockets, or starting a bunch of connection
attempts, before we cancel them.

[Adam Rice, 2015/06/19 13:06:53]

Even without this CL up to 10 requests will be started and then immediately torn
down, so it's not a huge regression in most cases.

I have a couple of other possible approaches in mind. My preferred approach is
to make |unowned_requests_| a type of Client. This causes the throttling logic
to be applied consistently, so closing a tab doesn't cause any new requests to
be started, but they will be started later as higher priority requests finish.
This may cause some visible behaviour changes; currently these requests are
always started immediately, whereas with scheduling applied they might have to
wait for other requests to finish.

The main reason I didn't try this approach is that really the common code should
be factored out of Client as (for example) it is meaningless for
|unowned_requests_| to have a ClientId. The code changes would end up being
quite extensive.

A simpler approach would be to post a task to the message loop, and start any
requests that had not been deleted when the task was run.

[mmenke, 2015/06/19 15:10:16]

You mean in the destructor, when we call UpdateThrottleState(), since
should_throttle() is currently always false?  Or do you mean something else?
We could certainly do that, though doesn't seem lovely, either.  Let me spend a
little time thinking if there's an easy way around this - I'll get back to you
later today.

One other option would be to have the RDH cancel pending requests before it
cancels started ones, and only then call OnClientDeleted, but that also seems a
bit hinky.  Caveats: I'm not sure the RDH currently has access to that
information, and there was one idea at one point to have the scheduler pause
requests before first network access, which this approach would break, unless we
exposed yet more weird info from the ResourceLoader.

[Adam Rice, 2015/06/23 17:06:52]

Sorry, I was confused because throttling is enabled by the test fixture in the
unit tests.

Without throttling the state goes from UNTHROTTLED -> UNTHROTTLED in the
destructor and no jobs are started.

With throttling enabled, the state goes from THROTTLED -> PAUSED in the
destructor and pending jobs are started (because PAUSED is not implemented).

So, this change does in fact cause a regression.
Another option: switch the order of the two lines in
ResourceDispatcherHostImpl::OnRenderViewHostDeleted().

+  RequestSet client_unowned_requests = client->StartAndRemoveAllRequests();
   for (RequestSet::iterator it = client_unowned_requests.begin();
        it != client_unowned_requests.end(); ++it) {
     unowned_requests_.insert(*it);
   }
 
   delete client;
   client_map_.erase(it);
 }
 
 void ResourceScheduler::OnLoadingStateChanged(int child_id,
@@ skipping 213 matching lines @@
   client->ReprioritizeRequest(
       request, old_priority_params, new_priority_params);
 }
 
 ResourceScheduler::ClientId ResourceScheduler::MakeClientId(
     int child_id, int route_id) {
   return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id;
 }
 
 }  // namespace content