Bug 1348361 - Part 1 - Added locking to IToplevelProtocol's management of the peer's pid; r?jld draft
authorAlex Gaynor <agaynor@mozilla.com>
Thu, 22 Feb 2018 10:36:55 -0500
changeset 768518 0617f3acb2b8a56050f898083f8e3f56149baa03
parent 768517 db044a29f878ba0e44a7bd137e87ce84ed4cd291
child 768519 baa56a97e189296f761e7b251550e4aa2f873289
push id102893
push userbmo:agaynor@mozilla.com
push dateFri, 16 Mar 2018 13:06:46 +0000
reviewersjld
bugs1348361
milestone61.0a1
Bug 1348361 - Part 1 - Added locking to IToplevelProtocol's management of the peer's pid; r?jld This will let us manipulate it from multiple threads in a future patch. MozReview-Commit-ID: 2AOgho8SEX9
dom/ipc/ContentParent.cpp
ipc/glue/ProtocolUtils.cpp
ipc/glue/ProtocolUtils.h
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -2051,16 +2051,17 @@ ContentParent::LaunchSubprocess(ProcessP
   nsCString schedulerPrefs = Scheduler::GetPrefs();
   extraArgs.push_back("-schedulerPrefs");
   extraArgs.push_back(schedulerPrefs.get());
 
   if (gSafeMode) {
     extraArgs.push_back("-safeMode");
   }
 
+  SetOtherProcessId(kInvalidProcessId, ProcessIdState::ePending);
   if (!mSubprocess->LaunchAndWaitForProcessHandle(extraArgs)) {
     NS_ERROR("failed to launch child in the parent");
     MarkAsDead();
     return false;
   }
 
   base::ProcessId procId = base::GetProcId(mSubprocess->GetChildProcessHandle());
 
--- a/ipc/glue/ProtocolUtils.cpp
+++ b/ipc/glue/ProtocolUtils.cpp
@@ -573,18 +573,20 @@ IProtocol::GetActorEventTarget()
 already_AddRefed<nsIEventTarget>
 IProtocol::GetActorEventTargetInternal(IProtocol* aActor)
 {
   return Manager()->GetActorEventTargetInternal(aActor);
 }
 
 IToplevelProtocol::IToplevelProtocol(ProtocolId aProtoId, Side aSide)
  : IProtocol(aSide),
+   mMonitor("mozilla.ipc.IToplevelProtocol.mMonitor"),
    mProtocolId(aProtoId),
    mOtherPid(mozilla::ipc::kInvalidProcessId),
+   mOtherPidState(ProcessIdState::eUnstarted),
    mLastRouteId(aSide == ParentSide ? kFreedActorId : kNullActorId),
    mLastShmemId(aSide == ParentSide ? kFreedActorId : kNullActorId),
    mEventTargetMutex("ProtocolEventTargetMutex")
 {
 }
 
 IToplevelProtocol::~IToplevelProtocol()
 {
@@ -600,23 +602,40 @@ IToplevelProtocol::OtherPid() const
   base::ProcessId pid = OtherPidMaybeInvalid();
   MOZ_RELEASE_ASSERT(pid != kInvalidProcessId);
   return pid;
 }
 
 base::ProcessId
 IToplevelProtocol::OtherPidMaybeInvalid() const
 {
+  MonitorAutoLock lock(mMonitor);
+
+  if (mOtherPidState == ProcessIdState::eUnstarted) {
+    // If you're asking for the pid of a process we haven't even tried to
+    // start, you get an invalid pid back immediately.
+    return kInvalidProcessId;
+  }
+
+  while (mOtherPidState < ProcessIdState::eReady) {
+    lock.Wait();
+  }
+  MOZ_RELEASE_ASSERT(mOtherPidState == ProcessIdState::eReady);
+
   return mOtherPid;
 }
 
 void
-IToplevelProtocol::SetOtherProcessId(base::ProcessId aOtherPid)
+IToplevelProtocol::SetOtherProcessId(base::ProcessId aOtherPid,
+                                     ProcessIdState aState)
 {
+  MonitorAutoLock lock(mMonitor);
   mOtherPid = aOtherPid;
+  mOtherPidState = aState;
+  lock.NotifyAll();
 }
 
 bool
 IToplevelProtocol::TakeMinidump(nsIFile** aDump, uint32_t* aSequence)
 {
   MOZ_RELEASE_ASSERT(GetSide() == ParentSide);
   return XRE_TakeMinidumpForChild(OtherPid(), aDump, aSequence);
 }
--- a/ipc/glue/ProtocolUtils.h
+++ b/ipc/glue/ProtocolUtils.h
@@ -265,29 +265,37 @@ class IToplevelProtocol : public IProtoc
 {
     template<class PFooSide> friend class Endpoint;
 
 protected:
     explicit IToplevelProtocol(ProtocolId aProtoId, Side aSide);
     ~IToplevelProtocol();
 
 public:
+    enum ProcessIdState {
+        eUnstarted,
+        ePending,
+        eReady,
+        eError
+    };
+
     using SchedulerGroupSet = nsILabelableRunnable::SchedulerGroupSet;
 
     void SetTransport(UniquePtr<Transport> aTrans)
     {
         mTrans = Move(aTrans);
     }
 
     Transport* GetTransport() const { return mTrans.get(); }
 
     ProtocolId GetProtocolId() const { return mProtocolId; }
 
-    base::ProcessId OtherPid() const override;
-    void SetOtherProcessId(base::ProcessId aOtherPid);
+    base::ProcessId OtherPid() const final;
+    void SetOtherProcessId(base::ProcessId aOtherPid,
+                           ProcessIdState aState = ProcessIdState::eReady);
 
     bool TakeMinidump(nsIFile** aDump, uint32_t* aSequence);
 
     virtual void OnChannelClose() = 0;
     virtual void OnChannelError() = 0;
     virtual void ProcessingError(Result aError, const char* aMsgName) {}
     virtual void OnChannelConnected(int32_t peer_pid) {}
 
@@ -426,22 +434,26 @@ protected:
                                                 nsIEventTarget* aEventTarget) override;
     virtual void ReplaceEventTargetForActorInternal(
       IProtocol* aActor,
       nsIEventTarget* aEventTarget) override;
 
     virtual already_AddRefed<nsIEventTarget>
     GetActorEventTargetInternal(IProtocol* aActor) override;
 
+    // This monitor protects mOtherPid and mOtherPidState. All other fields
+    // should only be accessed on the worker thread.
+    mutable mozilla::Monitor mMonitor;
   private:
     base::ProcessId OtherPidMaybeInvalid() const;
 
     ProtocolId mProtocolId;
     UniquePtr<Transport> mTrans;
     base::ProcessId mOtherPid;
+    ProcessIdState mOtherPidState;
     IDMap<IProtocol*> mActorMap;
     int32_t mLastRouteId;
     IDMap<Shmem::SharedMemory*> mShmemMap;
     Shmem::id_t mLastShmemId;
     bool mIsMainThreadProtocol;
 
     Mutex mEventTargetMutex;
     IDMap<nsCOMPtr<nsIEventTarget>> mEventTargetMap;