Bug 1395855. P1 - remove ChannelMediaResource::mOffset so we have less data race to worry about. r=cpearce
authorJW Wang <jwwang@mozilla.com>
Fri, 08 Sep 2017 15:09:37 +0800
changeset 663214 803f854237c1408d652d02dbcad0900f12a05960
parent 663213 1c376c20595a1468a84fbfc4ef23558a4577571a
child 663215 951c770131ae5da7afce2778aca453c98e6da141
push id79385
push userbmo:ttromey@mozilla.com
push dateTue, 12 Sep 2017 20:54:14 +0000
reviewerscpearce
bugs1395855
milestone57.0a1
Bug 1395855. P1 - remove ChannelMediaResource::mOffset so we have less data race to worry about. r=cpearce We have MediaCacheStream::mChannelOffset to keep the download positon. We don't need 2 variables for the same purpose. MozReview-Commit-ID: IpnEJWuA9A9
dom/media/MediaCache.cpp
dom/media/MediaCache.h
dom/media/MediaResource.cpp
dom/media/MediaResource.h
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -2152,16 +2152,23 @@ MediaCacheStream::Unpin()
 int64_t
 MediaCacheStream::GetLength()
 {
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   return mStreamLength;
 }
 
 int64_t
+MediaCacheStream::GetOffset() const
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  return mChannelOffset;
+}
+
+int64_t
 MediaCacheStream::GetNextCachedData(int64_t aOffset)
 {
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   return GetNextCachedDataInternal(aOffset);
 }
 
 int64_t
 MediaCacheStream::GetCachedDataEnd(int64_t aOffset)
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -280,16 +280,18 @@ public:
   void Unpin();
   // See comments above for NotifyDataLength 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();
+  // Return the offset where next channel data will write to. Main thread only.
+  int64_t GetOffset() 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.
--- a/dom/media/MediaResource.cpp
+++ b/dom/media/MediaResource.cpp
@@ -75,30 +75,28 @@ MediaResource::Destroy()
 NS_IMPL_ADDREF(MediaResource)
 NS_IMPL_RELEASE_WITH_DESTROY(MediaResource, Destroy())
 
 ChannelMediaResource::ChannelMediaResource(MediaResourceCallback* aCallback,
                                            nsIChannel* aChannel,
                                            nsIURI* aURI,
                                            bool aIsPrivateBrowsing)
   : BaseMediaResource(aCallback, aChannel, aURI)
-  , mOffset(0)
   , mReopenOnError(false)
   , mCacheStream(this, aIsPrivateBrowsing)
   , mSuspendAgent(mChannel)
 {
 }
 
 ChannelMediaResource::ChannelMediaResource(
   MediaResourceCallback* aCallback,
   nsIChannel* aChannel,
   nsIURI* aURI,
   const MediaChannelStatistics& aStatistics)
   : BaseMediaResource(aCallback, aChannel, aURI)
-  , mOffset(0)
   , mReopenOnError(false)
   , mCacheStream(this, /* aIsPrivateBrowsing = */ false)
   , mChannelStatistics(aStatistics)
   , mSuspendAgent(mChannel)
 {
 }
 
 ChannelMediaResource::~ChannelMediaResource()
@@ -279,36 +277,29 @@ ChannelMediaResource::OnStartRequest(nsI
       if (gotRangeHeader) {
         // We received 'Content-Range', so the server accepts range requests.
         // Notify media cache about the length and start offset of data received.
         // Note: If aRangeTotal == -1, then the total bytes is unknown at this stage.
         //       For now, tell the decoder that the stream is infinite.
         if (rangeTotal != -1) {
           contentLength = std::max(contentLength, rangeTotal);
         }
-        // Give some warnings if the ranges are unexpected.
-        // XXX These could be error conditions.
-        NS_WARNING_ASSERTION(
-          mOffset == rangeStart,
-          "response range start does not match current offset");
-        mOffset = rangeStart;
         mCacheStream.NotifyDataStarted(rangeStart);
       }
       acceptsRanges = gotRangeHeader;
-    } else if (mOffset > 0 && responseStatus == HTTP_OK_CODE) {
+    } else if (GetOffset() > 0 && responseStatus == HTTP_OK_CODE) {
       // If we get an OK response but we were seeking, or requesting a byte
       // range, then we have to assume that seeking doesn't work. We also need
       // to tell the cache that it's getting data for the start of the stream.
       mCacheStream.NotifyDataStarted(0);
-      mOffset = 0;
 
       // The server claimed it supported range requests.  It lied.
       acceptsRanges = false;
     }
