Bug 1218488 - clarify buffer ownership for nsICanvasRenderingContextInternal::GetBuffer; r=Bas,baku

This patch started life as making ImageEncoder.cpp:EncodingRunnable not
use nsAutoArrayPtr, but the API effects rippled out from there.  On the
whole, I think using UniquePtr throughout has made the code clearer.

dom/base/ImageEncoder.cpp

@@ -138,7 +138,7 @@ public:
 
   EncodingRunnable(const nsAString& aType,
                    const nsAString& aOptions,
-                   uint8_t* aImageBuffer,
+                   UniquePtr<uint8_t[]> aImageBuffer,
                    layers::Image* aImage,
                    imgIEncoder* aEncoder,
                    EncodingCompleteEvent* aEncodingCompleteEvent,
@@ -147,7 +147,7 @@ public:
                    bool aUsingCustomOptions)
     : mType(aType)
     , mOptions(aOptions)
-    , mImageBuffer(aImageBuffer)
+    , mImageBuffer(Move(aImageBuffer))
     , mImage(aImage)
     , mEncoder(aEncoder)
     , mEncodingCompleteEvent(aEncodingCompleteEvent)
@@ -161,7 +161,7 @@ public:
     nsCOMPtr<nsIInputStream> stream;
     nsresult rv = ImageEncoder::ExtractDataInternal(mType,
                                                     mOptions,
-                                                    mImageBuffer,
+                                                    mImageBuffer.get(),
                                                     mFormat,
                                                     mSize,
                                                     mImage,
@@ -175,7 +175,7 @@ public:
     if (rv == NS_ERROR_INVALID_ARG && mUsingCustomOptions) {
       rv = ImageEncoder::ExtractDataInternal(mType,
                                              EmptyString(),
-                                             mImageBuffer,
+                                             mImageBuffer.get(),
                                              mFormat,
                                              mSize,
                                              mImage,
@@ -220,7 +220,7 @@ public:
 private:
   nsAutoString mType;
   nsAutoString mOptions;
-  nsAutoArrayPtr<uint8_t> mImageBuffer;
+  UniquePtr<uint8_t[]> mImageBuffer;
   RefPtr<layers::Image> mImage;
   nsCOMPtr<imgIEncoder> mEncoder;
   RefPtr<EncodingCompleteEvent> mEncodingCompleteEvent;
@@ -287,7 +287,7 @@ nsresult
 ImageEncoder::ExtractDataAsync(nsAString& aType,
                                const nsAString& aOptions,
                                bool aUsingCustomOptions,
-                               uint8_t* aImageBuffer,
+                               UniquePtr<uint8_t[]> aImageBuffer,
                                int32_t aFormat,
                                const nsIntSize aSize,
                                EncodeCompleteCallback* aEncodeCallback)
@@ -306,7 +306,7 @@ ImageEncoder::ExtractDataAsync(nsAString& aType,
 
   nsCOMPtr<nsIRunnable> event = new EncodingRunnable(aType,
                                                      aOptions,
-                                                     aImageBuffer,
+                                                     Move(aImageBuffer),
                                                      nullptr,
                                                      encoder,
                                                      completeEvent,

dom/base/ImageEncoder.h

@@ -11,6 +11,7 @@
 #include "nsError.h"
 #include "mozilla/dom/File.h"
 #include "mozilla/dom/HTMLCanvasElementBinding.h"
+#include "mozilla/UniquePtr.h"
 #include "nsLayoutUtils.h"
 #include "nsSize.h"
 
@@ -58,7 +59,7 @@ public:
   static nsresult ExtractDataAsync(nsAString& aType,
                                    const nsAString& aOptions,
                                    bool aUsingCustomOptions,
-                                   uint8_t* aImageBuffer,
+                                   UniquePtr<uint8_t[]> aImageBuffer,
                                    int32_t aFormat,
                                    const nsIntSize aSize,
                                    EncodeCompleteCallback* aEncodeCallback);

dom/canvas/CanvasRenderingContext2D.cpp

@@ -1646,26 +1646,24 @@ CanvasRenderingContext2D::SetContextOptions(JSContext* aCx,
   return NS_OK;
 }
 
-void
+UniquePtr<uint8_t[]>
-CanvasRenderingContext2D::GetImageBuffer(uint8_t** aImageBuffer,
+CanvasRenderingContext2D::GetImageBuffer(int32_t* aFormat)
-                                         int32_t* aFormat)
 {
-  *aImageBuffer = nullptr;
   *aFormat = 0;
 
   EnsureTarget();
   RefPtr<SourceSurface> snapshot = mTarget->Snapshot();
   if (!snapshot) {
-    return;
+    return nullptr;
   }
 
   RefPtr<DataSourceSurface> data = snapshot->GetDataSurface();
   if (!data || data->GetSize() != IntSize(mWidth, mHeight)) {
-    return;
+    return nullptr;
   }
 
-  *aImageBuffer = SurfaceToPackedBGRA(data);
   *aFormat = imgIEncoder::INPUT_FORMAT_HOSTARGB;
+  return SurfaceToPackedBGRA(data);
 }
 
 nsString CanvasRenderingContext2D::GetHitRegion(const mozilla::gfx::Point& aPoint)
@@ -1691,15 +1689,15 @@ CanvasRenderingContext2D::GetInputStream(const char *aMimeType,
     return NS_ERROR_FAILURE;
   }
 
-  nsAutoArrayPtr<uint8_t> imageBuffer;
   int32_t format = 0;
-  GetImageBuffer(getter_Transfers(imageBuffer), &format);
+  UniquePtr<uint8_t[]> imageBuffer = GetImageBuffer(&format);
   if (!imageBuffer) {
     return NS_ERROR_FAILURE;
   }
 
-  return ImageEncoder::GetInputStream(mWidth, mHeight, imageBuffer, format,
+  return ImageEncoder::GetInputStream(mWidth, mHeight, imageBuffer.get(),
-                                      encoder, aEncoderOptions, aStream);
+                                      format, encoder, aEncoderOptions,
+                                      aStream);
 }
 
 SurfaceFormat

dom/canvas/CanvasRenderingContext2D.h

@@ -20,6 +20,7 @@
 #include "mozilla/dom/CanvasPattern.h"
 #include "mozilla/gfx/Rect.h"
 #include "mozilla/gfx/2D.h"
+#include "mozilla/UniquePtr.h"
 #include "gfx2DGlue.h"
 #include "imgIEncoder.h"
 #include "nsLayoutUtils.h"
@@ -526,7 +527,7 @@ public:
 
   friend class CanvasRenderingContext2DUserData;
 
-  virtual void GetImageBuffer(uint8_t** aImageBuffer, int32_t* aFormat) override;
+  virtual UniquePtr<uint8_t[]> GetImageBuffer(int32_t* aFormat) override;
 
 
   // Given a point, return hit region ID if it exists

dom/canvas/CanvasRenderingContextHelper.cpp

@@ -7,6 +7,7 @@
 #include "ImageEncoder.h"
 #include "mozilla/dom/CanvasRenderingContext2D.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/UniquePtr.h"
 #include "nsContentUtils.h"
 #include "nsDOMJSUtils.h"
 #include "nsIScriptContext.h"
@@ -49,10 +50,10 @@ CanvasRenderingContextHelper::ToBlob(JSContext* aCx,
     }
   }
 
-  uint8_t* imageBuffer = nullptr;
+  UniquePtr<uint8_t[]> imageBuffer;
   int32_t format = 0;
   if (mCurrentContext) {
-    mCurrentContext->GetImageBuffer(&imageBuffer, &format);
+    imageBuffer = mCurrentContext->GetImageBuffer(&format);
   }
 
   // Encoder callback when encoding is complete.
@@ -99,7 +100,7 @@ CanvasRenderingContextHelper::ToBlob(JSContext* aCx,
   aRv = ImageEncoder::ExtractDataAsync(type,
                                        params,
                                        usingCustomParseOptions,
-                                       imageBuffer,
+                                       Move(imageBuffer),
                                        format,
                                        GetWidthHeight(),
                                        callback);

dom/canvas/WebGLContext.cpp

@@ -1052,25 +1052,25 @@ WebGLContext::LoseOldestWebGLContextIfLimitExceeded()
     }
 }
 
-void
+UniquePtr<uint8_t[]>
-WebGLContext::GetImageBuffer(uint8_t** out_imageBuffer, int32_t* out_format)
+WebGLContext::GetImageBuffer(int32_t* out_format)
 {
-    *out_imageBuffer = nullptr;
     *out_format = 0;
 
     // Use GetSurfaceSnapshot() to make sure that appropriate y-flip gets applied
     bool premult;
     RefPtr<SourceSurface> snapshot =
       GetSurfaceSnapshot(mOptions.premultipliedAlpha ? nullptr : &premult);
-    if (!snapshot)
+    if (!snapshot) {
-        return;
+        return nullptr;
+    }
 
     MOZ_ASSERT(mOptions.premultipliedAlpha || !premult, "We must get unpremult when we ask for it!");
 
     RefPtr<DataSourceSurface> dataSurface = snapshot->GetDataSurface();
 
     return gfxUtils::GetImageBuffer(dataSurface, mOptions.premultipliedAlpha,
-                                    out_imageBuffer, out_format);
+                                    out_format);
 }
 
 NS_IMETHODIMP

dom/canvas/WebGLContext.h

@@ -237,8 +237,7 @@ public:
         return NS_ERROR_NOT_IMPLEMENTED;
     }
 
