1197259 - Allow non-XUL components to annotate crash reports

Status: NEW

(Toolkit :: Crash Reporting, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Crash report annotation is extremely helpful in figuring out non-reproducible crash bugs.  But currently it's only possible to make these annotations in XUL or one of its fully-integrated components.  So (for example) you can't add crash report annotations from SpiderMonkey or NSS code.

SpiderMonkey is actually linked into XUL.  But, like NSS, it doesn't use (or link to) any other XUL component.

The methods we need are in toolkit/crashreporter/nsExceptionHandler.cpp, which has all kinds of dependencies on XUL.  If non-XUL components try to use these methods directly (by linking to XUL or one of its object-file components), they will drag in most of XUL -- causing code bloat and (on some platforms) problems with multiply-defined symbols.

So we need to use them indirectly, via what I've called a stub library -- which finds their addresses using dlsym() or its equivalent.  Shortly I'll post a patch for this stub library.

We also need to create the glue needed by non-XUL components to allow them to use this stub library.  For the moment I'm only going to do this for SpiderMonkey, because we urgently need for SpiderMonkey to support crash report annotations in order to decipher bug 1124397.  The requisite glue code for NSS (and other non-XUL components) can be created later, by other people, as the need arises.

I'll also post a patch that adds SpiderMonkey-specific glue code.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stub library

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: SpiderMonkey glue

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Ted, please review the build system and crashreporter parts of both patches.

Jim, please review the (very small) Windows-specific part of the stub library.  I've tested it and it seems to work fine.

I'll look for someone to review the changes I've made to check_spidermonkey_style.py.  They're needed to allow SpiderMonkey code to #include "mozilla/nsCrashReporterStub.h".

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8651119 [details] [diff] [review]
SpiderMonkey glue

Jan, please review my changes to check_spidermonkey_style.py.  I picked you because you're already familiar with bug 1124397.  But please pass the review to someone else if you think that's appropriate.

As I mentioned above, these changes (or something like them) are needed to allow SpiderMonkey code to #include "mozilla/nsCrashReporterStub.h".

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8651117 [details] [diff] [review]
Stub library

I suspect I need to do a better job of dealing with the case of XUL *not* being loaded.  New stub library patch coming up.  See bug 1124397 comment #29.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stub library rev1: Check only once for XUL symbols

This gets rid of some of the bug 1124397 debug-logging patch test failures.  See bug 1124397 comment #34.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8652509 [details] [diff] [review]
Stub library rev1: Check only once for XUL symbols

I'll need to revise this patch yet again, to deal with the fact that AnnotateCrashReport() and friends may only be called from the main thread in a content process.  See bug 1124397 comment #37.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8652509 [details] [diff] [review]
Stub library rev1: Check only once for XUL symbols

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

ted has to approve exception handler changes, I'm not a peer there.

::: toolkit/crashreporter/stub/nsCrashReporterStub.cpp
@@ +37,5 @@
> +  // be expensive.  So we only check once for these symbols, hopefully well
> +  // after xul has been loaded (if it's going to be).
> +  if (!sXULSymbolsChecked) {
> +    if (!sAnnotateCrashReport_ptr) {
> +      sAnnotateCrashReport_ptr = (int (*)(const char*, const char*))

I think we can use (decltype(CrashReporter_AnnotateCrashReport)*) here. This works with other apis.

@@ +61,5 @@
> +  int retval = -1;
> +  if (LoadXULPtrs()) {
> +    retval = sAnnotateCrashReport_ptr(key, data);
> +  }
> +  return retval;

maybe simplify these up bit:
return LoadXULPtrs() ? sAnnotateCrashReport_ptr(key, data) : -1;

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stub library rev2: Clean up a bit more

This doesn't get rid of any more test failures, but it does simplify the process of loading XUL pointers.  I now do it only once, as the stub library is loaded.  As my comment says, I hope this is early enough for the content or plugin process not to have yet been sandboxed.

> I think we can use (decltype(CrashReporter_AnnotateCrashReport)*) here.

I decided it'd be safer not to do this.  I don't want the stub library to have any Mozilla-specific dependencies -- even to Types.h in mfbt, which redefines decltype if need be.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8653162 [details] [diff] [review]
Stub library rev2: Clean up a bit more

I once again tested this to make sure it works properly on both OS X and Windows.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8651119 [details] [diff] [review]
SpiderMonkey glue

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

Sorry for the delay. I'll clear the r? as there's not much I can say about this patch; it's mostly build changes.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

This is left over from when I was a Mozilla employee. I'll never get back to it.