411490 - about:crashes

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

shaver suggested that we have an "about:crashes" page as an easy (but unintrusive) way to find recently submitted crash reports.  Mossop already implemented the guts of this in Nightly Tester Tools, it might be nice to ship something built into firefox to make it easy for bug reporters to find their crashes.

Comment 1

[Dave Townsend [:mossop]]

I have this basically done

Comment 2

[Phil Ringnalda (:philor)]

Be nice to have in Thunderbird, too, except that it would need UI to open it, and somehow "View - Crashes" doesn't seem like a good menuitem.

Comment 3

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

This would be an invaluable help to testers and people who want to help with the debugging and problem solving process.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Here is the implementation for about:crashes. This is just implemented as a straight webpage with chrome privs. Uses the styling from the directory listing so it looks swanky. Simply scans over the crash reporter submitted directory for matching files and displays them sorted by date.

Provides a delete button to remove all crash reports.

Tested on all 3 main platforms.

Let me know if I require an additional review for the changes in docshell, though they are pretty clear cut.

Comment 5

[Dave Townsend [:mossop]]

According to the comment in docshell this code needs security review before being allowed to have chrome privileges in this way. The code displays an abbreviated form of filenames that are present on the local filesystem and provides a means to delete those files.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Missed off a file in hte previous patch

Comment 7

[(not currently active) Ted Mielczarek]

Just two offhand comments on the same topic:

1) bent did some work to let toolkit apps specify their profile dir, and the crash reporter respects that.  CCing him for insight.

2) Appending "Application Support" doesn't sound like the right thing to do there.  We really don't have a key for that?  In native code it's:
"FSFindFolder(kUserDomain, kApplicationSupportFolderType,
                           kCreateFolder, &foundRef);"

Eagerly awaiting a win32 build of this to try out!

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> Just two offhand comments on the same topic:
> 
> 1) bent did some work to let toolkit apps specify their profile dir, and the
> crash reporter respects that.  CCing him for insight.

I know the code you are referring to, but I looked in the actual source of the breakpad clients and it seemed to use pretty hard and fast rules for the Crash Reports directory.

> 2) Appending "Application Support" doesn't sound like the right thing to do
> there.  We really don't have a key for that?  In native code it's:
> "FSFindFolder(kUserDomain, kApplicationSupportFolderType,
>                            kCreateFolder, &foundRef);"

Nothing I could find and I did dig, I guess I could just add one to the directory service provider, up to Mano.

Comment 9

[(not currently active) Ted Mielczarek]

Aha,

http://lxr.mozilla.org/mozilla/source/toolkit/xre/nsXULAppAPI.h#183

You want UAppData, that already takes bent's stuff into account.  (Had to backtrack that from here: http://mxr.mozilla.org/mozilla/source/toolkit/xre/nsAppRunner.cpp#2656)

Comment 10

[Dave Townsend [:mossop]]

Right ok so the ifdeffed parts of the code to find the crash reports directory will change to just using UAppData except in the case of Thunderbird on mac where it needs to be the mac portion from there. I'll get that updated later on this evening probably.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, I agree with ted that as long as you use UAppData everything will work just fine.

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This switches to just UAppData for the majority of the time. Thunderbird however has to be special on OSX and changes its app path to ~/Library/Thunderbird, but breakpad continues to use ~/Library/Application Support/Thunderbird.

Comment 13

[Dave Townsend [:mossop]]

Test builds of this are up at https://build.mozilla.org/tryserver-builds/2008-01-15_06:[e-mail]-bug411490/

Comment 14

[(not currently active) Ted Mielczarek]

Just a few interface nits:
1) Can we put the "remove" button up top?  With a long list of reports, it's scrolled offscreen.
2) I'd like to change the title from "Crash Reporter" to "Submitted Crash Reports" or something like that.
3) I think "ID" should be at least "Report ID" or something clearer.

Pretty awesome!

Comment 15

[(not currently active) Ted Mielczarek]

I filed bug 412477 on a REST api for Socorro that could make this even more awesome.

