Introduce runtime.onSuspendCanceled() event.

Introduce runtime.onSuspendCanceled() event.

runtime.onSuspendCanceled() is sent after runtime.onSuspend() to indicate that
the extension / app will not be suspended after all.


BUG=136469
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148490

[benwells, 2012-07-19 04:01:21]

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_event_router.cc:460: }
Nit: blank line after early return.

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_event_router.h:227: bool IsClosing(Profile*
profile, const extensions::Extension* extension);
Could we rename this to MightBeClosing, MaybeClosing, FlaggedToClose... or
something to indicate it might not be closed?

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_process_manager.cc:118: bool is_closing;
Can we change this name too?

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_process_manager.cc:509: sequence_id ==
background_page_data_[extension_id].close_sequence_id) {
Any change to sequence_id will cancel the unload. 

Is this the right set of events to cancel unload? I don't know what changes
sequence_id but it is probably more than an event being sent to the event page

A related question: can the unload be cancelled without the onSuspendCanceled
event being sent? It should not be possible.

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/runtime.json (right):

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/common/extensio...
chrome/common/extensions/api/runtime.json:120: "description": "Sent to the event
page just before it is unloaded. This gives the extension opportunity to do some
clean up. Note that since the page is unloading, any asynchronous operations
started while handling this event are not guaranteed to complete. If more
activity for the event occurs before it gets unloaded the onSuspendCanceled
event will be sent and the page won't be unloaded. "
...more activity for the event *page* occurs...

[Aaron Boodman, 2012-07-19 04:19:48]

Can someone explain what issue you're trying to address here? This seems sadly
complicated.

[koz (OOO until 15th September), 2012-07-19 04:25:40]

On 2012/07/19 04:19:48, Aaron Boodman wrote:
> Can someone explain what issue you're trying to address here? This seems sadly
> complicated.

In a nutshell: if you give an extension five seconds to clean up before
unloading it, then any events that arrive during that time will have a five
second delay in being handled. See discussion towards the end of this bug:
http://crbug.com/133296

[koz (OOO until 15th September), 2012-07-19 06:52:11]

Cheers, Ben.

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_event_router.cc:460: }
On 2012/07/19 04:01:21, benwells wrote:
> Nit: blank line after early return.

Done.

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_event_router.h:227: bool IsClosing(Profile*
profile, const extensions::Extension* extension);
On 2012/07/19 04:01:21, benwells wrote:
> Could we rename this to MightBeClosing, MaybeClosing, FlaggedToClose... or
> something to indicate it might not be closed?

Hm, I don't think IsClosing() is such a bad name. The fact that it's
IsClosing(), not IsClosed() implies that it is in the process of closing, and
that it's cancelable is not something we need to document in the name of the
function.

In other words, closing is the five second process, not the thing that happens
instantaneously after a delay.

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_process_manager.cc:118: bool is_closing;
On 2012/07/19 04:01:21, benwells wrote:
> Can we change this name too? 

