Bug 1450607 - P2. Synchronously seek to prepare for resuming following stop request. r=gerald, a=RyanVM
authorJean-Yves Avenard <jyavenard@mozilla.com>
Thu, 24 May 2018 14:36:58 +0200
changeset 802282 27ec43a60a397c61b9951e1398aaeed2bcf96934
parent 802281 fa1d57969fadfed1811b2a18efdd5c127f3ab893
child 802283 83f865afba050de072c57aee10920af07c9d8d43
push id111850
push userbmo:tom@mozilla.com
push dateThu, 31 May 2018 16:41:37 +0000
reviewersgerald, RyanVM
bugs1450607, 1415090, 1464045
milestone60.0.2
Bug 1450607 - P2. Synchronously seek to prepare for resuming following stop request. r=gerald, a=RyanVM Bug 1415090 attempted to remove the need to access from the MediaResource members of MediaCacheStream from the main thread. However, by doing so the logic flow for resuming the channel changed from a synchronous access to an asynchronous one. This changed some assumptions and allowed the ChannelMediaResource to be used before the Seek call completed. For now, re-add a cross thread access to the MediaCacheStream. A more elegant fix will be worked on in bug 1464045 MozReview-Commit-ID: 2xBTjDEqrkI
dom/media/ChannelMediaResource.cpp
dom/media/MediaCache.cpp
dom/media/MediaCache.h
--- a/dom/media/ChannelMediaResource.cpp
+++ b/dom/media/ChannelMediaResource.cpp
@@ -374,16 +374,59 @@ 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. It is also possible that the server sends us fewer bytes than
+  // requested. 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.
+  /*
+   * | length |    offset |   reopen |
+   * +--------+-----------+----------+
+   * |     -1 |         0 |      yes |
+   * +--------+-----------+----------+
+   * |     -1 |       > 0 | seekable |
+   * +--------+-----------+----------+
+   * |      0 |         X |       no |
+   * +--------+-----------+----------+
+   * |    > 0 |         0 |      yes |
+   * +--------+-----------+----------+
+   * |    > 0 | != length | seekable |
+   * +--------+-----------+----------+
+   * |    > 0 | == length |       no |
+   */
+  if (aStatus != NS_ERROR_PARSED_DATA_CACHED && aStatus != NS_BINDING_ABORTED) {
+    auto lengthAndOffset = mCacheStream.GetLengthAndOffset();
+    int64_t length = lengthAndOffset.mLength;
+    int64_t offset = lengthAndOffset.mOffset;
+    if ((offset == 0 || mIsTransportSeekable) && offset != length) {
+      // 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(offset, false);
+      if (NS_SUCCEEDED(rv)) {
+        return rv;
+      }
+      // 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.
+      Close();
+    }
+  }
+
   mCacheStream.NotifyDataEnded(mLoadID, aStatus, true /*aReopenOnError*/);
   return NS_OK;
 }
 
 nsresult
 ChannelMediaResource::OnChannelRedirect(nsIChannel* aOld,
                                         nsIChannel* aNew,
                                         uint32_t aFlags,
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -244,17 +244,19 @@ public:
   // Verify invariants, especially block list invariants
   void Verify(AutoLock&);
 #else
   void Verify(AutoLock&) {}
 #endif
 
   mozilla::Monitor& Monitor()
   {
-    MOZ_DIAGNOSTIC_ASSERT(!NS_IsMainThread());
+    // This method should only be called outside the main thread.
+    // The MOZ_DIAGNOSTIC_ASSERT(!NS_IsMainThread()) assertion should be
+    // re-added as part of bug 1464045
     return mMonitor;
   }
 
   /**
    * An iterator that makes it easy to iterate through all streams that
    * have a given resource ID and are not closed.
    * Must be used while holding the media cache lock.
    */
@@ -2198,55 +2200,16 @@ MediaCacheStream::NotifyDataEndedInterna
   MOZ_ASSERT(OwnerThread()->IsOnCurrentThread());
   AutoLock lock(mMediaCache->Monitor());
 
   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. It is also possible that the server sends us fewer bytes than
-  // requested. 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.
-  /*
-   * | length |    offset |   reopen |
-   * +--------+-----------+----------+
-   * |     -1 |         0 |      yes |
-   * +--------+-----------+----------+
-   * |     -1 |       > 0 | seekable |
-   * +--------+-----------+----------+
-   * |      0 |         X |       no |
-   * +--------+-----------+----------+
-   * |    > 0 |         0 |      yes |
-   * +--------+-----------+----------+
-   * |    > 0 | != length | seekable |
-   * +--------+-----------+----------+
-   * |    > 0 | == length |       no |
-   */
-  if (aReopenOnError && aStatus != NS_ERROR_PARSED_DATA_CACHED &&
-      aStatus != NS_BINDING_ABORTED &&
-      (mChannelOffset == 0 || mIsTransportSeekable) &&
-      mChannelOffset != mStreamLength) {
-    // 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().
-  }
-
   // It is prudent to update channel/cache status before calling
   // CacheClientNotifyDataEnded() which will read |mChannelEnded|.
   mChannelEnded = true;
   mMediaCache->QueueUpdate(lock);
 
   UpdateDownloadStatistics(lock);
 
   if (NS_FAILED(aStatus)) {
@@ -2447,16 +2410,24 @@ MediaCacheStream::Unpin()
 int64_t
 MediaCacheStream::GetLength() const
 {
   MOZ_ASSERT(!NS_IsMainThread());
   AutoLock lock(mMediaCache->Monitor());
   return mStreamLength;
 }
 
+MediaCacheStream::LengthAndOffset
+MediaCacheStream::GetLengthAndOffset() const
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  AutoLock lock(mMediaCache->Monitor());
+  return { mStreamLength, mChannelOffset };
+}
+
 int64_t
 MediaCacheStream::GetNextCachedData(int64_t aOffset)
 {
   MOZ_ASSERT(!NS_IsMainThread());
   AutoLock lock(mMediaCache->Monitor());
   return GetNextCachedDataInternal(lock, aOffset);
 }
 
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -298,16 +298,25 @@ public:
   void Unpin();
   // See comments above for NotifyDataStarted about how the length
   // can vary over time. Returns -1 if no length is known. Returns the
   // reported length if we haven't got any better information. If
   // the stream ended normally we return the length we actually got.
   // If we've successfully read data beyond the originally reported length,
   // we return the end of the data we've read.
   int64_t GetLength() const;
+  // Return the length and offset where next channel data will write to. Main
+  // thread only.
+  // This method should be removed as part of bug 1464045.
+  struct LengthAndOffset
+  {
+    int64_t mLength;
+    int64_t mOffset;
+  };
+  LengthAndOffset GetLengthAndOffset() const;
   // Returns the unique resource ID. Call only on the main thread or while
   // holding the media cache lock.
   int64_t GetResourceID() { return mResourceID; }
   // Returns the end of the bytes starting at the given offset
   // which are in cache.
   int64_t GetCachedDataEnd(int64_t aOffset);
   // Returns the offset of the first byte of cached data at or after aOffset,
   // or -1 if there is no such cached data.