Comment 16

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This moves the button, changes those two strings and also removes the redundant makefile from toolkit/crashreporter/content

Comment 17

[Asaf Romano (gone)]

FORCE_DIR vs. FORCDIR, but what's it about anyway?

Comment 18

[Asaf Romano (gone)]

Some RTL support would be nice too (see places in which you've encoded right/left).

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #17)
> FORCE_DIR vs. FORCDIR, but what's it about anyway?

In general Breakpad uses the user data directory area for storing the submitted reports, but it works this out in it's own code. This normally matches except on Thunderbird on OSX. In this cast Breakpad still uses ~/Library/Application Support/Thunderbird whereas the Thunderbird data directory is in ~/Library/Thunderbird.

See here where the Thunderbird adjustment is made to UAppData making it useless for us:

http://mxr.mozilla.org/seamonkey/source/toolkit/xre/nsXREDirProvider.cpp#1013

And here where Breakpad actually decides on the crash reports directory and makes note of the Thunderbird inconsistency:

http://mxr.mozilla.org/seamonkey/source/toolkit/crashreporter/client/crashreporter_osx.mm#548

(In reply to comment #18)
> Some RTL support would be nice too (see places in which you've encoded
> right/left).

I'll get a new version knocked up with that now.

Comment 20

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Fixes the FORCE_DIR inconsistency and sorts out the rtl issue I believe.

Comment 21

[Dave Townsend [:mossop]]

Attachment: patch rev 4 retry

Hg is producing some interestingly screwed up patches for some reason, this one looks more sensible

Comment 22

[Dave Townsend [:mossop]]

Attachment: patch rev 5

From IRC:

<Mano> Mossop: should be something like <body dir="..chromedir..">
<Mano> Mossop: 2. I would rather set chromedir on individual elements

This fixes those comments.

Comment 23

[Asaf Romano (gone)]

Comment on attachment 298791 [details] [diff] [review]
patch rev 5

r=mano

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 298791 [details] [diff] [review]
patch rev 5

Seeking approval for this, it is pretty well contained and provides a damned useful feature for testers and bug reporters.

Comment 25

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

Comment on attachment 298791 [details] [diff] [review]
patch rev 5

a=beltzner for 1.9

Comment 26

[Dave Townsend [:mossop]]

I can haz scotch?

RCS file: /cvsroot/mozilla/toolkit/crashreporter/jar.mn,v
done
Checking in toolkit/crashreporter/jar.mn;
/cvsroot/mozilla/toolkit/crashreporter/jar.mn,v  <--  jar.mn
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/crashreporter/content/crashes.xhtml,v
done
Checking in toolkit/crashreporter/content/crashes.xhtml;
/cvsroot/mozilla/toolkit/crashreporter/content/crashes.xhtml,v  <--  crashes.xhtml
initial revision: 1.1
done
Checking in toolkit/locales/jar.mn;
/cvsroot/mozilla/toolkit/locales/jar.mn,v  <--  jar.mn
new revision: 1.48; previous revision: 1.47
done
RCS file: /cvsroot/mozilla/toolkit/locales/en-US/crashreporter/crashes.dtd,v
done
Checking in toolkit/locales/en-US/crashreporter/crashes.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/crashreporter/crashes.dtd,v  <--  crashes.dtd
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/locales/en-US/crashreporter/crashes.properties,v
done
Checking in toolkit/locales/en-US/crashreporter/crashes.properties;
/cvsroot/mozilla/toolkit/locales/en-US/crashreporter/crashes.properties,v  <--  crashes.properties
initial revision: 1.1
done
Checking in docshell/base/nsAboutRedirector.cpp;
/cvsroot/mozilla/docshell/base/nsAboutRedirector.cpp,v  <--  nsAboutRedirector.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in docshell/build/nsDocShellModule.cpp;
/cvsroot/mozilla/docshell/build/nsDocShellModule.cpp,v  <--  nsDocShellModule.cpp
new revision: 1.36; previous revision: 1.35
done

Comment 27

[Dave Townsend [:mossop]]

Attachment: missed bit

Missed this part of the patch on checkin somehow, if someone could check it in before the nightlies it'd be neat!

Comment 28

[Dave Townsend [:mossop]]

I'll catch it in the morning if noone else does it.

Comment 29

[Phil Ringnalda (:philor)]

browser/app/profile/firefox.js 1.255

Comment 30

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre. Also verified on Win Vista nightly. This will be a useful tool for QA for sure.

Comment 31

[Boris Zbarsky [:bzbarsky]]

So... I have two questions:

1)  This adds the about: page in all Gecko clients, but the preference only in Firefox.  Is there a reason for this?

2)  There's a nice comment in the docshell code being edited that says that any new privileged about: pages need security review.  Did this happen?  I see nothing to that effect in the bug...

