Closed Bug 913155 Opened 7 years ago Closed 6 years ago

Tab crashed page: Submit crashreport

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Attached patch submit-crash (obsolete) — Splinter Review
We want to have a way to submit crash reports from the tab crashed page. Most of the complexity in this patch comes from the fact that: 
When a browser crashes we first destruct the TabParent/PBrowser, and then its owner ContentParent/PBrowser. For every crashed TabParent we show the page crashed page. But only when the ContentParent is destroyed we actually find out if we have a crashID. Furthermore after tabs are killed it's very hard to find out which ContentParent mapped to which browser, this is why we keep a mapping of the owner childID for every crashed tab. After the tab crashed page is created, the page asks whether there is a crash-report for its child it. Anyway I am open to better solutions.
Attached patch submit-crash (obsolete) — Splinter Review
Fixed some problems after looking at the [Review] :)
Attachment #800309 - Attachment is obsolete: true
Do you have a mockup? I'd like this to be very similar in appearance and behavior to a crashed-plugin, and in particular we will want to allow people to enter comments and an email address. Not necessary in the short term, if you'd like it to be a followup, but if we can steal that UI and do it in one go that would be great.
FYI the B2G crash UI mockups are here:
http://people.mozilla.com/~lco/Crash_Reporting_B2G/R1_Crash%20Reports%20v1.pdf

Not sure how much of that translates to desktop, though.
Actually that looks like something we could use, for now I just want some way to submit crash reports while we are still in a test phase.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attached patch wip v2 (obsolete) — Splinter Review
This includes mostly what felipe wants, I think? Haven't renamed the JSM yet. I need to look at localizing the buttons and finding out why disabled on button doesn't work as expected.
Attachment #800310 - Attachment is obsolete: true
Comment on attachment 800955 [details] [diff] [review]
wip v2

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

Excellent!

::: browser/modules/AboutTabCrashed.jsm
@@ +65,5 @@
> +            return;
> +
> +          let submitCrash = browser.contentDocument.getElementById("submitCrash");
> +          submitCrash.style.display = "inline";
> +          submitCrash.textContent = "Submitted. Thank you!"; // Localization stuff.

for this you can use the browser.properties string bundle and access it using the aBrowser element that we got, like it's done here:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/webappsUI.jsm#121

::: browser/themes/linux/aboutTabCrashed.css
@@ +99,5 @@
> +
> +/* Hide it first, because not every crash, generates a crash report. */
> +#submitCrash {
> +  display: none;
> +}

you'll need to include this change to the win/osx themes too

I think you can remove that comment, the reason is clear

::: content/base/public/nsIFrameLoader.idl
@@ +257,5 @@
>     */
>    readonly attribute nsIDOMElement ownerElement;
>  
> +
> +  readonly attribute unsigned long long childID;

add a brief doc comment here

::: content/base/src/nsFrameLoader.cpp
@@ +2060,5 @@
>    NS_ENSURE_TRUE(rv, false);
>  
>    nsCOMPtr<Element> ownerElement = mOwnerContent;
>    mRemoteBrowser = ContentParent::CreateBrowserOrApp(context, ownerElement);
> +  mChildID = mRemoteBrowser->Manager()->ChildID();

the next line seems to indicate that mRemoteBrowser might be null :)
Attachment #800955 - Flags: feedback+
Attached patch v1 (obsolete) — Splinter Review
Only missing part is this still the weirdness with the "disabled" button.
Attachment #800955 - Attachment is obsolete: true
Can you attach a screenshot for those of us too lazy to try this out ourselves? Also, I guess I'll have to update my "Crash me now" extension to add back that "Crash content process" feature.
Attached image Screenshot
Comment on attachment 801569 [details]
Screenshot

This looks okay as a first cut, but I have a suggestion. I think we should present something more like:

[x] Tell Mozilla about this crash so they can fix it ( http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/crashreporter/crashreporter.ini#28 )
[ Reload this tab ]

The only actions the user can take here are "close the tab" or "reload", and there's already plenty of UI for closing the tab, so I'm not sure that needs a button. "Try again" is not very clear, "Reload this tab" seems more explicit. Also, in all of our other crash UI, "Submit this report" is a checkbox, not a button, so I think we should do that here.

I'll leave further design to the UX team, but I think those are simple changes.

Also, the strings for the plugin crash UI are here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd#53
So if somebody ticks that box, would we send the crash report when they close the tab or reload? I think one advantage of this UI is that we don't have to implement the complex logic to disable the submit button in every tab. In in general I like this idea. 

Should we reuse the strings by referencing this dtd file?
Yes, although it might be okay to only submit if they click "reload", and not handle the closing the tab case.

You should look at what we do for the plugin code, since we already have to handle the same situation there (one plugin process crash can result in multiple crashed plugin placeholders).
Attached patch submit-crash (obsolete) — Splinter Review
I think we should probably still submit a crashreporter if the tab is just closed. I looked at the plugin code before and it doesn't seem to relevant, it's quite different. We do reuse CrashSubmit.jsm.
Attachment #800998 - Attachment is obsolete: true
Attachment #801719 - Flags: review?(felipc)
Comment on attachment 801719 [details] [diff] [review]
submit-crash

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

nsFrameLoader changes seem simple to me but Olli should take a look.

::: browser/base/content/aboutTabCrashed.xhtml
@@ +15,5 @@
>    <!ENTITY % browserDTD
>      SYSTEM "chrome://browser/locale/browser.dtd">
>    %browserDTD;
> +  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
> +  %brandDTD;

not needed?

::: browser/modules/TabCrashReporter.jsm
@@ +51,5 @@
> +
> +  submitCrashReport: function (aBrowser) {
> +    let childID = this.browserMap.get(aBrowser);
> +    let chromeWin = aBrowser.ownerDocument.defaultView;
> +    let bundle = chromeWin.gNavigatorBundle;

chromeWin/bundle not used anymore

@@ +58,5 @@
> +    if (!dumpID)
> +      return
> +
> +    if (CrashSubmit.submit(dumpID)) {
> +      this.dumpMap.set(childID, null); // Avoid resubmission.

I think that on successful submission we'll need to clear the browserMap too. If another later crash happen with the same browser, and say that for some reason this time it didn't have a crashdump, we don't want to see the previous dumpID and display the checkbox.

Also, we should remove the checkbox from the other tabs if it succeeded.. Could both hide it or show a text "Crash report submitted" for example

@@ +62,5 @@
> +      this.dumpMap.set(childID, null); // Avoid resubmission.
> +    }
> +  },
> +
> +  onPageLoad: function (aBrowser) {

onAboutTabCrashedLoad

@@ +67,5 @@
> +    let dumpID = this.dumpMap.get(this.browserMap.get(aBrowser));
> +    if (!dumpID)
> +      return;
> +
> +    aBrowser.contentDocument.getElementById("report-box").style.display = "block";

please add a class to the css (e.g., .crashDumpAvailable) that sets display: block through the CSS, and call .classList.add("...") here


A suggestion for an alternative:
the cssClass param for error pages (&s=) seems to be a perfect fit for this case if you want to avoid the onPageLoad altogether.  The frameloader-crashed observer (or browser-crashed event) could set an attribute that nsDocShell::DisplayLoadError will pick up and set the proper value

Even the dumpId could be embedded in the url (using the error param &e=) so you don't need to worry about keeping them on a map and cleaning up
Attachment #801719 - Flags: review?(felipc)
Attachment #801719 - Flags: review?(bugs)
Attachment #801719 - Flags: feedback+
Comment on attachment 801719 [details] [diff] [review]
submit-crash

In the comment for childID explain what happens in non-e10s case, and
I wonder if childID should be called remoteChildID. I guess it should


* goes with the type and parameters have 'a' prefix, so
+nsFrameLoader::GetChildID(uint64_t *childID)
uint64_t* aChildID
Attachment #801719 - Flags: review?(bugs) → review+
(In reply to :Felipe Gomes from comment #14)
> Comment on attachment 801719 [details] [diff] [review]
> submit-crash
> 
> Review of attachment 801719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nsFrameLoader changes seem simple to me but Olli should take a look.
> 
> ::: browser/base/content/aboutTabCrashed.xhtml
> @@ +15,5 @@
> >    <!ENTITY % browserDTD
> >      SYSTEM "chrome://browser/locale/browser.dtd">
> >    %browserDTD;
> > +  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
> > +  %brandDTD;
> 
> not needed?
Required for "Tell &vendorShortName; .." in checkSendReport.
> 
> ::: browser/modules/TabCrashReporter.jsm
> @@ +51,5 @@
> > +
> > +  submitCrashReport: function (aBrowser) {
> > +    let childID = this.browserMap.get(aBrowser);
> > +    let chromeWin = aBrowser.ownerDocument.defaultView;
> > +    let bundle = chromeWin.gNavigatorBundle;
> 
> chromeWin/bundle not used anymore
> 
> @@ +58,5 @@
> > +    if (!dumpID)
> > +      return
> > +
> > +    if (CrashSubmit.submit(dumpID)) {
> > +      this.dumpMap.set(childID, null); // Avoid resubmission.
> 
> I think that on successful submission we'll need to clear the browserMap
> too. If another later crash happen with the same browser, and say that for
> some reason this time it didn't have a crashdump, we don't want to see the
> previous dumpID and display the checkbox.
> 
> Also, we should remove the checkbox from the other tabs if it succeeded..
> Could both hide it or show a text "Crash report submitted" for example
>
With the new design I hoped to just avoid this complexity.
> @@ +62,5 @@
> > +      this.dumpMap.set(childID, null); // Avoid resubmission.
> > +    }
> > +  },
> > +
> > +  onPageLoad: function (aBrowser) {
> 
> onAboutTabCrashedLoad
> 
> @@ +67,5 @@
> > +    let dumpID = this.dumpMap.get(this.browserMap.get(aBrowser));
> > +    if (!dumpID)
> > +      return;
> > +
> > +    aBrowser.contentDocument.getElementById("report-box").style.display = "block";
> 
> please add a class to the css (e.g., .crashDumpAvailable) that sets display:
> block through the CSS, and call .classList.add("...") here
> 
Ok that is neat!
> 
> A suggestion for an alternative:
> the cssClass param for error pages (&s=) seems to be a perfect fit for this
> case if you want to avoid the onPageLoad altogether.  The
> frameloader-crashed observer (or browser-crashed event) could set an
> attribute that nsDocShell::DisplayLoadError will pick up and set the proper
> value
> 
> Even the dumpId could be embedded in the url (using the error param &e=) so
> you don't need to worry about keeping them on a map and cleaning up

No this doesn't work, because this event happens after the error page will be loaded.
(In reply to Tom Schuster [:evilpie] from comment #16)
> Required for "Tell &vendorShortName; .." in checkSendReport.

Hmm ok.. this is a bit strange, but there are other strings in browser.dtd that depends on brand.dtd, so I don't see why not add another one.

> > 
> > A suggestion for an alternative:
> > the cssClass param for error pages (&s=) seems to be a perfect fit for this
> > case if you want to avoid the onPageLoad altogether.  The
> > frameloader-crashed observer (or browser-crashed event) could set an
> > attribute that nsDocShell::DisplayLoadError will pick up and set the proper
> > value
> > 
> > Even the dumpId could be embedded in the url (using the error param &e=) so
> > you don't need to worry about keeping them on a map and cleaning up
> 
> No this doesn't work, because this event happens after the error page will
> be loaded.

fwiw, it wouldn't be driven by this event anymore, it could be done by the oop-frameloader-crashed observer which happens before
Attached patch submit-crash v2 (obsolete) — Splinter Review
Attachment #801719 - Attachment is obsolete: true
Attachment #803826 - Flags: review?(felipc)
Attached patch submit-crash v2Splinter Review
For real this time! Contains most of the fixes requested. I will look at disabling the checkbox after submission in an other bug.
Attachment #803826 - Attachment is obsolete: true
Attachment #803826 - Flags: review?(felipc)
Attachment #803827 - Flags: review?(felipc)
Comment on attachment 803827 [details] [diff] [review]
submit-crash v2

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

::: browser/base/content/browser.js
@@ +1048,5 @@
>      Services.logins;
>  
> +#ifdef MOZ_CRASHREPORTER
> +    if (gMultiProcessBrowser)
> +      TabCrashReporter.init();

I missed this in the previous pass, sry. This will init TabCrashReporter for every new window.  So either move this to nsBrowserGlue, or in init() protect it from running more than once.

@@ +4284,5 @@
>            event.target.documentElement.removeAttribute("hasBrowserHandlers");
>        }, true);
> +
> +#ifdef MOZ_CRASHREPORTER
> +      if (doc.documentURI.toLowerCase().startsWith("about:tabcrashed"))

no need for toLowerCase()

::: browser/modules/TabCrashReporter.jsm
@@ +59,5 @@
> +      this.childMap.set(childID, null); // Avoid resubmission.
> +    }
> +  },
> +
> +  onPageLoad: function (aBrowser) {

change this function to be named onAboutTabCrashedLoad
Attachment #803827 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/394c53ce3e36
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.