Bug 1536850 - Properly detect when the browser is shutting down to avoid taking content process minidumps too late r=Ehsan a=pascalc
authorGabriele Svelto <gsvelto@mozilla.com>
Fri, 29 Mar 2019 19:17:56 +0000
changeset 526124 ea0b308de5c414a253e7d851286dea8d6879a43a
parent 526123 7fe4dd7263634fb16868854059f4f8269e8bb1cc
child 526125 778c3da2f7ae901e6cb55115217f8fb32a798bae
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersEhsan, pascalc
Bug 1536850 - Properly detect when the browser is shutting down to avoid taking content process minidumps too late r=Ehsan a=pascalc This code was already present but it had two flaws: the first one was that the variable used to prevent minidumps from being taken when shutting down was not being initialized which caused it to be true most of the time and thus prevented captures even in valid scenarios. The second was that we relied on the "profile-before-change" change event to detect shutdown but this often happens after we've already started closing tabs and shutting down processes thus the minidump generation would often race against it. Differential Revision: https://phabricator.services.mozilla.com/D24691
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -129,16 +129,17 @@
 #include "nsContentUtils.h"
 #include "nsDebugImpl.h"
 #include "nsDirectoryServiceDefs.h"
 #include "nsEmbedCID.h"
 #include "nsFrameLoader.h"
 #include "nsFrameMessageManager.h"
 #include "nsHashPropertyBag.h"
 #include "nsIAlertsService.h"
+#include "nsIAppStartup.h"
 #include "nsIClipboard.h"
 #include "nsICookie.h"
 #include "nsContentPermissionHelper.h"
 #include "nsIContentSecurityPolicy.h"
 #include "nsIContentProcess.h"
 #include "nsICycleCollectorListener.h"
 #include "nsIDocShellTreeOwner.h"
 #include "mozilla/dom/Document.h"
 ContentParent::Observe(nsISupports* aSubject, const char* aTopic,
                        const char16_t* aData) {
   if (mSubprocess && (!strcmp(aTopic, "profile-before-change") ||
                       !strcmp(aTopic, "xpcom-shutdown"))) {
-    mShuttingDown = true;
     // Make sure that our process will get scheduled.
     // Okay to call ShutDownProcess multiple times.
@@ -3372,17 +3371,21 @@ void ContentParent::ForceKillTimerCallba
 void ContentParent::GeneratePairedMinidump(const char* aReason) {
   // We're about to kill the child process associated with this content.
   // Something has gone wrong to get us here, so we generate a minidump
   // of the parent and child for submission to the crash server unless we're
   // already shutting down.
-  if (mCrashReporter && !mShuttingDown &&
+  nsCOMPtr<nsIAppStartup> appStartup = components::AppStartup::Service();
+  bool shuttingDown = false;
+  if (mCrashReporter &&
+      !(NS_SUCCEEDED(appStartup->GetShuttingDown(&shuttingDown)) &&
+        shuttingDown) &&
       Preferences::GetBool("dom.ipc.tabs.createKillHardCrashReports", false)) {
     // GeneratePairedMinidump creates two minidumps for us - the main
     // one is for the content process we're about to kill, and the other
     // one is for the main browser process. That second one is the extra
     // minidump tagging along, so we have to tell the crash reporter that
     // it exists and is being appended.
     nsAutoCString additionalDumps("browser");
@@ -5730,17 +5733,17 @@ mozilla::ipc::IPCResult ContentParent::R
             ("ParentIPC: Trying to attach already attached 0x%08" PRIx64
              " to 0x%08" PRIx64,
              aInit.mId, aInit.mParentId));
     return IPC_OK();
   if (!child) {
     RefPtr<BrowsingContextGroup> group =
-      BrowsingContextGroup::Select(aInit.mParentId, aInit.mOpenerId);
+        BrowsingContextGroup::Select(aInit.mParentId, aInit.mOpenerId);
     child = BrowsingContext::CreateFromIPC(std::move(aInit), group, this);
   child->Attach(/* aFromIPC */ true);
   return IPC_OK();
--- a/dom/ipc/ContentParent.h
+++ b/dom/ipc/ContentParent.h
@@ -1224,17 +1224,16 @@ class ContentParent final : public PCont
   enum class LifecycleState : uint8_t {
   LifecycleState mLifecycleState;
-  bool mShuttingDown;
   bool mIsForBrowser;
   // Whether this process is recording or replaying its execution, and any
   // associated recording file.
   RecordReplayState mRecordReplayState;
   nsString mRecordingFile;
   // When recording or replaying, the child process is a middleman. This vector