Closed Bug 1109650 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: General, defect)

x86
All
defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
e10s m5+ ---

People

(Reporter: mconley, Assigned: mossop)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → 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.
Flags: firefox-backlog+
Points: --- → 5
Flags: qe-verify+
Assignee: mconley → dtownsend
Status: NEW → ASSIGNED
Iteration: --- → 38.1 - 26 Jan
Blocks: 1112304
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8550389 - Flags: review?(smacleod)
Attachment #8550389 - Flags: review?(ttaubert)
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);
Attachment #8550389 - Flags: review?(ttaubert)
Attachment #8550389 - Flags: review?(smacleod)
Attachment #8550389 - Flags: feedback+
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
Duplicate of this bug: 1124940
Attached patch patch rev 2Splinter Review
(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.
Attachment #8550389 - Attachment is obsolete: true
Attachment #8555526 - Flags: review?(ttaubert)
Attachment #8555526 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/6d3e147b1539
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.