Fix a couple of crashes related to app-crashed balloon.

Fix a couple of crashes related to app-crashed balloon.

Close the crash-notification balloon if background contents is restarted for an app. And check for valid values when loading background contents.

BUG=82492, 82505
TEST=manually

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

[sadrul, 2011-05-13 05:04:43]

Currently navigating to a sad tab does not trigger a reload. That is done in:
http://codereview.chromium.org/7014032/

[Andrew T Wilson (Slow), 2011-05-14 01:01:37]

LGTM, with a couple of caveats - I think the ideal behavior would be to only
open the application tab if there was an application tab open previously, but if
this is hard to achieve I'm OK with this current behavior. It's your call.

We really do need to launch dynamically loaded BackgroundContents - it's OK to
omit that from this patch if you're planning to address it as part of
http://crbug.com/82492, but you should add a TODO to that effect to the code
that's opening the application tab.

http://codereview.chromium.org/7012031/diff/1/chrome/browser/background_conte...
File chrome/browser/background_contents_service.cc (right):

http://codereview.chromium.org/7012031/diff/1/chrome/browser/background_conte...
chrome/browser/background_contents_service.cc:80: if
(!Browser::OpenApplicationTab(profile_, extension, SINGLETON_TAB)) {
I'm not positive this is the right behavior:

1) We aren't reloading dynamically created BackgroundContents, and it seems like
we should.
2) Should we really bring up the launch page for the app? I'm thinking of the
case where an app is running in the background (no tabs are open for the app,
just a background contents) - the background contents crashes, the user clicks
the notification, and we launch the app rather than restarting the background
contents.

[sadrul, 2011-05-15 06:25:45]

Turns out the change I had before is just not necessary at all! The crash in
82505 happens because the return value from GetDictionaryWithoutPathExpansion is
not checked.

For 82492, I agree that it'd be best to close the balloon when the page is
reloaded. A simpler fix, in this CL, is to close any such balloon when the
background page is created for the app (i.e. Step 4 in the bug-report would
close the balloon).

[Andrew T Wilson (Slow), 2011-05-15 16:58:08]

http://codereview.chromium.org/7012031/diff/4001/chrome/browser/background_co...
File chrome/browser/background_contents_service.cc (right):

http://codereview.chromium.org/7012031/diff/4001/chrome/browser/background_co...
chrome/browser/background_contents_service.cc:68:
BackgroundContentsServiceFactory::GetForProfile(profile_)->
So, it seems like there's still a race condition that could lead to a crash
(user reloads page and clicks notification simultaneously). To be safe we should
check to make sure there's no BG contents here before calling
LoadBackgroundContentsForExtension() (or put that check in
LoadBackgroundContentsForExtension()).

[sadrul, 2011-05-15 17:56:54]

http://codereview.chromium.org/7012031/diff/4001/chrome/browser/background_co...
File chrome/browser/background_contents_service.cc (right):

http://codereview.chromium.org/7012031/diff/4001/chrome/browser/background_co...
chrome/browser/background_contents_service.cc:68:
BackgroundContentsServiceFactory::GetForProfile(profile_)->
On 2011/05/15 16:58:08, Andrew T Wilson wrote:
> So, it seems like there's still a race condition that could lead to a crash
> (user reloads page and clicks notification simultaneously). To be safe we
should
> check to make sure there's no BG contents here before calling
> LoadBackgroundContentsForExtension() (or put that check in
> LoadBackgroundContentsForExtension()).

Done (added the check in here).

[Andrew T Wilson (Slow), 2011-05-18 01:12:02]

On 2011/05/15 17:56:54, sadrul wrote:
>
http://codereview.chromium.org/7012031/diff/4001/chrome/browser/background_co...
> File chrome/browser/background_contents_service.cc (right):
> 
>
http://codereview.chromium.org/7012031/diff/4001/chrome/browser/background_co...
> chrome/browser/background_contents_service.cc:68:
> BackgroundContentsServiceFactory::GetForProfile(profile_)->
> On 2011/05/15 16:58:08, Andrew T Wilson wrote:
> > So, it seems like there's still a race condition that could lead to a crash
> > (user reloads page and clicks notification simultaneously). To be safe we
> should
> > check to make sure there's no BG contents here before calling
> > LoadBackgroundContentsForExtension() (or put that check in
> > LoadBackgroundContentsForExtension()).
> 
> Done (added the check in here).

LGTM