The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 17

Status

Firefox Graveyard
Webapp Runtime
P1
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: myk, Assigned: Felipe)

Tracking

14 Branch
Firefox 17

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
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: ? → +
(Reporter)

Updated

5 years ago
Target Milestone: Firefox 15 → ---

Updated

5 years ago
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 → --

Updated

5 years ago
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)

Updated

5 years ago
Assignee: nobody → felipc
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
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.
(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?
(Assignee)

Comment 6

5 years ago
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!"
(Assignee)

Comment 9

5 years ago
Thanks Juan for checking
(Assignee)

Updated

5 years ago
Attachment #643669 - Flags: review?(gavin.sharp)

Comment 10

5 years ago
(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).
(Assignee)

Comment 12

5 years ago
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
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #643669 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

5 years ago
Created attachment 644084 [details] [diff] [review]
Patch v2

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+
(Assignee)

Comment 16

5 years ago
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?
(Assignee)

Comment 17

5 years ago
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

Updated

5 years ago
Whiteboard: [qa+]

Updated

5 years ago
status-firefox16: --- → affected
https://hg.mozilla.org/mozilla-central/rev/77b55025a451
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 19

5 years ago
(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!]
(Reporter)

Comment 22

5 years ago
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+
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/e18b0ecc9b83
status-firefox16: affected → fixed

Updated

5 years ago
Whiteboard: [qa!] → [qa+]

Updated

5 years ago
status-firefox16: fixed → verified
Whiteboard: [qa+] → [qa!]

Updated

5 years ago
Blocks: 806515
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.