Fix outstanding request stats for multiple transfer navigations.

content/browser/loader/resource_dispatcher_host_impl.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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <algorithm>
 #include <set>
@@ skipping 986 matching lines @@
     const linked_ptr<ResourceLoader>& loader) {
   ResourceRequestInfoImpl* info = loader->GetRequestInfo();
   GlobalRoutingID old_routing_id(
       request_data.transferred_request_child_id, info->GetRouteID());
   GlobalRequestID old_request_id(request_data.transferred_request_child_id,
                                  request_data.transferred_request_request_id);
   GlobalRoutingID new_routing_id(child_id, route_id);
   GlobalRequestID new_request_id(child_id, request_id);
 
   // Clear out data that depends on |info| before updating it.
-  IncrementOutstandingRequestsMemory(-1, *info);
-  OustandingRequestsStats empty_stats = { 0, 0 };
-  OustandingRequestsStats old_stats = GetOutstandingRequestsStats(*info);
-  UpdateOutstandingRequestsStats(*info, empty_stats);
+  // We always need to move the memory stats to the new process.  In contrast,
+  // stats.num_requests is only sometimes tracked.
+  OustandingRequestsStats stats = IncrementOutstandingRequestsMemory(-1, *info);
+  bool should_update_count = stats.num_requests > 0;

[davidben, 2014/10/13 17:40:04]

Hrm. Is this the right criteria? OutstandingRequestsStats are tracked on a
per-child, so this is true if ARH has ever called
IncrementOutstandingRequestsCount on any response. Whereas this wants to check
if this particular request has ever seen an IncrementOutstandingRequestsCount.

Perhaps the ResourceRequestInfoImpl shouldn't be passed as const reference, so
you can set a flag there.

[Charlie Reis, 2014/10/14 23:54:12]

Oops-- you're totally right.  Awkward that we don't always increment the count,
but after a little digging I see that there's a reason.
Yeah...  I'm not thrilled with modifying the request info this way, but it does
seem like the most straightforward way to have both process-level counts and
know whether a particular request is affected.  New patch uploaded.

+  if (should_update_count)
+    IncrementOutstandingRequestsCount(-1, *info);
   pending_loaders_.erase(old_request_id);
 
   // ResourceHandlers should always get state related to the request from the
   // ResourceRequestInfo rather than caching it locally.  This lets us update
   // the info object when a transfer occurs.
   info->UpdateForTransfer(child_id, route_id, request_data.origin_pid,
                           request_id, request_data.parent_render_frame_id,
                           filter_->GetWeakPtr());
 
   // Update maps that used the old IDs, if necessary.  Some transfers in tests
   // do not actually use a different ID, so not all maps need to be updated.
   pending_loaders_[new_request_id] = loader;
-  UpdateOutstandingRequestsStats(*info, old_stats);
   IncrementOutstandingRequestsMemory(1, *info);
+  if (should_update_count)
+    IncrementOutstandingRequestsCount(1, *info);
   if (old_routing_id != new_routing_id) {
     if (blocked_loaders_map_.find(old_routing_id) !=
             blocked_loaders_map_.end()) {
       blocked_loaders_map_[new_routing_id] =
           blocked_loaders_map_[old_routing_id];
       blocked_loaders_map_.erase(old_routing_id);
     }
   }
   if (old_request_id != new_request_id) {
     DelegateMap::iterator it = delegate_map_.find(old_request_id);
@@ skipping 1114 matching lines @@
 
   // Add a flag to selectively bypass the data reduction proxy if the resource
   // type is not an image.
   if (request_data.resource_type != RESOURCE_TYPE_IMAGE)
     load_flags |= net::LOAD_BYPASS_DATA_REDUCTION_PROXY;
 
   return load_flags;
 }
 
 }  // namespace content