Bug 1498765 - Clean up ContentParent::KillHard handling. r=mccr8
authorJed Davis <jld@mozilla.com>
Fri, 19 Oct 2018 15:29:34 -0600
changeset 442119 1ca458791c2739f9c6bc7abe2b0672ed3518e9a3
parent 442118 a254c5207ba052f56527ea28d3dad50e0182cd33
child 442120 3f193025450a04021ca5709deb7f1e94c7add510
push id109107
push userjedavis@mozilla.com
push dateFri, 19 Oct 2018 21:29:52 +0000
treeherdermozilla-inbound@1ca458791c27 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1498765
milestone64.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 1498765 - Clean up ContentParent::KillHard handling. r=mccr8 MessageChannel shouldn't need to care about PContent-specific details.
dom/ipc/ContentParent.cpp
dom/ipc/ContentParent.h
ipc/glue/MessageChannel.cpp
ipc/glue/MessageChannel.h
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -1726,38 +1726,24 @@ namespace {
 
 void
 DelayedDeleteSubprocess(GeckoChildProcessHost* aSubprocess)
 {
   RefPtr<DeleteTask<GeckoChildProcessHost>> task = new DeleteTask<GeckoChildProcessHost>(aSubprocess);
   XRE_GetIOMessageLoop()->PostTask(task.forget());
 }
 
-// This runnable only exists to delegate ownership of the
-// ContentParent to this runnable, until it's deleted by the event
-// system.
-struct DelayedDeleteContentParentTask : public Runnable
-{
-  explicit DelayedDeleteContentParentTask(ContentParent* aObj)
-    : Runnable("dom::DelayedDeleteContentParentTask")
-    , mKungFuDeathGrip(aObj)
-  {
-  }
-
-  // No-op
-  NS_IMETHOD Run() override { return NS_OK; }
-
-  RefPtr<ContentParent> mKungFuDeathGrip;
-};
-
 } // namespace
 
 void
 ContentParent::ActorDestroy(ActorDestroyReason why)
 {
+  RefPtr<ContentParent> kungFuDeathGrip(mSelfRef.forget());
+  MOZ_RELEASE_ASSERT(kungFuDeathGrip);
+
   if (mForceKillTimer) {
     mForceKillTimer->Cancel();
     mForceKillTimer = nullptr;
   }
 
   // Signal shutdown completion regardless of error state, so we can
   // finish waiting in the xpcom-shutdown/profile-before-change observer.
   mIPCOpen = false;
@@ -1778,17 +1764,16 @@ ContentParent::ActorDestroy(ActorDestroy
     // ShutDownProcess below to perform other necessary clean up.
     mCalledClose = true;
   }
 
   // Make sure we always clean up.
   ShutDownProcess(why == NormalShutdown ? CLOSE_CHANNEL
                                         : CLOSE_CHANNEL_WITH_ERROR);
 
-  RefPtr<ContentParent> kungFuDeathGrip(this);
   nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
   if (obs) {
     size_t length = ArrayLength(sObserverTopics);
     for (size_t i = 0; i < length; ++i) {
       obs->RemoveObserver(static_cast<nsIObserver*>(this),
                           sObserverTopics[i]);
     }
   }
@@ -1865,17 +1850,19 @@ ContentParent::ActorDestroy(ActorDestroy
 
   // IPDL rules require actors to live on past ActorDestroy, but it
   // may be that the kungFuDeathGrip above is the last reference to
   // |this|.  If so, when we go out of scope here, we're deleted and
   // all hell breaks loose.
   //
   // This runnable ensures that a reference to |this| lives on at
   // least until after the current task finishes running.
-  NS_DispatchToCurrentThread(new DelayedDeleteContentParentTask(this));
+  NS_DispatchToCurrentThread(
+    NS_NewRunnableFunction("DelayedReleaseContentParent",
+                           [kungFuDeathGrip] { }));
 
   ContentProcessManager* cpm = ContentProcessManager::GetSingleton();
   nsTArray<ContentParentId> childIDArray =
     cpm->GetAllChildProcessById(this->ChildID());
 
   // Destroy any processes created by this ContentParent
   for(uint32_t i = 0; i < childIDArray.Length(); i++) {
     ContentParent* cp = cpm->GetContentProcessById(childIDArray[i]);
@@ -2240,16 +2227,19 @@ ContentParent::LaunchSubprocess(ProcessP
 #else
   if (!mSubprocess->LaunchAndWaitForProcessHandle(extraArgs)) {
 #endif
     NS_ERROR("failed to launch child in the parent");
     MarkAsDead();
     return false;
   }
 
+  // See also ActorDestroy.
+  mSelfRef = this;
+
 #ifdef ASYNC_CONTENTPROC_LAUNCH
   OpenWithAsyncPid(mSubprocess->GetChannel());
 #else
   base::ProcessId procId =
     base::GetProcId(mSubprocess->GetChildProcessHandle());
   Open(mSubprocess->GetChannel(), procId);
 #ifdef MOZ_CODE_COVERAGE
   Unused << SendShareCodeCoverageMutex(
@@ -2286,16 +2276,17 @@ ContentParent::LaunchSubprocess(ProcessP
 }
 
 ContentParent::ContentParent(ContentParent* aOpener,
                              const nsAString& aRemoteType,
                              RecordReplayState aRecordReplayState,
                              const nsAString& aRecordingFile,
                              int32_t aJSPluginID)
   : nsIContentParent()
+  , mSelfRef(nullptr)
   , mSubprocess(nullptr)
   , mLaunchTS(TimeStamp::Now())
   , mActivateTS(TimeStamp::Now())
   , mOpener(aOpener)
   , mRemoteType(aRemoteType)
   , mChildID(gContentChildID++)
   , mGeolocationWatchID(-1)
   , mJSPluginID(aJSPluginID)
@@ -3340,21 +3331,16 @@ ContentParent::KillHard(const char* aRea
   // process handle becomes invalid on the first call, causing a second call
   // to crash our process - more details in bug 890840.
   if (mCalledKillHard) {
     return;
   }
   mCalledKillHard = true;
   mForceKillTimer = nullptr;
 
-  MessageChannel* channel = GetIPCChannel();
-  if (channel) {
-    channel->SetInKillHardShutdown();
-  }
-
   // 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.
   if (mCrashReporter) {
     // 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
--- a/dom/ipc/ContentParent.h
+++ b/dom/ipc/ContentParent.h
@@ -1275,16 +1275,18 @@ public:
 
   nsresult SaveRecording(nsIFile* aFile, bool* aRetval);
 
   bool IsRecordingOrReplaying() const {
     return mRecordReplayState != eNotRecordingOrReplaying;
   }
 
 private:
+  // Released in ActorDestroy; deliberately not exposed to the CC.
+  RefPtr<ContentParent> mSelfRef;
 
   // If you add strong pointers to cycle collected objects here, be sure to
   // release these objects in ShutDownProcess.  See the comment there for more
   // details.
 
   ContentProcessHost* mSubprocess;
   const TimeStamp mLaunchTS; // used to calculate time to start content process
   TimeStamp mActivateTS;
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -629,17 +629,16 @@ MessageChannel::MessageChannel(const cha
     mSawInterruptOutMsg(false),
     mIsWaitingForIncoming(false),
     mAbortOnError(false),
     mNotifiedChannelDone(false),
     mFlags(REQUIRE_DEFAULT),
     mPeerPidSet(false),
     mPeerPid(-1),
     mIsPostponingSends(false),
-    mInKillHardShutdown(false),
     mBuildIDsConfirmedMatch(false)
 {
     MOZ_COUNT_CTOR(ipc::MessageChannel);
 
 #ifdef OS_WIN
     mTopFrame = nullptr;
     mIsSyncWaitingOnNonMainThread = false;
 #endif
@@ -793,20 +792,17 @@ MessageChannel::Clear()
     // Also don't clear mListener.  If we clear it, then sending a message
     // through this channel after it's Clear()'ed can cause this process to
     // crash.
     //
     // In practice, mListener owns the channel, so the channel gets deleted
     // before mListener.  But just to be safe, mListener is a weak pointer.
 
 #if !defined(ANDROID)
-    // KillHard shutdowns can occur with the channel in connected state. We are
-    // already collecting crash dump data about KillHard shutdowns and we
-    // shouldn't intentionally crash here.
-    if (!Unsound_IsClosed() && !mInKillHardShutdown) {
+    if (!Unsound_IsClosed()) {
         CrashReporter::AnnotateCrashReport(
           CrashReporter::Annotation::IPCFatalErrorProtocol, nsDependentCString(mName));
         switch (mChannelState) {
             case ChannelOpening:
                 MOZ_CRASH("MessageChannel destroyed without being closed " \
                           "(mChannelState == ChannelOpening).");
                 break;
             case ChannelConnected:
--- a/ipc/glue/MessageChannel.h
+++ b/ipc/glue/MessageChannel.h
@@ -326,20 +326,16 @@ private:
 
     static bool IsPumpingMessages() {
         return sIsPumpingMessages;
     }
     static void SetIsPumpingMessages(bool aIsPumping) {
         sIsPumpingMessages = aIsPumping;
     }
 
-    void SetInKillHardShutdown() {
-        mInKillHardShutdown = true;
-    }
-
 #ifdef OS_WIN
     struct MOZ_STACK_CLASS SyncStackFrame
     {
         SyncStackFrame(MessageChannel* channel, bool interrupt);
         ~SyncStackFrame();
 
         bool mInterrupt;
         bool mSpinNestedEvents;
@@ -859,18 +855,16 @@ private:
     bool mPeerPidSet;
     int32_t mPeerPid;
 
     // Channels can enter messages are not sent immediately; instead, they are
     // held in a queue until another thread deems it is safe to send them.
     bool mIsPostponingSends;
     std::vector<UniquePtr<Message>> mPostponedSends;
 
-    bool mInKillHardShutdown;
-
     bool mBuildIDsConfirmedMatch;
 };
 
 void
 CancelCPOWs();
 
 } // namespace ipc
 } // namespace mozilla