Bug 1628399 - Make sure that the main process can't access a crash report before it's fully populated r=KrisWright

In bug 1614933 we changed the order in which the crash annotations are sent
from a crashed content process to the main process in order to prevent a
deadlock that would arise if the child process would block writing into the
pipe used to sent the annotations.

This unfortunately introduced a race that I had missed. Here's the sequence of
event when generating a crash in the child process:

1) The child process enters the exception handler
2) It requests a dump from the main process and wait
3) Once the dump is written, the child process wakes up again and writes out
   the crash annotations
4) The child process quits

One the main process side it looks like this:

1) The crash generation thread receives a request to generate a dump
2) The dump is written out, the crash generation thread notifies the content
   process that it can resume execution. During this step the finished
   minidump is recorded in the `OnChildProcessDumpRequested()` callback.
3) The crash generation thread reads the crash annotations sent by the content
   process and finalizes the crash report
4) The main process grabs the finalized crash report

The key issue here is that the main process in step 4 is woken up when the
content process dies. Notice how there's no synchronization between the crash
generation thread and the main thread in the main process: if the crash
generation thread takes too long reading the crash annotations the main thread
might see an incomplete or missing crash report; that's because the event that
wakes it up - the content process ending execution - can happen in parallel
with step 3.

This is an issue that was accidentally introduced in Windows by bug 1614933
but was already present in both Linux and macOS. It was just very unlikely to
happen.

This patch fixes the issue by splitting step 3 in the main process in two
different stages: in the main process we grab the generated minidump in
`OnChildProcessDumpRequested()`, Breakpad then unblocks the child process and
we read the annotations in `OnChildProcessDumpWritten()`. We grab the
`dumpMapLock` Mutex in the latter and release it in the former to ensure that
the main process will have to wait for the crash report to be finalized when
`TakeCrashedChildProcess()` is called. This might appear somewhat confusing
and even causes debug builds to spit a harmless warning (we don't want Mutexes
to be taken and released from different scopes if we can help it).

To implement the above behavior in Breakpad a new callback was introduced in
Windows, an existing one was used under macOS and an unused one was used under
Linux. This accounts for the different way in which minidumps are generated on
the three platforms.

Differential Revision: https://phabricator.services.mozilla.com/D74496

toolkit/crashreporter/breakpad-client/linux/crash_generation/crash_generation_server.cc

@@ -273,9 +273,8 @@ CrashGenerationServer::ClientEvent(short revents)
     return true;
   }
 
+  ClientInfo info(crashing_pid, this);
   if (dump_callback_) {
-    ClientInfo info(crashing_pid, this);
-
     dump_callback_(dump_context_, info, minidump_filename);
   }
 
@@ -283,6 +282,10 @@ CrashGenerationServer::ClientEvent(short revents)
   // (Closing this will make the child's sys_read unblock and return 0.)
   close(signal_fd);
 
+  if (exit_callback_) {
+    exit_callback_(exit_context_, info);
+  }
+
   return true;
 }
 

toolkit/crashreporter/breakpad-client/windows/crash_generation/crash_generation_server.cc

