Bug 1415090. P3 - run MediaCacheStream::NotifyDataEnded() off the main thread. draft
authorJW Wang <jwwang@mozilla.com>
Fri, 10 Nov 2017 15:06:39 +0800
changeset 699434 502f829114d1cece67896e6c6bcbb26884bceb54
parent 699433 2d87df97cb747cf3d10972ee4b8781b2516089d9
child 699435 68ed9786c9e50e59c426691830f8a97b46bbb001
push id89568
push userjwwang@mozilla.com
push dateFri, 17 Nov 2017 06:22:23 +0000
bugs1415090
milestone59.0a1
Bug 1415090. P3 - run MediaCacheStream::NotifyDataEnded() off the main thread. Since NotifyDataEnded() run its code asynchronously, it is possible that a new channel is created and NotifyDataStarted() is called before NotifyDataEndedInternal() has a chance to run. We check the load ID to exit the function if necessary. We also need to fix data races when running NotifyDataEndedInternal() off the main thread in next patches. MozReview-Commit-ID: IIAc7dxHike
dom/media/ChannelMediaResource.cpp
dom/media/MediaCache.cpp
dom/media/MediaCache.h
--- a/dom/media/ChannelMediaResource.cpp
+++ b/dom/media/ChannelMediaResource.cpp
@@ -207,18 +207,20 @@ ChannelMediaResource::OnStartRequest(nsI
       // handled in OnStopRequest.)
       // A 416 error should treated as EOF here... it's possible
       // that we don't get Content-Length, we read N bytes, then we
       // suspend and resume, the resume reopens the channel and we seek to
       // offset N, but there are no more bytes, so we get a 416
       // "Requested Range Not Satisfiable".
       if (responseStatus == HTTP_REQUESTED_RANGE_NOT_SATISFIABLE_CODE) {
         // OnStopRequest will not be fired, so we need to do some of its
-        // work here.
-        mCacheStream.NotifyDataEnded(status);
+        // work here. Note we need to pass the load ID first so the following
+        // NotifyDataEnded() can pass the ID check.
+        mCacheStream.NotifyLoadID(mLoadID);
+        mCacheStream.NotifyDataEnded(mLoadID, status);
       } else {
         mCallback->NotifyNetworkError();
       }
 
       // This disconnects our listener so we don't get any more data. We
       // certainly don't want an error page to end up in our cache!
       CloseChannel();
       return NS_OK;
@@ -385,17 +387,17 @@ ChannelMediaResource::OnStopRequest(nsIR
   nsLoadFlags loadFlags;
   DebugOnly<nsresult> rv = mChannel->GetLoadFlags(&loadFlags);
   NS_ASSERTION(NS_SUCCEEDED(rv), "GetLoadFlags() failed!");
 
   if (loadFlags & nsIRequest::LOAD_BACKGROUND) {
     ModifyLoadFlags(loadFlags & ~nsIRequest::LOAD_BACKGROUND);
   }
 
-  mCacheStream.NotifyDataEnded(aStatus, aReopenOnError);
+  mCacheStream.NotifyDataEnded(mLoadID, aStatus, aReopenOnError);
   return NS_OK;
 }
 
 nsresult
 ChannelMediaResource::OnChannelRedirect(nsIChannel* aOld,
                                         nsIChannel* aNew,
                                         uint32_t aFlags,
                                         int64_t aOffset)
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -1939,16 +1939,24 @@ MediaCacheStream::NotifyDataLength(int64
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
   LOG("Stream %p DataLength: %" PRId64, this, aLength);
 
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   mStreamLength = aLength;
 }
 
 void
+MediaCacheStream::NotifyLoadID(uint32_t aLoadID)
+{
+  MOZ_ASSERT(aLoadID > 0);
+  ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
+  mLoadID = aLoadID;
+}
+
+void
 MediaCacheStream::NotifyDataStarted(uint32_t aLoadID,
                                     int64_t aOffset,
                                     bool aSeekable)
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
   MOZ_ASSERT(aLoadID > 0);
   LOG("Stream %p DataStarted: %" PRId64 " aLoadID=%u", this, aOffset, aLoadID);
 
@@ -2067,18 +2075,16 @@ MediaCacheStream::NotifyDataReceived(uin
   // avoid waking up reader threads unnecessarily
   mon.NotifyAll();
 }
 
 void
 MediaCacheStream::FlushPartialBlockInternal(bool aNotifyAll,
                                             ReentrantMonitorAutoEnter& aReentrantMonitor)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
-
   int32_t blockIndex = OffsetToBlockIndexUnchecked(mChannelOffset);
   int32_t blockOffset = OffsetInBlock(mChannelOffset);
   if (blockOffset > 0) {
     LOG("Stream %p writing partial block: [%d] bytes; "
         "mStreamOffset [%" PRId64 "] mChannelOffset[%"
         PRId64 "] mStreamLength [%" PRId64 "] notifying: [%s]",
         this, blockOffset, mStreamOffset, mChannelOffset, mStreamLength,
         aNotifyAll ? "yes" : "no");
@@ -2114,21 +2120,26 @@ MediaCacheStream::FlushPartialBlock()
   // stream, the decoder must subsequently choose correct start and end offsets
   // for reading/seeking.
   FlushPartialBlockInternal(false, mon);
 
   mMediaCache->QueueUpdate();
 }
 
 void
-MediaCacheStream::NotifyDataEnded(nsresult aStatus, bool aReopenOnError)
+MediaCacheStream::NotifyDataEndedInternal(uint32_t aLoadID,
+                                          nsresult aStatus,
+                                          bool aReopenOnError)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
+  ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
 
-  ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
+  if (mClosed || aLoadID != mLoadID) {
+    // Nothing to do if the stream is closed or a new load has begun.
+    return;
+  }
 
   // Note that aStatus might have succeeded --- this might be a normal close
   // --- even in situations where the server cut us off because we were
   // suspended. So we need to "reopen on error" in that case too. The only
   // cases where we don't need to reopen are when *we* closed the stream.
   // But don't reopen if we need to seek and we don't think we can... that would
   // cause us to just re-read the stream, which would be really bad.
   if (aReopenOnError && aStatus != NS_ERROR_PARSED_DATA_CACHED &&
@@ -2181,16 +2192,33 @@ MediaCacheStream::NotifyChannelRecreated
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   mChannelEnded = false;
   mDidNotifyDataEnded = false;
 }
 
 void
+MediaCacheStream::NotifyDataEnded(uint32_t aLoadID,
+                                  nsresult aStatus,
+                                  bool aReopenOnError)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  MOZ_ASSERT(aLoadID > 0);
+
+  RefPtr<ChannelMediaResource> client = mClient;
+  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
+    "MediaCacheStream::NotifyDataEnded",
+    [client, this, aLoadID, aStatus, aReopenOnError]() {
+      NotifyDataEndedInternal(aLoadID, aStatus, aReopenOnError);
+    });
+  OwnerThread()->Dispatch(r.forget());
+}
+
+void
 MediaCacheStream::NotifyClientSuspended(bool aSuspended)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   RefPtr<ChannelMediaResource> client = mClient;
   nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
     "MediaCacheStream::NotifyClientSuspended", [client, this, aSuspended]() {
       if (mClientSuspended != aSuspended) {
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -262,18 +262,26 @@ public:
   // the cache requested the offset in
   // ChannelMediaResource::CacheClientSeek, or because it defaulted to 0.
   void NotifyDataReceived(uint32_t aLoadID,
                           uint32_t aCount,
                           const uint8_t* aData);
   // Notifies the cache that the current bytes should be written to disk.
   // Called on the main thread.
   void FlushPartialBlock();
+
+  // Set the load ID so the following NotifyDataEnded() call can work properly.
+  // Used in some rare cases where NotifyDataEnded() is called without the
+  // preceding NotifyDataStarted().
+  void NotifyLoadID(uint32_t aLoadID);
+
   // Notifies the cache that the channel has closed with the given status.
-  void NotifyDataEnded(nsresult aStatus, bool aReopenOnError = false);
+  void NotifyDataEnded(uint32_t aLoadID,
+                       nsresult aStatus,
+                       bool aReopenOnError = false);
 
   // Notifies the stream that the channel is reopened. The stream should
   // reset variables such as |mDidNotifyDataEnded|.
   void NotifyChannelRecreated();
 
   // Notifies the stream that the suspend status of the client has changed.
   // Main thread only.
   void NotifyClientSuspended(bool aSuspended);
@@ -440,16 +448,20 @@ private:
   // any thread.
   int64_t GetNextCachedDataInternal(int64_t aOffset);
   // Writes |mPartialBlock| to disk.
   // Used by |NotifyDataEnded| and |FlushPartialBlock|.
   // If |aNotifyAll| is true, this function will wake up readers who may be
   // waiting on the media cache monitor. Called on the main thread only.
   void FlushPartialBlockInternal(bool aNotify, ReentrantMonitorAutoEnter& aReentrantMonitor);
 
+  void NotifyDataEndedInternal(uint32_t aLoadID,
+                               nsresult aStatus,
+                               bool aReopenOnError);
+
   // Instance of MediaCache to use with this MediaCacheStream.
   RefPtr<MediaCache> mMediaCache;
 
   ChannelMediaResource* const mClient;
 
   // These fields are main-thread-only.
   nsCOMPtr<nsIPrincipal> mPrincipal;