Closed
Bug 411490
Opened 16 years ago
Closed 16 years ago
about:crashes
Categories
(Toolkit :: Crash Reporting, defect)
Toolkit
Crash Reporting
Tracking
()
VERIFIED
FIXED
mozilla1.9beta3
People
(Reporter: ted, Assigned: mossop)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 6 obsolete files)
12.42 KB,
patch
|
asaf
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
670 bytes,
patch
|
Details | Diff | Splinter Review |
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•16 years ago
|
Assignee: nobody → dtownsend
Comment 2•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
Attachment #297149 -
Flags: review? → review?(mano)
Assignee | ||
Comment 5•16 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•16 years ago
|
||
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•16 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•16 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•16 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•16 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•16 years ago
|
||
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•16 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•16 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•16 years ago
|
||
I filed bug 412477 on a REST api for Socorro that could make this even more awesome.
Assignee | ||
Comment 16•16 years ago
|
||
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)
Comment 17•16 years ago
|
||
FORCE_DIR vs. FORCDIR, but what's it about anyway?
Comment 18•16 years ago
|
||
Some RTL support would be nice too (see places in which you've encoded right/left).
Assignee | ||
Comment 19•16 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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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 23•16 years ago
|
||
Comment on attachment 298791 [details] [diff] [review] patch rev 5 r=mano
Attachment #298791 -
Flags: review?(mano) → review+
Assignee | ||
Comment 24•16 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 25•16 years ago
|
||
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•16 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
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Updated•16 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 27•16 years ago
|
||
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•16 years ago
|
||
I'll catch it in the morning if noone else does it.
Comment 29•16 years ago
|
||
browser/app/profile/firefox.js 1.255
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9 M11
Comment 30•16 years ago
|
||
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
![]() |
||
Comment 31•16 years ago
|
||
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•16 years ago
|
||
I guess that's three questions, huh? ;)
Assignee | ||
Comment 33•16 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.
![]() |
||
Comment 34•16 years ago
|
||
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•16 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.
![]() |
||
Comment 36•16 years ago
|
||
> "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•16 years ago
|
||
Ok filed bug 414054 to make that changed.
Comment 38•16 years ago
|
||
This idea completely rocks
Comment 39•16 years ago
|
||
> 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•15 years ago
|
||
Pushed a browser chrome test to m-c: http://hg.mozilla.org/mozilla-central/rev/9373d3073796
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•