Fix bug where transient pages would miss events dispatched while it was

Fix bug where transient pages would miss events dispatched while it was
unloading.

The fix is a 2-parter:

Part 1: lazy_background_queue.* now checks if the background page is unloading before allowing a task to run. If it is unloading, we wait until the page unloads, then reload it.

Part 2: Part 1 exposed the problem which I had previously outlined in extension_process_manager.cc:403 - after we reload the page, we occasionally received messages from the renderer to decrement the keepalive count for the old page, throwing the count out of balance. The fix is to make sure we decrement keepalive counts only if the host we incremented them for is still active.

BUG=123243
TEST=no

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

[Matt Perry, 2012-04-19 18:57:36]

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_custom_bindings.cc (left):

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_custom_bindings.cc:33: class
ExtensionViewAccumulator : public content::RenderViewVisitor {
BTW, I moved this, mostly unchanged, to extension_helper.cc.

[Yoyo Zhou, 2012-04-19 21:47:13]

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_event_router.cc:412: ExtensionHost* host =
pm->GetBackgroundHostForExtension(extension->id());
You could put this inside has_lazy_background_page check so you don't have to do
it unless the extension actually has a transient page.

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_message_service.cc:102: int dest_port_id,
const std::string& channel_name,
nit: why this change?

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:376: bool
ExtensionProcessManager::IsBackgroundHostClosing(
It seems like you could simplify the usage here (in OnLazyBackgroundPage*) and
in LazyBackgroundTaskQueue with something such as
ExtensionHost* GetBackgroundHostForExtensionIfNotClosing()
although this is a weird abstraction.

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/lazy...
File chrome/browser/extensions/lazy_background_task_queue.cc (right):

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/lazy...
chrome/browser/extensions/lazy_background_task_queue.cc:70: // If this is the
first enqueued task, ensure the background page
This comment needs an "and we are not waiting for the background page to
unload".

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/lazy...
chrome/browser/extensions/lazy_background_task_queue.cc:160:
base::Unretained(this), profile, host->extension()->id()));
If we might be receiving this notification near shutdown, WeakPtrFactory might
be safer than Unretained.

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_custom_bindings.cc (left):

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_custom_bindings.cc:33: class
ExtensionViewAccumulator : public content::RenderViewVisitor {
On 2012/04/19 18:57:36, Matt Perry wrote:
> BTW, I moved this, mostly unchanged, to extension_helper.cc.

Ok, I was trying to understand what this was doing in this change. (It helps if
you note it in the CL description...)

Any particular reason to return a RenderView* array instead of the v8::Array?

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_custom_bindings.cc (right):

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_custom_bindings.cc:18: #include
"v8/include/v8.h"
nit: this is unalphabetical

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_helper.cc (right):

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_helper.cc:96: // Returns true is |type|
"isa" |match|.
nit: is -> if

[Matt Perry, 2012-04-19 22:21:53]

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_event_router.cc:412: ExtensionHost* host =
pm->GetBackgroundHostForExtension(extension->id());
On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> You could put this inside has_lazy_background_page check so you don't have to
do
> it unless the extension actually has a transient page.

Done.

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_message_service.cc:102: int dest_port_id,
const std::string& channel_name,
On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> nit: why this change?

Just bringing it up to style code:
http://dev.chromium.org/developers/coding-style#TOC-Code-formatting

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:376: bool
ExtensionProcessManager::IsBackgroundHostClosing(
On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> It seems like you could simplify the usage here (in OnLazyBackgroundPage*) and
> in LazyBackgroundTaskQueue with something such as
> ExtensionHost* GetBackgroundHostForExtensionIfNotClosing()
> although this is a weird abstraction.

I thought about doing something like that, but I think I prefer keeping this
check as an explicit separate step. I don't feel that strongly, though.

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/lazy...
File chrome/browser/extensions/lazy_background_task_queue.cc (right):

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/lazy...
chrome/browser/extensions/lazy_background_task_queue.cc:70: // If this is the
first enqueued task, ensure the background page
On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> This comment needs an "and we are not waiting for the background page to
> unload".

Done.

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/lazy...
chrome/browser/extensions/lazy_background_task_queue.cc:160:
base::Unretained(this), profile, host->extension()->id()));
On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> If we might be receiving this notification near shutdown, WeakPtrFactory might
> be safer than Unretained.

Done.

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_custom_bindings.cc (left):

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_custom_bindings.cc:33: class
ExtensionViewAccumulator : public content::RenderViewVisitor {
On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> On 2012/04/19 18:57:36, Matt Perry wrote:
> > BTW, I moved this, mostly unchanged, to extension_helper.cc.
> 
> Ok, I was trying to understand what this was doing in this change. (It helps
if
> you note it in the CL description...)
> 
> Any particular reason to return a RenderView* array instead of the v8::Array?

I'm using it for more than returning results to JavaScript. See
ExtensionHelper::GetBackgroundPage.

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_custom_bindings.cc (right):

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_custom_bindings.cc:18: #include
"v8/include/v8.h"
On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> nit: this is unalphabetical

Done.

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_helper.cc (right):

http://codereview.chromium.org/10114015/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_helper.cc:96: // Returns true is |type|
"isa" |match|.
On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> nit: is -> if

Done.

[Yoyo Zhou, 2012-04-19 22:32:55]

LGTM

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_message_service.cc:102: int dest_port_id,
const std::string& channel_name,
On 2012/04/19 22:21:53, Matt Perry wrote:
> On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> > nit: why this change?
> 
> Just bringing it up to style code:
> http://dev.chromium.org/developers/coding-style#TOC-Code-formatting

It doesn't look particularly recommended for function declarations.

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_process_manager.cc:376: bool
ExtensionProcessManager::IsBackgroundHostClosing(
On 2012/04/19 22:21:53, Matt Perry wrote:
> On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> > It seems like you could simplify the usage here (in OnLazyBackgroundPage*)
and
> > in LazyBackgroundTaskQueue with something such as
> > ExtensionHost* GetBackgroundHostForExtensionIfNotClosing()
> > although this is a weird abstraction.
> 
> I thought about doing something like that, but I think I prefer keeping this
> check as an explicit separate step. I don't feel that strongly, though.

I don't either, since it's not that convenient either way.

[Matt Perry, 2012-04-19 22:34:56]

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

http://codereview.chromium.org/10114015/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_message_service.cc:102: int dest_port_id,
const std::string& channel_name,
On 2012/04/19 22:32:55, Yoyo Zhou wrote:
> On 2012/04/19 22:21:53, Matt Perry wrote:
> > On 2012/04/19 21:47:13, Yoyo Zhou wrote:
> > > nit: why this change?
> > 
> > Just bringing it up to style code:
> > http://dev.chromium.org/developers/coding-style#TOC-Code-formatting
> 
> It doesn't look particularly recommended for function declarations.

See bullet point that starts "For function declarations and definitions"