Closed Bug 451709 Opened 16 years ago Closed 16 years ago

include obj-c exception info in crash reports

Categories

(Core :: General, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.1b1

People

(Reporter: jaas, Assigned: jaas)

Details

(Keywords: verified1.9.0.5, verified1.9.1)

Attachments

(1 file, 5 obsolete files)

Now that bug 444103 has landed we can include Obj-C exception information in crash reports.
Flags: blocking1.9.1+
Attached patch fix v1.0 (obsolete) — Splinter Review
Attachment #335059 - Flags: review?(ted.mielczarek)
Attachment #335059 - Flags: review?(ted.mielczarek) → review+
> +#include "nsString.h"

Could you make that 'nsStringGlue.h' instead? This file is exported in the SDK and external folks can't use the internal string headers.
Attached patch fix v1.1 (obsolete) — Splinter Review
Attachment #335059 - Attachment is obsolete: true
Attachment #335229 - Flags: review?(bent.mozilla)
Comment on attachment 335229 [details] [diff] [review]
fix v1.1

>+    if (nameBuffer)
>+      NS_Free(nameBuffer);
>+    if (reasonBuffer)
>+      NS_Free(reasonBuffer);

Those ifs are unnecessary as you've already checked them. r=me with those gone!
Attachment #335229 - Flags: review?(bent.mozilla) → review+
Attached patch fix v1.2 (obsolete) — Splinter Review
Thanks Ben!
Attachment #335229 - Attachment is obsolete: true
Attachment #335368 - Flags: superreview?(roc)
This is good, but can't we get a stack trace from the Obj-C exception which points back into the framework that threw the exception?

Also, is it really wise to have all this code in static nsObjCExceptionLog? Can't we share it across files somehow?
Attached patch fix v1.3 (obsolete) — Splinter Review
Attachment #335368 - Attachment is obsolete: true
Attachment #335368 - Flags: superreview?(roc)
Attached patch fix v1.4 (obsolete) — Splinter Review
Attachment #335563 - Attachment is obsolete: true
Attachment #335752 - Flags: review?(ted.mielczarek)
Comment on attachment 335752 [details] [diff] [review]
fix v1.4

As I said on IRC, I'm not wild about passing the exception object through the interface as a void*, but I don't have a huge objection to it either, so if this is better for you, then go for it.
Attachment #335752 - Flags: review?(ted.mielczarek) → review+
Attachment #335752 - Flags: superreview?(roc)
Comment on attachment 335752 [details] [diff] [review]
fix v1.4

>+  unichar* nameBuffer = (unichar*)NS_Alloc(sizeof(unichar) * ([name length] + 1));
> ...
>+  [name getCharacters:nameBuffer];

The apple docs say that [name getCharacters:nameBuffer] only fills nameBuffer up to [name length] so I think you want to null-terminate yourself.
Comment on attachment 335752 [details] [diff] [review]
fix v1.4

Rev the IID on nsICrashReporter.idl
Attachment #335752 - Flags: superreview?(roc) → superreview+
Attached patch fix v1.5Splinter Review
Attachment #335752 - Attachment is obsolete: true
landed on trunk
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 335838 [details] [diff] [review]
fix v1.5

Requesting approval for 1.9.0.x for this patch, since it (in conjunction with bug 444103, on which it depends) would really help decipher the extremely high number of Mac crashes that are lumped under nsObjCExceptionLogAbort(NSException*).
Attachment #335838 - Flags: approval1.9.0.4?
Attachment #335838 - Flags: approval1.9.0.5?
Attachment #335838 - Flags: approval1.9.0.4?
Attachment #335838 - Flags: approval1.9.0.4-
Comment on attachment 335838 [details] [diff] [review]
fix v1.5

bug 444103 isn't landing for 1.9.0.4, so this definitely can't land now.
Comment on attachment 335838 [details] [diff] [review]
fix v1.5

I'm rolling up a combination branch patch over in bug 444103. I'll request approval over there.
Attachment #335838 - Flags: approval1.9.0.5?
Fixed in combination with branch checkin of bug 444103. I have an extension that can be used to test this, I'll try to make a litmus test out of it.
Keywords: fixed1.9.0.5
Steps to test:
1) Install the Crash Me Now! Advanced extension: http://ted.mielczarek.org/code/mozilla/crashme-advanced.xpi and restart your browser
2) Click the "Tools -> Crash me! -> ObjC Exception!" menu item. Your browser should crash, and the crash reporter should appear.
3) Click the "Details" button. A panel should appear containing lot of text. Verify that this text is included:
"Notes: 
Obj-C Exception data:
Test Exception: Catch this!"
Flags: in-litmus?
Ted, does it mean we don't have to use crashme.xpi anymore or is the advanced version only functional on OS X?
Hardware: PC → All
The new version works on Win/Mac/Linux just like the old one. The only difference is that the advanced version gives you an option of how to crash. You can use either one.
I've asked because it would be easier to maintain only one version of crashme on Litmus. Because there is a sub menu available now all the other tests have to be updated as well. Which will confuse users with the simpler version because they cannot see this sub menu.

I'm not sure if this is the right place to discuss but let Tracy or Marcia decide about what to do on Litmus.
Ah, yeah, I can see that being annoying. The old version of crashme is still available, but I'm using the same extension ID for both, so you can't install them side-by-side. If you've got a better idea I'm open to suggestions. I could produce another simple extension that just provides the "ObjC Exception" functionality if that would make life simpler.
Verified fixed with the official 1.9.0.5 build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.5) Gecko/2008120121 Firefox/3.0.5). The data is now included.
Also verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081204 Shiretoko/3.1b3pre ID:20081204020603

Setting 1.9.1b1 as TM because a2 isn't selectable anymore.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.1b1
(In reply to comment #22)
> Ah, yeah, I can see that being annoying. The old version of crashme is still
> available, but I'm using the same extension ID for both, so you can't install
> them side-by-side. If you've got a better idea I'm open to suggestions. I could
> produce another simple extension that just provides the "ObjC Exception"
> functionality if that would make life simpler.

Tracy, could we replace all instances of the old crashme extension with the new one on Litmus?
You need to log in before you can comment on or make changes to this bug.