-    if (mOffset == 0 && contentLength >= 0 &&
+    if (GetOffset() == 0 && contentLength >= 0 &&
         (responseStatus == HTTP_OK_CODE ||
          responseStatus == HTTP_PARTIAL_RESPONSE_CODE)) {
       mCacheStream.NotifyDataLength(contentLength);
     }
     // XXX we probably should examine the Content-Range header in case
     // the server gave us a range which is not quite what we asked for
 
     // If we get an HTTP_OK_CODE response to our byte range request,
@@ -393,24 +384,24 @@ 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 || (GetLength() > 0 && mOffset != GetLength() &&
-                        mCacheStream.IsTransportSeekable()))) {
+      (GetOffset() == 0 || (GetLength() > 0 && GetOffset() != 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);
+    nsresult rv = CacheClientSeek(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);
@@ -430,30 +421,25 @@ ChannelMediaResource::OnStopRequest(nsIR
 }
 
 nsresult
 ChannelMediaResource::OnChannelRedirect(nsIChannel* aOld, nsIChannel* aNew,
                                         uint32_t aFlags)
 {
   mChannel = aNew;
   mSuspendAgent.NotifyChannelOpened(mChannel);
-  return SetupChannelHeaders();
+  return SetupChannelHeaders(GetOffset());
 }
 
 nsresult
 ChannelMediaResource::CopySegmentToCache(nsIPrincipal* aPrincipal,
                                          const char* aFromSegment,
                                          uint32_t aCount,
                                          uint32_t* aWriteCount)
 {
-  // Keep track of where we're up to.
-  LOG("CopySegmentToCache at mOffset [%" PRId64 "] add "
-      "[%d] bytes for decoder[%p]",
-      mOffset, aCount, mCallback.get());
-  mOffset += aCount;
   mCacheStream.NotifyDataReceived(aCount, aFromSegment, aPrincipal);
   *aWriteCount = aCount;
   return NS_OK;
 }
 
 
 struct CopySegmentClosure {
   nsCOMPtr<nsIPrincipal> mPrincipal;
@@ -523,73 +509,74 @@ ChannelMediaResource::Open(nsIStreamList
     }
   }
 
   nsresult rv = mCacheStream.Init(cl);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  MOZ_ASSERT(mOffset == 0, "Who set mOffset already?");
+  MOZ_ASSERT(GetOffset() == 0, "Who set offset already?");
   mListener = new Listener(this);
   *aStreamListener = mListener;
   NS_ADDREF(*aStreamListener);
   return NS_OK;
 }
 
 nsresult
-ChannelMediaResource::OpenChannel()
+ChannelMediaResource::OpenChannel(int64_t aOffset)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(mChannel);
   MOZ_ASSERT(!mListener, "Listener should have been removed by now");
 
   mListener = new Listener(this);
   nsresult rv = mChannel->SetNotificationCallbacks(mListener.get());
   NS_ENSURE_SUCCESS(rv, rv);
 
-  rv = SetupChannelHeaders();
+  rv = SetupChannelHeaders(aOffset);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = mChannel->AsyncOpen2(mListener);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Tell the media element that we are fetching data from a channel.
   MediaDecoderOwner* owner = mCallback->GetMediaOwner();
   NS_ENSURE_TRUE(owner, NS_ERROR_FAILURE);
   dom::HTMLMediaElement* element = owner->GetMediaElement();
   element->DownloadResumed(true);
 
   return NS_OK;
 }
 
