1109650 - Add "Restore all crashed tabs" button to tab crashed page

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

We settled on UX for the tab crashed page in bug 913651, and while it's not yet necessary to style that page as the spec requests, the "Restore all crashed tabs" button would be super handy to have, even in our little placeholder about:tabcrashed page.

Clicking that button should, naturally, restore all tab crashed pages. If the user has the pref set to only load restored tabs on demand (which is the default), then we shouldn't actually reload the page until the tab is selected.

This would be really nice for our Nightly testers to use, since we apparently crash tabs all of the time.

Comment 1

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

We should do this soon - while we're at it, we should maybe beef up the rest of about:tabcrashed to show the strings from bug 913651 to get them in under string freeze (as we will actually ship these strings).

Then we can style them all up down the road in bug 1110511.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch

This patch applies on top of bug 1112304 and adds a button to restore all crashed tabs. The current tab reloads immediately, the others restore as they would at startup, pinned tabs reload quickly, others wait till you switch to them.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8550389 [details] [diff] [review]
patch

Review of attachment 8550389 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/SessionStore.jsm
@@ +2041,5 @@
> +   */
> +  reviveAllCrashedTabs() {
> +    this._forEachBrowserWindow(window => {
> +      for (let tab of window.gBrowser.tabs) {
> +        let browser = tab.linkedBrowser;

We shouldn't do this here. We have ss.reviveCrashedTab() already and can iterate through all tabs of all windows in browser.js and call ss.reviveCrashedTab(tab) for all of those.

@@ +2688,5 @@
>      browser.__SS_data = tabData;
>      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
>      browser.setAttribute("pending", "true");
>      tab.setAttribute("pending", "true");
> +    tab.removeAttribute("crashed");

SessionStore, until now, doesn't know about that attribute - it doesn't set or remove it. It listens for oop-browser-crashed and maintains as WeakMap of crashed browsers. Why is it necessary now and can we let the part of the code do it that sets the attribute? Why wasn't this needed when reviving single tabs?

::: browser/components/sessionstore/test/browser_crashedTabs.js
@@ +399,5 @@
> +
> +  // Crash the tab
> +  yield crashBrowser(browser);
> +
> +  is(newTab2.getAttribute("crashed"), "true", "Second tab should be crashed too.");

That might not be true in the future with multiple content processes? Probably fine for now.

@@ +402,5 @@
> +
> +  is(newTab2.getAttribute("crashed"), "true", "Second tab should be crashed too.");
> +
> +  // Flush out any notifications from the crashed browser.
> +  TabState.flush(browser);

What state changes are there after the tab has crashed? Do we really need that?

@@ +406,5 @@
> +  TabState.flush(browser);
> +
> +  // Use SessionStore to revive all the tabs
> +  clickButton(browser, "restoreAll");
> +  yield promiseBrowserLoaded(browser);

yield promiseTabRestored(newTab);

Reviving a crashed browser restores its contents. See bug 1093655.

@@ +413,5 @@
> +  ok(!newTab2.hasAttribute("crashed"), "Second tab shouldn't be marked as crashed anymore.");
> +  ok(newTab2.hasAttribute("pending"), "Second tab should be pending.");
> +
> +  gBrowser.selectedTab = newTab2;
> +  yield promiseBrowserLoaded(browser2);

yield promiseTabRestored(newTab2);

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 2

(In reply to Tim Taubert [:ttaubert] from comment #3)
> @@ +2688,5 @@
> >      browser.__SS_data = tabData;
> >      browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
> >      browser.setAttribute("pending", "true");
> >      tab.setAttribute("pending", "true");
> > +    tab.removeAttribute("crashed");
> 
> SessionStore, until now, doesn't know about that attribute - it doesn't set
> or remove it. It listens for oop-browser-crashed and maintains as WeakMap of
> crashed browsers. Why is it necessary now and can we let the part of the
> code do it that sets the attribute? Why wasn't this needed when reviving
> single tabs?

This attribute shows a chrashed overlay on the tab icon, it's added by bug 1112304. It isn't needed because the patch in bug 1112304 removes it when a load commences but that doesn't happen for pending tabs. I've moved it to updateBrowserRemoteness which I guess sort of makes sense.

> ::: browser/components/sessionstore/test/browser_crashedTabs.js
> @@ +399,5 @@
> > +
> > +  // Crash the tab
> > +  yield crashBrowser(browser);
> > +
> > +  is(newTab2.getAttribute("crashed"), "true", "Second tab should be crashed too.");
> 
> That might not be true in the future with multiple content processes?
> Probably fine for now.

We will have to revisit this code then anyway.

> @@ +402,5 @@
> > +
> > +  is(newTab2.getAttribute("crashed"), "true", "Second tab should be crashed too.");
> > +
> > +  // Flush out any notifications from the crashed browser.
> > +  TabState.flush(browser);
> 
> What state changes are there after the tab has crashed? Do we really need
> that?

I guess not, removed from the rest of the test.

Comment 6

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/6d3e147b1539

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6d3e147b1539

Comment 8

[Ada [:adalucinet]]

Verified on Nightly 38.0a1 (Build ID: 20150204030234) with e10s enabled under Linux 12.04 32-bit, Windows 7 64-bit and Mac OS X 10.9.5 - "Restore All Crashed Tabs" button is present in tab crashed page, is the only one highlighted and is working accordingly.