Bug 518659. Make nsMediaCache call out to nsMediaCacheStream::ClientSeek/Suspend/Resume without holding its lock. r=doublec

content/media/nsMediaCache.cpp

@@ -975,11 +975,20 @@ nsMediaCache::PredictNextUseForIncomingData(nsMediaCacheStream* aStream)
       PR_MIN(millisecondsAhead, PR_INT32_MAX));
 }
 
+enum StreamAction { NONE, SEEK, RESUME, SUSPEND };
+
 void
 nsMediaCache::Update()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
+  // The action to use for each stream. We store these so we can make
+  // decisions while holding the cache lock but implement those decisions
+  // without holding the cache lock, since we need to call out to
+  // stream, decoder and element code.
+  nsAutoTArray<StreamAction,10> actions;
+
+  {
     nsAutoMonitor mon(mMonitor);
     mUpdateQueued = PR_FALSE;
 #ifdef DEBUG
@@ -1088,11 +1097,9 @@ nsMediaCache::Update()
       }
     }
 
-  // This array holds a list of streams which need to be closed due
-  // to fatal errors. We can't close streams immediately since it would
-  // confuse iteration over mStreams and generally just be confusing.
-  nsTArray<nsMediaCacheStream*> streamsToClose;
     for (PRUint32 i = 0; i < mStreams.Length(); ++i) {
+      actions.AppendElement(NONE);
+
       nsMediaCacheStream* stream = mStreams[i];
       if (stream->mClosed)
         continue;
@@ -1196,42 +1203,69 @@ nsMediaCache::Update()
         }
       }
 
-    nsresult rv = NS_OK;
       if (stream->mChannelOffset != desiredOffset && enableReading) {
         // We need to seek now.
         NS_ASSERTION(stream->mIsSeekable || desiredOffset == 0,
                      "Trying to seek in a non-seekable stream!");
-      // Round seek offset down to the start of the block
+        // Round seek offset down to the start of the block. This is essential
+        // because we don't want to think we have part of a block already
+        // in mPartialBlockBuffer.
         stream->mChannelOffset = (desiredOffset/BLOCK_SIZE)*BLOCK_SIZE;
+        actions[i] = SEEK;
+      } else if (enableReading && stream->mCacheSuspended) {
+        actions[i] = RESUME;
+      } else if (!enableReading && !stream->mCacheSuspended) {
+        actions[i] = SUSPEND;
+      }
+    }
+#ifdef DEBUG
+    mInUpdate = PR_FALSE;
+#endif
+  }
+
+  // Update the channel state without holding our cache lock. While we're
+  // doing this, decoder threads may be running and seeking, reading or changing
+  // other cache state. That's OK, they'll trigger new Update events and we'll
+  // get back here and revise our decisions. The important thing here is that
+  // performing these actions only depends on mChannelOffset and
+  // mCacheSuspended, which can only be written by the main thread (i.e., this
+  // thread), so we don't have races here.
+  for (PRUint32 i = 0; i < mStreams.Length(); ++i) {
+    nsMediaCacheStream* stream = mStreams[i];
+    nsresult rv = NS_OK;
+    switch (actions[i]) {
+    case SEEK:
       LOG(PR_LOG_DEBUG, ("Stream %p CacheSeek to %lld (resume=%d)", stream,
            (long long)stream->mChannelOffset, stream->mCacheSuspended));
       rv = stream->mClient->CacheClientSeek(stream->mChannelOffset,
                                             stream->mCacheSuspended);
       stream->mCacheSuspended = PR_FALSE;
-    } else if (enableReading && stream->mCacheSuspended) {
+      break;
+
+    case RESUME:
       LOG(PR_LOG_DEBUG, ("Stream %p Resumed", stream));
       rv = stream->mClient->CacheClientResume();
       stream->mCacheSuspended = PR_FALSE;
-    } else if (!enableReading && !stream->mCacheSuspended) {
+      break;
+
+    case SUSPEND:
       LOG(PR_LOG_DEBUG, ("Stream %p Suspended", stream));
       rv = stream->mClient->CacheClientSuspend();
       stream->mCacheSuspended = PR_TRUE;
+      break;
+
+    default:
+      break;
     }
 
     if (NS_FAILED(rv)) {
-      streamsToClose.AppendElement(stream);
-    }
-  }
-
       // Close the streams that failed due to error. This will cause all
       // client Read and Seek operations on those streams to fail. Blocked
       // Reads will also be woken up.
