Status

()

Toolkit
Crash Reporting
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: ted, Assigned: mossop)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9beta3
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

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

Updated

10 years ago
Assignee: nobody → dtownsend
(Assignee)

Comment 1

10 years ago
I have this basically done
Status: NEW → ASSIGNED
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.
This would be an invaluable help to testers and people who want to help with the debugging and problem solving process.
Flags: wanted1.9+
(Assignee)

Comment 4

10 years ago
Created attachment 297149 [details] [diff] [review]
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.
Attachment #297149 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #297149 - Flags: review? → review?(mano)
(Assignee)

Comment 5

10 years ago
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.
Whiteboard: [has patch]
(Assignee)

Comment 6

10 years ago
Created attachment 297152 [details] [diff] [review]
patch rev 1

Missed off a file in hte previous patch
Attachment #297149 - Attachment is obsolete: true
Attachment #297152 - Flags: review?(mano)
Attachment #297149 - Flags: review?(mano)
(Reporter)

Comment 7

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

Comment 8

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

Comment 9

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

Comment 10

10 years ago
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.
Yeah, I agree with ted that as long as you use UAppData everything will work just fine.
(Assignee)

Comment 12

10 years ago
Created attachment 297195 [details] [diff] [review]
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.
Attachment #297152 - Attachment is obsolete: true
Attachment #297195 - Flags: review?(mano)
Attachment #297152 - Flags: review?(mano)
(Assignee)

Comment 13

10 years ago
Test builds of this are up at https://build.mozilla.org/tryserver-builds/2008-01-15_06:04-dtownsend@mozilla.com-bug411490/
(Reporter)

Comment 14

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

Comment 15

10 years ago
I filed bug 412477 on a REST api for Socorro that could make this even more awesome.
(Assignee)

Comment 16

10 years ago
Created attachment 297331 [details] [diff] [review]
patch rev 3

This moves the button, changes those two strings and also removes the redundant makefile from toolkit/crashreporter/content
Attachment #297195 - Attachment is obsolete: true
Attachment #297331 - Flags: review?(mano)
Attachment #297195 - Flags: review?(mano)
FORCE_DIR vs. FORCDIR, but what's it about anyway?
Some RTL support would be nice too (see places in which you've encoded right/left).
(Assignee)

Comment 19

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

Comment 20

10 years ago
Created attachment 297972 [details] [diff] [review]
patch rev 4

Fixes the FORCE_DIR inconsistency and sorts out the rtl issue I believe.
Attachment #297331 - Attachment is obsolete: true
Attachment #297972 - Flags: review?(mano)
Attachment #297331 - Flags: review?(mano)
(Assignee)

Comment 21

10 years ago
Created attachment 298323 [details] [diff] [review]
patch rev 4 retry

Hg is producing some interestingly screwed up patches for some reason, this one looks more sensible
Attachment #297972 - Attachment is obsolete: true
Attachment #298323 - Flags: review?(mano)
Attachment #297972 - Flags: review?(mano)
(Assignee)

Comment 22

10 years ago
Created attachment 298791 [details] [diff] [review]
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.
Attachment #298323 - Attachment is obsolete: true
Attachment #298791 - Flags: review?(mano)
Attachment #298323 - Flags: review?(mano)
Comment on attachment 298791 [details] [diff] [review]
patch rev 5

r=mano
Attachment #298791 - Flags: review?(mano) → review+
(Assignee)

Comment 24

10 years ago
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.
Attachment #298791 - Flags: approval1.9?
Comment on attachment 298791 [details] [diff] [review]
patch rev 5

a=beltzner for 1.9
Attachment #298791 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 26

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Version: unspecified → Trunk
(Assignee)

Comment 27

10 years ago
Created attachment 299059 [details] [diff] [review]
missed bit

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

Comment 28

10 years ago
I'll catch it in the morning if noone else does it.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
browser/app/profile/firefox.js 1.255
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9 M11
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.
Status: RESOLVED → VERIFIED
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).
I guess that's three questions, huh?  ;)
(Assignee)

Comment 33

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

Comment 35

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

Comment 37

10 years ago
Ok filed bug 414054 to make that changed.
(Assignee)

Updated

10 years ago
Depends on: 414054

Updated

10 years ago
Depends on: 414050

Comment 38

10 years ago
This idea completely rocks
> 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
(Reporter)

Comment 40

8 years ago
Pushed a browser chrome test to m-c:
http://hg.mozilla.org/mozilla-central/rev/9373d3073796
Flags: in-testsuite+

Updated

7 years ago
Blocks: 557739
You need to log in before you can comment on or make changes to this bug.