Oilpan: move ImageBitmap to the oilpan heap.

Source/core/frame/ImageBitmapTest.cpp

Index: Source/core/frame/ImageBitmapTest.cpp
diff --git a/Source/core/frame/ImageBitmapTest.cpp b/Source/core/frame/ImageBitmapTest.cpp
index 6a6626ae7ab089d693e780761ea1fe4570191f76..acf6fc177d835b6cd41c3e5c3bf1ceec3e4bd553 100644
--- a/Source/core/frame/ImageBitmapTest.cpp
+++ b/Source/core/frame/ImageBitmapTest.cpp
@@ -81,6 +81,48 @@ protected:
     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)
@@ -88,10 +130,10 @@ 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());
@@ -142,12 +184,12 @@ TEST_F(ImageBitmapTest, ImageBitmapLiveResourcePriority)
     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);
@@ -174,7 +216,7 @@ TEST_F(ImageBitmapTest, ImageBitmapSourceChanged)
     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());
@@ -196,9 +238,9 @@ 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");