Closed Bug 759502 Opened 10 years ago Closed 10 years ago

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

Categories

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

14 Branch
defect

Tracking

(blocking-kilimanjaro:+, firefox16 verified)

VERIFIED FIXED
Firefox 17
blocking-kilimanjaro +
Tracking Status
firefox16 --- verified

People

(Reporter: myk, Assigned: Felipe)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 1 obsolete file)

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.
Priority: -- → P2
Target Milestone: --- → Firefox 15
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.
blocking-kilimanjaro: --- → ?
blocking-kilimanjaro: ? → +
Target Milestone: Firefox 15 → ---
QA Contact: 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.
Priority: P2 → --
Priority: -- → P1
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?
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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.
(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?
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
(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?
There is URL associated with this report, a bugzilla attachment followed by "Super Sensitive! Don't mess around!"
Thanks Juan for checking
Attachment #643669 - Flags: review?(gavin.sharp)
(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 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 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
oops I did not see the previous comments before adding mine.

(In reply to Robert Kaiser (:kairo@mozilla.com) 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 gavin@gavinsharp.com 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.
Attachment #643669 - Flags: review?(gavin.sharp)
Attached patch Patch v2Splinter Review
Using OnStateChange
Attachment #643669 - Attachment is obsolete: true
Attachment #644084 - Flags: review?(gavin.sharp)
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?
Attachment #644084 - Flags: review?(gavin.sharp) → review+
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?
I kept the check and can remove it later if you prefer

https://hg.mozilla.org/integration/mozilla-inbound/rev/77b55025a451
Target Milestone: --- → Firefox 17
Whiteboard: [qa+]
https://hg.mozilla.org/mozilla-central/rev/77b55025a451
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(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.
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?
Verified through seeing the URL in the crash report in comment 20 in socorro. Please nominate for Aurora uplift.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
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.
Attachment #644084 - Flags: approval-mozilla-aurora?
Comment on attachment 644084 [details] [diff] [review]
Patch v2

Low enough risk and the benefits of this enhancement are high, worth uplifting to Aurora.
Attachment #644084 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa!] → [qa+]
Whiteboard: [qa+] → [qa!]
Blocks: 806515
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.