Status

()

defect
VERIFIED FIXED
12 years ago
9 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)

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: nobody → dtownsend
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+
Posted patch patch rev 1 (obsolete) — Splinter Review
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?
Attachment #297149 - Flags: review? → review?(mano)
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]
Posted patch patch rev 1 (obsolete) — Splinter Review
Missed off a file in hte previous patch
Attachment #297149 - Attachment is obsolete: true
Attachment #297152 - Flags: review?(mano)
Attachment #297149 - Flags: review?(mano)
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!
(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.
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)
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.
Posted patch patch rev 2 (obsolete) — Splinter Review
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)
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!
I filed bug 412477 on a REST api for Socorro that could make this even more awesome.
Posted patch patch rev 3 (obsolete) — Splinter Review
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).
(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.
Posted patch patch rev 4 (obsolete) — Splinter Review
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)
Posted patch patch rev 4 retry (obsolete) — Splinter Review
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)
Posted patch patch rev 5Splinter Review
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

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+
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: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Version: unspecified → Trunk
Posted patch missed bitSplinter Review
Missed this part of the patch on checkin somehow, if someone could check it in before the nightlies it'd be neat!
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
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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?  ;)
(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!
(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".
Ok filed bug 414054 to make that changed.
Depends on: 414054
Depends on: 414050
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
Pushed a browser chrome test to m-c:
http://hg.mozilla.org/mozilla-central/rev/9373d3073796
Flags: in-testsuite+
Blocks: 557739
You need to log in before you can comment on or make changes to this bug.