-  for (PRUint32 i = 0; i < streamsToClose.Length(); ++i) {
-    streamsToClose[i]->CloseInternal(&mon);
+      nsAutoMonitor mon(mMonitor);
+      stream->CloseInternal(&mon);
+    }
   }
-#ifdef DEBUG
-  mInUpdate = PR_FALSE;
-#endif
 }
 
 class UpdateEvent : public nsRunnable

content/media/nsMediaCache.h

@@ -220,13 +220,14 @@ public:
   // aClient provides the underlying transport that cache will use to read
   // data for this stream.
   nsMediaCacheStream(nsMediaChannelStream* aClient)
-    : mClient(aClient), mResourceID(0), mChannelOffset(0),
-      mStreamOffset(0), mStreamLength(-1), mPlaybackBytesPerSecond(10000),
-      mPinCount(0), mCurrentMode(MODE_PLAYBACK),
-      mInitialized(PR_FALSE), mClosed(PR_FALSE),
+    : mClient(aClient), mResourceID(0), mInitialized(PR_FALSE),
       mIsSeekable(PR_FALSE), mCacheSuspended(PR_FALSE),
+      mUsingNullPrincipal(PR_FALSE),
+      mChannelOffset(0), mStreamLength(-1),  
+      mStreamOffset(0), mPlaybackBytesPerSecond(10000),
+      mPinCount(0), mCurrentMode(MODE_PLAYBACK),
       mMetadataInPartialBlockBuffer(PR_FALSE),
-      mUsingNullPrincipal(PR_FALSE) {}
+      mClosed(PR_FALSE) {}
   ~nsMediaCacheStream();
 
   // Set up this stream with the cache. Can fail on OOM. One
@@ -432,16 +433,32 @@ private:
   // All streams with the same mResourceID are loading the same
   // underlying resource and should share data.
   PRInt64                mResourceID;
+  // Set to true when Init or InitAsClone has been called
+  PRPackedBool           mInitialized;
 
-  // All other fields are all protected by the cache's monitor and
-  // can be accessed by by any thread.
+  // The following fields are protected by the cache's monitor but are
+  // only written on the main thread. 
+
+  // The last reported seekability state for the underlying channel
+  PRPackedBool mIsSeekable;
+  // true if the cache has suspended our channel because the cache is
+  // full and the priority of the data that would be received is lower
+  // than the priority of the data already in the cache
+  PRPackedBool mCacheSuspended;
+  // true if mPrincipal is a null principal because we saw data from
+  // multiple origins
+  PRPackedBool mUsingNullPrincipal;
   // The offset where the next data from the channel will arrive
   PRInt64      mChannelOffset;
-  // The offset where the reader is positioned in the stream
-  PRInt64           mStreamOffset;
   // The reported or discovered length of the data, or -1 if nothing is
   // known
   PRInt64      mStreamLength;
+
+  // The following fields are protected by the cache's monitor can can be written
+  // by any thread.
+
+  // The offset where the reader is positioned in the stream
+  PRInt64           mStreamOffset;
   // For each block in the stream data, maps to the cache entry for the
   // block, or -1 if the block is not cached.
   nsTArray<PRInt32> mBlocks;
@@ -460,22 +477,14 @@ private:
   PRUint32          mPinCount;
   // The last reported read mode
   ReadMode          mCurrentMode;
-  // Set to true when Init or InitAsClone has been called
-  PRPackedBool      mInitialized;
+  // true if some data in mPartialBlockBuffer has been read as metadata
+  PRPackedBool      mMetadataInPartialBlockBuffer;
   // Set to true when the stream has been closed either explicitly or
   // due to an internal cache error
   PRPackedBool      mClosed;
-  // The last reported seekability state for the underlying channel
-  PRPackedBool      mIsSeekable;
-  // true if the cache has suspended our channel because the cache is
-  // full and the priority of the data that would be received is lower
-  // than the priority of the data already in the cache
-  PRPackedBool      mCacheSuspended;
-  // true if some data in mPartialBlockBuffer has been read as metadata
-  PRPackedBool      mMetadataInPartialBlockBuffer;
-  // true if mPrincipal is a null principal because we saw data from
-  // multiple origins
-  PRPackedBool      mUsingNullPrincipal;
+
+  // The following field is protected by the cache's monitor but are
+  // only written on the main thread.
 
   // Data received for the block containing mChannelOffset. Data needs
   // to wait here so we can write back a complete block. The first

