Add an arbitrary delay before unloading lazy background pages.

Add an arbitrary delay before unloading lazy background pages.

BUG=119619
TEST=Make sure that lazy page process stays open until 10 seconds after the last of a series of events it's listening for.

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

[Yoyo Zhou, 2012-04-30 21:34:09]

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:427:
base::TimeDelta::FromSeconds(10));
I'm not sure if this should be a constant defined somewhere (or even a
command-line flag).

[Aaron Boodman, 2012-04-30 21:47:13]

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:427:
base::TimeDelta::FromSeconds(10));
On 2012/04/30 21:34:09, Yoyo Zhou wrote:
> I'm not sure if this should be a constant defined somewhere (or even a
> command-line flag).

Let's do a command-line flag so that we can experiment with different values.

[Matt Perry, 2012-04-30 21:58:44]

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:427:
base::TimeDelta::FromSeconds(10));
On 2012/04/30 21:47:13, Aaron Boodman wrote:
> On 2012/04/30 21:34:09, Yoyo Zhou wrote:
> > I'm not sure if this should be a constant defined somewhere (or even a
> > command-line flag).
> 
> Let's do a command-line flag so that we can experiment with different values.

I think we should have 2 timeouts, like I described in the bug:
- first timeout is a soft timeout. if the extension has any activity in the time
period, the timeout is reset. the purpose of this one is to avoid thrashing if
there are many events in sequence.
- once the first timeout expires, we dispatch the unload event, and start a
second timeout. this timeout is a hard timeout, and never resets. when it
expires, we unload the page. the purpose of this one is to give some (but not
infinite) time for the extension to do cleanup-on-unload things.

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:437: if (host &&
!background_page_data_[extension_id].is_closing) {
We should check whether close_sequence_id has changed. If it has, then we know
this idle notification is obsolete, and we shouldn't bother sending the message
(since we'd just ignore it the ack).

[Yoyo Zhou, 2012-05-01 22:15:13]

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:427:
base::TimeDelta::FromSeconds(10));
On 2012/04/30 21:58:45, Matt Perry wrote:
> On 2012/04/30 21:47:13, Aaron Boodman wrote:
> > On 2012/04/30 21:34:09, Yoyo Zhou wrote:
> > > I'm not sure if this should be a constant defined somewhere (or even a
> > > command-line flag).
> > 
> > Let's do a command-line flag so that we can experiment with different
values.
> 
> I think we should have 2 timeouts, like I described in the bug:
> - first timeout is a soft timeout. if the extension has any activity in the
time
> period, the timeout is reset. the purpose of this one is to avoid thrashing if
> there are many events in sequence.
> - once the first timeout expires, we dispatch the unload event, and start a
> second timeout. this timeout is a hard timeout, and never resets. when it
> expires, we unload the page. the purpose of this one is to give some (but not
> infinite) time for the extension to do cleanup-on-unload things.

This is the first one then. As for the second case, I thought we were doing
something with asynchronous responses to the unloading event.

Both timeouts are command-line-switchable, although the parsing is ad-hoc since
I couldn't find more examples of ints with default values.

http://codereview.chromium.org/10272019/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:437: if (host &&
!background_page_data_[extension_id].is_closing) {
On 2012/04/30 21:58:45, Matt Perry wrote:
> We should check whether close_sequence_id has changed. If it has, then we know
> this idle notification is obsolete, and we shouldn't bother sending the
message
> (since we'd just ignore it the ack).

True.

[Matt Perry, 2012-05-01 22:20:45]

lgtm

[Yoyo Zhou, 2012-05-02 00:14:10]

On 2012/05/01 22:20:45, Matt Perry wrote:
> lgtm

This change was about to add 15 seconds to any tests that wait for an event page
to unload. (I changed that to 2.) Luckily, I noticed because one of them does it
3 times (and thus exceeded the 45 second timeout), but I would hope that that
behavior only needs to be tested in lazy_background_page_apitest.

[Matt Perry, 2012-05-02 00:18:00]

On 2012/05/02 00:14:10, Yoyo Zhou wrote:
> On 2012/05/01 22:20:45, Matt Perry wrote:
> > lgtm
> 
> This change was about to add 15 seconds to any tests that wait for an event
page
> to unload. (I changed that to 2.) Luckily, I noticed because one of them does
it
> 3 times (and thus exceeded the 45 second timeout), but I would hope that that
> behavior only needs to be tested in lazy_background_page_apitest.

Doh, sorry, meant to warn you about that :)