3)  Docshell code changes require sr (and ideally r= from a docshell module owner, which brings me back to question 2).

Comment 32

[Boris Zbarsky [:bzbarsky]]

I guess that's three questions, huh?  ;)

Comment 33

[Dave Townsend [:mossop]]

(In reply to comment #31)
> So... I have two questions:
> 
> 1)  This adds the about: page in all Gecko clients, but the preference only in
> Firefox.  Is there a reason for this?

Yes, not all gecko clients will have the same breakpad server. I'm not sure of an ideal way to handle that

> 2)  There's a nice comment in the docshell code being edited that says that any
> new privileged about: pages need security review.  Did this happen?  I see
> nothing to that effect in the bug...

Crap crap crap, I did note this in comment 5 but failed to get this done. I will in all haste badger someone for this.

> 3)  Docshell code changes require sr (and ideally r= from a docshell module
> owner, which brings me back to question 2).

I did ask in comment 4 if an additional review were required I shoul have really checked further for myself though.

Thank you for spotting this. If you want this backed out then we can go that way, I'd prefer to just try to get these sorted post checkin though.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Hmm.... How about having the page make it clear what's wrong if the URI pref is missing, and either checking in changes to tbird/seamonkey to point to the right breakpad server (the same one, right?) or just filing bugs on them to get it done?  I agree that there's no good way to handle random other Gecko apps needing a different URI past making it clear that they need to set the pref.

And yeah, if you can set up a security review that sounds great.  No need to back out or anything, as long as said review happens before the next beta.  The docshell changes do look fine pending the security review.

Thanks for the quick response!

Comment 35

[Dave Townsend [:mossop]]

(In reply to comment #34)
> Hmm.... How about having the page make it clear what's wrong if the URI pref is
> missing, and either checking in changes to tbird/seamonkey to point to the
> right breakpad server (the same one, right?) or just filing bugs on them to get
> it done?  I agree that there's no good way to handle random other Gecko apps
> needing a different URI past making it clear that they need to set the pref.

Filed bug 414050 and submitted patches for getting the pref set in other apps. For apps without the pref, the simple option is just to display that no crash reports were found. Sounds to me like you are preferring a more explicit message like "Crash Report listing isn't configured in this application" or something similar?

> And yeah, if you can set up a security review that sounds great.  No need to
> back out or anything, as long as said review happens before the next beta.  The
> docshell changes do look fine pending the security review.

Reaching out on this now, if nothing else I am in MV next week which should help.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> "Crash Report listing isn't configured in this application"

Yeah, or even "The x.y preference needs to be set to the correct crash report server for this application".

Comment 37

[Dave Townsend [:mossop]]

Ok filed bug 414054 to make that changed.

Comment 38

[pd]

This idea completely rocks

Comment 39

[Daniel Veditz [:dveditz]]

> And yeah, if you can set up a security review that sounds great.

This happened on Feb 12, 2008:
http://wiki.mozilla.org/Firefox3/StatusMeetings/2008-02-12
http://wiki.mozilla.org/Breakpad/AboutCrashes/SecurityReview

Comment 40

[(not currently active) Ted Mielczarek]

Pushed a browser chrome test to m-c:
http://hg.mozilla.org/mozilla-central/rev/9373d3073796