Closed Bug 581341 Opened 10 years ago Closed 8 years ago

Remote CrashReporter::AnnotateCrashReport from child processes

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
fennec 8+ ---

People

(Reporter: ted, Assigned: jdm)

References

(Blocks 3 open bugs)

Details

Attachments

(6 files, 21 obsolete files)

54.04 KB, patch
Details | Diff | Splinter Review
2.33 KB, patch
Details | Diff | Splinter Review
4.99 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
7.24 KB, patch
Details | Diff | Splinter Review
1.73 KB, patch
ted
: review+
Details | Diff | Splinter Review
8.49 KB, patch
ted
: review+
Details | Diff | Splinter Review
In bug 578952 I noted that CrashReporter::AnnotateCrashReport won't work from child processes. This is something that we'll probably want to support, especially for content processes. I don't know that any of the other crash reporting APIs would be useful in child processes, except maybe AppendAppNotesToCrashReport.

http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsICrashReporter.idl
With the PCrashReporter framework, this should be pretty easy to add, and generally across all subprocess types.  I definitely wouldn't recommend blocking fennec-2.0 on this, but I would strongly recommend taking it if someone gets time to hack on it.

If we get a patch in the ff4 timeframe, it's going to need to be written so as not to disturb PPluginModule.  Post-ff4, the two should share most code.
Yeah, that code from bug 605832 should do most of the heavy lifting here.
Depends on: 605832
Blocks: 639725
I've got ideas for how to implement this.
Assignee: nobody → josh
Blocks: 641601
This is what I'm aiming for right now, but it needs some more testing before I ask for review. Let me know if I'm overreaching, since I decided to subsume the existing PluginModule workaround?
Please do subsume PluginModule; the concerns in comment 1 are made obsolete by having shipped ff4.
Whiteboard: [fennec-4.1?]
Blocks: 636081
Alright, I think it's game time for this patch.  I ended up touching the testshell because I discovered that I could create remote content crash xpcshell tests without too much effort.  I've tried out crashing Fennec proper, and the graphics annotations show up just fine.  I also fixed up bug 619227 while I was in the area, since I really didn't like having to find the environnment variable to set when I wanted the crash reporter to appear.
Attachment #522378 - Flags: review?(jones.chris.g)
Attachment #521901 - Attachment is obsolete: true
Attachment #522378 - Flags: review?(ted.mielczarek)
This will need another revision to fix some build problems with MOZ_CRASHREPORTER turned off, but that shouldn't impede functional review.
Comment on attachment 522378 [details] [diff] [review]
Make crash report annotation work OOP and subsume existing workarounds.

The life cycle that's evolved so far is (read "*" and "?" as Kleene
closure and option)
 - create PToplevel
 - PTopLevelChild->SendPCrashReporterCtor
 - (annotate|app notes|...)*
 - (hang, take paired dump)?
 - PToplevelParent::ActorDestroy
   * compute process-specific annotations
   * compute common annotations
   * if hang dumps exist, annotate use them
   * else try TakeMinidump; if exists annotate and use
   * fire off process-specific notification of crash/no-crash

There are two platform-specific hairs on the process
 - "child-process main-thread ID", needed in generating hang dumps
 - android lib mapping stuff

AFAICT all this can be fit into PCrashReporter; I think your
ProcessReporter interface is just CrashReporterParent.  Furthermore, I
think the hang-dump generation code should be moved into
CrashReporterParent.  We'll need that for PContent pretty soon, I
think, when we start dealing with windows BS there.  It's fine with me
if that goes into a followup, but if you're in a refactorin' mood now
would be a good time.

Minor engineering issues
 - annotating the process type can stay entirely in PCrashReporter;
   CrashReporterChild knows what process it's running in
 - CrashReporterChild knows that the child-process main thread is; can
   manage that info too
 - CrashReporterChild can manage the hang-dump IDs and the crash-dump
   ID
 - CrashReporterParent will want interfaces like

// Adds generic annotations
template<class Toplevel>
bool GeneratePairedMinidumps(Toplevel* t, const AnnotationTable& processSpecific);

bool GetPairedMinidumps(childDump, parentDump);

// Adds generic annotations
template<class Toplevel>
bool GetCrashMinidump(Toplevel* t, const AnnotationTable& processSpecific);