content/media/nsMediaStream.cpp

@@ -540,7 +540,7 @@ PRInt64 nsMediaChannelStream::Tell()
 
 void nsMediaChannelStream::Suspend(PRBool aCloseImmediately)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Don't call on main thread");
+  NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
 
   nsHTMLMediaElement* element = mDecoder->GetMediaElement();
   if (!element) {
@@ -569,7 +569,7 @@ void nsMediaChannelStream::Suspend(PRBool aCloseImmediately)
 
 void nsMediaChannelStream::Resume()
 {
-  NS_ASSERTION(NS_IsMainThread(), "Don't call on main thread");
+  NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
   NS_ASSERTION(mSuspendCount > 0, "Too many resumes!");
 
   nsHTMLMediaElement* element = mDecoder->GetMediaElement();
@@ -641,7 +641,9 @@ nsMediaChannelStream::DoNotifyDataReceived()
 void
 nsMediaChannelStream::CacheClientNotifyDataReceived()
 {
-  NS_ASSERTION(NS_IsMainThread(), "Don't call on main thread");
+  NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
+  // NOTE: this can be called with the media cache lock held, so don't
+  // block or do anything which might try to acquire a lock!
 
   if (mDataReceivedEvent.IsPending())
     return;
@@ -667,7 +669,9 @@ private:
 void
 nsMediaChannelStream::CacheClientNotifyDataEnded(nsresult aStatus)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Don't call on main thread");
+  NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
+  // NOTE: this can be called with the media cache lock held, so don't
+  // block or do anything which might try to acquire a lock!
 
   nsCOMPtr<nsIRunnable> event = new DataEnded(mDecoder, aStatus);
   NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
@@ -676,7 +680,7 @@ nsMediaChannelStream::CacheClientNotifyDataEnded(nsresult aStatus)
 nsresult
 nsMediaChannelStream::CacheClientSeek(PRInt64 aOffset, PRBool aResume)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Don't call on main thread");
+  NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
 
   CloseChannel();
 
@@ -703,12 +707,7 @@ nsMediaChannelStream::CacheClientSuspend()
   }
   Suspend(PR_FALSE);
 
-  // We have to spawn an event here since we're being called back from
-  // a sensitive place in nsMediaCache, which doesn't want us to reenter
-  // the decoder and cause deadlocks or other unpleasantness
-  nsCOMPtr<nsIRunnable> event =
-    NS_NEW_RUNNABLE_METHOD(nsMediaDecoder, mDecoder, NotifySuspendedStatusChanged);
-  NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+  mDecoder->NotifySuspendedStatusChanged();
   return NS_OK;
 }
 
@@ -721,12 +720,7 @@ nsMediaChannelStream::CacheClientResume()
     --mCacheSuspendCount;
   }
 
-  // We have to spawn an event here since we're being called back from
-  // a sensitive place in nsMediaCache, which doesn't want us to reenter
-  // the decoder and cause deadlocks or other unpleasantness
-  nsCOMPtr<nsIRunnable> event =
-    NS_NEW_RUNNABLE_METHOD(nsMediaDecoder, mDecoder, NotifySuspendedStatusChanged);
-  NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+  mDecoder->NotifySuspendedStatusChanged();
   return NS_OK;
 }
 

content/media/nsMediaStream.h

@@ -308,7 +308,7 @@ public:
   ~nsMediaChannelStream();
 
   // These are called on the main thread by nsMediaCache. These must
-  // not block or grab locks.
+  // not block or grab locks, because the media cache is holding its lock.
   // Notify that data is available from the cache. This can happen even
   // if this stream didn't read any data, since another stream might have
   // received data for the same resource.
@@ -317,6 +317,10 @@ public:
   // if this stream didn't read any data, since another stream might have
   // received data for the same resource.
   void CacheClientNotifyDataEnded(nsresult aStatus);
+
+  // These are called on the main thread by nsMediaCache. These shouldn't block,
+  // but they may grab locks --- the media cache is not holding its lock
+  // when these are called.
   // Start a new load at the given aOffset. The old load is cancelled
   // and no more data from the old load will be notified via
   // nsMediaCacheStream::NotifyDataReceived/Ended.