[e10s] make the "Try again" button reload only the selected tab

VERIFIED FIXED in Firefox 35



5 years ago
5 years ago


(Reporter: cpeterson, Assigned: Felipe)


Firefox 35
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags




(1 attachment)

This is a workaround for bug 1066653, where trying to reload all tabs crashes the browser.
We need to make sure this doesn't break crash reporting.
Assignee: nobody → felipc
I think this is the wrong way to go. Since this is a stop gap until we have bug 1065785, can we instead just revert to our previous behavior of the Try Again button just reloading one tab instead of all of them?
It will help ensure that the crash report is sent, reduce the chance that the offending tab(s) will crash the browser again, and let people continue using their session normally. And if someone really wants all tabs reloaded, they can quit and restart by themselves and let SessionStore do its job.
And reload the other crashed tabs on demand? That sounds like a better UX and probably simpler to implement. :)
Flags: firefox-backlog+
Flags: qe-verify?
Setting QE+ so that someone can double check that the crash reports are submitted correctly
Iteration: --- → 35.2
Points: --- → 2
Flags: qe-verify? → qe-verify+
Summary: [e10s] replace crashed tab page's "Try again" button to restart Firefox (and submit crash reports) instead of reloading all tabs → [e10s] make the "Try again" button reload only the selected tab
Posted patch PatchSplinter Review
This makes the Try Again button submit the crash report, remove the crashreport checkbox from the other tabs, and reload the selected tab.
Attachment #8490481 - Flags: review?(ally)
Comment on attachment 8490481 [details] [diff] [review]

(Whoever gets to it first :)
Attachment #8490481 - Flags: review?(evilpies)
FWIW I looked into setting the other tabs to be lazily restored. While setting browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE is enough for it to jump in the right functions, the SS data necessary for it to work is not there, so this will have to come from the more complete solution from bug 1065785.
Iteration: 35.2 → ---
Comment on attachment 8490481 [details] [diff] [review]

Review of attachment 8490481 [details] [diff] [review]:

Overall I think it looks pretty good. Please help me understand a section a bit more and I'll give you an r+ ?

::: browser/modules/TabCrashReporter.jsm
@@ +59,5 @@
>        return
>      if (CrashSubmit.submit(dumpID, { recordSubmission: true })) {
>        this.childMap.set(childID, null); // Avoid resubmission.
> +      this.removeSubmitCheckboxesForSameCrash(childID);

Could you please explain why this is necessary? I know this is a bandaid of sorts, but I don't quite get this part.

@@ +91,5 @@
> +    if (browser.isRemoteBrowser)
> +      return;
> +
> +    let doc = browser.contentDocument;
> +    if (!doc.documentURI.startsWith("about:tabcrashed"))

nit of nits: I understand we're supposed to brace even 1 line nits, but I notice that your code matches the style in the file, so you're off the hook

@@ +107,5 @@
>      let dumpID = this.childMap.get(this.browserMap.get(aBrowser));
>      if (!dumpID)
>        return;
> +    aBrowser.contentDocument.documentElement.classList.add("crashDumpAvailable");


::: browser/themes/linux/aboutTabCrashed.css
@@ +29,5 @@
>    margin: 0 auto;
>    display: none;
>  }
> +.crashDumpAvailable #report-box {

thank you for fixing the typo :)
Attachment #8490481 - Flags: review?(ally) → feedback+
Iteration: --- → 35.2
Attachment #8490481 - Flags: feedback+ → review+
Attachment #8490481 - Flags: review?(evilpies)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
QA Contact: jbecerra
QA Contact: jbecerra → cornel.ionce
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0

Verified fixed on Firefox 35.0a1 Nightly, build ID: 20140922030204
You need to log in before you can comment on or make changes to this bug.