910074 - Crash reporter annotations added in bug 867530 are not actually added

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mike Hommey [:glandium]]

Caught this when looking around at the code for something else:

AnnotateCrashReport does nothing when the CrashReporter::GetEnabled() returns false, which it does until SetExceptionHandler is called. But the AnnotateCrashReport calls added in bug 867530 are before SetExceptionHandler.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Move crash reporter annotations added in bug 867530 after SetExceptionHandler call

Comment 2

[(not currently active) Ted Mielczarek]

I don't understand how this code is supposed to work *at all*, since the normal startup "enable exception handling" code path doesn't actually go through nsXULAppInfo::SetEnabled. That's just for handling setting the .enabled property on nsICrashReporter.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 796432 [details] [diff] [review]
Move crash reporter annotations added in bug 867530 after SetExceptionHandler call

Indeed.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Move crash reporter annotations added in bug 867530 after both SetExceptionHandler call and xpcom initialization

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 796654 [details] [diff] [review]
Move crash reporter annotations added in bug 867530 after both SetExceptionHandler call and xpcom initialization

Review of attachment 796654 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/nsAppRunner.cpp
@@ +3680,5 @@
> +  // Needs to be set after xpcom initialization.
> +  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("FramePoisonBase"),
> +                                     nsPrintfCString("%.16llx", uint64_t(gMozillaPoisonBase)));
> +  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("FramePoisonSize"),
> +                                     nsPrintfCString("%lu", uint32_t(gMozillaPoisonSize)));

These won't get annotated in xpcshell, but I don't think that matters. (In the other location they would only get annotated in xpcshell.)

Comment 6

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> These won't get annotated in xpcshell, but I don't think that matters. (In
> the other location they would only get annotated in xpcshell.)

Note most nsAppRunner annotations are above that, so they too won't get annotated in xpcshell.

Comment 7

[(not currently active) Ted Mielczarek]

Yes, but most of those come from the appdata, which makes sense. It's not a particularly big deal, I just wanted to point that out.

Comment 8

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86f568d2d467

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/86f568d2d467