Fix a SkImage leak in ImageBitmap:cropImage()

third_party/WebKit/Source/core/frame/ImageBitmap.cpp

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/frame/ImageBitmap.h"
 
 #include "core/html/HTMLCanvasElement.h"
 #include "core/html/HTMLVideoElement.h"
 #include "core/html/ImageData.h"
 #include "platform/graphics/skia/SkiaUtils.h"
@@ skipping 129 matching lines @@
             return nullptr;
         decoder->setData(image->data(), true);
         skiaImage = ImageBitmap::getSkImageFromDecoder(std::move(decoder));
         if (!skiaImage)
             return nullptr;
     }
 
     if (cropRect == srcRect) {
         if (flipY)
             return StaticBitmapImage::create(flipSkImageVertically(skiaImage->newSubset(srcRect), premultiplyAlpha ? PremultiplyAlpha : DontPremultiplyAlpha));
-        SkImage* croppedSkImage = skiaImage->newSubset(srcRect);
+        RefPtr<SkImage> croppedSkImage = fromSkSp(skiaImage->makeSubset(srcRect));
         // Special case: The first parameter image is unpremul but we need to turn it into premul.
         if (premultiplyAlpha && imageFormat == DontPremultiplyAlpha)
-            return StaticBitmapImage::create(unPremulSkImageToPremul(croppedSkImage));
+            return StaticBitmapImage::create(unPremulSkImageToPremul(croppedSkImage.get()));
         // Call preroll to trigger image decoding.
         croppedSkImage->preroll();

[f(malita), 2016/06/15 08:41:54]

Unrelated to this CL: I think I understand why we want to preroll before
resolving the promise (http://crbug.com/580202), but I'm a bit concerned about
doing it in this place.  cropImage is called from most ImageBitmap constructors
- isn't this potentially clogging the main thread, when the ImageBitmap is
unrelated to a promise?

It seems a safer place to force the decode would be ImageBitmapLoader, and only
when resolving a promise.  WDYT?

[xidachen, 2016/06/15 08:56:11]

The problem is that ImageBitmapLoader is not always used. For example, when
createImageBitmap from an HTMLImageElement, it doesn't use the
ImageBitmapLoader, but calls the constructor of ImageBitmap(HTMLImageElement)
directly which jumps to cropImage.

Is there a way to check whether the croppedSkImage is always decoded or not? So
that we can call preroll() only in the case that it is not decoded yet?

[xidachen, 2016/06/15 08:57:29]

sorry, I meant is there a way to check whether the croppedSkImage is *already*
decoded or not?

-        return StaticBitmapImage::create(adoptRef(croppedSkImage));
+        return StaticBitmapImage::create(croppedSkImage.release());
     }
 
     sk_sp<SkSurface> surface = SkSurface::MakeRasterN32Premul(cropRect.width(), cropRect.height());
     if (!surface)
         return nullptr;
     if (srcRect.isEmpty())
         return StaticBitmapImage::create(adoptRef(surface->newImageSnapshot()));
 
     SkScalar dstLeft = std::min(0, -cropRect.x());
     SkScalar dstTop = std::min(0, -cropRect.y());
@@ skipping 345 matching lines @@
 FloatSize ImageBitmap::elementSize(const FloatSize&) const
 {
     return FloatSize(width(), height());
 }
 
 DEFINE_TRACE(ImageBitmap)
 {
 }
 
 } // namespace blink