913155 - Tab crashed page: Submit crashreport

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tom S [:evilpie]]

Attachment: submit-crash

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.

Comment 1

[Tom S [:evilpie]]

Attachment: submit-crash

Fixed some problems after looking at the [Review] :)

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[(not currently active) Ted Mielczarek]

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.

Comment 4

[Tom S [:evilpie]]

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.

Comment 5

[Tom S [:evilpie]]

Attachment: wip v2

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.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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 :)

Comment 7

[Tom S [:evilpie]]

Attachment: v1

Only missing part is this still the weirdness with the "disabled" button.

Comment 8

[(not currently active) Ted Mielczarek]

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.

Comment 9

[Tom S [:evilpie]]

Attachment: Screenshot

Comment 10

[(not currently active) Ted Mielczarek]

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

Comment 11

[Tom S [:evilpie]]

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?

Comment 12

[(not currently active) Ted Mielczarek]

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).

Comment 13

[Tom S [:evilpie]]

Attachment: submit-crash

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.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

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

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 16

[Tom S [:evilpie]]

(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.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

(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

Comment 18

[Tom S [:evilpie]]

Attachment: submit-crash v2

Comment 19

[Tom S [:evilpie]]

Attachment: submit-crash v2

For real this time! Contains most of the fixes requested. I will look at disabling the checkbox after submission in an other bug.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

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

Comment 21

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/394c53ce3e36

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/394c53ce3e36