The Toplevel parameterization allows us to use a kind of duck typing
in CrashReporterParent; it can just call
t->OtherProcess()/t->TakeMinidump()/etc. and the compiler will catch
those not being exposed or typed correctly.  Otherwise we'd need
another vtable through which to make these calls, for no gain IMHO.
All of the code needed for this is generic but lives in
PluginModuleParent atm.

 - PToplevelParent::ActorDestroy would use, GetPairedMinidumps() and
   GetCrashMinidump(), and look a lot like PluginModuleParent today.

Does this all make sense?  What you have now is looking good, but I'd
like to steer it down a slightly different path.
Attachment #522378 - Flags: review?(jones.chris.g) → review-
Comment on attachment 522378 [details] [diff] [review]
Make crash report annotation work OOP and subsume existing workarounds.

Why are you adding GetOOPEnabled? Why not just restore GetEnabled() to work in either process? That way callers don't have to check both, they can just use GetEnabled, since the individual APIs will work in either process.

Aside from that the changes look alright, I'll leave the IPC bits to cjones.
Attachment #522378 - Flags: review?(ted.mielczarek) → review-
I added GetOOPEnabled because most of the API isn't supposed to work in child processes, and the GetEnabled check makes that work right now.
Blocks: 616134
tracking-fennec: --- → 7+
Whiteboard: [fennec-4.1?]
I believe this is more in line with what you had in mind, Chris. Ted, I haven't made any changes to the infrastructure you didn't like last time, but I have explained my reasoning since then, so take another look with that in mind, please?
Attachment #536629 - Flags: review?(ted.mielczarek)
Attachment #522378 - Attachment is obsolete: true
Attachment #536629 - Flags: review?(jones.chris.g)
Comment on attachment 536629 [details] [diff] [review]
Make crash report annotation work OOP and subsume existing workarounds.

This is a sweet patch.  Cheers.

>+#ifdef MOZ_CRASHREPORTER
>+    return new CrashReporterChild;

Please use |new CrashReporterChild()|.  Doesn't matter here but this
form is for specialized uses and unnecessarily drew my attention.

>+#ifdef MOZ_CRASHREPORTER
>+void
>+ContentParent::WriteExtraDataForMinidump(CrashReporter::AnnotationTable& notes)

In general, prefer passing data instead of callbacks (I'm sr here so
can pontificate.  Woo!).  Callbacks are bad because they make the
behavior of your implementation include the behaviors of the
transitive closure of all the callbacks passed to the interface.  This
means you need to worry about nasty things like reentry etc.  Just say
no unless there's a compelling reason otherwise; having interface
users pass down an AnnotationTable isn't more code, is cheap enough
and this isn't perf-sensitive code anyway.  At least, if it were, we
would have serious problems ;).

>+    friend class CrashReporterParent;

|friend|ing for WriteExtraDataForMinidump?

