Bug 1348361 - Part 1 - Added locking to IToplevelProtocol's management of the peer's pid; r=jld
authorAlex Gaynor <agaynor@mozilla.com>
Thu, 22 Feb 2018 10:36:55 -0500
changeset 408645 dd3dc46a8d7d869e648ae2a1fbca30de1a51375a
parent 408644 958b3aa1565a6b39d83e966b02e6f5f003727f48
child 408646 d29a6c667ebae10002cbb3c9a0913cf0befc5e69
push id100996
push userbtara@mozilla.com
push dateSat, 17 Mar 2018 10:37:43 +0000
treeherdermozilla-inbound@97160a734959 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjld
bugs1348361
milestone61.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 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;