Bug 1415090. P2 - move the "reopen on error" code from ChannelMediaResource::OnStopRequest() to MediaCacheStream::NotifyDataEnded(). draft
authorJW Wang <jwwang@mozilla.com>
Fri, 10 Nov 2017 14:40:22 +0800
changeset 699433 2d87df97cb747cf3d10972ee4b8781b2516089d9
parent 699432 d1c0e0ad4478154e1ac4f8266b83e477a59403a7
child 699434 502f829114d1cece67896e6c6bcbb26884bceb54
push id89568
push userjwwang@mozilla.com
push dateFri, 17 Nov 2017 06:22:23 +0000
bugs1415090
milestone59.0a1
Bug 1415090. P2 - move the "reopen on error" code from ChannelMediaResource::OnStopRequest() to MediaCacheStream::NotifyDataEnded(). MozReview-Commit-ID: BA1tSk6ZqPS
dom/media/ChannelMediaResource.cpp
dom/media/MediaCache.cpp
dom/media/MediaCache.h
--- a/dom/media/ChannelMediaResource.cpp
+++ b/dom/media/ChannelMediaResource.cpp
@@ -385,41 +385,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);
   }
 
-  // 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 &&
-      aStatus != NS_BINDING_ABORTED &&
-      (GetOffset() == 0 || (GetLength() > 0 && GetOffset() != GetLength() &&
-                            mIsTransportSeekable))) {
-    // If the stream did close normally, restart the channel if we're either
-    // at the start of the resource, or if the server is seekable and we're
-    // not at the end of stream. We don't restart the stream if we're at the
-    // end because not all web servers handle this case consistently; see:
-    // https://bugzilla.mozilla.org/show_bug.cgi?id=1373618#c36
-    nsresult rv = Seek(GetOffset(), false);
-    if (NS_SUCCEEDED(rv)) {
-      return rv;
-    }
-    // If the reopen/reseek fails, just fall through and treat this
-    // error as fatal.
-  }
-
-  mCacheStream.NotifyDataEnded(aStatus);
-
+  mCacheStream.NotifyDataEnded(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
@@ -2114,22 +2114,46 @@ 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)
+MediaCacheStream::NotifyDataEnded(nsresult aStatus, bool aReopenOnError)
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
 
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
 
+  // 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 &&
+      aStatus != NS_BINDING_ABORTED &&
+      (mChannelOffset == 0 ||
+       (mStreamLength > 0 && mChannelOffset != mStreamLength &&
+        mIsTransportSeekable))) {
+    // If the stream did close normally, restart the channel if we're either
+    // at the start of the resource, or if the server is seekable and we're
+    // not at the end of stream. We don't restart the stream if we're at the
+    // end because not all web servers handle this case consistently; see:
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1373618#c36
+    mClient->CacheClientSeek(mChannelOffset, false);
+    return;
+    // Note CacheClientSeek() will call Seek() asynchronously which might fail
+    // and close the stream. This is OK for it is not an error we can recover
+    // from and we have a consistent behavior with that where CacheClientSeek()
+    // is called from MediaCache::Update().
+  }
+
   if (NS_FAILED(aStatus)) {
     // Disconnect from other streams sharing our resource, since they
     // should continue trying to load. Our load might have been deliberately
     // canceled and that shouldn't affect other streams.
     mResourceID = mMediaCache->AllocateResourceID();
   }
 
   // It is prudent to update channel/cache status before calling
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -263,17 +263,17 @@ public:
   // 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();
   // Notifies the cache that the channel has closed with the given status.
-  void NotifyDataEnded(nsresult aStatus);
+  void NotifyDataEnded(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);