Bug 1328642 Protect multi-thread stream access with a mutex. r=asuth a=lizzard
authorBen Kelly <ben@wanderview.com>
Wed, 18 Jan 2017 06:31:08 -0800
changeset 465640 4025860dec4be8fa9490e9351fadd9119c7c1262
parent 465639 65c61eec146d3d46eafe622641d49ef665ae3786
child 465641 1f694d0ba1e8cbfd7d87c27946c95ed326dd85bf
push id42661
push userfelipc@gmail.com
push dateTue, 24 Jan 2017 16:14:20 +0000
reviewersasuth, lizzard
bugs1328642
milestone51.0
Bug 1328642 Protect multi-thread stream access with a mutex. r=asuth a=lizzard
dom/cache/ReadStream.cpp
--- a/dom/cache/ReadStream.cpp
+++ b/dom/cache/ReadStream.cpp
@@ -94,29 +94,36 @@ private:
 
   // Weak ref to the stream control actor.  The actor will always call either
   // CloseStream() or CloseStreamWithoutReporting() before it's destroyed.  The
   // weak ref is cleared in the resulting NoteClosedOnOwningThread() or
   // ForgetOnOwningThread() method call.
   StreamControl* mControl;
 
   const nsID mId;
-  nsCOMPtr<nsIInputStream> mStream;
-  nsCOMPtr<nsIInputStream> mSnappyStream;
   nsCOMPtr<nsIThread> mOwningThread;
 
   enum State
   {
     Open,
     Closed,
     NumStates
   };
   Atomic<State> mState;
   Atomic<bool> mHasEverBeenRead;
 
+
+  // The wrapped stream objects may not be threadsafe.  We need to be able
+  // to close a stream on our owning thread while an IO thread is simultaneously
+  // reading the same stream.  Therefore, protect all access to these stream
+  // objects with a mutex.
+  Mutex mMutex;
+  nsCOMPtr<nsIInputStream> mStream;
+  nsCOMPtr<nsIInputStream> mSnappyStream;
+
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(cache::ReadStream::Inner, override)
 };
 
 // ----------------------------------------------------------------------------
 
 // Runnable to notify actors that the ReadStream has closed.  This must
 // be done on the thread associated with the PBackground actor.  Must be
 // cancelable to execute on Worker threads (which can occur when the
@@ -185,20 +192,22 @@ private:
 };
 
 // ----------------------------------------------------------------------------
 
 ReadStream::Inner::Inner(StreamControl* aControl, const nsID& aId,
                          nsIInputStream* aStream)
   : mControl(aControl)
   , mId(aId)
+  , mOwningThread(NS_GetCurrentThread())
+  , mState(Open)
+  , mHasEverBeenRead(false)
+  , mMutex("dom::cache::ReadStream")
   , mStream(aStream)
   , mSnappyStream(new SnappyUncompressInputStream(aStream))
-  , mOwningThread(NS_GetCurrentThread())
-  , mState(Open)
 {
   MOZ_ASSERT(mStream);
   MOZ_ASSERT(mControl);
   mControl->AddReadStream(this);
 }
 
 void
 ReadStream::Inner::Serialize(CacheReadStreamOrVoid* aReadStreamOut,
@@ -223,17 +232,21 @@ ReadStream::Inner::Serialize(CacheReadSt
     aRv.ThrowTypeError<MSG_CACHE_STREAM_CLOSED>();
     return;
   }
 
   MOZ_ASSERT(mControl);
 
   aReadStreamOut->id() = mId;
   mControl->SerializeControl(aReadStreamOut);
-  mControl->SerializeStream(aReadStreamOut, mStream, aStreamCleanupList);
+
+  {
+    MutexAutoLock lock(mMutex);
+    mControl->SerializeStream(aReadStreamOut, mStream, aStreamCleanupList);
+  }
 
   MOZ_ASSERT(aReadStreamOut->stream().type() ==
              IPCStream::TInputStreamParamsWithFds);
 
   // We're passing ownership across the IPC barrier with the control, so
   // do not signal that the stream is closed here.
   Forget();
 }
@@ -265,41 +278,53 @@ ReadStream::Inner::HasEverBeenRead() con
   MOZ_ASSERT(NS_GetCurrentThread() == mOwningThread);
   return mHasEverBeenRead;
 }
 
 nsresult
 ReadStream::Inner::Close()
 {
   // stream ops can happen on any thread
-  nsresult rv = mStream->Close();
+  nsresult rv = NS_OK;
+  {
+    MutexAutoLock lock(mMutex);
+    rv = mSnappyStream->Close();
+  }
   NoteClosed();
   return rv;
 }
 
 nsresult
 ReadStream::Inner::Available(uint64_t* aNumAvailableOut)
 {
   // stream ops can happen on any thread
-  nsresult rv = mSnappyStream->Available(aNumAvailableOut);
+  nsresult rv = NS_OK;
+  {
+    MutexAutoLock lock(mMutex);
+    rv = mSnappyStream->Available(aNumAvailableOut);
+  }
 
   if (NS_FAILED(rv)) {
     Close();
   }
 
   return rv;
 }
 
 nsresult
 ReadStream::Inner::Read(char* aBuf, uint32_t aCount, uint32_t* aNumReadOut)
 {
   // stream ops can happen on any thread
   MOZ_ASSERT(aNumReadOut);
 
-  nsresult rv = mSnappyStream->Read(aBuf, aCount, aNumReadOut);
+  nsresult rv = NS_OK;
+  {
+    MutexAutoLock lock(mMutex);
+    rv = mSnappyStream->Read(aBuf, aCount, aNumReadOut);
+  }
 
   if ((NS_FAILED(rv) && rv != NS_BASE_STREAM_WOULD_BLOCK) ||
       *aNumReadOut == 0) {
     Close();
   }
 
   mHasEverBeenRead = true;
 
@@ -312,18 +337,21 @@ ReadStream::Inner::ReadSegments(nsWriteS
 {
   // stream ops can happen on any thread
   MOZ_ASSERT(aNumReadOut);
 
   if (aCount) {
     mHasEverBeenRead = true;
   }
 
-  nsresult rv = mSnappyStream->ReadSegments(aWriter, aClosure, aCount,
-                                            aNumReadOut);
+  nsresult rv = NS_OK;
+  {
+    MutexAutoLock lock(mMutex);
+    rv = mSnappyStream->ReadSegments(aWriter, aClosure, aCount, aNumReadOut);
+  }
 
   if ((NS_FAILED(rv) && rv != NS_BASE_STREAM_WOULD_BLOCK &&
                         rv != NS_ERROR_NOT_IMPLEMENTED) || *aNumReadOut == 0) {
     Close();
   }
 
   // Verify bytes were actually read before marking as being ever read.  For
   // example, code can test if the stream supports ReadSegments() by calling
@@ -335,16 +363,17 @@ ReadStream::Inner::ReadSegments(nsWriteS
 
   return rv;
 }
 
 nsresult
 ReadStream::Inner::IsNonBlocking(bool* aNonBlockingOut)
 {
   // stream ops can happen on any thread
+  MutexAutoLock lock(mMutex);
   return mSnappyStream->IsNonBlocking(aNonBlockingOut);
 }
 
 ReadStream::Inner::~Inner()
 {
   // Any thread
   MOZ_ASSERT(mState == Closed);
   MOZ_ASSERT(!mControl);