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.

I've got ideas for how to implement this.

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.

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.

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.

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.

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.

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?

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.

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.)

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.

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.

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.

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.

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?

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.

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).

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.

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;

http://hg.mozilla.org/integration/mozilla-inbound/rev/9f45e986e783 http://hg.mozilla.org/integration/mozilla-inbound/rev/0ef81057b56f http://hg.mozilla.org/integration/mozilla-inbound/rev/f70d110ad2c9 http://hg.mozilla.org/integration/mozilla-inbound/rev/f64afb604e05 http://hg.mozilla.org/integration/mozilla-inbound/rev/461d34f67036 And bent's nits are http://hg.mozilla.org/integration/mozilla-inbound/rev/ebae45e5005c.

Backout because I pushed older versions of the patches: http://hg.mozilla.org/integration/mozilla-inbound/rev/45352943ab6c http://hg.mozilla.org/integration/mozilla-inbound/rev/4f72ab0813eb http://hg.mozilla.org/integration/mozilla-inbound/rev/1b9505774c93 http://hg.mozilla.org/integration/mozilla-inbound/rev/4b7e21df8765 http://hg.mozilla.org/integration/mozilla-inbound/rev/f3aee8cc69e3 http://hg.mozilla.org/integration/mozilla-inbound/rev/ec1c5047fa39

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.

(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.

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.

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.+

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.

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?

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.

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.

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.

(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?

Here's the queue. I think it makes more sense that just blindly queueing runnables.

Whoops, forgot the boolean change.

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.

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.

http://hg.mozilla.org/integration/mozilla-inbound/rev/942a7f001562 http://hg.mozilla.org/integration/mozilla-inbound/rev/47da01006e3d http://hg.mozilla.org/integration/mozilla-inbound/rev/82079836b062 http://hg.mozilla.org/integration/mozilla-inbound/rev/14e35982be4f http://hg.mozilla.org/integration/mozilla-inbound/rev/9b851182cd64 http://hg.mozilla.org/integration/mozilla-inbound/rev/43c07840c4a5

\o/

https://hg.mozilla.org/mozilla-central/rev/942a7f001562 https://hg.mozilla.org/mozilla-central/rev/47da01006e3d https://hg.mozilla.org/mozilla-central/rev/82079836b062 https://hg.mozilla.org/mozilla-central/rev/14e35982be4f https://hg.mozilla.org/mozilla-central/rev/9b851182cd64 https://hg.mozilla.org/mozilla-central/rev/43c07840c4a5