Replace Increment/DecrementKeepAliveCount by ScopedKeepAlives

Replace Increment/DecrementKeepAliveCount by ScopedKeepAlives

It allows to remove those methods from application_lifetime.h
and makes the KeepAliveRegistry move from KeepAlive increments
to ModuleRef increments to keep the browser alive.

BUG=587926
Committed: https://crrev.com/023777892ab897fc228dc74d5d2d16786c49d3fb
Cr-Commit-Position: refs/heads/master@{#380812}

[dgn, 2016-03-09 23:49:22]

dgn@chromium.org changed reviewers:
+ sky@chromium.org

[dgn, 2016-03-09 23:49:23]

PTAL

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager_unittest.cc (left):

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/background/b...
chrome/browser/background/background_mode_manager_unittest.cc:249:
chrome::IncrementKeepAliveCount();
This is not needed anymore because the reset() call is specific to a given
keep_alive, so it would not decrement the global count.

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (left):

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_sanity_browsertest.cc:774:
chrome::IncrementKeepAliveCount();
TODO: have a better look at this, which was added to unflake the test
(crbug.com/333554, 323847). Not sure which decrement call it pairs with.

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/lifetime/app...
File chrome/browser/lifetime/application_lifetime.cc (left):

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/lifetime/app...
chrome/browser/lifetime/application_lifetime.cc:279: // Increment the browser
process refcount as long as we're keeping the
moved to KeepAliveRegistry::OnKeepingAliveChanged

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/lifetime/app...
chrome/browser/lifetime/application_lifetime.cc:300: // Although we should have
a browser process, if there is none,
moved to KeepAliveRegistry::OnKeepingAliveChanged

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/lifetime/app...
File chrome/browser/lifetime/application_lifetime_aura.cc (left):

https://codereview.chromium.org/1778873002/diff/1/chrome/browser/lifetime/app...
chrome/browser/lifetime/application_lifetime_aura.cc:44:
DecrementKeepAliveCount();
Paired with the call in ash_init.cc

[sky, 2016-03-10 04:18:20]

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager_unittest.cc (left):

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager_unittest.cc:249:
chrome::IncrementKeepAliveCount();
Why don't you need this anymore?

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (left):

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_sanity_browsertest.cc:774:
chrome::IncrementKeepAliveCount();
Why is this no longer necessary?

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/lifetime...
File chrome/browser/lifetime/application_lifetime_aura.cc (right):

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime_aura.cc:45:
g_browser_process->platform_part()->UnregisterKeepAlive();
Seems wrong that this has to reach out to other code to cancel a keep alive.

https://codereview.chromium.org/1778873002/diff/80001/chrome/test/base/testin...
File chrome/test/base/testing_browser_process.h (right):

https://codereview.chromium.org/1778873002/diff/80001/chrome/test/base/testin...
chrome/test/base/testing_browser_process.h:144: int GetModuleRefCount() const;
nit: inline this and return same type as member.

[dgn, 2016-03-10 18:31:52]

PTAL

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager_unittest.cc (left):

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager_unittest.cc:249:
chrome::IncrementKeepAliveCount();
On 2016/03/10 04:18:20, sky wrote:
> Why don't you need this anymore?

As explained by the comment, the count is decremented, that happens in
application_lifetime_aura.cc. Resetting an null keep_alive does nothing so I can
just skip it.

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_sanity_browsertest.cc (left):

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_sanity_browsertest.cc:774:
chrome::IncrementKeepAliveCount();
On 2016/03/10 04:18:20, sky wrote:
> Why is this no longer necessary?

That was added to unflake the test a long time ago.
(crbug.com/333554, 323847). I can't find which call it's supposed to pair with.
We currently didn't do anything when the count is in excess, so it might be
superfluous now. I'll investigate some more if I see some new flakes.

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/lifetime...
File chrome/browser/lifetime/application_lifetime_aura.cc (right):

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime_aura.cc:45:
g_browser_process->platform_part()->UnregisterKeepAlive();
On 2016/03/10 04:18:20, sky wrote:
> Seems wrong that this has to reach out to other code to cancel a keep alive.

Hum... We are stateless here, while we need something to hold the reference to
the KeepAlive. The lifetime of that one is around the one of the whole browser
and it's chromeos specific, so it seemed to me like a reasonable place. I'm not
very familiar with the architecture though. 

I moved it to application_lifetime_aura.cc in the new patch. Is it better?
That's a global static so that might cause some issues in tests.

https://codereview.chromium.org/1778873002/diff/80001/chrome/test/base/testin...
File chrome/test/base/testing_browser_process.h (right):

https://codereview.chromium.org/1778873002/diff/80001/chrome/test/base/testin...
chrome/test/base/testing_browser_process.h:144: int GetModuleRefCount() const;
On 2016/03/10 04:18:20, sky wrote:
> nit: inline this and return same type as member.

Done.

[sky, 2016-03-10 20:38:36]

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/lifetime...
File chrome/browser/lifetime/application_lifetime_aura.cc (right):

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime_aura.cc:45:
g_browser_process->platform_part()->UnregisterKeepAlive();
On 2016/03/10 18:31:51, dgn wrote:
> On 2016/03/10 04:18:20, sky wrote:
> > Seems wrong that this has to reach out to other code to cancel a keep alive.
> 
> Hum... We are stateless here, while we need something to hold the reference to
> the KeepAlive. The lifetime of that one is around the one of the whole browser
> and it's chromeos specific, so it seemed to me like a reasonable place. I'm
not
> very familiar with the architecture though. 
> 
> I moved it to application_lifetime_aura.cc in the new patch. Is it better?
> That's a global static so that might cause some issues in tests.

I realize why you did the change. I was more commenting it that it's unfortunate
the code is like this. I prefer the previous approach. That at least has better
scoping.

[dgn, 2016-03-11 17:11:49]

PTAL

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/lifetime...
File chrome/browser/lifetime/application_lifetime_aura.cc (right):

https://codereview.chromium.org/1778873002/diff/80001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime_aura.cc:45:
g_browser_process->platform_part()->UnregisterKeepAlive();
On 2016/03/10 20:38:36, sky wrote:
> On 2016/03/10 18:31:51, dgn wrote:
> > On 2016/03/10 04:18:20, sky wrote:
> > > Seems wrong that this has to reach out to other code to cancel a keep
alive.
> > 
> > Hum... We are stateless here, while we need something to hold the reference
to
> > the KeepAlive. The lifetime of that one is around the one of the whole
browser
> > and it's chromeos specific, so it seemed to me like a reasonable place. I'm
> not
> > very familiar with the architecture though. 
> > 
> > I moved it to application_lifetime_aura.cc in the new patch. Is it better?
> > That's a global static so that might cause some issues in tests.
> 
> I realize why you did the change. I was more commenting it that it's
unfortunate
> the code is like this. I prefer the previous approach. That at least has
better
> scoping.

Ah ok. I reverted that part then.

[dgn, 2016-03-11 18:21:21]

I made the code stricter by enforcing that all keep alive should be cleared when
exiting, which was not done before. Right now I seem to be failing on ChromeOS
browser tests using the  kForceLoginManagerInTests switch, and for some mac
tests. Investigating. Maybe I'll just drop the DCHECK in the end or make it
conditional to some flag if it's test specific and not easily fixable.

[dgn, 2016-03-11 22:15:04]

PTAL.

Tests don't wait for chrome to properly shutdown, meaning that the ones
involving things with a long lifetime and that don't make sure to shut them down
crash during the tear down when hitting the DCHECK.

It definitely happens with the login manager, which is used in many chromeos
tests (30 failures in the latest run). 

I'm not sure about the cause of the failure on mac since I can't test it on my
workstation, but I assume it's the same situation, but with the app_controller.

Refactoring those tests to make them use a base class that properly shuts down
everything would fix it, but those are situations that should not happen outside
of tests, and that were not tested for before. So I'd just remove the DCHECKs
that I added in the previous CL, everything should be passing now.

https://codereview.chromium.org/1778873002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/login_browsertest.cc (right):

https://codereview.chromium.org/1778873002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/login_browsertest.cc:81:
LoginDisplayHost::default_host()->Finalize();
This is what would need to be added in all the places using the pair of switches
above.

https://codereview.chromium.org/1778873002/diff/140001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1778873002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:43: // DCHECK_EQ(0,
registered_count_) << " Registry: " << *this;
I had to remove those DCHECKs to pass browser_tests.

