Oilpan: simplify ImageBitmapTest heap clearing.

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 128 matching lines @@
     memoryCache()->updateDecodedResource(cachedImageInteriorCrop.get(), UpdateForPropertyChange);
     memoryCache()->updateDecodedResource(cachedImageExteriorCrop.get(), UpdateForPropertyChange);
     memoryCache()->updateDecodedResource(cachedImageOutsideCrop.get(), UpdateForPropertyChange);
 
     // HTMLImageElements should default to CacheLiveResourcePriorityLow.
     ASSERT_EQ(memoryCache()->priority(imageNoCrop->cachedImage()), MemoryCacheLiveResourcePriorityLow);
     ASSERT_EQ(memoryCache()->priority(imageInteriorCrop->cachedImage()), MemoryCacheLiveResourcePriorityLow);
     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);
         ASSERT_EQ(memoryCache()->priority(imageInteriorCrop->cachedImage()), MemoryCacheLiveResourcePriorityHigh);
         ASSERT_EQ(memoryCache()->priority(imageExteriorCrop->cachedImage()), MemoryCacheLiveResourcePriorityHigh);
 
         // 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);
     ASSERT_EQ(memoryCache()->priority(imageExteriorCrop->cachedImage()), MemoryCacheLiveResourcePriorityLow);
     ASSERT_EQ(memoryCache()->priority(imageOutsideCrop->cachedImage()), MemoryCacheLiveResourcePriorityLow);
 
     // There is still an ImageBitmap that references this image.
     ASSERT_EQ(memoryCache()->priority(imageInteriorCrop->cachedImage()), MemoryCacheLiveResourcePriorityHigh);
     imageBitmapInteriorCrop = nullptr;
 }
@@ skipping 31 matching lines @@
     {
         RefPtrWillBeRawPtr<ImageBitmap> 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