See previous comment.

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_process_manager.cc:509: sequence_id ==
background_page_data_[extension_id].close_sequence_id) {
On 2012/07/19 04:01:21, benwells wrote:
> Any change to sequence_id will cancel the unload. 
> 
> Is this the right set of events to cancel unload? I don't know what changes
> sequence_id but it is probably more than an event being sent to the event page
> 
> A related question: can the unload be cancelled without the onSuspendCanceled
> event being sent? It should not be possible.

Good point. I've changed it so that only dispatching an event can cause
onSuspendCancel().

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/runtime.json (right):

https://chromiumcodereview.appspot.com/10804020/diff/1/chrome/common/extensio...
chrome/common/extensions/api/runtime.json:120: "description": "Sent to the event
page just before it is unloaded. This gives the extension opportunity to do some
clean up. Note that since the page is unloading, any asynchronous operations
started while handling this event are not guaranteed to complete. If more
activity for the event occurs before it gets unloaded the onSuspendCanceled
event will be sent and the page won't be unloaded. "
On 2012/07/19 04:01:21, benwells wrote:
> ...more activity for the event *page* occurs...

Done.

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:439:
CancelSuspend(listener_profile, extension);
mpcomplete@: I'm assuming that this is the only place I need to cancel
suspension here, because this is the only place where IncrementInFlightEvents()
is called in this class, ie: any other events that get dispatched through this
class aren't participating in this keepalive scheme and so wouldn't cause an
onSuspendCancel(). Is that a valid assumption?

[Matt Perry, 2012-07-19 21:36:22]

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:439:
CancelSuspend(listener_profile, extension);
On 2012/07/19 06:52:12, koz wrote:
> mpcomplete@: I'm assuming that this is the only place I need to cancel
> suspension here, because this is the only place where
IncrementInFlightEvents()
> is called in this class, ie: any other events that get dispatched through this
> class aren't participating in this keepalive scheme and so wouldn't cause an
> onSuspendCancel(). Is that a valid assumption?

It depends on what we want to cancel a suspend. Just events? Then this is good.
Should we include a message sent by a content script (probably)? Then you need
to add it to the MessageService. Should we include a user inspecting the event
page (probably)?

The central location for all of these is
LazyBackgroundTaskQueue::AddPendingTask. I haven't vetted every call site, so
I'm not sure if we want to include everything as something that can cancel a
suspend event.

Things that definitely should *not* cancel a suspend (but would otherwise modify
the keepalive count) are things like any API calls or network requests made by
the extension.

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
chrome/browser/extensions/extension_process_manager.cc:464: is_closing = false;
Could you add a DCHECK that the lazy_keepalive_count is > 0 here? We should
enforce that the caller increments the keepalive count before cancelling the
suspend.

[koz (OOO until 15th September), 2012-07-20 06:08:08]

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:439:
CancelSuspend(listener_profile, extension);
On 2012/07/19 21:36:22, Matt Perry wrote:
> On 2012/07/19 06:52:12, koz wrote:
> > mpcomplete@: I'm assuming that this is the only place I need to cancel
> > suspension here, because this is the only place where
> IncrementInFlightEvents()
> > is called in this class, ie: any other events that get dispatched through
this
> > class aren't participating in this keepalive scheme and so wouldn't cause an
> > onSuspendCancel(). Is that a valid assumption?
> 
> It depends on what we want to cancel a suspend. Just events? Then this is
good.
> Should we include a message sent by a content script (probably)? Then you need
> to add it to the MessageService. Should we include a user inspecting the event
> page (probably)?
> 
> The central location for all of these is
> LazyBackgroundTaskQueue::AddPendingTask. I haven't vetted every call site, so
> I'm not sure if we want to include everything as something that can cancel a
> suspend event.
> 
> Things that definitely should *not* cancel a suspend (but would otherwise
modify
> the keepalive count) are things like any API calls or network requests made by
> the extension.

My reading of ExtensionMessageService indicates that creating a channel
increments the lazy keepalive count, but sending messages through a channel
doesn't; they don't need to because the channel they're going over is already
pinning the background page as active. So I've made the calls there to
IncrementLazyKeepaliveCount() be of the "suspend canceling" variety, which
should give us the right behaviour.

As for the dev tools, it seems that that logic is handled in
ExtensionProcessManager's Observe() function, so to make the dev tools behave
correctly here we just need to make their Increment a suspend canceling one,
too.

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
chrome/browser/extensions/extension_process_manager.cc:464: is_closing = false;
On 2012/07/19 21:36:22, Matt Perry wrote:
> Could you add a DCHECK that the lazy_keepalive_count is > 0 here? We should
> enforce that the caller increments the keepalive count before cancelling the
> suspend.

That's a good idea. In fact, it's probably a good idea to unify these two
methods, seeing as they need to be called in tandem. I've added an optional
cancel_suspend argument to IncrementLazyKeepaliveCount() that defaults to false.

[benwells, 2012-07-20 10:57:00]

https://chromiumcodereview.appspot.com/10804020/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/5003/chrome/browser/exte...
chrome/browser/extensions/extension_process_manager.cc:429: new
ExtensionMsg_CancelUnload(extension->id()));
Nit: braces around the if body as there is more than one physical line.

