Bug 1136762 - TSan: data race xpcom/io/nsPipe3.cpp:1061 CloseWithStatus. r=nfroyd.
authorJulian Seward <jseward@acm.org>
Wed, 17 Feb 2016 11:22:28 +0100
changeset 284487 5271840b178c83f3021833252c6595d600fcc6c8
parent 284486 8ac1b6ca916a6589e8ea7a284a6068bf70d03a95
child 284488 9e71a38057d1e37194c6a367d7df337e7088acca
push id71989
push userjseward@mozilla.com
push dateWed, 17 Feb 2016 10:29:11 +0000
treeherdermozilla-inbound@5271840b178c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnfroyd
bugs1136762
milestone47.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 1136762 - TSan: data race xpcom/io/nsPipe3.cpp:1061 CloseWithStatus. r=nfroyd.
xpcom/io/nsPipe3.cpp
--- a/xpcom/io/nsPipe3.cpp
+++ b/xpcom/io/nsPipe3.cpp
@@ -179,39 +179,48 @@ public:
     mBlocking = !aNonBlocking;
   }
 
   uint32_t Available();
 
   // synchronously wait for the pipe to become readable.
   nsresult Wait();
 
-  MonitorAction OnInputReadable(uint32_t aBytesWritten, nsPipeEvents&);
-  MonitorAction OnInputException(nsresult, nsPipeEvents&);
+  // These two don't acquire the monitor themselves.  Instead they
+  // expect their caller to have done so and to pass the monitor as
+  // evidence.
+  MonitorAction OnInputReadable(uint32_t aBytesWritten, nsPipeEvents&,
+                                const ReentrantMonitorAutoEnter& ev);
+  MonitorAction OnInputException(nsresult, nsPipeEvents&,
+                                 const ReentrantMonitorAutoEnter& ev);
 
   nsPipeReadState& ReadState()
   {
     return mReadState;
   }
 
   const nsPipeReadState& ReadState() const
   {
     return mReadState;
   }
 
   nsresult Status() const;
 
+  // A version of Status() that doesn't acquire the monitor.
+  nsresult Status(const ReentrantMonitorAutoEnter& ev) const;
+
 private:
   virtual ~nsPipeInputStream();
 
   RefPtr<nsPipe>               mPipe;
 
   int64_t                        mLogicalOffset;
   // Individual input streams can be closed without effecting the rest of the
-  // pipe.  So track individual input stream status separately.
+  // pipe.  So track individual input stream status separately.  |mInputStatus|
+  // is protected by |mPipe->mReentrantMonitor|.
   nsresult                       mInputStatus;
   bool                           mBlocking;
 
   // these variables can only be accessed while inside the pipe's monitor
   bool                           mBlocked;
   nsCOMPtr<nsIInputStreamCallback> mCallback;
   uint32_t                       mCallbackFlags;
 
@@ -352,16 +361,17 @@ private:
 
   ReentrantMonitor    mReentrantMonitor;
   nsSegmentedBuffer   mBuffer;
 
   int32_t             mWriteSegment;
   char*               mWriteCursor;
   char*               mWriteLimit;
 
+  // |mStatus| is protected by |mReentrantMonitor|.
   nsresult            mStatus;
   bool                mInited;
 };
 
 //-----------------------------------------------------------------------------
 
 // RAII class representing an active read segment.  When it goes out of scope
 // it automatically updates the read cursor and releases the read segment.
