Issue #2073 - m-c 1383404: make SourceBuffer::Compact more efficient (squashed)

The first part also means that Compact no longer needs the SurfaceCache lock (used to be via CreateChunk->CanHold),
which avoids potential deadlocks during shutdown that m-c 523950 would otherwise cause

5 files changed, 79 insertions, 54 deletions

diff --git a/image/ImageFactory.cpp b/image/ImageFactory.cpp
index 428be1424e..343f7b582b 100644
--- a/image/ImageFactory.cpp
+++ b/image/ImageFactory.cpp
@@ -111,8 +111,32 @@ BadImage(const char* aMessage, RefPtr<T>& aImage)
   return aImage.forget();
 }
 
+static void
+SetSourceSizeHint(RasterImage* aImage, uint32_t aSize)
+{
+  // Pass anything usable on so that the RasterImage can preallocate
+  // its source buffer.
+  if (aSize == 0) {
+    return;
+  }
+
+  // Bound by something reasonable
+  uint32_t sizeHint = std::min<uint32_t>(aSize, 20000000);
+  nsresult rv = aImage->SetSourceSizeHint(sizeHint);
+  if (NS_FAILED(rv)) {
+    // Flush memory, try to get some back, and try again.
+    rv = nsMemory::HeapMinimize(true);
+    nsresult rv2 = aImage->SetSourceSizeHint(sizeHint);
+    // If we've still failed at this point, things are going downhill.
+    if (NS_FAILED(rv) || NS_FAILED(rv2)) {
+      NS_WARNING("About to hit OOM in imagelib!");
+    }
+  }
+}
+
 /* static */ already_AddRefed<Image>
-ImageFactory::CreateAnonymousImage(const nsCString& aMimeType)
+ImageFactory::CreateAnonymousImage(const nsCString& aMimeType,
+                                   uint32_t aSizeHint /* = 0 */)
 {
   nsresult rv;
 
@@ -127,6 +151,7 @@ ImageFactory::CreateAnonymousImage(const nsCString& aMimeType)
     return BadImage("RasterImage::Init failed", newImage);
   }
 
+  SetSourceSizeHint(newImage, aSizeHint);
   return newImage.forget();
 }
 
@@ -231,25 +256,7 @@ ImageFactory::CreateRasterImage(nsIRequest* aRequest,
 
   newImage->SetInnerWindowID(aInnerWindowId);
 
-  uint32_t len = GetContentSize(aRequest);
-
-  // Pass anything usable on so that the RasterImage can preallocate
-  // its source buffer.
-  if (len > 0) {
-    // Bound by something reasonable
-    uint32_t sizeHint = std::min<uint32_t>(len, 20000000);
-    rv = newImage->SetSourceSizeHint(sizeHint);
-    if (NS_FAILED(rv)) {
-      // Flush memory, try to get some back, and try again.
-      rv = nsMemory::HeapMinimize(true);
-      nsresult rv2 = newImage->SetSourceSizeHint(sizeHint);
-      // If we've still failed at this point, things are going downhill.
-      if (NS_FAILED(rv) || NS_FAILED(rv2)) {
-        NS_WARNING("About to hit OOM in imagelib!");
-      }
-    }
-  }
-
+  SetSourceSizeHint(newImage, GetContentSize(aRequest));
   return newImage.forget();
 }
 
diff --git a/image/ImageFactory.h b/image/ImageFactory.h
index 6c2e0f5045..fdd6460ec1 100644
--- a/image/ImageFactory.h
+++ b/image/ImageFactory.h
@@ -51,9 +51,10 @@ public:
    * the usual image loading mechanism.
    *
    * @param aMimeType      The mimetype of the image.
+   * @param aSizeHint      The length of the source data for the image.
    */
   static already_AddRefed<Image>