https://chromiumcodereview.appspot.com/10804020/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/extension_process_manager.h (right):

https://chromiumcodereview.appspot.com/10804020/diff/5003/chrome/browser/exte...
chrome/browser/extensions/extension_process_manager.h:116: // The
|cancel_suspend| option defaults to false and indicates whether
I'd prefer |cancel_suspend| to be a non-optional enum.

Enum makes call sites easier to understand. Making this parameter optional is in
effect creating a default parameter value, which is against the google c++ style
guide.

[Matt Perry, 2012-07-20 18:04:17]

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:439:
CancelSuspend(listener_profile, extension);
On 2012/07/20 06:08:09, koz wrote:
> On 2012/07/19 21:36:22, Matt Perry wrote:
> > On 2012/07/19 06:52:12, koz wrote:
> > > mpcomplete@: I'm assuming that this is the only place I need to cancel
> > > suspension here, because this is the only place where
> > IncrementInFlightEvents()
> > > is called in this class, ie: any other events that get dispatched through
> this
> > > class aren't participating in this keepalive scheme and so wouldn't cause
an
> > > onSuspendCancel(). Is that a valid assumption?
> > 
> > It depends on what we want to cancel a suspend. Just events? Then this is
> good.
> > Should we include a message sent by a content script (probably)? Then you
need
> > to add it to the MessageService. Should we include a user inspecting the
event
> > page (probably)?
> > 
> > The central location for all of these is
> > LazyBackgroundTaskQueue::AddPendingTask. I haven't vetted every call site,
so
> > I'm not sure if we want to include everything as something that can cancel a
> > suspend event.
> > 
> > Things that definitely should *not* cancel a suspend (but would otherwise
> modify
> > the keepalive count) are things like any API calls or network requests made
by
> > the extension.
> 
> My reading of ExtensionMessageService indicates that creating a channel
> increments the lazy keepalive count, but sending messages through a channel
> doesn't; they don't need to because the channel they're going over is already
> pinning the background page as active. So I've made the calls there to
> IncrementLazyKeepaliveCount() be of the "suspend canceling" variety, which
> should give us the right behaviour.
> 
> As for the dev tools, it seems that that logic is handled in
> ExtensionProcessManager's Observe() function, so to make the dev tools behave
> correctly here we just need to make their Increment a suspend canceling one,
> too.

After thinking about it some more, I think it makes the most sense to cancel the
suspend within LazyBackgroundTaskQueue::AddPendingTask. Logically, anything that
would normally restart the event page should also prevent it from closing. Stuff
that merely keeps it alive (like network requests and extension API calls) does
not fall in that category.

Sorry for not thinking this through earlier. I think it will be cleaner and
easier to reason about this way, though.

[koz (OOO until 15th September), 2012-07-23 07:20:04]

