539552 - Connect up the crash report minidump location with the toplevel actor that crashed

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benjamin Smedberg]

When a plugin crashes, we want to tell the toplevel PluginModuleParent about the location of the minidump file before we fire any ActorDestroy notifications. The ActorDestroy notifications will then use the minidump location to provide extra annotations and eventually submit the report.

We may also want the following information from IPDL:
* The startup time of the plugin process

Comment 1

[Benjamin Smedberg]

cjones mentioned "associate the crashing PID with a toplevel actor"

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If we could look up the miniump location from within ActorDestroy(), would that suffice?  Also, what should be done with the placeholder code in nsExceptionHandler::OnChildProcessDumpRequested?  Is that being obsoleted by this mechanism?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Keep track of child process minidumps, offer pid-based lookup API

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add a GetMinidump() method to top-level, parent-side IPDL classes

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Placeholder code in PluginModuleParent that uses new interface

PluginModuleParent just prints a message after grabbing the minidump.  bsmedberg, I only r?-ed this code for demo purposes, please feel free to cancel and edit yourself.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Keep track of child process minidumps, offer pid-based lookup API, v 0.1

Fixed some minor issues that Windows testing exposed

Comment 7

[Benjamin Smedberg]

To my knowledge, nobody has ever used or tested the multi-thread versions of the template hashtables, and I've wanted to remove them. I'd recommend using an external mutex.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 421606 [details] [diff] [review]
 Keep track of child process minidumps, offer pid-based lookup API, v 0.1

