759502 - include URL of active window or webapp origin in URL field of webapp runtime crash report

Status: VERIFIED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

When Firefox crashes, the crash reporter sends the server the URL of the active tab, but when the webapp runtime crashes, the crash reporter, when enabled by the fix for bug 745980, does not send the server the URL of the active window.

It should do so, via the URL field of the crash report (or perhaps it should send the server the origin of the app instead), so we have that information when diagnosing the crash.

Comment 1

[Jason Smith [:jsmith]]

Likely this is debatable, but nominating for k9o. We need this fixed - this a big help to diagnosing crashes with the desktop web runtime, as having the URL tells us the source of the problem, including being able to reverse engineer what app crashed.

Comment 2

[Jason Smith [:jsmith]]

Marking for re-triage after talking with Marcia from crashkill. There's going to be a time where we're going to really say we need these URLs to diagnose a "tough" crash that might be hard to reproduce, especially when our user base jumps higher overtime. I don't think we should block on this, but I'm wondering if this should be a P1 instead of a P2.

Comment 3

[Jason Smith [:jsmith]]

We need to find an owner for this bug, as we would like to get this uplifted if possible to FF 16 for more effective crash data. Felipe or Myk - Could you take this?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

This should be the patch. But I don't know how to manually verify that the crash is being submitted with the right information. I added a print at the end function and it is being called at the right moment.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Felipe Gomes (:felipe) from comment #4)
> Created attachment 643669 [details] [diff] [review]
> Patch
> 
> This should be the patch. But I don't know how to manually verify that the
> crash is being submitted with the right information. I added a print at the
> end function and it is being called at the right moment.

Hmm. We could manually verify this by causing a crash in the web runtime with this patch. Then, we need a link to the crash report. Next, someone from crashkill could take a look at the crash report and see if the URL was included or not.

Myk has a test app for testing crashes btw here - http://mykzilla.org/app/. Could you invoke a crash with this test app with your patch applied and paste the link to the crash report here?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Here's a link to one crash report I submitted with it:
https://crash-stats.mozilla.com/report/index/bp-1869f59a-27d5-4d75-9eee-8917f2120719

Can someone look if the URL was included in the report? The URL is of a bugzilla attachment

Comment 7

[Jason Smith [:jsmith]]

(In reply to :Felipe Gomes [offline 20-24, slow resp. 25-28] from comment #6)
> Here's a link to one crash report I submitted with it:
> https://crash-stats.mozilla.com/report/index/bp-1869f59a-27d5-4d75-9eee-
> 8917f2120719
> 
> Can someone look if the URL was included in the report? The URL is of a
> bugzilla attachment

Juan, Kairo, or Marcia - Could you look at this crash report and see if there is a URL associated with it?

Comment 8

[juan becerra [:juanb]]

There is URL associated with this report, a bugzilla attachment followed by "Super Sensitive! Don't mess around!"

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Thanks Juan for checking

Comment 10

[Robert Kaiser]

(In reply to juan becerra [:juanb] from comment #8)
> "Super Sensitive! Don't mess around!"

That's added to every URL and email shown, just to ensure you know this can be privacy-relevant. :)

Felipe, is that the URL of the app that we're getting there now, or how do we determine which URL we get there?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 643669 [details] [diff] [review]
Patch

The Firefox version does something slightly different:
http://hg.mozilla.org/mozilla-central/annotate/01929e390ba5/browser/base/content/browser.js#l4402

I think onStateChange fires earlier, so we have a better chance of catching crashes caused by the loading of a page?

It also checks gCrashReporter.enabled, and doesn't do the URI sanitizing (seems like something that should probably be fixed).

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 643669 [details] [diff] [review]
Patch

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

::: webapprt/content/webapp.js
@@ +189,5 @@
> +function updateCrashReportURL(aURI) {
> +#ifdef MOZ_CRASHREPORTER
> +  let uri = aURI.clone();
> +  if (uri.userPass != "") {
> +    uri.userPass = "";

I forgot to add a try/catch around this setter, consider it added

Comment 13

[:Felipe Gomes (needinfo for replies!)]

oops I did not see the previous comments before adding mine.

(In reply to Robert Kaiser (:[e-mail]) from comment #10)
> Felipe, is that the URL of the app that we're getting there now, or how do
> we determine which URL we get there?

That's the URL of the current page being displayed by the app at the time of the crash

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> The Firefox version does something slightly different:
> http://hg.mozilla.org/mozilla-central/annotate/01929e390ba5/browser/base/
> content/browser.js#l4402
> 
> I think onStateChange fires earlier, so we have a better chance of catching
> crashes caused by the loading of a page?

Yeah.. We also do onTabSelect (http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#1348) so I wasn't sure if the stateChange was just to override the URL for a crash caused by a page loading in a background tab.  The testcase that I tried with OnLocationChange has the final URL of the crashing page, but that doesn't prove it is soon enough for other cases. I will change it to OnStateChange.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v2

Using OnStateChange

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 644084 [details] [diff] [review]
Patch v2

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

::: webapprt/content/webapp.js
@@ +39,5 @@
> +  onStateChange: function onStateChange(aProgress, aRequest, aFlags, aStatus) {
> +    if (aRequest instanceof Ci.nsIChannel &&
> +        aFlags & Ci.nsIWebProgressListener.STATE_START &&
> +        aFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) {
> +      updateCrashReportURL(aRequest.URI);

The STATE_IS_DOCUMENT check shouldn't be needed given that you pass NOTIFY_STATE_DOCUMENT, right?

Comment 16

[:Felipe Gomes (needinfo for replies!)]

right, but I'm afraid of someone adding more notifications in the future and not realizing the need to check STATE_IS_DOCUMENT then. Should I keep it?

Comment 17

[:Felipe Gomes (needinfo for replies!)]

I kept the check and can remove it later if you prefer

https://hg.mozilla.org/integration/mozilla-inbound/rev/77b55025a451

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/77b55025a451

Comment 19

[Robert Kaiser]

(In reply to :Felipe Gomes [offline 20-24, slow resp. 25-28] from comment #13)
> That's the URL of the current page being displayed by the app at the time of
> the crash

OK, in the longer run, we should monitor if we need to send some identifier of the app itself as well. For now, I guess that URL is good.

Comment 20

[Jason Smith [:jsmith]]

I've generated a crash report on Windows 7 below with the latest Nightly:

https://crash-stats.mozilla.com/report/index/bp-02dc573f-d9d1-49fb-b357-9d6c02120723

Could someone take a look at this crash report and verify that the URL is equal to https://bugzilla.mozilla.org/attachment.cgi?id=208471?

Comment 21

[Jason Smith [:jsmith]]

Verified through seeing the URL in the crash report in comment 20 in socorro. Please nominate for Aurora uplift.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 644084 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is an enhancement rather than a bugfix and is a followup to integration of the crash reporter into the desktop webapp runtime (bug 745980).
User impact if declined: It will be harder for developers to diagnose and fix crashes that are caused by a specific page a webapp loads.
Testing completed (on m-c, etc.): This landed on central six days ago and has been verified by QA.
Risk to taking this patch (and alternatives if risky): It's possible that the patch could cause a performance or functionality regression in the runtime, although the change is relatively minor, so the risk seems low.
String or UUID changes made by this patch: None.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 644084 [details] [diff] [review]
Patch v2

Low enough risk and the benefits of this enhancement are high, worth uplifting to Aurora.

Comment 24

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e18b0ecc9b83