On 2012/07/20 18:04:17, Matt Perry wrote:
>
https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
> File chrome/browser/extensions/extension_event_router.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
> chrome/browser/extensions/extension_event_router.cc:439:
> CancelSuspend(listener_profile, extension);
> On 2012/07/20 06:08:09, koz wrote:
> > On 2012/07/19 21:36:22, Matt Perry wrote:
> > > On 2012/07/19 06:52:12, koz wrote:
> > > > mpcomplete@: I'm assuming that this is the only place I need to cancel
> > > > suspension here, because this is the only place where
> > > IncrementInFlightEvents()
> > > > is called in this class, ie: any other events that get dispatched
through
> > this
> > > > class aren't participating in this keepalive scheme and so wouldn't
cause
> an
> > > > onSuspendCancel(). Is that a valid assumption?
> > > 
> > > It depends on what we want to cancel a suspend. Just events? Then this is
> > good.
> > > Should we include a message sent by a content script (probably)? Then you
> need
> > > to add it to the MessageService. Should we include a user inspecting the
> event
> > > page (probably)?
> > > 
> > > The central location for all of these is
> > > LazyBackgroundTaskQueue::AddPendingTask. I haven't vetted every call site,
> so
> > > I'm not sure if we want to include everything as something that can cancel
a
> > > suspend event.
> > > 
> > > Things that definitely should *not* cancel a suspend (but would otherwise
> > modify
> > > the keepalive count) are things like any API calls or network requests
made
> by
> > > the extension.
> > 
> > My reading of ExtensionMessageService indicates that creating a channel
> > increments the lazy keepalive count, but sending messages through a channel
> > doesn't; they don't need to because the channel they're going over is
already
> > pinning the background page as active. So I've made the calls there to
> > IncrementLazyKeepaliveCount() be of the "suspend canceling" variety, which
> > should give us the right behaviour.
> > 
> > As for the dev tools, it seems that that logic is handled in
> > ExtensionProcessManager's Observe() function, so to make the dev tools
behave
> > correctly here we just need to make their Increment a suspend canceling one,
> > too.
> 
> After thinking about it some more, I think it makes the most sense to cancel
the
> suspend within LazyBackgroundTaskQueue::AddPendingTask. Logically, anything
that
> would normally restart the event page should also prevent it from closing.
Stuff
> that merely keeps it alive (like network requests and extension API calls)
does
> not fall in that category.
> 
> Sorry for not thinking this through earlier. I think it will be cleaner and
> easier to reason about this way, though.

That's cool. Would it work to cancel suspend from AddPendingTask() though? We
can have ShouldEnqueueTask() return true when an extension host is closing (as
it does before this patch), and have AddPendingTask() cancel the suspend and
execute the Task synchronously, but this breaks assumptions made in (at least)
ExtensionEventDispatcher, which will dispatch the event to the same
ExtensionHost twice: once for each of the lazy and non-lazy listeners.

It seems that the right way would be to have everybody who would call
AddPendingTask() to use it unconditionally (rather than using
ShouldEnqueueTask()) and let the LazyBackgroundTaskQueue manage policy around
this, but that seems like a big change.

Do you think that it would be okay to use two versions of
IncrementLazyKeepalive() value approach for now? It seems to be the approach
most in line with the code currently.

[Matt Perry, 2012-07-23 19:14:42]