>+    template<class Toplevel>
>+    static PCrashReporterChild* CreateCrashReporter(Toplevel* actor) {

The return value isn't used AFAICT, just make it void.  Include a
hard-assert here that this is only called once, or at least only
called when Toplevel doesn't already have a crash reporter.

>diff --git a/dom/ipc/CrashReporterParent.h b/dom/ipc/CrashReporterParent.h
>--- a/dom/ipc/CrashReporterParent.h
>+++ b/dom/ipc/CrashReporterParent.h

For template-heavy .h's with non-trivial function definitions, it's
easier to scan interfaces when there are separate decls and (inline,
of course) defitions.  Move the impls to the bottom of the file, or if
you want to be really stylish add CrashReporterParent-inl.h and put
the defs there.

>+  GenerateHangMinidump(Toplevel* t)
>+  GenerateCrashMinidump(Toplevel* t)

Hmm ... I think there's value in distinguishing these from
GeneratePairedMinidumps(), since they're creating different things.
These two are really making crash reports, and the former is indeed
creating paired minidumps.  I think s/Minidump/CrashReport/ would be
fine here.

My eyes kind of glazed over while I read the PluginModuleParent
changes (sorry, flying on one hour of ~sleep).  Let's get one more
patch with the above fixed.

It would be easier for ted and me if you separated the
toolkit/crashreporter stuff from the dom/ipc stuff.
Attachment #536629 - Flags: review?(jones.chris.g)
Comment on attachment 536629 [details] [diff] [review]
Make crash report annotation work OOP and subsume existing workarounds.

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

I only have one issue with this patch, the rest looks okay. (Although I skipped most of the IPC parts, of course.)

::: testing/xpcshell/xpcshell.ini
@@ +83,5 @@
>  
> +[include:toolkit/crashreporter/test/unit_ipc/xpcshell.ini]
> +skip-if.os = linux
> +run-if.config = mozcrashreporter
> +run-if.config = ipc

I'm not sure that this actually works, FWIW. (You have duplicated keys, for one, and didn't we remove --disable-ipc already?)

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +1127,4 @@
>  
> +nsresult AnnotateCrashReport(const nsACString& key, const nsACString& data)
> +{
> +  if (!GetEnabled() && !GetOOPEnabled())

This kind of sucks. Can we just make "GetEnabled" return true for either kind of process, and then have "isOOP" or something if things need to check that specifically? Otherwise all our if (GetEnabled()) checks are going to wind up looking like this.

::: toolkit/crashreporter/test/unit_ipc/test_content_annotation.js
@@ +1,2 @@
> +load("../unit/head_crashreporter.js");
> +

Thanks for including a test! Can you test AppendAppNotes as well here?

::: toolkit/xre/nsSigHandlers.cpp
@@ +261,5 @@
>  
>  #if defined(CRAWL_STACK_ON_SIGSEGV)
> +  if (!CrashReporter::GetEnabled() &&
> +      !CrashReporter::GetOOPEnabled() &&
> +      !getenv("XRE_NO_WINDOWS_CRASH_DIALOG")) {

Is this actually related, or just extraneous? (You'll need someone else to review it if it's needed.)
Attachment #536629 - Flags: review?(ted.mielczarek) → review+
Attachment #539354 - Flags: review?(bent.mozilla)
Attachment #536629 - Attachment is obsolete: true
The patches I'm uploading now are scaffolding that seems to be rock solid. All of the work is done, but I alternately cause oranges on mac/linux or windows because of platform inconsistencies with minidump paths. Once that mess is cleared up I'll put the remaining patches up.
Attachment #539354 - Attachment description: Disallow re-entry of IPC xpcshell command evaluation. → Part 4: Disallow re-entry of IPC xpcshell command evaluation.
Attachment #539359 - Attachment description: Disable alternate signal handlers if crash reporter is enabled. → Part 3: Disable alternate signal handlers if crash reporter is enabled.
Attached patch Part 6: Test. (obsolete) — Splinter Review
All previous review comments addressed, one way or another. Chris, the friending occurs because TakeMinidump is private to the actors. I'm sure I can dig around in the IPDL generator to fix this if you want.
Comment on attachment 539495 [details] [diff] [review]
Part 6: Test.

Ted, I made a couple other changes to the subprocess head file, so I'd appreciate you glancing over them to make sure they're ok.
Attachment #539495 - Flags: review?(ted.mielczarek)
Attachment #539493 - Attachment is obsolete: true
Comment on attachment 539354 [details] [diff] [review]
Part 4: Disallow re-entry of IPC xpcshell command evaluation.

As discussed on irc I don't think we should make testshell very smart here. If a test spins an event loop that the test writer wasn't expecting then we should fix the test (and nuke the nested loop if possible!) rather than hide that detail.
Attachment #539354 - Flags: review?(bent.mozilla) → review-
Attachment #539494 - Attachment is obsolete: true
Attachment #539494 - Flags: review?(bent.mozilla)
Comment on attachment 539495 [details] [diff] [review]
Part 6: Test.

There's a new version coming up, but I'm waiting for try results before asking for review of the test changes.
Attachment #539495 - Flags: review?(ted.mielczarek)
Comment on attachment 539626 [details] [diff] [review]
Part 5: Always run the IPC testshell callback, regardless of execution success or failure.

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

::: ipc/testshell/TestShellParent.cpp
@@ +141,5 @@
>  
>    return JS_TRUE;
>  }
>  
> +class AsyncCommandCallback : public nsRunnable {

I don't think you should make this run asynchronously, as discussed on irc.

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +756,5 @@
>  {
>    if (!gTestShellParent)
>      return true;
> +  ContentParent* cp = ContentParent::GetSingleton(PR_FALSE);
> +  return cp ? cp->DestroyTestShell(gTestShellParent) : true;

This seems wrong... Something should unset gTestShellParent if ContentParent dies. Add an ActorDestroy to TestShellParent too?
Attachment #539626 - Flags: review?(bent.mozilla) → review-
Comment on attachment 539492 [details] [diff] [review]
Part 1: Make crash report annotation work OOP and subsume existing workarounds.

Code looks OK.  Nits/comment stuff follow.

>+            (void) crashReporter->GenerateCrashReport(this, NULL);

I don't like (void) --- for functions that have a return value that
can be ignored, it's not helpful IMHO, and for functions that have a
return value that needs to be explicitly ignored
(NS_ATTR_WARN_UNUSED_RESULT), |(void)| doesn't do the trick.  Use
nothing for ignore-able return values and |unused << Foo()| for ones
that have to explicitly be ignored.  Note too that |(void)| missed the
convert-to-|void|-return signature change, which |unused <<| wouldn't
have.

>+  GenerateHangCrashReport(CrashReporter::AnnotationTable* processNotes);

|typedef CrashReporter::AnnotationTable AnnotationTable| in
 CrashReporterParent so that you can write AnnotationTable.

All these |processNotes| args should be |const AnnotationTable*|.

These new interfaces need documentation.

r=me with nit fixes and comments.
Attachment #539492 - Flags: review?(jones.chris.g) → review+
Comment on attachment 539359 [details] [diff] [review]
Part 3: Disable alternate signal handlers if crash reporter is enabled.

As discussed on IRC, I think there are ordering issues here with the crashreporter (which gets installed after this hook in the main process).
Attachment #539359 - Flags: review?(benjamin) → review-
Attachment #539492 - Attachment is obsolete: true
Attachment #539496 - Attachment is obsolete: true
Attached patch Part 6: Test. (obsolete) — Splinter Review
Attachment #539495 - Attachment is obsolete: true
Attachment #539354 - Attachment is obsolete: true
Attachment #539359 - Attachment is obsolete: true
Attachment #539626 - Attachment is obsolete: true
Now (finally) passes try on all platforms. I have a grudging respect for our wide test coverage. I think I've already received r+ for the meat of these patches, and the only differences now are minor ones to fix build/test problems.
Attachment #541544 - Flags: review?(benjamin) → review+
Comment on attachment 541545 [details] [diff] [review]
Part 5: Always run the IPC testshell callback, regardless of execution success or failure.

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

::: dom/ipc/ContentParent.cpp
@@ +329,5 @@
>  
> +TestShellParent*
> +ContentParent::GetTestShellSingleton()
> +{
> +    if (!ManagedPTestShellParent().Length())

Make this:

  if (ManagedPTestShellParent().IsEmpty())

And please add braces to single statement if blocks.

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +714,1 @@
>      }

Nit: newline after this.

@@ +716,5 @@
> +    if (testShell) {
> +      return testShell;
> +    }
> +    testShell = parent->CreateTestShell();
> +    return testShell;

Make this:

  if (!testShell) {
    testShell = parent->CreateTestShell();
  }
  return testShell;

@@ +762,2 @@
>      return true;
> +  return cp->DestroyTestShell(tsp);

Make this:

  return tsp ? cp->DestroyTestShell(tsp) : true;
Attachment #541545 - Flags: review?(bent.mozilla) → review+
Depends on: 668385
Comment on attachment 541544 [details] [diff] [review]
Part 3: Install child process exception handler before activating breakpad.

># HG changeset patch
># User Josh Matthews <josh@joshmatthews.net>
># Date 1308871831 14400
># Node ID 9d08257cbc6a98a61c41d7b79245dfdb605f2e11
># Parent  f8aa713fb717c41d424f3410998d3fd7703f8a0e
>Bug 581341 - Part 3: Install child process exception handler before activating breakpad. r=bsmedberg
>
>diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp
>--- a/toolkit/xre/nsEmbedFunctions.cpp
>+++ b/toolkit/xre/nsEmbedFunctions.cpp
>+
>+  SetupErrorHandling(aArgv[0]);  
>+
> #if defined(MOZ_CRASHREPORTER)
>   if (aArgc < 1)
>     return 1;

You seem to be touching argv[0] before knowing if there's anything there.
looks like this has been backed out again by jdm, it's not in inbound.
Whiteboard: [inbound]
(In reply to comment #39)
> You seem to be touching argv[0] before knowing if there's anything there.

XRE_InitChildProcess has:

NS_ENSURE_ARG_MIN(aArgc, 2);
NS_ENSURE_ARG_POINTER(aArgv);
NS_ENSURE_ARG_POINTER(aArgv[0]);

and the line immediately following the context quoted in comment 39 has

 const char* const crashReporterArg = aArgv[--aArgc];
Chris, if you could sign off on the reversed direction and rpc-ness of the crash reporter actor creation, that would be swell. Try says it's quite happy with this series of patche, finally.
Attachment #544819 - Flags: review?(jones.chris.g)
Attachment #541542 - Attachment is obsolete: true
Ted, I want your approval that the change to the LSP annotation gatherer is ok. This was necessary to make windows xpcshell tests pass, because occasionally the annotator thread would finish before the crash reporter message was received from the parent.
Attachment #544822 - Flags: review?(ted.mielczarek)
Attachment #541543 - Attachment is obsolete: true
Comment on attachment 544819 [details] [diff] [review]
Part 1: Make crash report annotation work OOP and subsume existing workarounds.

We discussed rpc-ifying the plugin crash reporter.  Why are you rpc-ifying the content crash reporter?
Because I like to live life on the edge. I'll fix that.
Comment on attachment 544822 [details] [diff] [review]
Part 2: Hook up CrashReporter functions to IPC.

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

::: widget/src/windows/LSPAnnotator.cpp
@@ +76,5 @@
> +    nsresult rv = cr->AnnotateCrashReport(NS_LITERAL_CSTRING("Winsock_LSP"), mString);
> +    if (NS_FAILED(rv)) {
> +      // If the crash reporter is enabled but annotation failed, we're probably
> +      // out-of-process and haven't properly setup up the remote bridge yet.
> +      // Let's give this another shot in a bit.

This is a bit icky, is there a way to push back the call to this function until things are ready, instead of racing and retrying? Not going to r- you on that (this isn't code I own!) but seems like it would be nicer.+
Attachment #544822 - Flags: review?(ted.mielczarek) → review+
Josh, what's the plan here. Were you thinking we'd take this for aurora? If so, that window is closing fast and we'd need a risk assessment.
I really want to, but I haven't been able to land it without exposing new test breakages yet. I'm churning away.
tracking-fennec: 7+ → 8+
Attached patch Hack around an MSVC PGO bug. (obsolete) — Splinter Review
Attachment #557578 - Flags: review?(ted.mielczarek)
Attachment #544819 - Attachment is obsolete: true
Attachment #544819 - Flags: review?(jones.chris.g)
Attachment #557578 - Attachment is obsolete: true
Attachment #557578 - Flags: review?(ted.mielczarek)
Attachment #544822 - Attachment is obsolete: true
Attachment #541544 - Attachment is obsolete: true
Attachment #541545 - Attachment is obsolete: true
Attachment #541546 - Attachment is obsolete: true
Blocks: 684500
Attached patch Part 6: Test.Splinter Review
Attachment #560011 - Flags: review?(ted.mielczarek)
Comment on attachment 560009 [details] [diff] [review]
Part 5: Always run the IPC testshell callback, regardless of execution success or failure.

The switch to multiple content processes wrecked the previous solution for shutting down the test shell. Ben, does this solution look ok?
Attachment #560009 - Attachment description: Part 5: Always run the IPC testshell callback, regardless of execution success or failure. 581341 - Part 5: Always run the IPC testshell callback, regardless of execution success or failure. → Part 5: Always run the IPC testshell callback, regardless of execution success or failure.
Attachment #560009 - Flags: review?(bent.mozilla)
Comment on attachment 560006 [details] [diff] [review]
Part 2: Hook up CrashReporter functions to IPC.  * *

Ted, remember how I added a delayed LSP annotator runnable previously? I've generalized that to all crash reporter annotations; let me know if you'd like any changes.
Attachment #560006 - Flags: review?(ted.mielczarek)
I believe I've hit all the reviewers necessary; all other patches should be able to carry forward the last r+.
Comment on attachment 560009 [details] [diff] [review]
Part 5: Always run the IPC testshell callback, regardless of execution success or failure.

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

Looks ok, please fix the following and r=me.

::: dom/ipc/ContentParent.cpp
@@ +400,5 @@
> +TestShellParent*
> +ContentParent::GetTestShellSingleton()
> +{
> +    if (!ManagedPTestShellParent().Length())
> +        return nsnull;

Nit: Please brace.

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +711,5 @@
>  #endif // XP_MACOSX
>  }
>  
>  namespace {
> +nsRefPtr<ContentParent> gContentParent = nsnull;

static nsRefPtr is a no-no. Use a raw pointer and manually addref/release. Add a comment here saying that the pointer carries a reference.

@@ +716,5 @@
>  TestShellParent* GetOrCreateTestShellParent()
>  {
> +    if (!gContentParent) {
> +        gContentParent = ContentParent::GetNewOrUsed();
> +    } else if (!gContentParent->IsAlive()) {

Nit: else on next line.

@@ +764,5 @@
>  bool
>  XRE_ShutdownTestShell()
>  {
> +    if (!gContentParent)
> +        return true;

Nit: Please brace.
Attachment #560009 - Flags: review?(bent.mozilla) → review+
Comment on attachment 560011 [details] [diff] [review]
Hack around an MSVC PGO bug.

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

This is actually pretty clean. I'm impressed!

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +1100,5 @@
>                                 NS_LITERAL_CSTRING("\n"));
>    return PL_DHASH_NEXT;
>  }
>  
> +// This function is miscompiled with MSVC 2005/2008 when PGO is on.  See bug 684500.

The bug number is extraneous, it's in the blame.

@@ +1102,5 @@
>  }
>  
> +// This function is miscompiled with MSVC 2005/2008 when PGO is on.  See bug 684500.
> +#ifdef _MSC_VER
> +#pragma optimize("", off)

This actually works? We had mixed results in the past, although that might have been due to inlining etc.
Attachment #560011 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted, :luser] from comment #60)
> >  
> > +// This function is miscompiled with MSVC 2005/2008 when PGO is on.  See bug 684500.
> > +#ifdef _MSC_VER
> > +#pragma optimize("", off)
> 
> This actually works? We had mixed results in the past, although that might
> have been due to inlining etc.

It's worked on every try push so far!
Comment on attachment 560006 [details] [diff] [review]
Part 2: Hook up CrashReporter functions to IPC.  * *

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

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +1143,5 @@
> +  
> + private:
> +  nsCString mKey;
> +  nsCString mData;
> +  bool mAnnotate;

Using a bool here is not exactly clear. Could you maybe use an enum instead?

@@ +1161,5 @@
> +    PCrashReporterChild* reporter = CrashReporterChild::GetCrashReporter();
> +    if (!reporter) {
> +      nsRefPtr<DelayedNoteRunnable> event = new DelayedNoteRunnable(key, data);
> +      return NS_DispatchToMainThread(event);
> +    }

So this whole construct exists so that if the IPC crashreporter bits aren't set up yet, we'll handle it later?

Do we have confidence that this will only take one cycle through the event loop, or would it make more sense to instead stick these in a queue and flush them all out when things get properly intitialized?
Attachment #560006 - Flags: review?(ted.mielczarek) → review+
Here's the queue. I think it makes more sense that just blindly queueing runnables.
Attachment #562782 - Flags: review?(ted.mielczarek)
Attachment #560006 - Attachment is obsolete: true
Whoops, forgot the boolean change.
Attachment #562783 - Flags: review?(ted.mielczarek)
Attachment #562782 - Attachment is obsolete: true
Attachment #562782 - Flags: review?(ted.mielczarek)
Comment on attachment 562783 [details] [diff] [review]
Part 2: Hook up CrashReporter functions to IPC.  * *

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

Meta-note: you have trychooser syntax in your patch description, remember to remove that before landing.

This looks good modulo one thing I'm not sure about.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +239,5 @@
>  
> +// If annotations are attempted before the crash reporter is enabled,
> +// they queue up here.
> +class DelayedNoteRunnable;
> +nsTArray<nsRefPtr<DelayedNoteRunnable> > gDelayedAnnotations;

Is it safe to have a static nsTArray of nsRefPtrs? Get someone who knows the answer to sign off on that, please.
Attachment #562783 - Flags: review?(ted.mielczarek) → review+
nsTArray_base() does some initialization and normally we try to avoid static constructors.
http://hg.mozilla.org/mozilla-central/annotate/7f4867717226/xpcom/glue/nsTArray-inl.h#l44

I'd also expect some issues with leak checking tools running before the destructor, but I'm not up-to-date on the order in which those run.
Yeah, the try run showed a leaking nsTArray_base for every single run. I'm going to need to find some other way to do that, obviously.
Hey Josh, how's this work coming along?  Still looking to land soon?
Yep! I've been doing some try runs and getting some strange results on Android though, so I want to make sure that I'm not causing some of the oranges I saw.
Depends on: 691424
Blocks: 693467
Depends on: 1367591
You need to log in before you can comment on or make changes to this bug.