-    virtual void GetImageBuffer(uint8_t** out_imageBuffer,
+    virtual UniquePtr<uint8_t[]> GetImageBuffer(int32_t* out_format) override;
-                                int32_t* out_format) override;
     NS_IMETHOD GetInputStream(const char* mimeType,
                               const char16_t* encoderOptions,
                               nsIInputStream** out_stream) override;

dom/canvas/nsICanvasRenderingContextInternal.h

@@ -14,6 +14,7 @@
 #include "mozilla/dom/HTMLCanvasElement.h"
 #include "mozilla/dom/OffscreenCanvas.h"
 #include "mozilla/RefPtr.h"
+#include "mozilla/UniquePtr.h"
 
 #define NS_ICANVASRENDERINGCONTEXTINTERNAL_IID \
 { 0xb84f2fed, 0x9d4b, 0x430b, \
@@ -96,7 +97,7 @@ public:
   NS_IMETHOD InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, int32_t width, int32_t height) = 0;
 
   // Creates an image buffer. Returns null on failure.
-  virtual void GetImageBuffer(uint8_t** imageBuffer, int32_t* format) = 0;
+  virtual mozilla::UniquePtr<uint8_t[]> GetImageBuffer(int32_t* format) = 0;
 
   // Gives you a stream containing the image represented by this context.
   // The format is given in mimeTime, for example "image/png".