On 2012/07/23 07:20:04, koz wrote:
> On 2012/07/20 18:04:17, Matt Perry wrote:
> >
>
https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
> > File chrome/browser/extensions/extension_event_router.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
> > chrome/browser/extensions/extension_event_router.cc:439:
> > CancelSuspend(listener_profile, extension);
> > On 2012/07/20 06:08:09, koz wrote:
> > > On 2012/07/19 21:36:22, Matt Perry wrote:
> > > > On 2012/07/19 06:52:12, koz wrote:
> > > > > mpcomplete@: I'm assuming that this is the only place I need to cancel
> > > > > suspension here, because this is the only place where
> > > > IncrementInFlightEvents()
> > > > > is called in this class, ie: any other events that get dispatched
> through
> > > this
> > > > > class aren't participating in this keepalive scheme and so wouldn't
> cause
> > an
> > > > > onSuspendCancel(). Is that a valid assumption?
> > > > 
> > > > It depends on what we want to cancel a suspend. Just events? Then this
is
> > > good.
> > > > Should we include a message sent by a content script (probably)? Then
you
> > need
> > > > to add it to the MessageService. Should we include a user inspecting the
> > event
> > > > page (probably)?
> > > > 
> > > > The central location for all of these is
> > > > LazyBackgroundTaskQueue::AddPendingTask. I haven't vetted every call
site,
> > so
> > > > I'm not sure if we want to include everything as something that can
cancel
> a
> > > > suspend event.
> > > > 
> > > > Things that definitely should *not* cancel a suspend (but would
otherwise
> > > modify
> > > > the keepalive count) are things like any API calls or network requests
> made
> > by
> > > > the extension.
> > > 
> > > My reading of ExtensionMessageService indicates that creating a channel
> > > increments the lazy keepalive count, but sending messages through a
channel
> > > doesn't; they don't need to because the channel they're going over is
> already
> > > pinning the background page as active. So I've made the calls there to
> > > IncrementLazyKeepaliveCount() be of the "suspend canceling" variety, which
> > > should give us the right behaviour.
> > > 
> > > As for the dev tools, it seems that that logic is handled in
> > > ExtensionProcessManager's Observe() function, so to make the dev tools
> behave
> > > correctly here we just need to make their Increment a suspend canceling
one,
> > > too.
> > 
> > After thinking about it some more, I think it makes the most sense to cancel
> the
> > suspend within LazyBackgroundTaskQueue::AddPendingTask. Logically, anything
> that
> > would normally restart the event page should also prevent it from closing.
> Stuff
> > that merely keeps it alive (like network requests and extension API calls)
> does
> > not fall in that category.
> > 
> > Sorry for not thinking this through earlier. I think it will be cleaner and
> > easier to reason about this way, though.
> 
> That's cool. Would it work to cancel suspend from AddPendingTask() though? We
> can have ShouldEnqueueTask() return true when an extension host is closing (as
> it does before this patch), and have AddPendingTask() cancel the suspend and
> execute the Task synchronously, but this breaks assumptions made in (at least)
> ExtensionEventDispatcher, which will dispatch the event to the same
> ExtensionHost twice: once for each of the lazy and non-lazy listeners.
> 
> It seems that the right way would be to have everybody who would call
> AddPendingTask() to use it unconditionally (rather than using
> ShouldEnqueueTask()) and let the LazyBackgroundTaskQueue manage policy around
> this, but that seems like a big change.
> 
> Do you think that it would be okay to use two versions of
> IncrementLazyKeepalive() value approach for now? It seems to be the approach
> most in line with the code currently.

I see your point. I'm still uneasy about having to make the decision at every
call site. What if we moved the cancel to ShouldEnqueueTask? It's a bit weird to
have a boolean function have a side effect, but I think it's better than the
alternative.

[Matt Perry, 2012-07-23 19:15:49]

https://chromiumcodereview.appspot.com/10804020/diff/13001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/13001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:548:
MaybePostOnLazyBackgroundPageIdle(extension_id);
This change confuses me. If we're canceling a suspend, how is the page
transitioning to idle?

[koz (OOO until 15th September), 2012-07-24 06:47:26]