@@ -101,6 +101,7 @@ CrashGenerationServer::CrashGenerationServer(
     void* connect_context,
     OnClientDumpRequestCallback dump_callback,
     void* dump_context,
+    OnClientDumpWrittenCallback written_callback,
     OnClientExitedCallback exit_callback,
     void* exit_context,
     OnClientUploadRequestCallback upload_request_callback,
@@ -116,6 +117,7 @@ CrashGenerationServer::CrashGenerationServer(
       connect_context_(connect_context),
       dump_callback_(dump_callback),
       dump_context_(dump_context),
+      written_callback_(written_callback),
       exit_callback_(exit_callback),
       exit_context_(exit_context),
       upload_request_callback_(upload_request_callback),
@@ -892,11 +894,15 @@ void CrashGenerationServer::HandleDumpRequest(const ClientInfo& client_info) {
     }
   }
 
-  SetEvent(client_info.dump_generated_handle());
-
   if (dump_callback_ && execute_callback) {
     dump_callback_(dump_context_, client_info, dump_path);
   }
+
+  SetEvent(client_info.dump_generated_handle());
+
+  if (written_callback_ && execute_callback) {
+    written_callback_(dump_context_, client_info);
+  }
 }
 
 void CrashGenerationServer::set_include_context_heap(bool enabled) {

toolkit/crashreporter/breakpad-client/windows/crash_generation/crash_generation_server.h

@@ -57,6 +57,9 @@ class CrashGenerationServer {
                                               const ClientInfo& client_info,
                                               const std::wstring& file_path);
 
+  typedef void (*OnClientDumpWrittenCallback)(void* context,
+                                              const ClientInfo& client_info);
+
   typedef void (*OnClientExitedCallback)(void* context,
                                          const ClientInfo& client_info);
 
@@ -72,8 +75,9 @@ class CrashGenerationServer {
   //     the Everyone group read access on the pipe.
   // Parameter connect_callback: Callback for a new client connection.
   // Parameter connect_context: Context for client connection callback.
-  // Parameter crash_callback: Callback for a client crash dump request.
-  // Parameter crash_context: Context for client crash dump request callback.
+  // Parameter dump_callback: Callback for a client crash dump request.
+  // Parameter dump_context: Context for client crash dump request callback.
+  // Parameter written_callback: Callback called after a crash dump was written.
   // Parameter exit_callback: Callback for client process exit.
   // Parameter exit_context: Context for client exit callback.
   // Parameter generate_dumps: Whether to automatically generate dumps.
@@ -88,6 +92,7 @@ class CrashGenerationServer {
                         void* connect_context,
                         OnClientDumpRequestCallback dump_callback,
                         void* dump_context,
+                        OnClientDumpWrittenCallback written_callback,
                         OnClientExitedCallback exit_callback,
                         void* exit_context,
                         OnClientUploadRequestCallback upload_request_callback,
@@ -256,6 +261,9 @@ class CrashGenerationServer {
   // Context for client dump request callback.
   void* dump_context_;
 
+  // Callback for a client dump written.
+  OnClientDumpWrittenCallback written_callback_;
+
   // Callback for client process exit.
   OnClientExitedCallback exit_callback_;
 

toolkit/crashreporter/nsExceptionHandler.cpp

@@ -276,7 +276,8 @@ struct ChildProcessData : public nsUint32HashKey {
   explicit ChildProcessData(KeyTypePointer aKey)
       : nsUint32HashKey(aKey),
         sequence(0),
-        annotations(nullptr)
+        annotations(nullptr),
+        minidumpOnly(false)
 #ifdef MOZ_CRASHREPORTER_INJECTOR
         ,
         callback(nullptr)
@@ -289,6 +290,7 @@ struct ChildProcessData : public nsUint32HashKey {
   // indicate which process crashed first.
   uint32_t sequence;
   UniquePtr<AnnotationTable> annotations;
+  bool minidumpOnly;  // If true then no annotations are present
 #ifdef MOZ_CRASHREPORTER_INJECTOR
   InjectorCrashCallback* callback;
 #endif
@@ -3184,11 +3186,9 @@ static void ReadExceptionTimeAnnotations(AnnotationTable& aAnnotations,
   }
 }
 
-static void PopulateContentProcessAnnotations(AnnotationTable& aAnnotations,
-                                              uint32_t aPid) {
+static void PopulateContentProcessAnnotations(AnnotationTable& aAnnotations) {
   MergeContentCrashAnnotations(aAnnotations);
   AddCommonAnnotations(aAnnotations);
-  ReadExceptionTimeAnnotations(aAnnotations, aPid);
 }
 
 // It really only makes sense to call this function when
@@ -3255,13 +3255,13 @@ static void OnChildProcessDumpRequested(void* aContext,
     bool runCallback;
 #endif
     {
-      MutexAutoLock lock(*dumpMapLock);
+      dumpMapLock->Lock();
       ChildProcessData* pd = pidToMinidump->PutEntry(pid);
       MOZ_ASSERT(!pd->minidump);
       pd->minidump = minidump;
       pd->sequence = ++crashSequence;
       pd->annotations = MakeUnique<AnnotationTable>();
-      PopulateContentProcessAnnotations(*(pd->annotations), pid);
+      PopulateContentProcessAnnotations(*(pd->annotations));
 #ifdef MOZ_CRASHREPORTER_INJECTOR
       runCallback = nullptr != pd->callback;
 #endif
@@ -3272,6 +3272,17 @@ static void OnChildProcessDumpRequested(void* aContext,
   }
 }
 
+static void OnChildProcessDumpWritten(void* aContext,
+                                      const ClientInfo& aClientInfo) {
+  uint32_t pid = aClientInfo.pid();
+  ChildProcessData* pd = pidToMinidump->GetEntry(pid);
+  MOZ_ASSERT(pd);
+  if (!pd->minidumpOnly) {
+    ReadExceptionTimeAnnotations(*(pd->annotations), pid);
+  }
+  dumpMapLock->Unlock();
+}
+
 static bool OOPInitialized() { return pidToMinidump != nullptr; }
 
 void OOPInit() {
@@ -3310,7 +3321,7 @@ void OOPInit() {
       std::wstring(NS_ConvertASCIItoUTF16(childCrashNotifyPipe).get()),
       nullptr,           // default security attributes
       nullptr, nullptr,  // we don't care about process connect here
-      OnChildProcessDumpRequested, nullptr, nullptr,
+      OnChildProcessDumpRequested, nullptr, OnChildProcessDumpWritten, nullptr,
       nullptr,           // we don't care about process exit here
       nullptr, nullptr,  // we don't care about upload request here
       true,              // automatically generate dumps
@@ -3328,9 +3339,8 @@ void OOPInit() {
   const std::string dumpPath =
       gExceptionHandler->minidump_descriptor().directory();
   crashServer = new CrashGenerationServer(
-      serverSocketFd, OnChildProcessDumpRequested, nullptr, nullptr,
-      nullptr,  // we don't care about process exit here
-      true, &dumpPath);
+      serverSocketFd, OnChildProcessDumpRequested, nullptr,
+      OnChildProcessDumpWritten, nullptr, true, &dumpPath);
 
 #elif defined(XP_MACOSX)
   childCrashNotifyPipe = mozilla::Smprintf("gecko-crash-server-pipe.%i",
@@ -3338,11 +3348,11 @@ void OOPInit() {
                              .release();
   const std::string dumpPath = gExceptionHandler->dump_path();
 
-  crashServer = new CrashGenerationServer(childCrashNotifyPipe, nullptr,
-                                          nullptr, OnChildProcessDumpRequested,
-                                          nullptr, nullptr, nullptr,
-                                          true,  // automatically generate dumps
-                                          dumpPath);
+  crashServer = new CrashGenerationServer(
+      childCrashNotifyPipe, nullptr, nullptr, OnChildProcessDumpRequested,
+      nullptr, OnChildProcessDumpWritten, nullptr,
+      true,  // automatically generate dumps
+      dumpPath);
 #endif
 
   if (!crashServer->Start()) MOZ_CRASH("can't start crash reporter server()");
@@ -3411,6 +3421,7 @@ void InjectCrashReporterIntoProcess(DWORD processID,
     ChildProcessData* pd = pidToMinidump->PutEntry(processID);
     MOZ_ASSERT(!pd->minidump && !pd->callback);
     pd->callback = cb;
+    pd->minidumpOnly = true;
   }
 
   nsCOMPtr<nsIRunnable> r = new InjectCrashRunnable(processID);
@@ -3563,7 +3574,7 @@ bool TakeMinidumpForChild(uint32_t childPid, nsIFile** dump,
   NS_IF_ADDREF(*dump = pd->minidump);
   // Only Flash process minidumps don't have annotations. Once we get rid of
   // the Flash processes this check will become redundant.
-  if (pd->annotations) {
+  if (!pd->minidumpOnly) {
     aAnnotations = *(pd->annotations);
   }
   if (aSequence) {