gfx/2d/DataSurfaceHelpers.cpp

@@ -110,7 +110,7 @@ CopyBGRXSurfaceDataToPackedBGRArray(uint8_t* aSrc, uint8_t* aDst,
   }
 }
 
-uint8_t*
+UniquePtr<uint8_t[]>
 SurfaceToPackedBGRA(DataSourceSurface *aSurface)
 {
   SurfaceFormat format = aSurface->GetFormat();
@@ -120,25 +120,25 @@ SurfaceToPackedBGRA(DataSourceSurface *aSurface)
 
   IntSize size = aSurface->GetSize();
 
-  uint8_t* imageBuffer = new (std::nothrow) uint8_t[size.width * size.height * sizeof(uint32_t)];
+  UniquePtr<uint8_t[]> imageBuffer(
+    new (std::nothrow) uint8_t[size.width * size.height * sizeof(uint32_t)]);
   if (!imageBuffer) {
     return nullptr;
   }
 
   DataSourceSurface::MappedSurface map;
   if (!aSurface->Map(DataSourceSurface::MapType::READ, &map)) {
-    delete [] imageBuffer;
     return nullptr;
   }
 
-  CopySurfaceDataToPackedArray(map.mData, imageBuffer, size,
+  CopySurfaceDataToPackedArray(map.mData, imageBuffer.get(), size,
                                map.mStride, 4 * sizeof(uint8_t));
 
   aSurface->Unmap();
 
   if (format == SurfaceFormat::B8G8R8X8) {
     // Convert BGRX to BGRA by setting a to 255.
-    ConvertBGRXToBGRA(reinterpret_cast<uint8_t *>(imageBuffer), size, size.width * sizeof(uint32_t));
+    ConvertBGRXToBGRA(imageBuffer.get(), size, size.width * sizeof(uint32_t));
   }
 
   return imageBuffer;

gfx/2d/DataSurfaceHelpers.h

@@ -8,6 +8,8 @@
 
 #include "2D.h"
 
+#include "mozilla/UniquePtr.h"
+
 namespace mozilla {
 namespace gfx {
 
@@ -25,11 +27,9 @@ CopySurfaceDataToPackedArray(uint8_t* aSrc, uint8_t* aDst, IntSize aSrcSize,
                              int32_t aSrcStride, int32_t aBytesPerPixel);
 
 /**
- * Convert aSurface to a packed buffer in BGRA format. The pixel data is
+ * Convert aSurface to a packed buffer in BGRA format.
- * returned in a buffer allocated with new uint8_t[]. The caller then has
- * ownership of the buffer and is responsible for delete[]'ing it.
  */
-uint8_t*
+UniquePtr<uint8_t[]>
 SurfaceToPackedBGRA(DataSourceSurface *aSurface);
 
 /**

gfx/thebes/gfxUtils.cpp

@@ -1547,26 +1547,24 @@ gfxUtils::CopyAsDataURI(DrawTarget* aDT)
   }
 }
 
-/* static */ void
+/* static */ UniquePtr<uint8_t[]>
 gfxUtils::GetImageBuffer(gfx::DataSourceSurface* aSurface,
                          bool aIsAlphaPremultiplied,
-                         uint8_t** outImageBuffer,
                          int32_t* outFormat)
 {
-    *outImageBuffer = nullptr;
     *outFormat = 0;
 
     DataSourceSurface::MappedSurface map;
     if (!aSurface->Map(DataSourceSurface::MapType::READ, &map))
-        return;
+        return nullptr;
 
     uint32_t bufferSize = aSurface->GetSize().width * aSurface->GetSize().height * 4;
-    uint8_t* imageBuffer = new (fallible) uint8_t[bufferSize];
+    UniquePtr<uint8_t[]> imageBuffer(new (fallible) uint8_t[bufferSize]);
     if (!imageBuffer) {
         aSurface->Unmap();
-        return;
+        return nullptr;
     }
-    memcpy(imageBuffer, map.mData, bufferSize);
+    memcpy(imageBuffer.get(), map.mData, bufferSize);
 
     aSurface->Unmap();
 
@@ -1577,12 +1575,12 @@ gfxUtils::GetImageBuffer(gfx::DataSourceSurface* aSurface,
         // Yes, it is THAT silly.
         // Except for different lossy conversions by color,
         // we could probably just change the label, and not change the data.
-        gfxUtils::ConvertBGRAtoRGBA(imageBuffer, bufferSize);
+        gfxUtils::ConvertBGRAtoRGBA(imageBuffer.get(), bufferSize);
         format = imgIEncoder::INPUT_FORMAT_RGBA;
     }
 
-    *outImageBuffer = imageBuffer;
     *outFormat = format;
+    return imageBuffer;
 }
 
 /* static */ nsresult
@@ -1598,15 +1596,14 @@ gfxUtils::GetInputStream(gfx::DataSourceSurface* aSurface,
     if (!encoder)
         return NS_ERROR_FAILURE;
 
-    nsAutoArrayPtr<uint8_t> imageBuffer;
     int32_t format = 0;
-    GetImageBuffer(aSurface, aIsAlphaPremultiplied, getter_Transfers(imageBuffer), &format);
+    UniquePtr<uint8_t[]> imageBuffer = GetImageBuffer(aSurface, aIsAlphaPremultiplied, &format);
     if (!imageBuffer)
         return NS_ERROR_FAILURE;
 
     return dom::ImageEncoder::GetInputStream(aSurface->GetSize().width,
                                              aSurface->GetSize().height,
-                                             imageBuffer, format,
+                                             imageBuffer.get(), format,
                                              encoder, aEncoderOptions, outStream);
 }
 

gfx/thebes/gfxUtils.h

@@ -10,6 +10,7 @@
 #include "imgIContainer.h"
 #include "mozilla/gfx/2D.h"
 #include "mozilla/RefPtr.h"
+#include "mozilla/UniquePtr.h"
 #include "nsColor.h"
 #include "nsPrintfCString.h"
 #include "mozilla/gfx/Rect.h"
@@ -282,10 +283,9 @@ public:
     static nsCString GetAsDataURI(DrawTarget* aDT);
     static nsCString GetAsLZ4Base64Str(DataSourceSurface* aSourceSurface);
 
-    static void GetImageBuffer(DataSourceSurface* aSurface,
+    static mozilla::UniquePtr<uint8_t[]> GetImageBuffer(DataSourceSurface* aSurface,
-                               bool aIsAlphaPremultiplied,
+                                                        bool aIsAlphaPremultiplied,
-                               uint8_t** outImageBuffer,
+                                                        int32_t* outFormat);
-                               int32_t* outFormat);
 
     static nsresult GetInputStream(DataSourceSurface* aSurface,
                                    bool aIsAlphaPremultiplied,

widget/windows/WinUtils.cpp

@@ -1212,7 +1212,7 @@ AsyncFaviconDataReady::OnComplete(nsIURI *aFaviconURI,
 
   // Allocate a new buffer that we own and can use out of line in
   // another thread.
-  uint8_t *data = SurfaceToPackedBGRA(dataSurface);
+  UniquePtr<uint8_t[]> data = SurfaceToPackedBGRA(dataSurface);
   if (!data) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
@@ -1220,7 +1220,7 @@ AsyncFaviconDataReady::OnComplete(nsIURI *aFaviconURI,
   int32_t dataLength = stride * size.height;
 
   // AsyncEncodeAndWriteIcon takes ownership of the heap allocated buffer
-  nsCOMPtr<nsIRunnable> event = new AsyncEncodeAndWriteIcon(path, data,
+  nsCOMPtr<nsIRunnable> event = new AsyncEncodeAndWriteIcon(path, Move(data),
                                                             dataLength,
                                                             stride,
                                                             size.width,
@@ -1234,7 +1234,7 @@ AsyncFaviconDataReady::OnComplete(nsIURI *aFaviconURI,
 
 // Warning: AsyncEncodeAndWriteIcon assumes ownership of the aData buffer passed in
 AsyncEncodeAndWriteIcon::AsyncEncodeAndWriteIcon(const nsAString &aIconPath,
-                                                 uint8_t *aBuffer,
+                                                 UniquePtr<uint8_t[]> aBuffer,
                                                  uint32_t aBufferLength,
                                                  uint32_t aStride,
                                                  uint32_t aWidth,
@@ -1242,7 +1242,7 @@ AsyncEncodeAndWriteIcon::AsyncEncodeAndWriteIcon(const nsAString &aIconPath,
                                                  const bool aURLShortcut) :
   mURLShortcut(aURLShortcut),
   mIconPath(aIconPath),
-  mBuffer(aBuffer),
+  mBuffer(Move(aBuffer)),
   mBufferLength(aBufferLength),
   mStride(aStride),
   mWidth(aWidth),
@@ -1257,7 +1257,7 @@ NS_IMETHODIMP AsyncEncodeAndWriteIcon::Run()
   // Note that since we're off the main thread we can't use
   // gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget()
   RefPtr<DataSourceSurface> surface =
-    Factory::CreateWrappingDataSourceSurface(mBuffer, mStride,
+    Factory::CreateWrappingDataSourceSurface(mBuffer.get(), mStride,
                                              IntSize(mWidth, mHeight),
                                              SurfaceFormat::B8G8R8A8);
 

widget/windows/WinUtils.h

@@ -36,6 +36,7 @@
 
 #include "mozilla/Attributes.h"
 #include "mozilla/EventForwards.h"
+#include "mozilla/UniquePtr.h"
 
 /**
  * NS_INLINE_DECL_IUNKNOWN_REFCOUNTING should be used for defining and
@@ -459,15 +460,15 @@ public:
 
   // Warning: AsyncEncodeAndWriteIcon assumes ownership of the aData buffer passed in
   AsyncEncodeAndWriteIcon(const nsAString &aIconPath,
-                          uint8_t *aData, uint32_t aDataLen, uint32_t aStride,
+                          UniquePtr<uint8_t[]> aData, uint32_t aDataLen,
-                          uint32_t aWidth, uint32_t aHeight,
+                          uint32_t aStride, uint32_t aWidth, uint32_t aHeight,
                           const bool aURLShortcut);
 
 private:
   virtual ~AsyncEncodeAndWriteIcon();
 
   nsAutoString mIconPath;
-  nsAutoArrayPtr<uint8_t> mBuffer;
+  UniquePtr<uint8_t[]> mBuffer;
   HMODULE sDwmDLL;
   uint32_t mBufferLength;
   uint32_t mStride;

widget/windows/nsWindowGfx.cpp

@@ -640,7 +640,7 @@ nsresult nsWindowGfx::CreateIcon(imgIContainer *aContainer,
   MOZ_ASSERT(dataSurface->GetFormat() == SurfaceFormat::B8G8R8A8);
 
   uint8_t* data = nullptr;
-  nsAutoArrayPtr<uint8_t> autoDeleteArray;
+  UniquePtr<uint8_t[]> autoDeleteArray;
   if (map.mStride == BytesPerPixel(dataSurface->GetFormat()) * iconSize.width) {
     // Mapped data is already packed
     data = map.mData;
@@ -653,7 +653,8 @@ nsresult nsWindowGfx::CreateIcon(imgIContainer *aContainer,
     dataSurface->Unmap();
     map.mData = nullptr;
 
-    data = autoDeleteArray = SurfaceToPackedBGRA(dataSurface);
+    autoDeleteArray = SurfaceToPackedBGRA(dataSurface);
+    data = autoDeleteArray.get();
     NS_ENSURE_TRUE(data, NS_ERROR_FAILURE);
   }