On 2012/07/23 19:14:42, Matt Perry wrote:
> On 2012/07/23 07:20:04, koz wrote:
> > On 2012/07/20 18:04:17, Matt Perry wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
> > > File chrome/browser/extensions/extension_event_router.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10804020/diff/9001/chrome/browser/exte...
> > > chrome/browser/extensions/extension_event_router.cc:439:
> > > CancelSuspend(listener_profile, extension);
> > > On 2012/07/20 06:08:09, koz wrote:
> > > > On 2012/07/19 21:36:22, Matt Perry wrote:
> > > > > On 2012/07/19 06:52:12, koz wrote:
> > > > > > mpcomplete@: I'm assuming that this is the only place I need to
cancel
> > > > > > suspension here, because this is the only place where
> > > > > IncrementInFlightEvents()
> > > > > > is called in this class, ie: any other events that get dispatched
> > through
> > > > this
> > > > > > class aren't participating in this keepalive scheme and so wouldn't
> > cause
> > > an
> > > > > > onSuspendCancel(). Is that a valid assumption?
> > > > > 
> > > > > It depends on what we want to cancel a suspend. Just events? Then this
> is
> > > > good.
> > > > > Should we include a message sent by a content script (probably)? Then
> you
> > > need
> > > > > to add it to the MessageService. Should we include a user inspecting
the
> > > event
> > > > > page (probably)?
> > > > > 
> > > > > The central location for all of these is
> > > > > LazyBackgroundTaskQueue::AddPendingTask. I haven't vetted every call
> site,
> > > so
> > > > > I'm not sure if we want to include everything as something that can
> cancel
> > a
> > > > > suspend event.
> > > > > 
> > > > > Things that definitely should *not* cancel a suspend (but would
> otherwise
> > > > modify
> > > > > the keepalive count) are things like any API calls or network requests
> > made
> > > by
> > > > > the extension.
> > > > 
> > > > My reading of ExtensionMessageService indicates that creating a channel
> > > > increments the lazy keepalive count, but sending messages through a
> channel
> > > > doesn't; they don't need to because the channel they're going over is
> > already
> > > > pinning the background page as active. So I've made the calls there to
> > > > IncrementLazyKeepaliveCount() be of the "suspend canceling" variety,
which
> > > > should give us the right behaviour.
> > > > 
> > > > As for the dev tools, it seems that that logic is handled in
> > > > ExtensionProcessManager's Observe() function, so to make the dev tools
> > behave
> > > > correctly here we just need to make their Increment a suspend canceling
> one,
> > > > too.
> > > 
> > > After thinking about it some more, I think it makes the most sense to
cancel
> > the
> > > suspend within LazyBackgroundTaskQueue::AddPendingTask. Logically,
anything
> > that
> > > would normally restart the event page should also prevent it from closing.
> > Stuff
> > > that merely keeps it alive (like network requests and extension API calls)
> > does
> > > not fall in that category.
> > > 
> > > Sorry for not thinking this through earlier. I think it will be cleaner
and
> > > easier to reason about this way, though.
> > 
> > That's cool. Would it work to cancel suspend from AddPendingTask() though?
We
> > can have ShouldEnqueueTask() return true when an extension host is closing
(as
> > it does before this patch), and have AddPendingTask() cancel the suspend and
> > execute the Task synchronously, but this breaks assumptions made in (at
least)
> > ExtensionEventDispatcher, which will dispatch the event to the same
> > ExtensionHost twice: once for each of the lazy and non-lazy listeners.
> > 
> > It seems that the right way would be to have everybody who would call
> > AddPendingTask() to use it unconditionally (rather than using
> > ShouldEnqueueTask()) and let the LazyBackgroundTaskQueue manage policy
around
> > this, but that seems like a big change.
> > 
> > Do you think that it would be okay to use two versions of
> > IncrementLazyKeepalive() value approach for now? It seems to be the approach
> > most in line with the code currently.
> 
> I see your point. I'm still uneasy about having to make the decision at every
> call site. What if we moved the cancel to ShouldEnqueueTask? It's a bit weird
to
> have a boolean function have a side effect, but I think it's better than the
> alternative.

I think adding that side effect to AddPendingTask() would be an equally
disruptive change (in terms of correctness) to changing the semantics of
AddPendingTask() itself, so I've opted for that.

Now AddPendingTask() tries to execute the provided task synchronously if it can
(canceling any suspend on the background page), and only failing that will it
enqueue the event. Any time a caller wants to interact with the ExtensionHost of
a lazy event page they should go through AddPendingTask() so it can handle the
case where the ExtensionHost is down (or suspending, etc).

WDYT?

[koz (OOO until 15th September), 2012-07-24 06:49:52]

https://chromiumcodereview.appspot.com/10804020/diff/13001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/13001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:548:
MaybePostOnLazyBackgroundPageIdle(extension_id);
On 2012/07/23 19:15:49, Matt Perry wrote:
> This change confuses me. If we're canceling a suspend, how is the page
> transitioning to idle?

