Oilpan: move ImageBitmap to the oilpan heap.

Source/core/frame/ImageBitmapTest.cpp

 /*
  * Copyright (c) 2013, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 63 matching lines @@
         m_testingMemoryCache = adoptPtr(memoryCache());
         // Yield the ownership of the global memory cache back.
         setMemoryCacheForTesting(m_globalMemoryCache.leakPtr());
     }
 
     SkBitmap m_bitmap, m_bitmap2;
     OwnPtr<MemoryCache> m_testingMemoryCache;
     OwnPtr<MemoryCache> m_globalMemoryCache;
 };
 
+// SetUp() installs a memory cache per test, which means the image
+// resources that a test adds has to be removed before the cache is
+// torn down and replaced. As a consequence of ImageBitmaps being
+// allocated on a GCed heap (with Oilpan enabled), we have to
+// explicitly detach their resources upon test completion to achieve
+// such timely removal. The AutoImageBitmap wrapper class takes
+// care of this.
+//
+// NOTE: if a test creates ImageBitmaps, it must involve this wrapper
+// class, unless it explicitly calls detach(). If not, expect to see
+// asserts when a ImageBitmap is later on GCed and it tries to remove
+// itself from another MemoryCache.
+//
+class AutoImageBitmap {

[haraken, 2014/03/10 15:25:03]

I'm not fully convinced why we need AutoImageBitmap.

Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert
Heap::collectGarbage() to proper places to guarantee that the image resources
are collected before the memory cache is torn down?

[sof, 2014/03/10 15:55:57]

I considered triggering a GC for each test too blunt an instrument.

[haraken, 2014/03/10 16:04:55]

We were doing that in the experimental branch.

Mads: What do you think?

[sof, 2014/03/10 21:07:21]

I see; if preferable, I'll switch over of course.

[sof, 2014/03/12 10:02:25]

Need to make a choice here between:

 - The test TearDown()s perform a garbage collection.
 - A test explicitly uses AutoImageBitmap to release any ImageBitmaps & have
them be evicted from the test's memory cache in a timely manner.

[Erik Corry, 2014/03/12 10:06:15]

My feeling is that an explicit call to GC in the tear-down is closer to the code
path in the finished product, so that's the way we want to test it.

[haraken, 2014/03/12 10:34:09]

Agreed.

+    STACK_ALLOCATED();
+public:
+    AutoImageBitmap()
+    {
+    }
+
+    AutoImageBitmap(const PassRefPtrWillBeRawPtr<ImageBitmap>& imageBitmap)
+    {
+        m_imageBitmap = imageBitmap;
+    }
+
+    AutoImageBitmap(const AutoImageBitmap& other)
+    {
+        m_imageBitmap = other.m_imageBitmap;
+    }
+
+    ~AutoImageBitmap()
+    {
+        m_imageBitmap->detach();

[haraken, 2014/03/10 15:25:03]

This looks dangerous. AutoImageBitmap is on-heap (Here "on-heap" means that
"it's managed by oilpan's GC"). ImageBitmap is also on-heap. Thus
AutoImageBitmap's destructor shouldn't touch ImageBitmap.

[sof, 2014/03/10 15:55:57]

It's on the stack and not finalized; what is the concern?

[haraken, 2014/03/10 16:04:55]

Sorry, you're right.

+    }
+
+    ImageBitmap* get() const { return m_imageBitmap.get(); }
+    RefPtrWillBeRawPtr<ImageBitmap> operator->() const { return m_imageBitmap; }
+
+private:
+    RefPtrWillBeMember<ImageBitmap> m_imageBitmap;

[haraken, 2014/03/10 15:25:03]

This can be RefPtrWillBeRawPtr because AutoImageBitmap is stack allocated.

[sof, 2014/03/10 21:07:21]

I've switched to that representation, but am curious why Member is being
avoided.

[Erik Corry, 2014/03/10 21:18:56]

I'm not sure, actually.

For bare stack allocated variables I think it's fine to use RawPtr.  It turns
out that even trivial 'smart' pointers are code generated worse, especially on
32 bit targets.  This is to do with the old ABIs where structs/classes are not
passed as efficiently as raw pointers.  Therefore, and because it is less
verbose, we are going to end up with just "ImageBitmap*". RawPtr is what we have
now so that it ends up with just the * type.

For something inside an on-stack struct the first issue does not apply, and I
think there is a nice documenting effect of using Member and I'd be inclined to
keep it that way.

+};
+
 // Verifies that the image resource held by an ImageBitmap is the same as the
 // one held by the HTMLImageElement.
 TEST_F(ImageBitmapTest, ImageResourceConsistency)
 {
     RefPtr<HTMLImageElement> imageElement = HTMLImageElement::create(*Document::create().get());
     imageElement->setImageResource(new ImageResource(BitmapImage::create(NativeImageSkia::create(m_bitmap)).get()));
 
-    RefPtr<ImageBitmap> imageBitmapNoCrop = ImageBitmap::create(imageElement.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height()));
-    RefPtr<ImageBitmap> imageBitmapInteriorCrop = ImageBitmap::create(imageElement.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width() / 2, m_bitmap.height() / 2));
-    RefPtr<ImageBitmap> imageBitmapExteriorCrop = ImageBitmap::create(imageElement.get(), IntRect(-m_bitmap.width() / 2, -m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
-    RefPtr<ImageBitmap> imageBitmapOutsideCrop = ImageBitmap::create(imageElement.get(), IntRect(-m_bitmap.width(), -m_bitmap.height(), m_bitmap.width(), m_bitmap.height()));
+    AutoImageBitmap imageBitmapNoCrop = ImageBitmap::create(imageElement.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height()));
+    AutoImageBitmap imageBitmapInteriorCrop = ImageBitmap::create(imageElement.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width() / 2, m_bitmap.height() / 2));
+    AutoImageBitmap imageBitmapExteriorCrop = ImageBitmap::create(imageElement.get(), IntRect(-m_bitmap.width() / 2, -m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
+    AutoImageBitmap imageBitmapOutsideCrop = ImageBitmap::create(imageElement.get(), IntRect(-m_bitmap.width(), -m_bitmap.height(), m_bitmap.width(), m_bitmap.height()));
 
     ASSERT_EQ(imageBitmapNoCrop->bitmapImage().get(), imageElement->cachedImage()->image());
     ASSERT_EQ(imageBitmapInteriorCrop->bitmapImage().get(), imageElement->cachedImage()->image());
     ASSERT_EQ(imageBitmapExteriorCrop->bitmapImage().get(), imageElement->cachedImage()->image());
 
     RefPtr<Image> emptyImage = imageBitmapOutsideCrop->bitmapImage();
     ASSERT_NE(emptyImage.get(), imageElement->cachedImage()->image());
 }
 
 // Verifies that HTMLImageElements are given an elevated CacheLiveResourcePriority when used to construct an ImageBitmap.
@@ skipping 30 matching lines @@
     memoryCache()->insertInLiveDecodedResourcesList(cachedImageInteriorCrop.get());
     memoryCache()->insertInLiveDecodedResourcesList(cachedImageExteriorCrop.get());
     memoryCache()->insertInLiveDecodedResourcesList(cachedImageOutsideCrop.get());
 
     // HTMLImageElements should default to CacheLiveResourcePriorityLow.
     ASSERT_EQ(imageNoCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityLow);
     ASSERT_EQ(imageInteriorCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityLow);
     ASSERT_EQ(imageExteriorCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityLow);
     ASSERT_EQ(imageOutsideCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityLow);
 
-    RefPtr<ImageBitmap> imageBitmapInteriorCrop = ImageBitmap::create(imageInteriorCrop.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
+    AutoImageBitmap imageBitmapInteriorCrop = ImageBitmap::create(imageInteriorCrop.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
     {
-        RefPtr<ImageBitmap> imageBitmapNoCrop = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height()));
-        RefPtr<ImageBitmap> imageBitmapInteriorCrop2 = ImageBitmap::create(imageInteriorCrop.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
-        RefPtr<ImageBitmap> imageBitmapExteriorCrop = ImageBitmap::create(imageExteriorCrop.get(), IntRect(-m_bitmap.width() / 2, -m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
-        RefPtr<ImageBitmap> imageBitmapOutsideCrop = ImageBitmap::create(imageOutsideCrop.get(), IntRect(-m_bitmap.width(), -m_bitmap.height(), m_bitmap.width(), m_bitmap.height()));
+        AutoImageBitmap imageBitmapNoCrop = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height()));
+        AutoImageBitmap imageBitmapInteriorCrop2 = ImageBitmap::create(imageInteriorCrop.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
+        AutoImageBitmap imageBitmapExteriorCrop = ImageBitmap::create(imageExteriorCrop.get(), IntRect(-m_bitmap.width() / 2, -m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
+        AutoImageBitmap imageBitmapOutsideCrop = ImageBitmap::create(imageOutsideCrop.get(), IntRect(-m_bitmap.width(), -m_bitmap.height(), m_bitmap.width(), m_bitmap.height()));
 
         // Images that are referenced by ImageBitmaps have CacheLiveResourcePriorityHigh.
         ASSERT_EQ(imageNoCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityHigh);
         ASSERT_EQ(imageInteriorCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityHigh);
         ASSERT_EQ(imageExteriorCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityHigh);
 
         // ImageBitmaps that do not contain any of the source image do not elevate CacheLiveResourcePriority.
         ASSERT_EQ(imageOutsideCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityLow);
     }
 
     // CacheLiveResourcePriroity should return to CacheLiveResourcePriorityLow when no ImageBitmaps reference the image.
     ASSERT_EQ(imageNoCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityLow);
     ASSERT_EQ(imageExteriorCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityLow);
     ASSERT_EQ(imageOutsideCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityLow);
 
     // There is still an ImageBitmap that references this image.
     ASSERT_EQ(imageInteriorCrop->cachedImage()->cacheLiveResourcePriority(), Resource::CacheLiveResourcePriorityHigh);
 }
 
 // Verifies that ImageBitmaps constructed from HTMLImageElements hold a reference to the original Image if the HTMLImageElement src is changed.
 TEST_F(ImageBitmapTest, ImageBitmapSourceChanged)
 {
     RefPtr<HTMLImageElement> image = HTMLImageElement::create(*Document::create().get());
     ResourcePtr<ImageResource> originalImageResource = new ImageResource(BitmapImage::create(NativeImageSkia::create(m_bitmap)).get());
     image->setImageResource(originalImageResource.get());
 
-    RefPtr<ImageBitmap> imageBitmap = ImageBitmap::create(image.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height()));
+    AutoImageBitmap imageBitmap = ImageBitmap::create(image.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height()));
     ASSERT_EQ(imageBitmap->bitmapImage().get(), originalImageResource->image());
 
     ResourcePtr<ImageResource> newImageResource = new ImageResource(BitmapImage::create(NativeImageSkia::create(m_bitmap2)).get());
     image->setImageResource(newImageResource.get());
 
     // The ImageBitmap should contain the same data as the original cached image but should no longer hold a reference.
     ASSERT_NE(imageBitmap->bitmapImage().get(), originalImageResource->image());
     ASSERT_EQ(imageBitmap->bitmapImage()->nativeImageForCurrentFrame()->bitmap().pixelRef()->pixels(),
         originalImageResource->image()->nativeImageForCurrentFrame()->bitmap().pixelRef()->pixels());
 
     ASSERT_NE(imageBitmap->bitmapImage().get(), newImageResource->image());
     ASSERT_NE(imageBitmap->bitmapImage()->nativeImageForCurrentFrame()->bitmap().pixelRef()->pixels(),
         newImageResource->image()->nativeImageForCurrentFrame()->bitmap().pixelRef()->pixels());
 }
 
 // Verifies that ImageBitmaps constructed from ImageBitmaps hold onto their own Image.
 TEST_F(ImageBitmapTest, ImageResourceLifetime)
 {
     RefPtr<HTMLCanvasElement> canvasElement = HTMLCanvasElement::create(*Document::create().get());
     canvasElement->setHeight(40);
     canvasElement->setWidth(40);
-    RefPtr<ImageBitmap> imageBitmapDerived;
+    AutoImageBitmap imageBitmapDerived;
     {
-        RefPtr<ImageBitmap> imageBitmapFromCanvas = ImageBitmap::create(canvasElement.get(), IntRect(0, 0, canvasElement->width(), canvasElement->height()));
+        AutoImageBitmap imageBitmapFromCanvas = ImageBitmap::create(canvasElement.get(), IntRect(0, 0, canvasElement->width(), canvasElement->height()));
         imageBitmapDerived = ImageBitmap::create(imageBitmapFromCanvas.get(), IntRect(0, 0, 20, 20));
     }
     CanvasRenderingContext* context = canvasElement->getContext("2d");
     TrackExceptionState exceptionState;
     toCanvasRenderingContext2D(context)->drawImage(imageBitmapDerived.get(), 0, 0, exceptionState);
 }
 
 } // namespace