Graceful handling of extension process crashes

Display an infobar alert when extension process crashes.

Also correctly handle crashes in task manager. The infobar allows user to restart the extension process.

TEST=Install buildbot extension, see its content in the shelf. Open task manager, kill extension process. Click "restart" in the infobar that should appear. The extension content in the shelf should re-appear after the crash.

http://crbug.com/14111

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

[Paweł Hajdan Jr., 2009-06-18 16:31:51]

Please review:

erikkay: extension restarting
ben: UI (screenshot in the bugtracker)
jcampan: Task Manager bits

[Erik does not do reviews, 2009-06-18 16:55:39]

I like the cleanup in ExtensionView.

http://codereview.chromium.org/126289/diff/2001/2005
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/126289/diff/2001/2005#newcode52
Line 52: IDR_INFOBAR_PLUGIN_CRASHED);
add TODO + bug for a new icon.  I imagine we don't want the same icon as plugins
here.

http://codereview.chromium.org/126289/diff/2001/2005#newcode116
Line 116: bool ExtensionHost::IsViewLive() const {
This should be IsRenderViewLive() to avoid confusion with the ExtensionView.

http://codereview.chromium.org/126289/diff/2001/2005#newcode128
Line 128: #if !defined(OS_WIN)
I think you should just wrap the view_->Recover... method with #if
defined(TOOLKIT_VIEWS) and ditch the NOTIMPLEMENTED.  The manager_ part of this
should work as-is on other platforms for the background page.

http://codereview.chromium.org/126289/diff/2001/2005#newcode146
Line 146: TabContents* current_tab = GetBrowser()->GetSelectedTabContents();
I'll defer to the Ben here, but it feels a bit weird to just grab the current
selected tab.  What if the crash happens right as I'm switching tabs?  Won't I
miss the infobar?  Is there a better way to do a browser-wide info bar?

[Glen Murphy, 2009-06-18 17:23:24]

> http://codereview.chromium.org/126289/diff/2001/2005#newcode146
> Line 146: TabContents* current_tab = GetBrowser()->GetSelectedTabContents();
> I'll defer to the Ben here, but it feels a bit weird to just grab the current
> selected tab.  What if the crash happens right as I'm switching tabs?  Won't I
> miss the infobar?  Is there a better way to do a browser-wide info bar?

Infobars should only be used for tab-content related notifications; we should
minimize the use of them outside of that context.

We don't currently have anything better, however - we need notifications to
exist to cover this case (i.e. when we get notifications, remove this infobar
code and use those instead).

[Paweł Hajdan Jr., 2009-06-18 19:47:03]

On 2009/06/18 16:55:39, Erik Kay wrote:
> I like the cleanup in ExtensionView.
> 
> http://codereview.chromium.org/126289/diff/2001/2005
> File chrome/browser/extensions/extension_host.cc (right):
> 
> http://codereview.chromium.org/126289/diff/2001/2005#newcode52
> Line 52: IDR_INFOBAR_PLUGIN_CRASHED);
> add TODO + bug for a new icon.  I imagine we don't want the same icon as
plugins
> here.

Filed http://crbug.com/14591

> http://codereview.chromium.org/126289/diff/2001/2005#newcode116
> Line 116: bool ExtensionHost::IsViewLive() const {
> This should be IsRenderViewLive() to avoid confusion with the ExtensionView.

Done.

> http://codereview.chromium.org/126289/diff/2001/2005#newcode128
> Line 128: #if !defined(OS_WIN)
> I think you should just wrap the view_->Recover... method with #if
> defined(TOOLKIT_VIEWS) and ditch the NOTIMPLEMENTED.  The manager_ part of
this
> should work as-is on other platforms for the background page.

Done.

[Erik does not do reviews, 2009-06-18 19:54:29]

LGTM from my perspective, although it looks like you should add a TODO and file
a bug about switching this to use browser-level notifications when they arrive
(Glen is there a bug filed about this that he can reference?).

[jcampan, 2009-06-18 22:40:05]

Changes to task_manager_resource_providers.cc LGTM

[Paweł Hajdan Jr., 2009-06-19 05:58:06]

beng/glen/nickbaum: ping. Does the UI look ok? You can see a screenshot at
http://code.google.com/p/chromium/issues/detail?id=14111#c3