Bug 1415397 - use Span<> to replace low level pointer arithmetic in ReadFromCache(). draft
authorJW Wang <jwwang@mozilla.com>
Thu, 02 Nov 2017 11:36:56 +0800
changeset 695340 44a34bf84af3e6b157c77e64632ac7574b25b392
parent 695307 e2f87726b6082db0ae8a0866f65bff6b7062a07c
child 695341 200b4064870244c4a63184f43bc3dd48163ae240
child 695426 6810fcee08b8112e86be6193a8c47912600e2855
push id88396
push userjwwang@mozilla.com
push dateThu, 09 Nov 2017 03:07:36 +0000
bugs1415397
milestone58.0a1
Bug 1415397 - use Span<> to replace low level pointer arithmetic in ReadFromCache(). MozReview-Commit-ID: HH6KXtMfSIJ
dom/media/MediaCache.cpp
dom/media/MediaCache.h
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -9,16 +9,17 @@
 #include "ChannelMediaResource.h"
 #include "FileBlockCache.h"
 #include "MediaBlockCacheBase.h"
 #include "MediaPrefs.h"
 #include "MediaResource.h"
 #include "MemoryBlockCache.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/ClearOnShutdown.h"
+#include "mozilla/ErrorNames.h"
 #include "mozilla/Logging.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/ReentrantMonitor.h"
 #include "mozilla/Services.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/SystemGroup.h"
 #include "mozilla/Telemetry.h"
 #include "nsContentUtils.h"
