Don't close crashed tabs from extensions with background pages.

Don't close tabs from crashed extensions with background pages.
Make the crashed extension reload when the sad tab is reloaded.
('Extensions' includes packaged apps.)

BUG=71629
BUG=94177
TEST=Added ExtensionCrashRecoveryTest.ReloadTabsWithBackgroundPage. Manually: Open a packaged app with a background page. Kill it from Task Manager. Its tab should not close. Reload the tab and it should show the app (and the crash notification bubble should be gone).


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

[Yoyo Zhou, 2011-11-10 04:12:40]

Deleted the blurb about the task manager bug since I landed the fix for that.

[Matt Perry, 2011-11-10 22:07:59]

mostly lgtm

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right):

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_crash_recovery_browsertest.cc:473:
content::NOTIFICATION_LOAD_STOP,
This should be unnecessary. NavigateToURL already waits for the navigation to
complete.

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_info_map.cc (right):

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_info_map.cc:69: reason ==
extension_misc::UNLOAD_REASON_TERMINATE)
nit: pull this condition out into a shared bool like 'was_disabled'

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:142: // Reload
the extension if it extension has crashed.
it = the

http://codereview.chromium.org/8496024/diff/9001/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8496024/diff/9001/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:99: // TODO(yoz):
Should we try to reload extensions here instead??
content/ shouldn't know about extensions. if we want to reload them at this
point, it would have to be in the delegate. just remove this TODO.

[Matt Perry, 2011-11-10 22:18:12]

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:144:
service->ReloadExtension(site.host());
Wait, this won't work for unpacked extensions, right? Since we don't have their
manifests in memory, we have to load them asynchronously.

I guess it's not a big deal, but ideally we would make the reload operation
async, and wait until the extension is loaded. I'm sure that's a lot more work
though.

[Yoyo Zhou, 2011-11-10 23:26:30]

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right):

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_crash_recovery_browsertest.cc:473:
content::NOTIFICATION_LOAD_STOP,
On 2011/11/10 22:07:59, Matt Perry wrote:
> This should be unnecessary. NavigateToURL already waits for the navigation to
> complete.

Removed.

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_info_map.cc (right):

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_info_map.cc:69: reason ==
extension_misc::UNLOAD_REASON_TERMINATE)
On 2011/11/10 22:07:59, Matt Perry wrote:
> nit: pull this condition out into a shared bool like 'was_disabled'

Done.

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:142: // Reload
the extension if it extension has crashed.
On 2011/11/10 22:07:59, Matt Perry wrote:
> it = the

Fixed.

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:144:
service->ReloadExtension(site.host());
On 2011/11/10 22:18:13, Matt Perry wrote:
> Wait, this won't work for unpacked extensions, right? Since we don't have
their
> manifests in memory, we have to load them asynchronously.
> 
> I guess it's not a big deal, but ideally we would make the reload operation
> async, and wait until the extension is loaded. I'm sure that's a lot more work
> though.

Surprisingly, it does seem to work (although I'm sure there is a race, e.g. if I
set a breakpoint in the debugger at this point, it only works flakily).

We do have a function to load unpacked extensions synchronously
(LoadFromCommandLine), but it would take more to make ReloadExtension use it.

Blocking navigation (the reload) seems harder to accomplish.

I'll add a TODO for this.

http://codereview.chromium.org/8496024/diff/9001/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8496024/diff/9001/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:99: // TODO(yoz):
Should we try to reload extensions here instead??
On 2011/11/10 22:07:59, Matt Perry wrote:
> content/ shouldn't know about extensions. if we want to reload them at this
> point, it would have to be in the delegate. just remove this TODO.

Yeah, this was from an earlier approach.

[Matt Perry, 2011-11-10 23:28:21]

lgtm

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8496024/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:144:
service->ReloadExtension(site.host());
On 2011/11/10 23:26:30, Yoyo Zhou wrote:
> On 2011/11/10 22:18:13, Matt Perry wrote:
> > Wait, this won't work for unpacked extensions, right? Since we don't have
> their
> > manifests in memory, we have to load them asynchronously.
> > 
> > I guess it's not a big deal, but ideally we would make the reload operation
> > async, and wait until the extension is loaded. I'm sure that's a lot more
work
> > though.
> 
> Surprisingly, it does seem to work (although I'm sure there is a race, e.g. if
I
> set a breakpoint in the debugger at this point, it only works flakily).
> 
> We do have a function to load unpacked extensions synchronously
> (LoadFromCommandLine), but it would take more to make ReloadExtension use it.

We shouldn't block the UI thread on reload for this.

> Blocking navigation (the reload) seems harder to accomplish.
> 
> I'll add a TODO for this.

SGTM.

[commit-bot: I haz the power, 2011-11-11 18:11:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/8496024/9002

[commit-bot: I haz the power, 2011-11-11 19:27:58]

Change committed as 109675