Bug 1373618 - Prevent ChannelMediaResource from making a "Range: bytes=$length-" request at end of stream. r?jwwang draft
authorChris Pearce <cpearce@mozilla.com>
Tue, 04 Jul 2017 12:05:23 +1200
changeset 603385 9b6a25e81e594257e34015f7502b9aeae39895fd
parent 603384 53a5192b436827f667db9df036b07d245e35d97a
child 635927 a6ff6737933b87a3553b04e7f5635a431c2a8863
push id66785
push userbmo:cpearce@mozilla.com
push dateTue, 04 Jul 2017 00:18:14 +0000
reviewersjwwang
bugs1373618
milestone56.0a1
Bug 1373618 - Prevent ChannelMediaResource from making a "Range: bytes=$length-" request at end of stream. r?jwwang Not all web servers consistently handle an HTTP 1.1 Byte Range Request for "Range: bytes=$length-". Apache responds with 416, but others do not. So to make us impervious to servers that respond with something other than 416, just have us not make such a request when we know that the server thinks our requested range is unsatisfiable. We make such a request when we reach end of stream for a stream that has been suspended/resumed. We are now more likely to suspend and resume streams with the recent changes to our throttling logic. MozReview-Commit-ID: 6URqzjLglOM
dom/media/MediaResource.cpp
--- a/dom/media/MediaResource.cpp
+++ b/dom/media/MediaResource.cpp
@@ -407,25 +407,29 @@ ChannelMediaResource::OnStopRequest(nsIR
   }
 
   // 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 (mReopenOnError &&
-      aStatus != NS_ERROR_PARSED_DATA_CACHED && aStatus != NS_BINDING_ABORTED &&
-      (mOffset == 0 || mCacheStream.IsTransportSeekable())) {
-    // If the stream did close normally, then if the server is seekable we'll
-    // just seek to the end of the resource and get an HTTP 416 error because
-    // there's nothing there, so this isn't bad.
+  if (mReopenOnError && aStatus != NS_ERROR_PARSED_DATA_CACHED &&
+      aStatus != NS_BINDING_ABORTED &&
+      (mOffset == 0 || (GetLength() > 0 && mOffset != GetLength() &&
+                        mCacheStream.IsTransportSeekable()))) {
+    // 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 = CacheClientSeek(mOffset, false);
-    if (NS_SUCCEEDED(rv))
+    if (NS_SUCCEEDED(rv)) {
       return rv;
+    }
     // If the reopen/reseek fails, just fall through and treat this
     // error as fatal.
   }
 
   if (!mIgnoreClose) {
     mCacheStream.NotifyDataEnded(aStatus);
 
     // Move this request back into the foreground.  This is necessary for