Fix outstanding request stats for multiple transfer navigations.

Fix outstanding request stats for multiple transfer navigations.

The request stats are kept per process, and we were incorrectly transferring
the entire process's stats when only a single request was being transferred.

BUG=423570
TEST=Visit a page with 2 cross-site iframes using --site-per-process.

Committed: https://crrev.com/e27ab4e207348d7829ee0ce7400f1d3129046441
Cr-Commit-Position: refs/heads/master@{#299769}

[Charlie Reis, 2014-10-13 16:06:00]

creis@chromium.org changed reviewers:
+ davidben@chromium.org

[Charlie Reis, 2014-10-13 16:06:01]

David, would you have a chance to look over this?  We were doing the wrong
accounting if two requests were active in the same process during a transfer. 
It's tricky because the count is only sometimes updated (via
AsyncResourceHandler).

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

https://codereview.chromium.org/648803002/diff/130004/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/648803002/diff/130004/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1010: bool
should_update_count = stats.num_requests > 0;
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:53:12]

Patchset #4 (id:350001) has been deleted

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

Thanks for catching that!

https://codereview.chromium.org/648803002/diff/130004/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/648803002/diff/130004/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1010: bool
should_update_count = stats.num_requests > 0;
On 2014/10/13 17:40:04, David Benjamin wrote:
> 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.

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.

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

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.

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (left):

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:252: bool
HasSufficientResourcesForRequest(const net::URLRequest* request_);
No idea how |request_| slipped in as a parameter name.

[davidben, 2014-10-15 19:15:12]

lgtm with a couple comments.

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1734:
ResourceRequestInfoImpl& info) {
Nit: probably should be passed in as pointer.

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:347: // shared memory
buffers are issued or finished. |count| indicates whether the
First sentence should probably say:

  Called when an flight request allocates or releases a shared memory buffer.

It's not allocated when the request is issued. Just when we first need the
buffer in ARH::OnWillRead. (I guess another option would be to count it ahead of
time because the RDH knows which requests end at ARHs and which end at different
leaf handlers. The current logic delays until we start reading the response
body.)

[Charlie Reis, 2014-10-15 19:48:00]

Thanks!

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1734:
ResourceRequestInfoImpl& info) {
On 2014/10/15 19:15:12, David Benjamin wrote:
> Nit: probably should be passed in as pointer.

Yep.

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/648803002/diff/370001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:347: // shared memory
buffers are issued or finished. |count| indicates whether the
On 2014/10/15 19:15:12, David Benjamin wrote:
> First sentence should probably say:
> 
>   Called when an flight request allocates or releases a shared memory buffer.
> 
> It's not allocated when the request is issued. Just when we first need the
> buffer in ARH::OnWillRead. (I guess another option would be to count it ahead
of
> time because the RDH knows which requests end at ARHs and which end at
different
> leaf handlers. The current logic delays until we start reading the response
> body.)

Done.

[Charlie Reis, 2014-10-15 19:48:05]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-10-15 19:48:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/648803002/390001

[commit-bot: I haz the power, 2014-10-15 21:24:40]

Committed patchset #5 (id:390001)

[commit-bot: I haz the power, 2014-10-15 21:25:36]

Patchset 5 (id:??) landed as
https://crrev.com/e27ab4e207348d7829ee0ce7400f1d3129046441
Cr-Commit-Position: refs/heads/master@{#299769}