Bug 1373618 - Prevent ChannelMediaResource from making a "Range: bytes=$length-" request at end of stream. r=jwwang
authorChris Pearce <cpearce@mozilla.com>
Tue, 04 Jul 2017 12:05:23 +1200
changeset 367183 da639765d99943765d67b9ab04465b57812114cf
parent 367182 44222e50fcf18fd9e4442d3c170d9b1715649c6c
child 367184 59555f5a60be7ffa9d44922bf2e63917622e4ddc
child 367224 fef489e8c2a193dde885adc48deb74cc883a5881
push id45886
push usercpearce@mozilla.com
push dateTue, 04 Jul 2017 04:27:12 +0000
treeherderautoland@da639765d999 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwwang
bugs1373618
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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