1373271 - Handle crashes in preloaded browsers in tabbrowser

Status: RESOLVED FIXED

(Firefox Graveyard :: Activity Streams: General, enhancement)

Description

[Kate Hudson :k88hudson]

When testing with activity-stream pref'd on, we discovered that crashes in a preloaded browser were throwing exceptions in following test:

browser/components/sessionstore/test/browser_background_tab_crash.js

The main problem is that preloaded browsers don't actually contain any tabs, so getTabForContent will end up returning undefined.

Instead, we should just releases the crashed preloaded browser.

Comment 1

[Kate Hudson :k88hudson]

Attachment: Bug 1373271 - Handle crashes in preloaded browsers in tabbrowser

Review commit: https://reviewboard.mozilla.org/r/149498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149498/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8878087 [details]
Bug 1373271 - Handle crashes in preloaded browsers in tabbrowser

https://reviewboard.mozilla.org/r/149498/#review154062

This looks great, k88hudson, thanks!

Just two suggestions, see below.

::: browser/base/content/tabbrowser.xml:5621
(Diff revision 1)
>            let browser = event.originalTarget;
> +
> +          // Preloaded browsers do not actually have any tabs. If one crashes,
> +          // it should simply be removed.
> +          if (browser === this._preloadedBrowser) {
> +            let preloaded = this._getPreloadedBrowser();

Maybe it'd be worth mentioning that we're not just doing `browser.remove();` because `_getPreloadedBrowser()` is needed to _consume_ the preloaded browser.

::: browser/components/sessionstore/test/browser_background_tab_crash.js:232
(Diff revision 1)
> +  }
> +
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:robots");
> +  Assert.ok(gBrowser._preloadedBrowser);
> +
> +  await BrowserTestUtils.crashBrowser(gBrowser._preloadedBrowser, false);

By default, the preloaded browser is at about:newtab, which is not crash-able.

So I think this exception is going to be thrown:

http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#990

We should probably either:

a) Ensure that Activity Stream is enabled for this particular subtest, and that the preloaded browser is browsed to it (and remote).
b) Manually browse the preloaded browser to a remote location, like http://example.com (which is a special URI in Mochitests, and doesn't actually hit the network, but is treated like a web address).

Comment 3

[Kate Hudson :k88hudson]

Comment on attachment 8878087 [details]
Bug 1373271 - Handle crashes in preloaded browsers in tabbrowser

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149498/diff/1-2/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8878087 [details]
Bug 1373271 - Handle crashes in preloaded browsers in tabbrowser

https://reviewboard.mozilla.org/r/149498/#review154612

Assuming a green try run, r=me! Good stuff!

::: browser/components/sessionstore/test/browser_background_tab_crash.js:223
(Diff revision 2)
>        await tabRestored;
>      });
>    });
>  });
> +
> +// Tests that preloaded tabs are removed and no unexpected errors are thrown.

"Tests that preloaded tabs are removed" -> "Tests that crashed preloaded tabs are removed".

Comment 5

[Kate Hudson :k88hudson]

Comment on attachment 8878087 [details]
Bug 1373271 - Handle crashes in preloaded browsers in tabbrowser

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149498/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65ff5356cdfa
Handle crashes in preloaded browsers in tabbrowser r=mconley

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for apparently leaking a window: https://treeherder.mozilla.org/logviewer.html#?job_id=108355458&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/14f4d223c014d7dae4302e7cbe2450e0488d3c3f

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8878087 [details]
Bug 1373271 - Handle crashes in preloaded browsers in tabbrowser

https://reviewboard.mozilla.org/r/149498/#review155750

::: browser/components/sessionstore/test/browser_background_tab_crash.js:236
(Diff revision 3)
> +  // Release any existing preloaded browser
> +  gBrowser._getPreloadedBrowser();

Whoops, I missed this. :/

We're consuming the preloaded browser here, but it's still in the DOM. You should probably do somethign like:

```js
let preloaded = gBrowser._getPreloadedBrowser();
preloaded.remove();
```

To fully clear it out.

Comment 9

[Kate Hudson :k88hudson]

Comment on attachment 8878087 [details]
Bug 1373271 - Handle crashes in preloaded browsers in tabbrowser

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149498/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/761383b60ca2
Handle crashes in preloaded browsers in tabbrowser r=mconley

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/761383b60ca2