Limit when Resource#getBitmap can be called

Only call Resource#getBitmap once per onResourceLoaded(), and assert
DynamicResource#getBitmap is only called when dirty.

This makes it possible to release the local Bitmap copy in
BitmapDynamicResource when it is passed to the native side.

The only user of BitmapDynamicResource is LayerTitleCache, which
releases resources explicitly itself, so this CL doesn't reduce
memory usage. However, this should make memory leak less likely
for the next user.

Bug: 965580
Change-Id: I00c3032f05dbe0cd66881cd99063ed4fb82cb0fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1626992
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664378}

ui/android/java/src/org/chromium/ui/resources/Resource.java

@@ -17,9 +17,8 @@ public interface Resource {
     /**
      * The {@link Bitmap} can only be used in
      * {@link ResourceLoader.ResourceLoaderCallback#onResourceLoaded(int, int, Resource)}, where it
-     * would be deep-copied into the CC layer, so it is encouraged to make sure we don't keep an
-     * extra copy at the Java side unnecessarily.
-     * This may be called more than once so if possible avoid doing redundant work.
+     * would be called exactly once per invocation, and deep-copied into the CC layer, so it is
+     * encouraged to make sure we don't keep an extra copy at the Java side unnecessarily.
      * @return A {@link Bitmap} representing the resource.
      */
     Bitmap getBitmap();

ui/android/java/src/org/chromium/ui/resources/ResourceManager.java

@@ -130,14 +130,16 @@ public class ResourceManager implements ResourceLoaderCallback {
     @SuppressWarnings("cast")
     @Override
     public void onResourceLoaded(int resType, int resId, Resource resource) {
-        if (resource == null || resource.getBitmap() == null) return;
+        if (resource == null) return;
+        Bitmap bitmap = resource.getBitmap();
+        if (bitmap == null) return;
 
         saveMetadataForLoadedResource(resType, resId, resource);
 
         if (mNativeResourceManagerPtr == 0) return;
 
-        nativeOnResourceReady(mNativeResourceManagerPtr, resType, resId, resource.getBitmap(),
-                resource.createNativeResource());
+        nativeOnResourceReady(
+                mNativeResourceManagerPtr, resType, resId, bitmap, resource.createNativeResource());
     }
 
     @Override

ui/android/java/src/org/chromium/ui/resources/dynamics/BitmapDynamicResource.java

@@ -13,9 +13,7 @@ import org.chromium.ui.resources.statics.NinePatchData;
 /**
  * A basic implementation of {@link DynamicResource} to handle updatable bitmaps.
  */
-public class BitmapDynamicResource implements DynamicResource {
-    private static final Rect EMPTY_RECT = new Rect();
-
+public class BitmapDynamicResource extends DynamicResource {
     private final int mResId;
     private Bitmap mBitmap;
     private final Rect mSize = new Rect();
@@ -40,15 +38,18 @@ public class BitmapDynamicResource implements DynamicResource {
         // is no bitmap to start with. See http://crbug.com/471234 for more.
         if (bitmap == null) return;
         mIsDirty = true;
-        if (mBitmap != null) mBitmap.recycle();
         mBitmap = bitmap;
         mSize.set(0, 0, mBitmap.getWidth(), mBitmap.getHeight());
     }
 
     @Override
     public Bitmap getBitmap() {
+        super.getBitmap();
+        assert mBitmap != null: "setBitmap() should be called before calling getBitmap() again";
         mIsDirty = false;
-        return mBitmap;
+        Bitmap bitmap = mBitmap;
+        mBitmap = null;
+        return bitmap;
     }
 
     @Override

ui/android/java/src/org/chromium/ui/resources/dynamics/DynamicResource.java

@@ -4,6 +4,9 @@
 
 package org.chromium.ui.resources.dynamics;
 
+import android.graphics.Bitmap;
+import android.support.annotation.CallSuper;
+
 import org.chromium.ui.resources.Resource;
 import org.chromium.ui.resources.ResourceLoader.ResourceLoaderCallback;
 
@@ -11,7 +14,19 @@ import org.chromium.ui.resources.ResourceLoader.ResourceLoaderCallback;
  * A representation of a dynamic resource.  The contents of the resource might change from frame to
  * frame.
  */
-public interface DynamicResource extends Resource {
+public abstract class DynamicResource implements Resource {
+    /**
+     * {@link DynamicResourceLoader#loadResource(int)} only notifies {@link ResourceLoaderCallback}
+     * if the resource is dirty. Therefore, if the resource is not dirty, this should not be called.
+     * @return null. This method should be overridden, and ignore the return value here.
+     */
+    @Override
+    @CallSuper
+    public Bitmap getBitmap() {
+        assert isDirty() : "getBitmap() should not be called when not dirty";
+        return null;
+    }
+
     /**
      * Note that this is called for every access to the resource during a frame.  If a resource is
      * dirty, it should not be dirty again during the same looper call.
@@ -24,5 +39,5 @@ public interface DynamicResource extends Resource {
      *
      * @return Whether or not this resource is dirty and the CC component should be rebuilt.
      */
-    boolean isDirty();
+    abstract boolean isDirty();
 }

ui/android/java/src/org/chromium/ui/resources/dynamics/ViewResourceAdapter.java

@@ -22,7 +22,7 @@ import org.chromium.ui.resources.statics.NinePatchData;
  * {@link View} are invalidated.  For {@link ViewGroup}s the easiest way to do this is to override
  * {@link View#invalidateChildInParent(int[], Rect)}.
  */
-public class ViewResourceAdapter implements DynamicResource, OnLayoutChangeListener {
+public class ViewResourceAdapter extends DynamicResource implements OnLayoutChangeListener {
     private final View mView;
     private final Rect mDirtyRect = new Rect();
 
@@ -42,12 +42,12 @@ public class ViewResourceAdapter implements DynamicResource, OnLayoutChangeListe
     /**
      * If this resource is dirty ({@link #isDirty()} returned {@code true}), it will recapture a
      * {@link Bitmap} of the {@link View}.
-     * @see {@link DynamicResource#getBitmap()}.
+     * @see DynamicResource#getBitmap()
      * @return A {@link Bitmap} representing the {@link View}.
      */
     @Override
     public Bitmap getBitmap() {
-        if (!isDirty() && mBitmap != null) return mBitmap;
+        super.getBitmap();
 
         TraceEvent.begin("ViewResourceAdapter:getBitmap");
         if (validateBitmap()) {

ui/android/junit/src/org/chromium/ui/resources/dynamics/BitmapDynamicResourceTest.java

@@ -53,4 +53,16 @@ public class BitmapDynamicResourceTest {
         mResource.setBitmap(bitmap2);
         assertTrue(isGarbageCollected(bitmapWeakReference));
     }
+
+    @Test
+    public void testGetBitmapGCed() {
+        Bitmap bitmap = Bitmap.createBitmap(1, 2, Bitmap.Config.ARGB_8888);
+        WeakReference<Bitmap> bitmapWeakReference = new WeakReference<>(bitmap);
+        mResource.setBitmap(bitmap);
+        bitmap = null;
+        assertFalse(isGarbageCollected(bitmapWeakReference));
+
+        mResource.getBitmap();
+        assertTrue(isGarbageCollected(bitmapWeakReference));
+    }
 }

ui/android/junit/src/org/chromium/ui/resources/dynamics/ViewResourceAdapterTest.java

@@ -172,6 +172,8 @@ public class ViewResourceAdapterTest {
         assertEquals(bitmap, mAdapter.getBitmap());
 
         mAdapter.dropCachedBitmap();
+        mAdapter.invalidate(null);
+        assertTrue(mAdapter.isDirty());
         assertNotEquals(bitmap, mAdapter.getBitmap());
     }