@@ -30,19 +31,21 @@
 #include "nsThreadUtils.h"
 #include "prio.h"
 #include <algorithm>
 
 namespace mozilla {
 
 #undef LOG
 #undef LOGI
+#undef LOGE
 LazyLogModule gMediaCacheLog("MediaCache");
 #define LOG(...) MOZ_LOG(gMediaCacheLog, LogLevel::Debug, (__VA_ARGS__))
 #define LOGI(...) MOZ_LOG(gMediaCacheLog, LogLevel::Info, (__VA_ARGS__))
+#define LOGE(...) NS_DebugBreak(NS_DEBUG_WARNING, nsPrintfCString(__VA_ARGS__).get(), nullptr, __FILE__, __LINE__)
 
 // For HTTP seeking, if number of bytes needing to be
 // seeked forward is less than this value then a read is
 // done rather than a byte range request.
 //
 // If we assume a 100Mbit connection, and assume reissuing an HTTP seek causes
 // a delay of 200ms, then in that 200ms we could have simply read ahead 2MB. So
 // setting SEEK_VS_READ_THRESHOLD to 1MB sounds reasonable.
@@ -2457,16 +2460,82 @@ MediaCacheStream::ThrottleReadahead(bool
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   if (mThrottleReadahead != bThrottle) {
     LOGI("Stream %p ThrottleReadahead %d", this, bThrottle);
     mThrottleReadahead = bThrottle;
     mMediaCache->QueueUpdate();
   }
 }
 
+uint32_t
+MediaCacheStream::ReadPartialBlock(int64_t aOffset, Span<char> aBuffer)
+{
+  mMediaCache->GetReentrantMonitor().AssertCurrentThreadIn();
+  MOZ_ASSERT(IsOffsetAllowed(aOffset));
+
+  if (OffsetToBlockIndexUnchecked(mChannelOffset) !=
+        OffsetToBlockIndexUnchecked(aOffset) ||
+      aOffset >= mChannelOffset) {
+    // Not in the partial block or no data to read.
+    return 0;
+  }
+
+  auto source = MakeSpan<const uint8_t>(
+    mPartialBlockBuffer.get() + OffsetInBlock(aOffset),
+    OffsetInBlock(mChannelOffset) - OffsetInBlock(aOffset));
+  // We have |source.Length() <= BLOCK_SIZE < INT32_MAX| to guarantee
+  // that |bytesToRead| can fit into a uint32_t.
+  uint32_t bytesToRead = std::min(aBuffer.Length(), source.Length());
+  memcpy(aBuffer.Elements(), source.Elements(), bytesToRead);
+  return bytesToRead;
+}
+
+Result<uint32_t, nsresult>
+MediaCacheStream::ReadBlockFromCache(int64_t aOffset, Span<char> aBuffer)
+{
+  mMediaCache->GetReentrantMonitor().AssertCurrentThreadIn();
+  MOZ_ASSERT(IsOffsetAllowed(aOffset));
+
+  // OffsetToBlockIndexUnchecked() is always non-negative.
+  uint32_t index = OffsetToBlockIndexUnchecked(aOffset);
+  int32_t cacheBlock = index < mBlocks.Length() ? mBlocks[index] : -1;
+  if (cacheBlock < 0) {
+    // Not in the cache.
+    return 0;
+  }
+
+  if (aBuffer.Length() > size_t(BLOCK_SIZE)) {
+    // Clamp the buffer to avoid overflow below since we will read at most
+    // BLOCK_SIZE bytes.
+    aBuffer = aBuffer.First(BLOCK_SIZE);
+  }
+  // |BLOCK_SIZE - OffsetInBlock(aOffset)| <= BLOCK_SIZE
+  int32_t bytesToRead =
+    std::min<int32_t>(BLOCK_SIZE - OffsetInBlock(aOffset), aBuffer.Length());
+  int32_t bytesRead = 0;
+  nsresult rv =
+    mMediaCache->ReadCacheFile(cacheBlock * BLOCK_SIZE + OffsetInBlock(aOffset),
+                               aBuffer.Elements(),
+                               bytesToRead,
+                               &bytesRead);
+
+  // Ensure |cacheBlock * BLOCK_SIZE + OffsetInBlock(aOffset)| won't overflow.
+  static_assert(INT64_MAX >= BLOCK_SIZE * (uint32_t(INT32_MAX) + 1),
+                "BLOCK_SIZE too large!");
+
+  if (NS_FAILED(rv)) {
+    nsCString name;
+    GetErrorName(rv, name);
+    LOGE("Stream %p ReadCacheFile failed, rv=%s", this, name.Data());
+    return mozilla::Err(rv);
+  }
+
+  return bytesRead;
+}
+
 int64_t
 MediaCacheStream::Tell()
 {
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   return mStreamOffset;
 }
 
 nsresult
@@ -2601,77 +2670,59 @@ MediaCacheStream::ReadAt(int64_t aOffset
 
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
   nsresult rv = Seek(nsISeekableStream::NS_SEEK_SET, aOffset);
   if (NS_FAILED(rv)) return rv;
   return Read(aBuffer, aCount, aBytes);
 }
 
 nsresult
-MediaCacheStream::ReadFromCache(char* aBuffer, int64_t aOffset, int64_t aCount)
+MediaCacheStream::ReadFromCache(char* aBuffer, int64_t aOffset, uint32_t aCount)
 {
   ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
 
+  // The buffer we are about to fill.
+  auto buffer = MakeSpan<char>(aBuffer, aCount);
+
   // Read one block (or part of a block) at a time
-  uint32_t count = 0;
   int64_t streamOffset = aOffset;
-  while (count < aCount) {
+  while (!buffer.IsEmpty()) {
     if (mClosed) {
       // We need to check |mClosed| in each iteration which might be changed
       // after calling |mMediaCache->ReadCacheFile|.
       return NS_ERROR_FAILURE;
     }
-    int32_t streamBlock = OffsetToBlockIndex(streamOffset);
-    if (streamBlock < 0) {
-      break;
-    }
-    uint32_t offsetInStreamBlock =
-      uint32_t(streamOffset - streamBlock*BLOCK_SIZE);
-    int64_t size = std::min<int64_t>(aCount - count, BLOCK_SIZE - offsetInStreamBlock);
 
-    if (mStreamLength >= 0) {
-      // Don't try to read beyond the end of the stream
-      int64_t bytesRemaining = mStreamLength - streamOffset;
-      if (bytesRemaining <= 0) {
-        return NS_ERROR_FAILURE;
-      }
-      size = std::min(size, bytesRemaining);
-      // Clamp size until 64-bit file size issues are fixed.
-      size = std::min(size, int64_t(INT32_MAX));
+    if (!IsOffsetAllowed(streamOffset)) {
+      LOGE("Stream %p invalid offset=%" PRId64, this, streamOffset);
+      return NS_ERROR_ILLEGAL_VALUE;
+    }
+
+    Result<uint32_t, nsresult> rv = ReadBlockFromCache(streamOffset, buffer);
+    if (rv.isErr()) {
+      return rv.unwrapErr();
     }
 
-    int32_t bytes;
-    int32_t channelBlock = OffsetToBlockIndexUnchecked(mChannelOffset);
-    int32_t cacheBlock =
-      size_t(streamBlock) < mBlocks.Length() ? mBlocks[streamBlock] : -1;
-    if (channelBlock == streamBlock && streamOffset < mChannelOffset) {
-      // We can just use the data in mPartialBlockBuffer. In fact we should
-      // use it rather than waiting for the block to fill and land in
-      // the cache.
-      // Clamp bytes until 64-bit file size issues are fixed.
-      int64_t toCopy = std::min<int64_t>(size, mChannelOffset - streamOffset);
-      bytes = std::min(toCopy, int64_t(INT32_MAX));
-      MOZ_ASSERT(bytes >= 0 && bytes <= toCopy, "Bytes out of range.");
-      memcpy(aBuffer + count,
-        mPartialBlockBuffer.get() + offsetInStreamBlock, bytes);
-    } else {
-      if (cacheBlock < 0) {
-        // We expect all blocks to be cached! Fail!
-        return NS_ERROR_FAILURE;
-      }
-      int64_t offset = cacheBlock*BLOCK_SIZE + offsetInStreamBlock;
-      MOZ_ASSERT(size >= 0 && size <= INT32_MAX, "Size out of range.");
-      nsresult rv = mMediaCache->ReadCacheFile(
-        offset, aBuffer + count, int32_t(size), &bytes);
-      if (NS_FAILED(rv)) {
-        return rv;
-      }
+    uint32_t bytes = rv.unwrap();
+    if (bytes > 0) {
+      // Read data from the cache successfully. Let's try next block.
+      streamOffset += bytes;
+      buffer = buffer.From(bytes);
+      continue;
     }
-    streamOffset += bytes;
-    count += bytes;
+
+    // The partial block is our last chance to get data.
+    bytes = ReadPartialBlock(streamOffset, buffer);
+    if (bytes < buffer.Length()) {
+      // Not enough data to read.
+      return NS_ERROR_FAILURE;
+    }
+
+    // Return for we've got all the requested bytes.
+    return NS_OK;
   }
 
   return NS_OK;
 }
 
 nsresult
 MediaCacheStream::Init(int64_t aContentLength)
 {
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -3,16 +3,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef MediaCache_h_
 #define MediaCache_h_
 
 #include "Intervals.h"
+#include "mozilla/Result.h"
 #include "mozilla/UniquePtr.h"
 #include "nsCOMPtr.h"
 #include "nsHashKeys.h"
 #include "nsTArray.h"
 #include "nsTHashtable.h"
 
 class nsIEventTarget;
 class nsIPrincipal;
@@ -309,19 +310,17 @@ public:
   // growing. The stream should be pinned while this runs and while its results
   // are used, to ensure no data is evicted.
   nsresult GetCachedRanges(MediaByteRangeSet& aRanges);
 
   // Reads from buffered data only. Will fail if not all data to be read is
   // in the cache. Will not mark blocks as read. Can be called from the main
   // thread. It's the caller's responsibility to wrap the call in a pin/unpin,
   // and also to check that the range they want is cached before calling this.
-  nsresult ReadFromCache(char* aBuffer,
-                         int64_t aOffset,
-                         int64_t aCount);
+  nsresult ReadFromCache(char* aBuffer, int64_t aOffset, uint32_t aCount);
 
   // IsDataCachedToEndOfStream returns true if all the data from
   // aOffset to the end of the stream (the server-reported end, if the
   // real end is not known) is in cache. If we know nothing about the
   // end of the stream, this returns false.
   bool IsDataCachedToEndOfStream(int64_t aOffset);
   // The mode is initially MODE_PLAYBACK.
   void SetReadMode(ReadMode aMode);
@@ -421,16 +420,26 @@ private:
     nsTHashtable<Entry> mEntries;
 
     // The index of the first block in the list, or -1 if the list is empty.
     int32_t mFirstBlock;
     // The number of blocks in the list.
     int32_t mCount;
   };
 
+  // Read data from the partial block and return the number of bytes read
+  // successfully. 0 if aOffset is not an offset in the partial block or there
+  // is nothing to read.
+  uint32_t ReadPartialBlock(int64_t aOffset, Span<char> aBuffer);
+
+  // Read data from the cache block specified by aOffset. Return the number of
+  // bytes read successfully or an error code if any failure.
+  Result<uint32_t, nsresult> ReadBlockFromCache(int64_t aOffset,
+                                                Span<char> aBuffer);
+
   // Returns the end of the bytes starting at the given offset
   // which are in cache.
   // This method assumes that the cache monitor is held and can be called on
   // any thread.
   int64_t GetCachedDataEndInternal(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.
   // This method assumes that the cache monitor is held and can be called on