Oilpan: simplify ImageBitmapTest heap clearing.

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 fecdbca702074224c7fe378703f0d61e91d0742c..b9ca50f72f23513763b24e167f4311c0134bb5b4 100644
--- a/Source/core/frame/ImageBitmapTest.cpp
+++ b/Source/core/frame/ImageBitmapTest.cpp
@@ -146,12 +146,12 @@ TEST_F(ImageBitmapTest, ImageBitmapLiveResourcePriority)
     ASSERT_EQ(memoryCache()->priority(imageExteriorCrop->cachedImage()), MemoryCacheLiveResourcePriorityLow);
     ASSERT_EQ(memoryCache()->priority(imageOutsideCrop->cachedImage()), MemoryCacheLiveResourcePriorityLow);
 
-    RefPtrWillBeRawPtr<ImageBitmap> imageBitmapInteriorCrop = ImageBitmap::create(imageInteriorCrop.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
+    RefPtrWillBePersistent<ImageBitmap> imageBitmapInteriorCrop = ImageBitmap::create(imageInteriorCrop.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
     {
-        RefPtrWillBeRawPtr<ImageBitmap> imageBitmapNoCrop = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height()));
-        RefPtrWillBeRawPtr<ImageBitmap> imageBitmapInteriorCrop2 = ImageBitmap::create(imageInteriorCrop.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
-        RefPtrWillBeRawPtr<ImageBitmap> imageBitmapExteriorCrop = ImageBitmap::create(imageExteriorCrop.get(), IntRect(-m_bitmap.width() / 2, -m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
-        RefPtrWillBeRawPtr<ImageBitmap> imageBitmapOutsideCrop = ImageBitmap::create(imageOutsideCrop.get(), IntRect(-m_bitmap.width(), -m_bitmap.height(), m_bitmap.width(), m_bitmap.height()));
+        RefPtrWillBeRawPtr<ImageBitmap> imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height()));

[Mads Ager (chromium), 2014/05/19 14:22:47]

I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather silly if
the test doesn't check anything in release mode?

[sof, 2014/05/19 14:33:29]

While ASSERT_EQ() may hide some other, later failures in this test, isn't this
ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) ?

[Mads Ager (chromium), 2014/05/19 15:00:48]

I don't really care much about the distinction on that page. However, it seems
that there is another difference. These variables are definitely not unused.
They are used in the code that checks below. I believe that EXPECT_EQ will check
in release mode as well which would allow us to get rid of the ALLOW_UNUSED on
these variables that should not be unused in this test. I believe we had similar
issues in the heap_unit_tests which was the main reason for switching to using
EXPECT_EQ. 

[sof, 2014/05/19 15:10:10]

ok, i see. The variables are unused (imageBitmapNoCrop vs imageNoCrop, for
instance.)

[Mads Ager (chromium), 2014/05/19 15:15:02]

d'oh, yes, I misread that.

That raises another concern though: if these are unused, there is nothing that
guarantees that they will stay alive with Oilpan. The compiler is free to not
have those variables on the stack, nor in registers. Therefore, these unused
objects can die at one of the next allocations. So, maybe these should really be
in Persistents if it is important that they stay alive for this scope? That
could also be a cause of the flakiness. :)

[sof, 2014/05/19 15:17:04]

That could be, but the failure reported is outside this block & suggesting that
they haven't been collected.

+        RefPtrWillBeRawPtr<ImageBitmap> imageBitmapInteriorCrop2 ALLOW_UNUSED = ImageBitmap::create(imageInteriorCrop.get(), IntRect(m_bitmap.width() / 2, m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
+        RefPtrWillBeRawPtr<ImageBitmap> imageBitmapExteriorCrop ALLOW_UNUSED = ImageBitmap::create(imageExteriorCrop.get(), IntRect(-m_bitmap.width() / 2, -m_bitmap.height() / 2, m_bitmap.width(), m_bitmap.height()));
+        RefPtrWillBeRawPtr<ImageBitmap> imageBitmapOutsideCrop ALLOW_UNUSED = 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(memoryCache()->priority(imageNoCrop->cachedImage()), MemoryCacheLiveResourcePriorityHigh);
@@ -160,16 +160,9 @@ TEST_F(ImageBitmapTest, ImageBitmapLiveResourcePriority)
 
         // ImageBitmaps that do not contain any of the source image do not elevate CacheLiveResourcePriority.
         ASSERT_EQ(memoryCache()->priority(imageOutsideCrop->cachedImage()), MemoryCacheLiveResourcePriorityLow);
-
-        // Stub out references to the ImageBitmaps created and force a
-        // garbage collection to have the ImageBitmaps be collected and
-        // destructed.
-        imageBitmapNoCrop = nullptr;
-        imageBitmapInteriorCrop2 = nullptr;
-        imageBitmapExteriorCrop = nullptr;
-        imageBitmapOutsideCrop = nullptr;
-        Heap::collectGarbage(ThreadState::HeapPointersOnStack);
     }
+    // Force a garbage collection to sweep out the local ImageBitmaps.
+    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
 
     // CacheLiveResourcePriroity should return to CacheLiveResourcePriorityLow when no ImageBitmaps reference the image.
     ASSERT_EQ(memoryCache()->priority(imageNoCrop->cachedImage()), MemoryCacheLiveResourcePriorityLow);