@@ -517,17 +527,20 @@ nsPipe::Release()
 {
   MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");
   nsrefcnt count = --mRefCnt;
   NS_LOG_RELEASE(this, count, "nsPipe");
   if (count == 0) {
     delete (this);
     return 0;
   }
-  if (mOriginalInput && count == 1) {
+  // Avoid racing on |mOriginalInput| by only looking at it when
+  // the refcount is 1, that is, we are the only pointer (hence only
+  // thread) to access it.
+  if (count == 1 && mOriginalInput) {
     mOriginalInput = nullptr;
     return 1;
   }
   return count;
 }
 
 NS_IMETHODIMP
 nsPipe::Init(bool aNonBlockingIn,
@@ -861,17 +874,18 @@ nsPipe::AdvanceWriteCursor(uint32_t aByt
       if (mBuffer.GetSize() >= mBuffer.GetMaxSize()) {
         mOutput.SetWritable(false);
       }
     }
 
     // notify input stream that pipe now contains additional data
     bool needNotify = false;
     for (uint32_t i = 0; i < mInputList.Length(); ++i) {
-      if (mInputList[i]->OnInputReadable(aBytesWritten, events) == NotifyMonitor) {
+      if (mInputList[i]->OnInputReadable(aBytesWritten, events, mon)
+          == NotifyMonitor) {
         needNotify = true;
       }
     }
 
     if (needNotify) {
       mon.NotifyAll();
     }
   }
@@ -902,17 +916,18 @@ nsPipe::OnInputStreamException(nsPipeInp
     }
 
     // Otherwise just close the particular stream that hit an exception.
     for (uint32_t i = 0; i < mInputList.Length(); ++i) {
       if (mInputList[i] != aStream) {
         continue;
       }
 
-      MonitorAction action = mInputList[i]->OnInputException(aReason, events);
+      MonitorAction action = mInputList[i]->OnInputException(aReason, events,
+                                                             mon);
       mInputList.RemoveElementAt(i);
 
       // Notify after element is removed in case we re-enter as a result.
       if (action == NotifyMonitor) {
         mon.NotifyAll();
       }
 
       return;
@@ -943,17 +958,18 @@ nsPipe::OnPipeException(nsresult aReason
     for (uint32_t i = 0; i < mInputList.Length(); ++i) {
       // an output-only exception applies to the input end if the pipe has
       // zero bytes available.
       if (aOutputOnly && mInputList[i]->Available()) {
         tmpInputList.AppendElement(mInputList[i]);
         continue;
       }
 
-      if (mInputList[i]->OnInputException(aReason, events) == NotifyMonitor) {
+      if (mInputList[i]->OnInputException(aReason, events, mon)
+          == NotifyMonitor) {
         needNotify = true;
       }
     }
     mInputList = tmpInputList;
 
     if (mOutput.OnOutputException(aReason, events) == NotifyMonitor) {
       needNotify = true;
     }
@@ -1131,32 +1147,34 @@ nsPipeInputStream::Available()
 
 nsresult
 nsPipeInputStream::Wait()
 {
   NS_ASSERTION(mBlocking, "wait on non-blocking pipe input stream");
 
   ReentrantMonitorAutoEnter mon(mPipe->mReentrantMonitor);
 
-  while (NS_SUCCEEDED(Status()) && (mReadState.mAvailable == 0)) {
+  while (NS_SUCCEEDED(Status(mon)) && (mReadState.mAvailable == 0)) {
     LOG(("III pipe input: waiting for data\n"));
 
     mBlocked = true;
     mon.Wait();
     mBlocked = false;
 
     LOG(("III pipe input: woke up [status=%x available=%u]\n",
-         Status(), mReadState.mAvailable));
+         Status(mon), mReadState.mAvailable));
   }
 
-  return Status() == NS_BASE_STREAM_CLOSED ? NS_OK : Status();
+  return Status(mon) == NS_BASE_STREAM_CLOSED ? NS_OK : Status(mon);
 }
 
 MonitorAction
-nsPipeInputStream::OnInputReadable(uint32_t aBytesWritten, nsPipeEvents& aEvents)
+nsPipeInputStream::OnInputReadable(uint32_t aBytesWritten,
+                                   nsPipeEvents& aEvents,
+                                   const ReentrantMonitorAutoEnter& ev)
 {
   MonitorAction result = DoNotNotifyMonitor;
 
   mPipe->mReentrantMonitor.AssertCurrentThreadIn();
   mReadState.mAvailable += aBytesWritten;
 
   if (mCallback && !(mCallbackFlags & WAIT_CLOSURE_ONLY)) {
     aEvents.NotifyInputReady(this, mCallback);
@@ -1165,17 +1183,18 @@ nsPipeInputStream::OnInputReadable(uint3
   } else if (mBlocked) {
     result = NotifyMonitor;
   }
 
   return result;
 }
 
 MonitorAction
-nsPipeInputStream::OnInputException(nsresult aReason, nsPipeEvents& aEvents)
+nsPipeInputStream::OnInputException(nsresult aReason, nsPipeEvents& aEvents,
+                                    const ReentrantMonitorAutoEnter& ev)
 {
   LOG(("nsPipeInputStream::OnInputException [this=%x reason=%x]\n",
        this, aReason));
 
   MonitorAction result = DoNotNotifyMonitor;
 
   NS_ASSERTION(NS_FAILED(aReason), "huh? successful exception");
 
@@ -1197,16 +1216,18 @@ nsPipeInputStream::OnInputException(nsre
   return result;
 }
 
 NS_IMETHODIMP
 nsPipeInputStream::CloseWithStatus(nsresult aReason)
 {
   LOG(("III CloseWithStatus [this=%x reason=%x]\n", this, aReason));
 
+  ReentrantMonitorAutoEnter mon(mPipe->mReentrantMonitor);
+
   if (NS_FAILED(mInputStatus)) {
     return NS_OK;
   }
 
   if (NS_SUCCEEDED(aReason)) {
     aReason = NS_BASE_STREAM_CLOSED;
   }
 
@@ -1222,18 +1243,18 @@ nsPipeInputStream::Close()
 
 NS_IMETHODIMP
 nsPipeInputStream::Available(uint64_t* aResult)
 {
   // nsPipeInputStream supports under 4GB stream only
   ReentrantMonitorAutoEnter mon(mPipe->mReentrantMonitor);
 
   // return error if closed
-  if (!mReadState.mAvailable && NS_FAILED(Status())) {
-    return Status();
+  if (!mReadState.mAvailable && NS_FAILED(Status(mon))) {
+    return Status(mon);
   }
 
   *aResult = (uint64_t)mReadState.mAvailable;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPipeInputStream::ReadSegments(nsWriteSegmentFun aWriter,
@@ -1335,17 +1356,17 @@ nsPipeInputStream::AsyncWait(nsIInputStr
     }
 
     nsCOMPtr<nsIInputStreamCallback> proxy;
     if (aTarget) {
       proxy = NS_NewInputStreamReadyEvent(aCallback, aTarget);
       aCallback = proxy;
     }
 
-    if (NS_FAILED(Status()) ||
+    if (NS_FAILED(Status(mon)) ||
        (mReadState.mAvailable && !(aFlags & WAIT_CLOSURE_ONLY))) {
       // stream is already closed or readable; post event.
       pipeEvents.NotifyInputReady(this, aCallback);
     } else {
       // queue up callback object to be notified when data becomes available
       mCallback = aCallback;
       mCallbackFlags = aFlags;
     }
@@ -1361,18 +1382,18 @@ nsPipeInputStream::Seek(int32_t aWhence,
 }
 
 NS_IMETHODIMP
 nsPipeInputStream::Tell(int64_t* aOffset)
 {
   ReentrantMonitorAutoEnter mon(mPipe->mReentrantMonitor);
 
   // return error if closed
-  if (!mReadState.mAvailable && NS_FAILED(Status())) {
-    return Status();
+  if (!mReadState.mAvailable && NS_FAILED(Status(mon))) {
+    return Status(mon);
   }
 
   *aOffset = mLogicalOffset;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPipeInputStream::SetEOF()
@@ -1475,19 +1496,26 @@ nsPipeInputStream::GetCloneable(bool* aC
 
 NS_IMETHODIMP
 nsPipeInputStream::Clone(nsIInputStream** aCloneOut)
 {
   return mPipe->CloneInputStream(this, aCloneOut);
 }
 
 nsresult
+nsPipeInputStream::Status(const ReentrantMonitorAutoEnter& ev) const
+{
+  return NS_FAILED(mInputStatus) ? mInputStatus : mPipe->mStatus;
+}
+
+nsresult
 nsPipeInputStream::Status() const
 {
-  return NS_FAILED(mInputStatus) ? mInputStatus : mPipe->mStatus;
+  ReentrantMonitorAutoEnter mon(mPipe->mReentrantMonitor);
+  return Status(mon);
 }
 
 nsPipeInputStream::~nsPipeInputStream()
 {
   Close();
 }
 
 //-----------------------------------------------------------------------------