Bug 1498765 - Clean up ContentParent::KillHard handling. r=mccr8
authorJed Davis <jld@mozilla.com>
Fri, 19 Oct 2018 15:29:34 -0600
changeset 490452 1ca458791c2739f9c6bc7abe2b0672ed3518e9a3
parent 490451 a254c5207ba052f56527ea28d3dad50e0182cd33
child 490453 3f193025450a04021ca5709deb7f1e94c7add510
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersmccr8
bugs1498765
milestone64.0a1
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