This doesn't transition us to idle straight away, only after a timeout, and then
only if the keepalive count is zero.

We need to do this because CancelSuspend() is called from AddPendingTask(),
which itself can be called in contexts where the lazy keepalive count is zero.
If we don't schedule a call to OnLazyBackgroundPageIdle() then the background
page won't get killed until the keepalive count goes up and returns to zero
again.

[Matt Perry, 2012-07-24 19:25:15]

After seeing the change to AddPendingTask, it seems too dangerous to remove the
ShouldEnqueueTask bits. Many of the callers actually want to do something
different in the case where that method returns false.

I think it would be better to keep call sites as they were, and move
CancelSuspend to ShouldEnqueueTask. If you disagree, the next-best option is to
go with your original approach of an extra param on IncrementLazyKeepaliveCount.
If you go that route, I would make the param required, and update all call
sites, so we are explicitly making the decision at every caller whether it
should cancel the suspend.

Sorry for all the back and forth. This stuff can be really subtle, and I'm
afraid to make it more complicated and/or fragile.

https://chromiumcodereview.appspot.com/10804020/diff/13001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/13001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:548:
MaybePostOnLazyBackgroundPageIdle(extension_id);
On 2012/07/24 06:49:52, koz wrote:
> On 2012/07/23 19:15:49, Matt Perry wrote:
> > This change confuses me. If we're canceling a suspend, how is the page
> > transitioning to idle?
> 
> This doesn't transition us to idle straight away, only after a timeout, and
then
> only if the keepalive count is zero.
> 
> We need to do this because CancelSuspend() is called from AddPendingTask(),
> which itself can be called in contexts where the lazy keepalive count is zero.
> If we don't schedule a call to OnLazyBackgroundPageIdle() then the background
> page won't get killed until the keepalive count goes up and returns to zero
> again.

AddPendingTask is often called when the keepalive count is 0, because the page
hasn't started yet. But then the page starts and the keepalive count should
increment. AddPendingTask by definition means the extension is active.

