Bug 1360308 - Part 3: Support off-main-thread I/O in CrashReporter::CreateMinidumpsAndPair(). r=gsvelto
authorCervantes Yu <cyu@mozilla.com>
Thu, 22 Jun 2017 18:53:10 +0800
changeset 365493 4f17147356744f39fcd1756c019540a90eda9192
parent 365492 2b28e4ee269cb5559f0f28a1421d284cb0c98658
child 365494 5f978d7caf2f81d26372462ecee97fa33cd52a4d
push id32077
push userkwierso@gmail.com
push dateThu, 22 Jun 2017 21:02:06 +0000
treeherdermozilla-central@4c6668cbaccb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgsvelto
bugs1360308
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1360308 - Part 3: Support off-main-thread I/O in CrashReporter::CreateMinidumpsAndPair(). r=gsvelto This moves the I/O operations of writing minidumps in CrashReporter::CreateMinidumpsAndPair() off the main thread and use callbacks to notify the completion of the operations. MozReview-Commit-ID: 9wpTDDUp9GN
ipc/glue/CrashReporterHost.cpp
ipc/glue/CrashReporterHost.h
toolkit/crashreporter/nsExceptionHandler.cpp
toolkit/crashreporter/nsExceptionHandler.h
--- a/ipc/glue/CrashReporterHost.cpp
+++ b/ipc/glue/CrashReporterHost.cpp
@@ -126,32 +126,33 @@ CrashReporterHost::GenerateMinidumpAndPa
   mCreateMinidumpCallback.Init(Move(aCallback), aAsync);
 
   if (!childHandle) {
     NS_WARNING("Failed to get child process handle.");
     mCreateMinidumpCallback.Invoke(false);
     return;
   }
 
-  nsCOMPtr<nsIFile> targetDump;
-  if (!CrashReporter::CreateMinidumpsAndPair(childHandle,
-                                             mThreadId,
-                                             aPairName,
-                                             aMinidumpToPair,
-                                             getter_AddRefs(targetDump))) {
-    mCreateMinidumpCallback.Invoke(false);
-    return;
-  }
+  std::function<void(bool)> callback =
+    [this](bool aResult) {
+      if (aResult &&
+          CrashReporter::GetIDFromMinidump(this->mTargetDump, this->mDumpID)) {
+        this->mCreateMinidumpCallback.Invoke(true);
+      } else {
+        this->mCreateMinidumpCallback.Invoke(false);
+      }
+    };
 
-  if (!CrashReporter::GetIDFromMinidump(targetDump, mDumpID)) {
-    mCreateMinidumpCallback.Invoke(false);
-    return;
-  }
-
-  mCreateMinidumpCallback.Invoke(true);
+  CrashReporter::CreateMinidumpsAndPair(childHandle,
+                                        mThreadId,
+                                        aPairName,
+                                        aMinidumpToPair,
+                                        getter_AddRefs(mTargetDump),
+                                        Move(callback),
+                                        aAsync);
 }
 
 /* static */ void
 CrashReporterHost::NotifyCrashService(GeckoProcessType aProcessType,
                                       const nsString& aChildDumpID,
                                       const AnnotationTable* aNotes)
 {
   if (!NS_IsMainThread()) {
--- a/ipc/glue/CrashReporterHost.h
+++ b/ipc/glue/CrashReporterHost.h
@@ -159,14 +159,15 @@ private:
   Shmem mShmem;
   ThreadId mThreadId;
   time_t mStartTime;
 #ifdef MOZ_CRASHREPORTER
   AnnotationTable mExtraNotes;
 #endif
   nsString mDumpID;
   bool mFinalized;
+  nsCOMPtr<nsIFile> mTargetDump;
 };
 
 } // namespace ipc
 } // namespace mozilla
 
 #endif // mozilla_ipc_CrashReporterHost_h
--- a/toolkit/crashreporter/nsExceptionHandler.cpp
+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
@@ -310,16 +310,18 @@ struct ChildProcessData : public nsUint3
 #endif
 };
 
 typedef nsTHashtable<ChildProcessData> ChildMinidumpMap;
 static ChildMinidumpMap* pidToMinidump;
 static uint32_t crashSequence;
 static bool OOPInitialized();
 