>diff --git a/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/client_info.h b/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/client_info.h
>--- a/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/client_info.h
>+++ b/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/client_info.h
>@@ -32,6 +32,8 @@
> 
> namespace google_breakpad {
> 
>+class CrashGenerationServer;
>+
> struct ClientInfo {
>   CrashGenerationServer* crash_server_;
>   pid_t pid_;

Is this a related change? Seems out of place.

>diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp
>--- a/toolkit/crashreporter/nsExceptionHandler.cpp
>+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
>+// Needs to be thread-safe because it might be accessed inside of a
>+// minidump-request callback on another thread
>+typedef nsInterfaceHashtableMT<nsUint32HashKey, nsIFile> ChildMinidumpMap;
>+static ChildMinidumpMap* pidToMinidump;

I'd follow bsmedberg's advice here re: threadsafety.

>+bool
> UnsetRemoteExceptionHandler()
> {
>   delete gExceptionHandler;
>   gExceptionHandler = NULL;
>+
>+  delete pidToMinidump;
>+  pidToMinidump = NULL;

This is in the wrong place, this is called from the child process. It needs to be in UnsetExceptionHandler.

>+
>+#if defined(XP_WIN)
>+  PR_Free(childCrashNotifyPipe);
>+  childCrashNotifyPipe = NULL;
>+#endif

I think this is also in the wrong place. This probably wants to be in a OOPShutdown or something like that, which could then get called from UnsetExceptionHandler (or directly from the IPC code).


>diff --git a/toolkit/crashreporter/nsExceptionHandler.h b/toolkit/crashreporter/nsExceptionHandler.h
>--- a/toolkit/crashreporter/nsExceptionHandler.h
>+++ b/toolkit/crashreporter/nsExceptionHandler.h
>@@ -42,6 +42,8 @@
> #include "nsXPCOM.h"
> #include "nsStringGlue.h"
> 
>+#include "nsIFile.h"
>+
> #if defined(XP_WIN32)
> #ifdef WIN32_LEAN_AND_MEAN
> #undef WIN32_LEAN_AND_MEAN
>@@ -65,10 +67,12 @@ nsresult SetupExtraData(nsILocalFile* aA
> #ifdef XP_WIN32
>   nsresult WriteMinidumpForException(EXCEPTION_POINTERS* aExceptionInfo);
> 
>+#  if defined(MOZ_IPC)

There are a few more APIs here that could be #ifdef MOZ_IPC.

>diff --git a/toolkit/xre/nsEmbedFunctions.cpp b/toolkit/xre/nsEmbedFunctions.cpp
>--- a/toolkit/xre/nsEmbedFunctions.cpp
>+++ b/toolkit/xre/nsEmbedFunctions.cpp
>@@ -259,6 +259,15 @@ static MessageLoop* sIOMessageLoop;
> static MessageLoop* sIOMessageLoop;
> 
> #if defined(MOZ_CRASHREPORTER)
>+// FIXME/bug 539522: this out-of-place function is stuck here because
>+// IPDL wants access to this crashreporter interface, and
>+// crashreporter is built in such a way to make that awkward
>+PRBool
>+XRE_GetMinidumpForChild(PRUint32 aChildPid, nsIFile** aDump)
>+{
>+  return CrashReporter::GetMinidumpForChild(aChildPid, aDump);
>+}
>+

Do we need to do this via IPDL? Seems like we could expose this via XPCOM or something and make things simpler, and then just provide the child PID on the event that gets sent to the browser chrome.

Comment 9

[Benjamin Smedberg]

Well... remember that there are going to be *many* events sent to browser chrome (one per plugin instance). I wanted a single place where we could move the minidump to pending/ and write the .extra file, and I thought the toplevel IPDL ActorDestroy method would be a good place to kick that off. Remember that the toplevel actor will be storing metadata that we want to send with the report, such as the plugin name/version data.

Comment 10

[(not currently active) Ted Mielczarek]

Ah, ok. I thought we'd do that in the callback in nsExceptionHandler.cpp, but maybe getting all the necessary data there would be a pain.

Comment 11

[Benjamin Smedberg]

Checking in ep_unittest.pl;
/cvsroot/mozilla/webtools/tinderbox/ep_unittest.pl,v  <--  ep_unittest.pl
new revision: 1.6; previous revision: 1.5
done

Comment 12

[Benjamin Smedberg]

Whoops, wrong bug!

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 421581 [details] [diff] [review]
Add a GetMinidump() method to top-level, parent-side IPDL classes

The IPDL changes look fine to me.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: write out a .extra file in the minidump callback

This will conflict a little bit with cjones' patch, but I think this is part of the right approach. This writes out almost all the data that's been set in the parent process into a .extra file for the child process' dump. There's a short blacklist of things that we ignore because they don't make sense. I think we can take this + cjones' patch, then make the IPDL AbnormalShutdown case append the rest of the interesting data to that file (plugin filename, child process startup time, etc) and then move both the .dmp and .extra to the Crash Reports/pending directory.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #8)
> (From update of attachment 421606 [details] [diff] [review])
> >diff --git a/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/client_info.h b/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/client_info.h
> >--- a/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/client_info.h
> >+++ b/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/client_info.h
> >@@ -32,6 +32,8 @@
> > 
> > namespace google_breakpad {
> > 
> >+class CrashGenerationServer;
> >+
> > struct ClientInfo {
> >   CrashGenerationServer* crash_server_;
> >   pid_t pid_;
> 
> Is this a related change? Seems out of place.
> 

Yes.  I'll split it out into a separate patch.

> >diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp
> >--- a/toolkit/crashreporter/nsExceptionHandler.cpp
> >+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
> >+// Needs to be thread-safe because it might be accessed inside of a
> >+// minidump-request callback on another thread
> >+typedef nsInterfaceHashtableMT<nsUint32HashKey, nsIFile> ChildMinidumpMap;
> >+static ChildMinidumpMap* pidToMinidump;
> 
> I'd follow bsmedberg's advice here re: threadsafety.
> 
> >+bool
> > UnsetRemoteExceptionHandler()
> > {
> >   delete gExceptionHandler;
> >   gExceptionHandler = NULL;
> >+
> >+  delete pidToMinidump;
> >+  pidToMinidump = NULL;
> 
> This is in the wrong place, this is called from the child process. It needs to
> be in UnsetExceptionHandler.
> 
> >+
> >+#if defined(XP_WIN)
> >+  PR_Free(childCrashNotifyPipe);
> >+  childCrashNotifyPipe = NULL;
> >+#endif
> 
> I think this is also in the wrong place. This probably wants to be in a
> OOPShutdown or something like that, which could then get called from
> UnsetExceptionHandler (or directly from the IPC code).
> 

Gah!  Epic fail, thanks for the catch.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 0: Forward-declare CrashGenerationServer from client_info.h so that it can be included before crash_generation_server.h

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 1, v2: Add a GetMinidump() method to top-level, parent-side IPDL classes

Addressed comments.  I went ahead and shuffled some functions in nsExceptionHandler.h to make them easier to |ifdef MOZ_IPC|, so there's a little more diff there than you might expect.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 1, v3: Has ted's .extra patch folded in

Comment 19

[(not currently active) Ted Mielczarek]

Attachment: write out some more .extra data in PluginModuleParent

This fleshes out the code in PluginModuleParent, and make it add ProcessType, StartupTime, and PluginFilename. (PluginName and PluginVersion I left as a TODO for bug 539841.)

I'll add the data to the event in a separate patch.

Comment 20

[Benjamin Smedberg]

Comment on attachment 421881 [details] [diff] [review]
write out some more .extra data in PluginModuleParent

nsCRT.h has FILE_PATH_SEPARATOR, let's use that instead of adding a new #define

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 421742 [details] [diff] [review]
Part 1, v3: Has ted's .extra patch folded in

Your parts look fine. I have a slightly updated version of this that I'll attach, and bsmedberg can give my half a once-over.

Comment 22

[(not currently active) Ted Mielczarek]

cjones: actually, looking again, it looks like you lost the hunk that deletes pidToMinidump. I noted that it was misplaced in the first patch, did you mean to move it but forgot?

Comment 23

[(not currently active) Ted Mielczarek]

Attachment: Part 1, v4: fix oopcrashdialog.js, add CrashTime info

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #22)
> cjones: actually, looking again, it looks like you lost the hunk that deletes
> pidToMinidump. I noted that it was misplaced in the first patch, did you mean
> to move it but forgot?

Nope, I meant to toast it.  It falls under bug 539451.

Comment 25

[(not currently active) Ted Mielczarek]

I pushed all these patches to e10s:
http://hg.mozilla.org/projects/electrolysis/rev/69efd75e70ca
http://hg.mozilla.org/projects/electrolysis/rev/c278aa8c9eff
http://hg.mozilla.org/projects/electrolysis/rev/c15099808e2c
http://hg.mozilla.org/projects/electrolysis/rev/cca0bf3db70a

I'm going to move any work that's left to a new bug, this one is kind of crowded.

Comment 26

[(not currently active) Ted Mielczarek]

Filed bug 541076 for adding the minidump id to the PluginCrashed event.

Comment 27

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/bab9a9bd06b4
http://hg.mozilla.org/mozilla-central/rev/4309b6909234
http://hg.mozilla.org/mozilla-central/rev/d2f40c5d85f9
http://hg.mozilla.org/mozilla-central/rev/69153c9be237

Comment 28

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/34207a86b238

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 30

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430