Fix "crashed extension" infobar browser crashes....

Fix "crashed extension" infobar browser crashes.

This is a general rework of how "crashed extension" infobar works
and how the extension is actually recovered after the crash.

TEST=See bug.
http://crbug.com/15888

[Paweł Hajdan Jr., 2009-08-07 17:44:08]

Please review:

aa: extension stuff
Ben: infobar abuse (if any)

There is a slight duplication of the code which removes the infobar from
TabContents and sets infobar_delegate_ to NULL. If usage of the infobar is
correct here, I woul remove the duplication by extracting that code to
CloseInfoBar private method.

[Aaron Boodman, 2009-08-07 17:49:43]

This approach seems dicey to me. Why not just copy the extension name into the
ExtensionCrashedInfoBar instance? Then you don't have to track anything, and
it's like a 2 line change.

[Paweł Hajdan Jr., 2009-08-07 17:52:07]

That'd be the simplest fix for the crasher, but it also seems more correct
to close the infobar when it's no longer needed. BTW, now that I added this
delete and it didn't segfault, it probably means there was a leak before.

On Fri, Aug 7, 2009 at 10:49, <aa@chromium.org> wrote:

> This approach seems dicey to me. Why not just copy the extension name
> into the ExtensionCrashedInfoBar instance? Then you don't have to track
> anything, and it's like a 2 line change.
>
>
> http://codereview.chromium.org/164151
>

[Aaron Boodman, 2009-08-07 17:56:45]

Yeah, I think you are supposed to delete yourself in ::InfoBarClosed().

IMO, I think this is such an edge case that it is better to implement
a simple fix that is more likely to stay working.

- a

On Fri, Aug 7, 2009 at 10:51 AM, Pawe=C5=82 Hajdan
Jr.<phajdan.jr@chromium.org> wrote:
> That'd be the simplest fix for the crasher, but it also seems more correc=
t
> to close the infobar when it's no longer needed. BTW, now that I added th=
is
> delete and it didn't segfault, it probably means there was a leak before.
>
> On Fri, Aug 7, 2009 at 10:49, <aa@chromium.org> wrote:
>>
>> This approach seems dicey to me. Why not just copy the extension name
>> into the ExtensionCrashedInfoBar instance? Then you don't have to track
>> anything, and it's like a 2 line change.
>>
>> http://codereview.chromium.org/164151
>
>

[Aaron Boodman, 2009-08-07 18:16:37]

I take it back, I didn't see that you need to communicate back to
extension_host_.

[Aaron Boodman, 2009-08-07 20:28:59]

Sorry, looking at this some more I see some other issues:

I think that if an extension has multiple toolstrips, this will do weird things.
It looks like it will show multiple crash toolstrips for the same extension
(such an extension will have multiple ExtensionHosts for the same window).

Maybe it would be better to completely decouple these two things, like:

* When an extension crashes, all the extension hosts remove themselves from the
container, just like what happens on unload.

* The browser windows listen for process crash notifications, and if they see
one for an extension, they put up an infobar if that extension had one or more
toolstrips.

* When you click "reload" on the infobar, we just reload the extension, using
ExtensionsService.

[Paweł Hajdan Jr., 2009-08-10 18:43:58]

Please review:

aa: extension stuff
Ben: if I didn't abuse anything I shouldn't

[Aaron Boodman, 2009-08-10 22:33:04]

LGTM

This is very nice. Good work, and thanks for your patience on my requested
changes.

http://codereview.chromium.org/164151/diff/9/18
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/164151/diff/9/18#newcode2111
Line 2111: ExtensionsService* extensions_service =
You can also just get this from profile_. In fact that would probably be better
since we don't want to show this infobar in incognito windows (we don't run
extensions in incognito windows).

http://codereview.chromium.org/164151/diff/9/22
File chrome/browser/extensions/crashed_extension_infobar.h (right):

http://codereview.chromium.org/164151/diff/9/22#newcode17
Line 17: class CrashedExtensionInfoBarDelegate : public ConfirmInfoBarDelegate {
need class level comments.

http://codereview.chromium.org/164151/diff/9/22#newcode18
Line 18: public:
Needs class level comments.

http://codereview.chromium.org/164151/diff/9/22#newcode35
Line 35: const std::string extension_id_;
Need field comments.

http://codereview.chromium.org/164151/diff/9/12
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/164151/diff/9/12#newcode400
Line 400: {
Nit: brace should go on previous line.

http://codereview.chromium.org/164151/diff/9/12#newcode405
Line 405: // (either fully operational, or fully unloaded, but not
half-crashed).
Nice, and good comment.

[Ben Goodger (Google), 2009-08-13 03:31:32]

Seems OK to me.

-Ben

On Mon, Aug 10, 2009 at 11:43 AM, <phajdan.jr@chromium.org> wrote:
> Please review:
>
> aa: extension stuff
> Ben: if I didn't abuse anything I shouldn't
>
> http://codereview.chromium.org/164151
>

[Aaron Boodman, 2009-08-18 00:53:44]

This can be closed now, right?

[Paweł Hajdan Jr., 2009-08-18 20:09:45]

No, still can't get it into the tree for good.

On Mon, Aug 17, 2009 at 17:53, <aa@chromium.org> wrote:

> This can be closed now, right?
>
>
> http://codereview.chromium.org/164151
>