384207 - Crash Reporter client should include a URL field

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

add "url" and "comment" area to breakpad ui.

originally mentioned as a comment of bug #358082, but that bug is now fixed and also bug #366971 (marked a dup of bug #358082)

currently the breakpad ui asks for email address, per beltzner's mockup, but there is no text area enter my comments and the URL.

See also bug #375083 about "
Send URL of active tab to the crash report as metadata".

Comment 1

[(not currently active) Ted Mielczarek]

There's already bug 382538 on a comment field, I guess we don't have a bug on entering the URL.

Comment 2

[(not reading, please use [e-mail] instead)]

morphing bug, per Ted's comment.

Comment 3

[Benjamin Smedberg]

I don't think we want a field for entering the URL... we should just record the latest URL if possible, and let any other information come in through the comments field.

Comment 4

[(not currently active) Ted Mielczarek]

We ought to at least display what we collected from the app, with a checkbox to let the user not send it.

Comment 5

[Benjamin Smedberg]

I think we should display it in the details view. Why display it in the standard view, and why do we need a checkbox? If it's for privacy, I think that's a false sense of privacy, because the URL can end up in the minidump data quite easily.

Comment 6

[Arie Paap [:wildmyron]]

I've come across another issue where the wrong url is sent.

Some crashes which happen during page load crash before |this._updateCrashReportURL(aWindow)| is called. This results in a crash report with the previous url from the tab in place of the url causing the crash. Example: bp-d49092eb-442b-11dc-8d02-001a4bd43ef6
The crashing url for this report is http://www.unl.edu/rhames/bonobo/bonobo.htm

I don't really understand what happened in bug 375083 but I gather that sessionStore isn't being triggered early enough during page load to update the current tab. I also see this sometimes with session store not saving a crashing tab (couldn't find a bug for that) but as that would lead to a cycle of crashes when restoring it doesn't seem desirable;e to fix.

I would like to have an editable URL field so I can submit the correct url for crash reports related to this kind of crash.

Comment 7

[Adam Guthrie]

beltzner, what are your thoughts on this? The URL is displayed when you choose "View Data", but not everyone is going to look there. It seems rather sneaky to me to send the URL without telling the user.

Comment 8

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

(In reply to comment #5)
> I think we should display it in the details view. Why display it in the
> standard view, and why do we need a checkbox? If it's for privacy, I think
> that's a false sense of privacy, because the URL can end up in the minidump
> data quite easily.

There's a difference between "can end up in the minidump" and "is always sent intentionally in it's own field [and displayed in the web interface for anyone to see]". The raw minidump data isn't available to the public and extracting the URL from it is presumably nontrivial.

Not giving the users a choice to opt-out of sending the URL sounds like the wrong thing to do, and could result in fewer submitted reports from people who are privacy conscious.

Comment 9

[(not currently active) Ted Mielczarek]

Ok, my plan for this is:

If the .extra data from the crashing app includes a "URL" field, display a textbox containing the URL (labelled URL), to allow the user to edit or delete it.  This should make things work ok for Thunderbird et. al.

Comment 10

[Damon Sicore (:damons)]

Moving this to P1.  Make it happen, Ted.  :)

Comment 11

[Mike Schroepfer]

Ted is this rollin?

Comment 12

[(not currently active) Ted Mielczarek]

It's in progress.  Just a little slow cause I have to do it x 3 platforms.

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: mac support + added string

Just gonna do each platform separately, since they're all different anyway.  This patch does include the extra string in locales/ though.  My cocoa skills are mostly copypasta, so let me know if I'm doing anything stupid here.

This patch adds a URL field with a checkbox above it for "Include this URL in the crash report".  The box is checked by default, and the URL field is pre-filled with the URL from the extra data.  If no URL was provided with the extra data, the checkbox and text field are hidden completely.

Screenshot: http://people.mozilla.com/~tmielczarek/crashreporter-url.png

FYI:
-    int oldRestartWidth = restartFrame.size.width;
+    float oldRestartWidth = restartFrame.size.width;

That kills a compiler warning, apparently I was supposed to be using float there.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: WIP windows patch

Comment 15

[Dave Camp (:dcamp)]

Comment on attachment 289528 [details] [diff] [review]
mac support + added string

Changing the email field updates the checkbox and details field appropriately (see controlTextDidChange).   The URL button should probably do the same thing.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

This is gonna change a little as per bug 404855: we're going to have a checkbox (default on) which allows the user to exclude their URL, and they'll be able to see the URL itself if they choose to view the report data. That way we don't have to include a textbox, which makes the dialog a little larger.

(removing uiwanted)

Comment 17

[(not currently active) Ted Mielczarek]

Attachment: screenshot on OS X

So here's what I've got currently.

Comment 18

[(not currently active) Ted Mielczarek]

A few thoughts:

re: comment 6:
I'll file a different bug about trying to get the URL more accurately.  We shouldn't make anyone type in a URL just because we aren't getting it right.  We should be able to watch a different event for when navigation happens and grab the URL at the start.

re: comment 7:
I'm still not 100% happy with the fact that the URL still isn't visible with my latest changes, but the crash reporter UI is getting way overloaded.  We need to keep it sane so it doesn't scare people.  I think having the checkbox there is hopefully warning enough to people that we're including a URL.

Comment 19

[(not currently active) Ted Mielczarek]

Attachment: revised os x patch

Ok, this has the string changes + the OS X gui changes.

Comment 20

[(not currently active) Ted Mielczarek]

Attachment: osx + win patch [checked in]

Ok, combined osx+win patch.

Comment 21

[Dave Camp (:dcamp)]

Comment on attachment 290593 [details] [diff] [review]
osx + win patch [checked in]


>   [viewReportScrollView retain];
>   [viewReportScrollView removeFromSuperview];
> 
>+  if (gQueryParameters.find("URL") != gQueryParameters.end()) {
>+    // save the URL value in case the 

need to finish that thought :)

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 290593 [details] [diff] [review]
osx + win patch [checked in]

Checked this in, will get Linux as soon as I get back to a Linux build env.

Comment 23

[(not currently active) Ted Mielczarek]

Attachment: linux patch

Turns out this was pretty easy for Linux!

Comment 24

[(not currently active) Ted Mielczarek]

Checked the Linux bits in.

Comment 25

[Carsten Book [:Tomcat]]

verified fixed with Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b2) Gecko/2007121016 Firefox/3.0b2 and XP Version

- > Changing to verified