-nsresult ChannelMediaResource::SetupChannelHeaders()
+nsresult
+ChannelMediaResource::SetupChannelHeaders(int64_t aOffset)
 {
   // Always use a byte range request even if we're reading from the start
   // of the resource.
   // This enables us to detect if the stream supports byte range
   // requests, and therefore seeking, early.
   nsCOMPtr<nsIHttpChannel> hc = do_QueryInterface(mChannel);
   if (hc) {
     // Use |mOffset| if seeking in a complete file download.
     nsAutoCString rangeString("bytes=");
-    rangeString.AppendInt(mOffset);
+    rangeString.AppendInt(aOffset);
     rangeString.Append('-');
     nsresult rv = hc->SetRequestHeader(NS_LITERAL_CSTRING("Range"), rangeString, false);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Send Accept header for video and audio types only (Bug 489071)
     NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
     MediaDecoderOwner* owner = mCallback->GetMediaOwner();
     NS_ENSURE_TRUE(owner, NS_ERROR_FAILURE);
     dom::HTMLMediaElement* element = owner->GetMediaElement();
     NS_ENSURE_TRUE(element, NS_ERROR_FAILURE);
     element->SetRequestHeaders(hc);
   } else {
-    NS_ASSERTION(mOffset == 0, "Don't know how to seek on this channel type");
+    NS_ASSERTION(aOffset == 0, "Don't know how to seek on this channel type");
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
 
 nsresult ChannelMediaResource::Close()
 {
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
@@ -749,20 +736,20 @@ void ChannelMediaResource::Resume()
       element->DownloadResumed();
     } else {
       int64_t totalLength = GetLength();
       // If mOffset is at the end of the stream, then we shouldn't try to
       // seek to it. The seek will fail and be wasted anyway. We can leave
       // the channel dead; if the media cache wants to read some other data
       // in the future, it will call CacheClientSeek itself which will reopen the
       // channel.
-      if (totalLength < 0 || mOffset < totalLength) {
+      if (totalLength < 0 || GetOffset() < totalLength) {
         // There is (or may be) data to read at mOffset, so start reading it.
         // Need to recreate the channel.
-        CacheClientSeek(mOffset, false);
+        CacheClientSeek(GetOffset(), false);
         element->DownloadResumed();
       } else {
         // The channel remains dead. Do not notify DownloadResumed() which
         // will leave the media element in NETWORK_LOADING state.
       }
     }
   }
 }
@@ -858,32 +845,30 @@ ChannelMediaResource::CacheClientSeek(in
 {
   NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread");
 
   LOG("CacheClientSeek requested for aOffset [%" PRId64 "] for decoder [%p]",
       aOffset, mCallback.get());
 
   CloseChannel();
 
-  mOffset = aOffset;
-
   if (aResume) {
     mSuspendAgent.Resume();
   }
 
   // Don't create a new channel if we are still suspended. The channel will
   // be recreated when we are resumed.
   if (mSuspendAgent.IsSuspended()) {
     return NS_OK;
   }
 
   nsresult rv = RecreateChannel();
   NS_ENSURE_SUCCESS(rv, rv);
 
-  return OpenChannel();
+  return OpenChannel(aOffset);
 }
 
 nsresult
 ChannelMediaResource::CacheClientSuspend()
 {
   Suspend(false);
   return NS_OK;
 }
@@ -957,16 +942,22 @@ ChannelMediaResource::GetDownloadRate(bo
 }
 
 int64_t
 ChannelMediaResource::GetLength()
 {
   return mCacheStream.GetLength();
 }
 
+int64_t
+ChannelMediaResource::GetOffset() const
+{
+  return mCacheStream.GetOffset();
+}
+
 // ChannelSuspendAgent
 
 bool
 ChannelSuspendAgent::Suspend()
 {
   MOZ_ASSERT(NS_IsMainThread());
   SuspendInternal();
   return (++mSuspendCount == 1);
--- a/dom/media/MediaResource.h
+++ b/dom/media/MediaResource.h
@@ -533,25 +533,27 @@ protected:
   // These are called on the main thread by Listener.
   nsresult OnStartRequest(nsIRequest* aRequest);
   nsresult OnStopRequest(nsIRequest* aRequest, nsresult aStatus);
   nsresult OnDataAvailable(nsIRequest* aRequest,
                            nsIInputStream* aStream,
                            uint32_t aCount);
   nsresult OnChannelRedirect(nsIChannel* aOld, nsIChannel* aNew, uint32_t aFlags);
 
-  // Opens the channel, using an HTTP byte range request to start at mOffset
+  // Opens the channel, using an HTTP byte range request to start at aOffset
   // if possible. Main thread only.
-  nsresult OpenChannel();
+  nsresult OpenChannel(int64_t aOffset);
   nsresult RecreateChannel();
   // Add headers to HTTP request. Main thread only.
-  nsresult SetupChannelHeaders();
+  nsresult SetupChannelHeaders(int64_t aOffset);
   // Closes the channel. Main thread only.
   void CloseChannel();
 
+  int64_t GetOffset() const;
+
   // Parses 'Content-Range' header and returns results via parameters.
   // Returns error if header is not available, values are not parse-able or
   // values are out of range.
   nsresult ParseContentRangeHeader(nsIHttpChannel * aHttpChan,
                                    int64_t& aRangeStart,
                                    int64_t& aRangeEnd,
                                    int64_t& aRangeTotal);
 
@@ -563,17 +565,16 @@ protected:
                                      uint32_t* aWriteCount);
 
   nsresult CopySegmentToCache(nsIPrincipal* aPrincipal,
                               const char* aFromSegment,
                               uint32_t aCount,
                               uint32_t* aWriteCount);
 
   // Main thread access only
-  int64_t            mOffset;
   RefPtr<Listener> mListener;
   // When this flag is set, if we get a network error we should silently
   // reopen the stream.
   bool               mReopenOnError;
 
   // Any thread access
   MediaCacheStream mCacheStream;