+static nsIThread* sMinidumpWriterThread;
+
 #ifdef MOZ_CRASHREPORTER_INJECTOR
 static nsIThread* sInjectorThread;
 
 class ReportInjectedCrash : public Runnable
 {
 public:
   explicit ReportInjectedCrash(uint32_t pid) : mPID(pid) { }
 
@@ -3555,16 +3557,21 @@ OOPDeinit()
 
 #ifdef MOZ_CRASHREPORTER_INJECTOR
   if (sInjectorThread) {
     sInjectorThread->Shutdown();
     NS_RELEASE(sInjectorThread);
   }
 #endif
 
+  if (sMinidumpWriterThread) {
+    sMinidumpWriterThread->Shutdown();
+    NS_RELEASE(sMinidumpWriterThread);
+  }
+
   delete crashServer;
   crashServer = nullptr;
 
   delete dumpMapLock;
   dumpMapLock = nullptr;
 
   delete pidToMinidump;
   pidToMinidump = nullptr;
@@ -4037,55 +4044,74 @@ bool TakeMinidump(nsIFile** aResult, boo
   }
 
   if (aMoveToPending) {
     MoveToPending(*aResult, nullptr, nullptr);
   }
   return true;
 }
 
-bool
-CreateMinidumpsAndPair(ProcessHandle aTargetPid,
-                       ThreadId aTargetBlamedThread,
-                       const nsACString& aIncomingPairName,
-                       nsIFile* aIncomingDumpToPair,
-                       nsIFile** aMainDumpOut)
+inline void
+InvokeCallback(bool aAsync,
+               std::function<void()>&& aCallback,
+               RefPtr<nsIThread>&& aCallbackThread)
 {
-  if (!GetEnabled()) {
-    return false;
+  if (aAsync) {
+    MOZ_ASSERT(!!aCallbackThread);
+    Unused << aCallbackThread->Dispatch(NS_NewRunnableFunction(Move(aCallback)),
+                                        NS_DISPATCH_SYNC);
+  } else {
+    aCallback();
   }
-
+}
+
+void
+CreateMinidumpsAndPairInternal(ProcessHandle aTargetPid,
+                               ThreadId aTargetBlamedThread,
+                               nsCString aIncomingPairName,
+                               nsCOMPtr<nsIFile> aIncomingDumpToPair,
+                               nsIFile** aMainDumpOut,
+                               std::function<void(bool)>&& aCallback,
+                               RefPtr<nsIThread>&& aCallbackThread,
+                               bool aAsync)
+{
   AutoIOInterposerDisable disableIOInterposition;
 
 #ifdef XP_MACOSX
   mach_port_t targetThread = GetChildThread(aTargetPid, aTargetBlamedThread);
 #else
   ThreadId targetThread = aTargetBlamedThread;
 #endif
 
   xpstring dump_path;
 #ifndef XP_LINUX
   dump_path = gExceptionHandler->dump_path();
 #else
   dump_path = gExceptionHandler->minidump_descriptor().directory();
 #endif
 
+  std::function<void()> runnable;
   // dump the target
   nsCOMPtr<nsIFile> targetMinidump;
   if (!google_breakpad::ExceptionHandler::WriteMinidumpForChild(
          aTargetPid,
          targetThread,
          dump_path,
          PairedDumpCallbackExtra,
          static_cast<void*>(&targetMinidump)
 #ifdef XP_WIN32
          , GetMinidumpType()
 #endif
-      ))
-    return false;
+      )) {
+    runnable = [&](){
+      aCallback(false);
+    };
+    InvokeCallback(aAsync, Move(runnable), Move(aCallbackThread));
+    return;
+  }
 
   nsCOMPtr<nsIFile> targetExtra;
   GetExtraFileForMinidump(targetMinidump, getter_AddRefs(targetExtra));
 
   // If aIncomingDumpToPair isn't valid, create a dump of this process.
   nsCOMPtr<nsIFile> incomingDump;
   if (aIncomingDumpToPair == nullptr) {
     if (!google_breakpad::ExceptionHandler::WriteMinidump(
@@ -4094,34 +4120,90 @@ CreateMinidumpsAndPair(ProcessHandle aTa
         true,
 #endif
         PairedDumpCallback,
         static_cast<void*>(&incomingDump)
 #ifdef XP_WIN32
         , GetMinidumpType()
 #endif
         )) {
-      targetMinidump->Remove(false);
-      targetExtra->Remove(false);
-      return false;
+      runnable = [&](){
+        targetMinidump->Remove(false);
+        targetExtra->Remove(false);
+        aCallback(false);
+      };
+      InvokeCallback(aAsync, Move(runnable), Move(aCallbackThread));
+      return;
     }
   } else {
     incomingDump = aIncomingDumpToPair;
   }
 
-  RenameAdditionalHangMinidump(incomingDump, targetMinidump, aIncomingPairName);
-
-  if (ShouldReport()) {
-    MoveToPending(targetMinidump, targetExtra, nullptr);
-    MoveToPending(incomingDump, nullptr, nullptr);
+  runnable = [&](){
+    RenameAdditionalHangMinidump(incomingDump, targetMinidump, aIncomingPairName);
+
+    if (ShouldReport()) {
+      MoveToPending(targetMinidump, targetExtra, nullptr);
+      MoveToPending(incomingDump, nullptr, nullptr);
+    }
+
+    targetMinidump.forget(aMainDumpOut);
+    aIncomingPairName = nullptr; // Release the ptr on the main thread.
+
+    aCallback(true);
+  };
+  InvokeCallback(aAsync, Move(runnable), Move(aCallbackThread));
+}
+
+void
+CreateMinidumpsAndPair(ProcessHandle aTargetPid,
+                       ThreadId aTargetBlamedThread,
+                       const nsACString& aIncomingPairName,
+                       nsIFile* aIncomingDumpToPair,
+                       nsIFile** aMainDumpOut,
+                       std::function<void(bool)>&& aCallback,
+                       bool aAsync)
+{
+  if (!GetEnabled()) {
+    aCallback(false);
+    return;
   }
 
-  targetMinidump.forget(aMainDumpOut);
-
-  return true;
+  if (aAsync &&
+      !sMinidumpWriterThread &&
+      NS_FAILED(NS_NewNamedThread("Minidump Writer", &sMinidumpWriterThread))) {
+    aCallback(false);
+    return;
+  }
+
+  nsCOMPtr<nsIFile> incomingDumpToPair = aIncomingDumpToPair;
+  nsCString incomingPairName(aIncomingPairName);
+  std::function<void(bool)> callback = Move(aCallback);
+  // Don't call do_GetCurrentThread() is this is called synchronously because
+  // 1. it's unnecessary, and 2. more importantly, it might create one if called
+  // from a native thread, and the thread will be leakded.
+  RefPtr<nsIThread> callbackThread = aAsync ? do_GetCurrentThread() : nullptr;
+
+  std::function<void()> doDump = [=]() mutable {
+    CreateMinidumpsAndPairInternal(aTargetPid,
+                                   aTargetBlamedThread,
+                                   incomingPairName,
+                                   incomingDumpToPair,
+                                   aMainDumpOut,
+                                   Move(callback),
+                                   Move(callbackThread),
+                                   aAsync);
+  };
+
+  if (aAsync) {
+    sMinidumpWriterThread->Dispatch(NS_NewRunnableFunction(Move(doDump)),
+                                    nsIEventTarget::DISPATCH_NORMAL);
+  } else {
+    doDump();
+  }
 }
 
 bool
 CreateAdditionalChildMinidump(ProcessHandle childPid,
                               ThreadId childBlamedThread,
                               nsIFile* parentMinidump,
                               const nsACString& name)
 {
--- a/toolkit/crashreporter/nsExceptionHandler.h
+++ b/toolkit/crashreporter/nsExceptionHandler.h
@@ -3,16 +3,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef nsExceptionHandler_h__
 #define nsExceptionHandler_h__
 
 #include "mozilla/Assertions.h"
 
+#include <functional>
 #include <stddef.h>
 #include <stdint.h>
 #include "nsError.h"
 #include "nsStringGlue.h"
 
 #if defined(XP_WIN32)
 #ifdef WIN32_LEAN_AND_MEAN
 #undef WIN32_LEAN_AND_MEAN
@@ -193,21 +194,24 @@ ThreadId CurrentThreadId();
  *   passes in.
  * @param aIncomingDumpToPair Existing dump to pair with the new dump. if this
  *   is null, TakeMinidumpAndPair will take a new minidump of the calling
  *   process and thread and use it in aIncomingDumpToPairs place.
  * @param aTargetDumpOut The target minidump file paired up with
  *   aIncomingDumpToPair.
  * @return bool indicating success or failure
  */
-bool CreateMinidumpsAndPair(ProcessHandle aTargetPid,
-                            ThreadId aTargetBlamedThread,
-                            const nsACString& aIncomingPairName,
-                            nsIFile* aIncomingDumpToPair,
-                            nsIFile** aTargetDumpOut);
+void
+CreateMinidumpsAndPair(ProcessHandle aTargetPid,
+                       ThreadId aTargetBlamedThread,
+                       const nsACString& aIncomingPairName,
+                       nsIFile* aIncomingDumpToPair,
+                       nsIFile** aTargetDumpOut,
+                       std::function<void(bool)>&& aCallback,
+                       bool aAsync);
 
 // Create an additional minidump for a child of a process which already has
 // a minidump (|parentMinidump|).
 // The resulting dump will get the id of the parent and use the |name| as
 // an extension.
 bool CreateAdditionalChildMinidump(ProcessHandle childPid,
                                    ThreadId childBlamedThread,
                                    nsIFile* parentMinidump,