[sky, 2016-03-11 23:15:10]

LGTM

[dgn, 2016-03-11 23:23:59]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-03-11 23:24:15]

On 2016/03/11 23:15:10, sky wrote:
> LGTM

Thanks! Have a good week end!

[commit-bot: I haz the power, 2016-03-11 23:24:23]

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

[commit-bot: I haz the power, 2016-03-12 00:58:48]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-12 01:00:40]

Description was changed from

==========
Replace Increment/DecrementKeepAliveCount by ScopedKeepAlives

It allows to remove those methods from application_lifetime.h
and makes the KeepAliveRegistry move from KeepAlive increments
to ModuleRef increments to keep the browser alive.

BUG=587926
==========

to

==========
Replace Increment/DecrementKeepAliveCount by ScopedKeepAlives

It allows to remove those methods from application_lifetime.h
and makes the KeepAliveRegistry move from KeepAlive increments
to ModuleRef increments to keep the browser alive.

BUG=587926

Committed: https://crrev.com/023777892ab897fc228dc74d5d2d16786c49d3fb
Cr-Commit-Position: refs/heads/master@{#380812}
==========

[commit-bot: I haz the power, 2016-03-12 01:00:41]

Patchset 9 (id:??) landed as
https://crrev.com/023777892ab897fc228dc74d5d2d16786c49d3fb
Cr-Commit-Position: refs/heads/master@{#380812}