I think this is better done with an increment/decrement keepalive count pair.
Something like:
  IncrementKeepaliveCount();
  CancelSuspend();
  run the pending task
  DecrementKeepaliveCount();

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (left):

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:773: if (handler_pid >
0) {
This potentially changes the behavior.. If handler_pid is > 0, we now always try
to load the bg page. That may not be what they want.

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
File chrome/browser/extensions/api/runtime/runtime_api.cc (left):

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
chrome/browser/extensions/api/runtime/runtime_api.cc:56: error_ =
kNoBackgroundPageError;
We lost this error in the transition. Could you add it back before adding the
task: if (!GetExtension()->has_background_page()) error_ = kNoBg;

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
File chrome/browser/extensions/extension_event_router.cc (left):

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
chrome/browser/extensions/extension_event_router.cc:479: if
(queue->ShouldEnqueueTask(profile, extension)) {
Doesn't removing this result in a double dispatch of the event? It means we'd
always call DispatchPendingEvent, even if we *shouldn't* enqueue the task - i.e.
if the event page is already loaded. Since a regular listener has been
registered in that case, we'll dispatch to the regular listener, then dispatch
again for this lazy listener.

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
chrome/browser/extensions/extension_event_router.cc:232: } else {
Did you mean to make this change? It seems unrelated.

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
File chrome/browser/extensions/lazy_background_task_queue.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
chrome/browser/extensions/lazy_background_task_queue.cc:77: return true;
I think you still want to run the task with a NULL host here. Some of the call
sites (the settings handler) want to still call their task even if there is no
background page.

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (left):

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:558: if
(render_process_id == -1) {
This change is incorrect. The render view might not be the background page. It
could be a popup, etc.

[koz (OOO until 15th September), 2012-07-25 07:48:31]

Yes, I definintely agree that this stuff is subtle and appreciate your
thoroughness here.

I've managed to get it working with ShouldEnqueueTask() calling CancelSuspend().
It required changing ExtensionEventRouter so that it dispatches to lazy
listeners before non-lazy listeners. I don't think either approach is ideal but
at the same time perhaps a refactor here needs a more incremental approach.

https://chromiumcodereview.appspot.com/10804020/diff/13001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/13001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:548:
MaybePostOnLazyBackgroundPageIdle(extension_id);
On 2012/07/24 19:25:15, Matt Perry wrote:
> On 2012/07/24 06:49:52, koz wrote:
> > On 2012/07/23 19:15:49, Matt Perry wrote:
> > > This change confuses me. If we're canceling a suspend, how is the page
> > > transitioning to idle?
> > 
> > This doesn't transition us to idle straight away, only after a timeout, and
> then
> > only if the keepalive count is zero.
> > 
> > We need to do this because CancelSuspend() is called from AddPendingTask(),
> > which itself can be called in contexts where the lazy keepalive count is
zero.
> > If we don't schedule a call to OnLazyBackgroundPageIdle() then the
background
> > page won't get killed until the keepalive count goes up and returns to zero
> > again.
> 
> AddPendingTask is often called when the keepalive count is 0, because the page
> hasn't started yet. But then the page starts and the keepalive count should
> increment. AddPendingTask by definition means the extension is active.
> 
> I think this is better done with an increment/decrement keepalive count pair.
> Something like:
>   IncrementKeepaliveCount();
>   CancelSuspend();
>   run the pending task
>   DecrementKeepaliveCount();

Done.

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (left):

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:773: if (handler_pid >
0) {
On 2012/07/24 19:25:15, Matt Perry wrote:
> This potentially changes the behavior.. If handler_pid is > 0, we now always
try
> to load the bg page. That may not be what they want.

Ah, true. We probably only want to change the else clause here.

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
File chrome/browser/extensions/extension_event_router.cc (left):

https://chromiumcodereview.appspot.com/10804020/diff/17018/chrome/browser/ext...
chrome/browser/extensions/extension_event_router.cc:479: if
(queue->ShouldEnqueueTask(profile, extension)) {
On 2012/07/24 19:25:15, Matt Perry wrote:
> Doesn't removing this result in a double dispatch of the event? It means we'd
> always call DispatchPendingEvent, even if we *shouldn't* enqueue the task -
i.e.
> if the event page is already loaded. Since a regular listener has been
> registered in that case, we'll dispatch to the regular listener, then dispatch
> again for this lazy listener.

The idea here was to make it so lazy background pages don't register non-lazy
event handlers, so the lazy dispatch will always either send to a currently
existing ExtensionHost or add a pending task.

[Matt Perry, 2012-07-25 19:21:55]

LGTM! Thanks for your patience.

https://chromiumcodereview.appspot.com/10804020/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/21001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:542:
DecrementLazyKeepaliveCount(extension);
Could you add a comment why this is necessary?

(The ugliness of this makes me slightly prefer the approach of passing
should_cancel_suspend to IncrementLazyKeepaliveCount, but both approaches are
fine.)

[koz (OOO until 15th September), 2012-07-26 00:40:08]

Awesome! Thanks Matt.

https://chromiumcodereview.appspot.com/10804020/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10804020/diff/21001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:542:
DecrementLazyKeepaliveCount(extension);
On 2012/07/25 19:21:55, Matt Perry wrote:
> Could you add a comment why this is necessary?
> 
> (The ugliness of this makes me slightly prefer the approach of passing
> should_cancel_suspend to IncrementLazyKeepaliveCount, but both approaches are
> fine.)

I've added a comment and will leave it this way, because leveraging the existing
code's behaviour seems like a win.

(I also moved it to inside the conditional because it's unnecessary to do it if
the extension isn't closing).

[commit-bot: I haz the power, 2012-07-26 00:40:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10804020/19008

[commit-bot: I haz the power, 2012-07-26 02:30:44]

Change committed as 148490