-  CreateAnonymousImage(const nsCString& aMimeType);
+  CreateAnonymousImage(const nsCString& aMimeType, uint32_t aSizeHint = 0);
 
   /**
    * Creates a new multipart/x-mixed-replace image wrapper, and initializes it
diff --git a/image/SourceBuffer.cpp b/image/SourceBuffer.cpp
index de066e29fc..5961cc66ff 100644
--- a/image/SourceBuffer.cpp
+++ b/image/SourceBuffer.cpp
@@ -204,30 +204,27 @@ SourceBuffer::Compact()
     return NS_OK;
   }
 
-  Maybe<Chunk> newChunk = CreateChunk(length, /* aRoundUp = */ false);
-  if (MOZ_UNLIKELY(!newChunk || newChunk->AllocationFailed())) {
-    NS_WARNING("Failed to allocate chunk for SourceBuffer compacting - OOM?");
+  Chunk& mergeChunk = mChunks[0];
+  if (MOZ_UNLIKELY(!mergeChunk.SetCapacity(length))) {
+    NS_WARNING("Failed to reallocate chunk for SourceBuffer compacting - OOM?");
     return NS_OK;
   }
 
-  // Copy our old chunks into the new chunk.
-  for (uint32_t i = 0 ; i < mChunks.Length() ; ++i) {
-    size_t offset = newChunk->Length();
-    MOZ_ASSERT(offset < newChunk->Capacity());
-    MOZ_ASSERT(offset + mChunks[i].Length() <= newChunk->Capacity());
+  // Copy our old chunks into the newly reallocated first chunk.
+  for (uint32_t i = 1 ; i < mChunks.Length() ; ++i) {
+    size_t offset = mergeChunk.Length();
+    MOZ_ASSERT(offset < mergeChunk.Capacity());
+    MOZ_ASSERT(offset + mChunks[i].Length() <= mergeChunk.Capacity());
 
-    memcpy(newChunk->Data() + offset, mChunks[i].Data(), mChunks[i].Length());
-    newChunk->AddLength(mChunks[i].Length());
+    memcpy(mergeChunk.Data() + offset, mChunks[i].Data(), mChunks[i].Length());
+    mergeChunk.AddLength(mChunks[i].Length());
   }
 
-  MOZ_ASSERT(newChunk->Length() == newChunk->Capacity(),
+  MOZ_ASSERT(mergeChunk.Length() == mergeChunk.Capacity(),
              "Compacted chunk has slack space");
 
-  // Replace the old chunks with the new, compact chunk.
-  mChunks.Clear();
-  if (MOZ_UNLIKELY(NS_FAILED(AppendChunk(Move(newChunk))))) {
-    return HandleError(NS_ERROR_OUT_OF_MEMORY);
-  }
+  // Remove the redundant chunks.
+  mChunks.RemoveElementsAt(1, mChunks.Length() - 1);
   mChunks.Compact();
 
   return NS_OK;
@@ -317,7 +314,7 @@ SourceBuffer::ExpectLength(size_t aExpectedLength)
     return NS_OK;
   }
 
-  if (MOZ_UNLIKELY(NS_FAILED(AppendChunk(CreateChunk(aExpectedLength))))) {
+  if (MOZ_UNLIKELY(NS_FAILED(AppendChunk(CreateChunk(aExpectedLength, /* aRoundUp */ false))))) {
     return HandleError(NS_ERROR_OUT_OF_MEMORY);
   }
 
diff --git a/image/SourceBuffer.h b/image/SourceBuffer.h
index e5aff1dcdd..6e3ef7a538 100644
--- a/image/SourceBuffer.h
+++ b/image/SourceBuffer.h
@@ -358,7 +358,7 @@ private:
   // Chunk type and chunk-related methods.
   //////////////////////////////////////////////////////////////////////////////
 
-  class Chunk
+  class Chunk final
   {
   public:
     explicit Chunk(size_t aCapacity)
@@ -366,13 +366,18 @@ private:
       , mLength(0)
     {
       MOZ_ASSERT(aCapacity > 0, "Creating zero-capacity chunk");
-      mData.reset(new (fallible) char[mCapacity]);
+      mData = static_cast<char*>(malloc(mCapacity));
+    }
+
+    ~Chunk()
+    {
+      free(mData);
     }
 
     Chunk(Chunk&& aOther)
       : mCapacity(aOther.mCapacity)
       , mLength(aOther.mLength)
-      , mData(Move(aOther.mData))
+      , mData(aOther.mData)
     {
       aOther.mCapacity = aOther.mLength = 0;
       aOther.mData = nullptr;
@@ -380,9 +385,10 @@ private:
 
     Chunk& operator=(Chunk&& aOther)
     {
+      free(mData);
       mCapacity = aOther.mCapacity;
       mLength = aOther.mLength;
-      mData = Move(aOther.mData);
+      mData = aOther.mData;
       aOther.mCapacity = aOther.mLength = 0;
       aOther.mData = nullptr;
       return *this;
@@ -395,7 +401,7 @@ private:
     char* Data() const
     {
       MOZ_ASSERT(mData, "Allocation failed but nobody checked for it");
-      return mData.get();
+      return mData;
     }
 
     void AddLength(size_t aAdditionalLength)
@@ -404,13 +410,26 @@ private:
       mLength += aAdditionalLength;
     }
 
+    bool SetCapacity(size_t aCapacity)
+    {
+      MOZ_ASSERT(mData, "Allocation failed but nobody checked for it");
+      char* data = static_cast<char*>(realloc(mData, aCapacity));
+      if (!data) {
+        return false;
+      }
+
+      mData = data;
+      mCapacity = aCapacity;
+      return true;
+    }
+
   private:
     Chunk(const Chunk&) = delete;
     Chunk& operator=(const Chunk&) = delete;
 
     size_t mCapacity;
     size_t mLength;
-    UniquePtr<char[]> mData;
+    char* mData;
   };
 
   nsresult AppendChunk(Maybe<Chunk>&& aChunk);
@@ -454,7 +473,7 @@ private:
   mutable Mutex mMutex;
 
   /// The data in this SourceBuffer, stored as a series of Chunks.
-  FallibleTArray<Chunk> mChunks;
+  AutoTArray<Chunk, 1> mChunks;
 
   /// Consumers which are waiting to be notified when new data is available.
   nsTArray<RefPtr<IResumable>> mWaitingConsumers;
diff --git a/image/imgTools.cpp b/image/imgTools.cpp
index 29905c1ab3..3ac31102e5 100644
--- a/image/imgTools.cpp
+++ b/image/imgTools.cpp
@@ -67,15 +67,6 @@ imgTools::DecodeImage(nsIInputStream* aInStr,
 
   NS_ENSURE_ARG_POINTER(aInStr);
 
-  // Create a new image container to hold the decoded data.
-  nsAutoCString mimeType(aMimeType);
-  RefPtr<image::Image> image = ImageFactory::CreateAnonymousImage(mimeType);
-  RefPtr<ProgressTracker> tracker = image->GetProgressTracker();
-
-  if (image->HasError()) {
-    return NS_ERROR_FAILURE;
-  }
-
   // Prepare the input stream.
   nsCOMPtr<nsIInputStream> inStream = aInStr;
   if (!NS_InputStreamIsBuffered(aInStr)) {
@@ -92,6 +83,16 @@ imgTools::DecodeImage(nsIInputStream* aInStr,
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ENSURE_TRUE(length <= UINT32_MAX, NS_ERROR_FILE_TOO_BIG);
 
+  // Create a new image container to hold the decoded data.
+  nsAutoCString mimeType(aMimeType);
+  RefPtr<image::Image> image =
+    ImageFactory::CreateAnonymousImage(mimeType, uint32_t(length));
+  RefPtr<ProgressTracker> tracker = image->GetProgressTracker();
+
+  if (image->HasError()) {
+    return NS_ERROR_FAILURE;
+  }
+
   // Send the source data to the Image.
   rv = image->OnImageDataAvailable(nullptr, nullptr, inStream, 0,
                                    uint32_t(length));