Prevent imbalanced keepalive counts in ProcessManager

Prevent imbalanced keepalive counts in ProcessManager

URLRequests can be canceled (e.g. on destruction) before
they're ever started, meaning that a NetworkDelegate
may be notified of request completion without ever being
notified of request start.

extensions::ProcessManager was making the incorrect assumption
that this couldn't happen (i.e. that a completion notification
must always follow a corresponding start notification), and was
indiscriminately decrementing keepalive count on all URLRequest
completion notifications.

This CL fixes the glitch.

BUG=535716
R=rdevlin.cronin@chromium.org

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

[Ken Rockot(use gerrit already), 2015-09-27 01:26:46]

Please take a look!

[Devlin, 2015-09-28 16:10:34]

rdevlin.cronin@chromium.org changed reviewers:
+ clamy@chromium.org

[Devlin, 2015-09-28 16:10:34]

+clamy@

Individually, this looks good, as does clamy@'s at
https://codereview.chromium.org/1368383003/.  Together, I don't like them. ;)

I would prefer to have only one of these objects handle the "network request
never started" logic.  If clamy's goes in, that's fine, but then I think the
proper change is to simply (D)CHECK in process manager that the request started.
 Similarly, if this goes in, then clamy's is unnecessary.  Overall, it just
makes the code more straight-forward to understand, and potentially less prone
to future bugs.

FWIW, my (small) preference would be to land clamy's, and have this one just
check validity, but I don't really mind either way.  I just don't want both
solving the same problem. :)

[Ken Rockot(use gerrit already), 2015-09-28 16:44:50]

On 2015/09/28 16:10:34, Devlin wrote:
> +clamy@
> 
> Individually, this looks good, as does clamy@'s at
> https://codereview.chromium.org/1368383003/.  Together, I don't like them. ;)
> 
> I would prefer to have only one of these objects handle the "network request
> never started" logic.  If clamy's goes in, that's fine, but then I think the
> proper change is to simply (D)CHECK in process manager that the request
started.
>  Similarly, if this goes in, then clamy's is unnecessary.  Overall, it just
> makes the code more straight-forward to understand, and potentially less prone
> to future bugs.
> 
> FWIW, my (small) preference would be to land clamy's, and have this one just
> check validity, but I don't really mind either way.  I just don't want both
> solving the same problem. :)

I would agree that clamy's is the better solution here and I lgtm'd it - however
given the recent comment from mmenke it might not be correct yet. Will wait for
further resolution there.

[Ken Rockot(use gerrit already), 2015-09-28 18:15:10]

On 2015/09/28 16:44:50, Ken Rockot wrote:
> On 2015/09/28 16:10:34, Devlin wrote:
> > +clamy@
> > 
> > Individually, this looks good, as does clamy@'s at
> > https://codereview.chromium.org/1368383003/.  Together, I don't like them.
;)
> > 
> > I would prefer to have only one of these objects handle the "network request
> > never started" logic.  If clamy's goes in, that's fine, but then I think the
> > proper change is to simply (D)CHECK in process manager that the request
> started.
> >  Similarly, if this goes in, then clamy's is unnecessary.  Overall, it just
> > makes the code more straight-forward to understand, and potentially less
prone
> > to future bugs.
> > 
> > FWIW, my (small) preference would be to land clamy's, and have this one just
> > check validity, but I don't really mind either way.  I just don't want both
> > solving the same problem. :)
> 
> I would agree that clamy's is the better solution here and I lgtm'd it -
however
> given the recent comment from mmenke it might not be correct yet. Will wait
for
> further resolution there.

Given the discussion on the other CL, I'm inclined to go with this patch
instead.

[Devlin, 2015-09-28 18:21:29]

On 2015/09/28 18:15:10, Ken Rockot wrote:

Okay, sgtm.  Is there a browsertest where we can shove a few extra lines testing
this? (I'm just expecting to call the ProcessManager::OnNetworkRequest* methods
directly, not to try and ensure an edge-case url request.)

[Ken Rockot(use gerrit already), 2015-09-28 21:35:00]

Added a new browser test to cover this. WDYT?

[Devlin, 2015-09-28 21:51:48]

lgtm

https://codereview.chromium.org/1366393002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1366393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:172: EXPECT_EQ(1u,
frames.size());
nitty nit: may as well make this an assert, since you dereference [0] on line
183.

https://codereview.chromium.org/1366393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:190: // in keepalive
decrement.
nit: add
// Regression test for crbug.com/535716.

[Ken Rockot(use gerrit already), 2015-09-28 21:55:21]

https://codereview.chromium.org/1366393002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1366393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:172: EXPECT_EQ(1u,
frames.size());
On 2015/09/28 21:51:48, Devlin wrote:
> nitty nit: may as well make this an assert, since you dereference [0] on line
> 183.

Done.

https://codereview.chromium.org/1366393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:190: // in keepalive
decrement.
On 2015/09/28 21:51:48, Devlin wrote:
> nit: add
> // Regression test for crbug.com/535716.

Done.

[Ken Rockot(use gerrit already), 2015-09-28 22:53:53]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-09-28 22:53:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1366393002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2015-09-28 22:55:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1366393002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1366393002/40001

[commit-bot: I haz the power, 2015-09-28 23:23:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-28 23:24:10]

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