Add KeepaliveImpulse to extension process manager.

Add KeepaliveImpulse to extension process manager.

Impulses are an efficient way to handle certain types
of activity from extensions. E.g. the work in progress
to keep NaCl plugins in background pages alive. An
Increment / Decrement approach would require extensive
and fragile instrumentation of all APIs. Impulses
can be sent upon any api activity and act as an
implicit increment with a timed out decrement.

See design doc:
https://docs.google.com/a/chromium.org/document/d/1mI0lS1rfAf-BAGLmWAEcWy37Xq9dOvgfMx8OqeUMXts/edit#

BUG=298339
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238261

[Matt Perry, 2013-11-19 21:35:07]

lgtm

https://codereview.chromium.org/60613004/diff/20004/extensions/browser/proces...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/60613004/diff/20004/extensions/browser/proces...
extensions/browser/process_manager.cc:388: int
ProcessManager::DecrementLazyKeepaliveCountById(std::string extension_id) {
nit: I think this is similar enough to DecrementLazyKeepaliveCount to qualify
for an exception to the function override rule, and could have the same name.

https://codereview.chromium.org/60613004/diff/20004/extensions/browser/proces...
extensions/browser/process_manager.cc:460: FROM_HERE,
nit: indent to 4

[scheib, 2013-11-19 23:18:25]

Thanks, comments addressed.

Hmm, lots of try job failures with e.g.
 ExtensionNotificationRegistration
 ProcessGrouping
 EventPage
timing out or crashing. I'm a bit stumped, but will dwell on it. Any insight?

https://codereview.chromium.org/60613004/diff/20004/extensions/browser/proces...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/60613004/diff/20004/extensions/browser/proces...
extensions/browser/process_manager.cc:388: int
ProcessManager::DecrementLazyKeepaliveCountById(std::string extension_id) {
On 2013/11/19 21:35:08, Matt Perry wrote:
> nit: I think this is similar enough to DecrementLazyKeepaliveCount to qualify
> for an exception to the function override rule, and could have the same name.

Done.

https://codereview.chromium.org/60613004/diff/20004/extensions/browser/proces...
extensions/browser/process_manager.cc:460: FROM_HERE,
On 2013/11/19 21:35:08, Matt Perry wrote:
> nit: indent to 4

Done.

[Matt Perry, 2013-11-19 23:31:38]

The unit_test failures look like maybe MessageLoop::current() is NULL. I don't
know why the browser_tests would be timing out.

[scheib, 2013-11-21 21:17:17]

Matt, please take a look at my change to test specified idle & suspend times
being ms instead of seconds. The issue with browser tests was posting a task
with no delay. I've changed all tests to use a non zero idle timer. However 1
second wasn't really necessary, so I moved them all to 1 ms.

[scheib, 2013-11-28 00:17:50]

I've set the test timeout and suspend values back to 1 second. There are tests
that depend on a longer time out - and I'll leave that out of this change.

[scheib, 2013-11-28 00:23:00]

tapted, please take a look (and for owners coverage). This expanded when I
updated the test timeout units to milliseconds, you're an owner for
chrome/browser/apps/app_browsertest_util.cc

[tapted, 2013-11-28 03:03:36]

apps/app_browsertest_util.cc lgtm

I wanted to make sure I understood what was going on though -- combined with the
LazyBackgroundPageIdle sequence_id stuff, it's all pretty complex! Some comments
inline that might help general readability for others who aren't as close to
this lifetime stuff, but it's mostly just me 'thinking aloud'.

I think part of me is sad that Chrome will ping the CPU every 10 seconds, even
with no renderers, but there's probably already precedent for that, and this one
seems pretty minor.

Also depending how `KeepaliveImpulse` is exposed, there might be scope for
abuse/misuse, but that's another CL.

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.cc:211: CHECK(idle_time_msec > 0);  //
OnKeepaliveImpulseCheck requires non zero.
optional nit: maybe CHECK_NE(0u, idle_time_msec)? (can't really argue that
outputting the value is handy though, since it would always be zero).

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.cc:438: bool this_impulse =
i->second.keepalive_impulse;
(optional) This might be easier to read without the temporaries (I don't think
the order matters). i.e.

if (i->second.previous_keepalive_impulse && !i->second.keepalive_impulse)
  DecrementLazyKeepaliveCount(i->first);

i->second.previous_keepalive_impulse = i->second.keepalive_impulse;
i->second.keepalive_impulse = false;


.. it still feels tricky though. I think I understood it better after realising
the sequence doesn't really matter. e.g. You could have

bool BackgroundPageData::HasKeepalive() const {
  return keepalive_impulse || previous_keepalive_impulse;
}

Then here you could go

for (..) {
  if (!i->second.HasKeepalive())
    continue;

  i->second.previous_keepalive_impulse = i->second.keepalive_impulse;
  i->second.keepalive_impulse = false;
  if (!i->second.HasKeepalive())
    DecrementLazyKeepaliveCount(i->first);
}

and in KeepaliveImpulse() do

if (!bd.HasKeepalive())
  IncrementLazyKeepaliveCount(extension);
bd.keepalive_impulse = true;


Maybe HasKeepalive -> IsImpulseIdle to get rid of the inversions.

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.cc:444:
DecrementLazyKeepaliveCount(i->first);
I thought it weird that the return value wasn't checked... but it looks like
nothing uses the return value - maybe the functions should just return void?

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.cc:447: if (base::MessageLoop::current()) {
Maybe a comment here to explain this check (e.g. Should
DecrementLazyKeepaliveCount() be checking too?)

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
File extensions/browser/process_manager.h (right):

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.h:174: int
DecrementLazyKeepaliveCount(std::string extension_id);
nit: const std::string& extension_id - seems more common in this file

[tapted, 2013-11-28 05:05:24]

oops something else I noticed -- The comments in extensions/common/switches.cc
still say "time in seconds.." - they should be updated too.

[scheib, 2013-12-02 21:22:37]

Thanks, comments addressed.

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.cc:211: CHECK(idle_time_msec > 0);  //
OnKeepaliveImpulseCheck requires non zero.
On 2013/11/28 03:03:37, tapted wrote:
> optional nit: maybe CHECK_NE(0u, idle_time_msec)? (can't really argue that
> outputting the value is handy though, since it would always be zero).

I think this check output is pretty clear as is with
":FATAL:process_manager.cc(212)] Check failed: idle_time_msec > 0"

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.cc:438: bool this_impulse =
i->second.keepalive_impulse;
On 2013/11/28 03:03:37, tapted wrote:
> (optional) This might be easier to read without the temporaries (I don't think
> the order matters). i.e.
> 
> if (i->second.previous_keepalive_impulse && !i->second.keepalive_impulse)
>   DecrementLazyKeepaliveCount(i->first);
> 
> i->second.previous_keepalive_impulse = i->second.keepalive_impulse;
> i->second.keepalive_impulse = false;
> 
> 
> .. it still feels tricky though. I think I understood it better after
realising
> the sequence doesn't really matter. e.g. You could have
> 
> bool BackgroundPageData::HasKeepalive() const {
>   return keepalive_impulse || previous_keepalive_impulse;
> }
> 
> Then here you could go
> 
> for (..) {
>   if (!i->second.HasKeepalive())
>     continue;
> 
>   i->second.previous_keepalive_impulse = i->second.keepalive_impulse;
>   i->second.keepalive_impulse = false;
>   if (!i->second.HasKeepalive())
>     DecrementLazyKeepaliveCount(i->first);
> }
> 
> and in KeepaliveImpulse() do
> 
> if (!bd.HasKeepalive())
>   IncrementLazyKeepaliveCount(extension);
> bd.keepalive_impulse = true;
> 
> 
> Maybe HasKeepalive -> IsImpulseIdle to get rid of the inversions.

Done; your first suggestion. The latter didn't reuse much code and I think the
balance is too far into indirection.

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.cc:444:
DecrementLazyKeepaliveCount(i->first);
On 2013/11/28 03:03:37, tapted wrote:
> I thought it weird that the return value wasn't checked... but it looks like
> nothing uses the return value - maybe the functions should just return void?

Done.

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.cc:447: if (base::MessageLoop::current()) {
On 2013/11/28 03:03:37, tapted wrote:
> Maybe a comment here to explain this check (e.g. Should
> DecrementLazyKeepaliveCount() be checking too?)

Done.

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
File extensions/browser/process_manager.h (right):

https://codereview.chromium.org/60613004/diff/740001/extensions/browser/proce...
extensions/browser/process_manager.h:174: int
DecrementLazyKeepaliveCount(std::string extension_id);
On 2013/11/28 03:03:37, tapted wrote:
> nit: const std::string& extension_id - seems more common in this file

Done.

https://codereview.chromium.org/60613004/diff/760001/extensions/common/switch...
File extensions/common/switches.cc (right):

https://codereview.chromium.org/60613004/diff/760001/extensions/common/switch...
extensions/common/switches.cc:37: // The time in milliseconds that an extension
event page can be idle before it
Done.

[commit-bot: I haz the power, 2013-12-02 21:24:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/60613004/760001

[commit-bot: I haz the power, 2013-12-03 01:25:34]

Change committed as 238261