Fix memory leak in TabGridViewHolder
http://crrev.com/c/1595128 (Release thumbnail when TabGridView is
recycled) tried to fix the memory leak in TabGridView, but
http://crrev.com/c/1595292 (Fix memory leak in RoundedCornerImageView)
didn't fully fix the memory leak in RoundedCornerImageView (see
issue 969245), so the memory leak issue in Grid Tab Switcher (GTS)
turned out to be still there.
This CL avoids using setImageResource(0) in GTS, and uses
setImageDrawable(null) along with specified fill color instead.
TBR=dtrainor@chromium.org
(cherry picked from commit 5fd063948aa151756ad1599e57bce9c5e9c8f77f)
Bug: 970073
Change-Id: Iec97ea2b9ec2925f34a4077e4a7069973e2a2b78
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1642161
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#666183}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1660050
Cr-Commit-Position: refs/branch-heads/3809@{#422}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}
diff --git a/base/BUILD.gn b/base/BUILD.gn
index 0f01a6a6..610e69e 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -3293,6 +3293,7 @@
deps = [
":base_java",
":base_java_test_support",
+ "//base/test:test_support_java",
"//third_party/android_support_test_runner:runner_java",
"//third_party/hamcrest:hamcrest_java",
"//third_party/junit:junit",
@@ -3318,6 +3319,7 @@
"android/javatests/src/org/chromium/base/task/SequencedTaskRunnerImplTest.java",
"android/javatests/src/org/chromium/base/task/SingleThreadTaskRunnerImplTest.java",
"android/javatests/src/org/chromium/base/task/TaskRunnerImplTest.java",
+ "android/javatests/src/org/chromium/base/util/GarbageCollectionTestUtilsTest.java",
]
}
@@ -3418,7 +3420,6 @@
testonly = true
java_files = [
"android/junit/src/org/chromium/base/metrics/test/ShadowRecordHistogram.java",
- "android/junit/src/org/chromium/base/util/GarbageCollectionTestUtil.java",
"test/android/junit/src/org/chromium/base/task/test/BackgroundShadowAsyncTask.java",
"test/android/junit/src/org/chromium/base/task/test/CustomShadowAsyncTask.java",
"test/android/junit/src/org/chromium/base/test/BaseRobolectricTestRunner.java",
@@ -3451,7 +3452,7 @@
"android/junit/src/org/chromium/base/process_launcher/ChildConnectionAllocatorTest.java",
"android/junit/src/org/chromium/base/process_launcher/ChildProcessConnectionTest.java",
"android/junit/src/org/chromium/base/task/TaskTraitsTest.java",
- "android/junit/src/org/chromium/base/util/GarbageCollectionTestUtilTest.java",
+ "android/junit/src/org/chromium/base/util/GarbageCollectionTestUtilsUnitTest.java",
"test/android/junit/src/org/chromium/base/test/SetUpStatementTest.java",
"test/android/junit/src/org/chromium/base/test/TestListInstrumentationRunListenerTest.java",
"test/android/junit/src/org/chromium/base/test/util/AnnotationProcessingUtilsTest.java",
@@ -3471,6 +3472,7 @@
":base_java_test_support",
":base_junit_test_support",
":jni_java",
+ "//base/test:test_support_java",
"//third_party/hamcrest:hamcrest_java",
]
}
diff --git a/base/android/javatests/src/org/chromium/base/util/GarbageCollectionTestUtilsTest.java b/base/android/javatests/src/org/chromium/base/util/GarbageCollectionTestUtilsTest.java
new file mode 100644
index 0000000..f58cbebe
--- /dev/null
+++ b/base/android/javatests/src/org/chromium/base/util/GarbageCollectionTestUtilsTest.java
@@ -0,0 +1,42 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base.util;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import static org.chromium.base.GarbageCollectionTestUtils.canBeGarbageCollected;
+
+import android.graphics.Bitmap;
+import android.support.test.annotation.UiThreadTest;
+import android.support.test.filters.SmallTest;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.GarbageCollectionTestUtils;
+import org.chromium.base.test.BaseJUnit4ClassRunner;
+
+import java.lang.ref.WeakReference;
+
+/**
+ * Tests for {@link GarbageCollectionTestUtils}.
+ */
+@RunWith(BaseJUnit4ClassRunner.class)
+public class GarbageCollectionTestUtilsTest {
+ @Test
+ @SmallTest
+ @UiThreadTest
+ public void testCanBeGarbageCollected() {
+ Bitmap bitmap = Bitmap.createBitmap(1, 2, Bitmap.Config.ARGB_8888);
+ WeakReference<Bitmap> bitmapWeakReference = new WeakReference<>(bitmap);
+ assertNotNull(bitmapWeakReference.get());
+ assertFalse(canBeGarbageCollected(bitmapWeakReference));
+
+ bitmap = null;
+ assertTrue(canBeGarbageCollected(bitmapWeakReference));
+ }
+}
diff --git a/base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtil.java b/base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtil.java
deleted file mode 100644
index c8a13e8..0000000
--- a/base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtil.java
+++ /dev/null
@@ -1,24 +0,0 @@
-// Copyright 2019 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-package org.chromium.base.util;
-
-import java.lang.ref.WeakReference;
-
-/**
- * Util for doing garbage collection tests.
- */
-public class GarbageCollectionTestUtil {
- /**
- * Do garbage collection and see if an object is released.
- * @param reference A {@link WeakReference} pointing to the object.
- * @return Whether the object can be garbage-collected.
- */
- public static boolean isGarbageCollected(WeakReference<?> reference) {
- Runtime runtime = Runtime.getRuntime();
- runtime.runFinalization();
- runtime.gc();
- return reference.get() == null;
- }
-}
diff --git a/base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtilTest.java b/base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtilsUnitTest.java
similarity index 68%
rename from base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtilTest.java
rename to base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtilsUnitTest.java
index c6cf781..decab3ad 100644
--- a/base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtilTest.java
+++ b/base/android/junit/src/org/chromium/base/util/GarbageCollectionTestUtilsUnitTest.java
@@ -8,7 +8,7 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
-import static org.chromium.base.util.GarbageCollectionTestUtil.isGarbageCollected;
+import static org.chromium.base.GarbageCollectionTestUtils.canBeGarbageCollected;
import android.graphics.Bitmap;
@@ -16,24 +16,25 @@
import org.junit.runner.RunWith;
import org.robolectric.annotation.Config;
+import org.chromium.base.GarbageCollectionTestUtils;
import org.chromium.base.test.BaseRobolectricTestRunner;
import java.lang.ref.WeakReference;
/**
- * Tests for {@link GarbageCollectionTestUtil}.
+ * Tests for {@link GarbageCollectionTestUtils}.
*/
@RunWith(BaseRobolectricTestRunner.class)
@Config(manifest = Config.NONE)
-public class GarbageCollectionTestUtilTest {
+public class GarbageCollectionTestUtilsUnitTest {
@Test
- public void testIsGarbageCollected() {
+ public void testCanBeGarbageCollected() {
Bitmap bitmap = Bitmap.createBitmap(1, 2, Bitmap.Config.ARGB_8888);
WeakReference<Bitmap> bitmapWeakReference = new WeakReference<>(bitmap);
assertNotNull(bitmapWeakReference.get());
- assertFalse(isGarbageCollected(bitmapWeakReference));
+ assertFalse(canBeGarbageCollected(bitmapWeakReference));
bitmap = null;
- assertTrue(isGarbageCollected(bitmapWeakReference));
+ assertTrue(canBeGarbageCollected(bitmapWeakReference));
}
}
diff --git a/base/test/BUILD.gn b/base/test/BUILD.gn
index 88e1600..b987a4bc3 100644
--- a/base/test/BUILD.gn
+++ b/base/test/BUILD.gn
@@ -496,6 +496,7 @@
]
srcjar_deps = [ ":test_support_java_aidl" ]
java_files = [
+ "android/java/src/org/chromium/base/GarbageCollectionTestUtils.java",
"android/java/src/org/chromium/base/MainReturnCodeResult.java",
"android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java",
"android/java/src/org/chromium/base/MultiprocessTestClientService.java",
diff --git a/base/test/android/java/src/org/chromium/base/GarbageCollectionTestUtils.java b/base/test/android/java/src/org/chromium/base/GarbageCollectionTestUtils.java
new file mode 100644
index 0000000..2755a1d
--- /dev/null
+++ b/base/test/android/java/src/org/chromium/base/GarbageCollectionTestUtils.java
@@ -0,0 +1,52 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base;
+
+import android.os.Build;
+
+import java.lang.ref.WeakReference;
+
+/**
+ * Utils for doing garbage collection tests.
+ */
+public class GarbageCollectionTestUtils {
+ /**
+ * Relying on just one single GC might make instrumentation tests flaky.
+ * Note that {@link #MAX_GC_ITERATIONS} * {@link #GC_SLEEP_TIME} should not be too large,
+ * since there are tests asserting objects NOT garbage collected.
+ */
+ private final static int MAX_GC_ITERATIONS = 3;
+ private final static long GC_SLEEP_TIME = 100;
+
+ /**
+ * Do garbage collection and see if an object is released.
+ * @param reference A {@link WeakReference} pointing to the object.
+ * @return Whether the object can be garbage-collected.
+ */
+ public static boolean canBeGarbageCollected(WeakReference<?> reference) {
+ // Robolectric tests, one iteration is enough.
+ final int iterations = isInRobolectric() ? 1 : MAX_GC_ITERATIONS;
+ final long sleepTime = isInRobolectric() ? 0 : GC_SLEEP_TIME;
+ Runtime runtime = Runtime.getRuntime();
+ for (int i = 0; i < iterations; i++) {
+ runtime.runFinalization();
+ runtime.gc();
+ if (reference.get() == null) return true;
+
+ // Pause for a while and then go back around the loop to try again.
+ try {
+ Thread.sleep(sleepTime);
+ } catch (InterruptedException e) {
+ // Ignore any interrupts and just try again.
+ }
+ }
+
+ return reference.get() == null;
+ }
+
+ private static boolean isInRobolectric() {
+ return Build.FINGERPRINT.equals("robolectric");
+ }
+}
diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn
index 0f2b8f1..17311184c 100644
--- a/chrome/android/BUILD.gn
+++ b/chrome/android/BUILD.gn
@@ -748,6 +748,7 @@
"$google_play_services_package:google_play_services_tasks_java",
"//base:base_java",
"//base:base_java_test_support",
+ "//base/test:test_support_java",
"//chrome/android:app_hooks_java",
"//chrome/android:chrome_java",
"//chrome/android/features/tab_ui:java",
diff --git a/chrome/android/features/tab_ui/java/res/layout/tab_grid_card_item.xml b/chrome/android/features/tab_ui/java/res/layout/tab_grid_card_item.xml
index 70061a53..c982719 100644
--- a/chrome/android/features/tab_ui/java/res/layout/tab_grid_card_item.xml
+++ b/chrome/android/features/tab_ui/java/res/layout/tab_grid_card_item.xml
@@ -51,7 +51,8 @@
android:importantForAccessibility="no"
android:src="@color/thumbnail_placeholder_on_primary_bg"
app:cornerRadiusBottomStart="@dimen/default_rounded_corner_radius"
- app:cornerRadiusBottomEnd="@dimen/default_rounded_corner_radius"/>
+ app:cornerRadiusBottomEnd="@dimen/default_rounded_corner_radius"
+ app:roundedfillColor="@color/default_bg_color_light"/>
<View
style="@style/HorizontalDivider"
android:layout_below="@id/tab_title"/>
diff --git a/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridViewHolder.java b/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridViewHolder.java
index 9b1bba9..7440db4 100644
--- a/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridViewHolder.java
+++ b/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridViewHolder.java
@@ -73,7 +73,7 @@
}
public void resetThumbnail() {
- thumbnail.setImageResource(0);
+ thumbnail.setImageDrawable(null);
thumbnail.setMinimumHeight(thumbnail.getWidth());
}
}
diff --git a/chrome/android/features/tab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabListViewHolderTest.java b/chrome/android/features/tab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabListViewHolderTest.java
index 19a7c2a..74be540 100644
--- a/chrome/android/features/tab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabListViewHolderTest.java
+++ b/chrome/android/features/tab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabListViewHolderTest.java
@@ -7,9 +7,10 @@
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.chromium.base.GarbageCollectionTestUtils.canBeGarbageCollected;
+
import android.graphics.Bitmap;
import android.graphics.drawable.BitmapDrawable;
-import android.graphics.drawable.ColorDrawable;
import android.graphics.drawable.Drawable;
import android.os.Build;
import android.support.test.annotation.UiThreadTest;
@@ -31,6 +32,7 @@
import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.ui.modelutil.PropertyModelChangeProcessor;
+import java.lang.ref.WeakReference;
import java.util.concurrent.atomic.AtomicBoolean;
/**
@@ -152,12 +154,7 @@
@UiThreadTest
public void testThumbnail() throws Exception {
mGridModel.set(TabProperties.THUMBNAIL_FETCHER, mMockThumbnailProvider);
- // This should have set the image resource id to 0 and reset it.
- if (Build.VERSION.SDK_INT > Build.VERSION_CODES.KITKAT) {
- Assert.assertNull(mTabGridViewHolder.thumbnail.getDrawable());
- } else {
- assertThat(mTabGridViewHolder.thumbnail.getDrawable(), instanceOf(ColorDrawable.class));
- }
+ Assert.assertNull(mTabGridViewHolder.thumbnail.getDrawable());
mGridModel.set(TabProperties.THUMBNAIL_FETCHER, null);
mShouldReturnBitmap = true;
@@ -168,6 +165,60 @@
@Test
@MediumTest
@UiThreadTest
+ public void testThumbnailGCAfterNullBitmap() throws Exception {
+ mShouldReturnBitmap = true;
+ mGridModel.set(TabProperties.THUMBNAIL_FETCHER, mMockThumbnailProvider);
+ assertThat(mTabGridViewHolder.thumbnail.getDrawable(), instanceOf(BitmapDrawable.class));
+ Bitmap bitmap = ((BitmapDrawable) mTabGridViewHolder.thumbnail.getDrawable()).getBitmap();
+ WeakReference<Bitmap> ref = new WeakReference<>(bitmap);
+ bitmap = null;
+
+ mGridModel.set(TabProperties.THUMBNAIL_FETCHER, null);
+ Assert.assertFalse(canBeGarbageCollected(ref));
+
+ mShouldReturnBitmap = false;
+ mGridModel.set(TabProperties.THUMBNAIL_FETCHER, mMockThumbnailProvider);
+ Assert.assertTrue(canBeGarbageCollected(ref));
+ }
+
+ @Test
+ @MediumTest
+ @UiThreadTest
+ public void testThumbnailGCAfterNewBitmap() throws Exception {
+ mShouldReturnBitmap = true;
+ mGridModel.set(TabProperties.THUMBNAIL_FETCHER, mMockThumbnailProvider);
+ assertThat(mTabGridViewHolder.thumbnail.getDrawable(), instanceOf(BitmapDrawable.class));
+ Bitmap bitmap = ((BitmapDrawable) mTabGridViewHolder.thumbnail.getDrawable()).getBitmap();
+ WeakReference<Bitmap> ref = new WeakReference<>(bitmap);
+ bitmap = null;
+
+ mGridModel.set(TabProperties.THUMBNAIL_FETCHER, null);
+ Assert.assertFalse(canBeGarbageCollected(ref));
+
+ mGridModel.set(TabProperties.THUMBNAIL_FETCHER, mMockThumbnailProvider);
+ Assert.assertTrue(canBeGarbageCollected(ref));
+ }
+
+ @Test
+ @MediumTest
+ @UiThreadTest
+ public void testResetThumbnailGC() throws Exception {
+ mShouldReturnBitmap = true;
+ mGridModel.set(TabProperties.THUMBNAIL_FETCHER, mMockThumbnailProvider);
+ assertThat(mTabGridViewHolder.thumbnail.getDrawable(), instanceOf(BitmapDrawable.class));
+ Bitmap bitmap = ((BitmapDrawable) mTabGridViewHolder.thumbnail.getDrawable()).getBitmap();
+ WeakReference<Bitmap> ref = new WeakReference<>(bitmap);
+ bitmap = null;
+
+ Assert.assertFalse(canBeGarbageCollected(ref));
+
+ mTabGridViewHolder.resetThumbnail();
+ Assert.assertTrue(canBeGarbageCollected(ref));
+ }
+
+ @Test
+ @MediumTest
+ @UiThreadTest
public void testClickToSelect() throws Exception {
mTabGridViewHolder.itemView.performClick();
Assert.assertTrue(mSelectClicked.get());
diff --git a/ui/android/BUILD.gn b/ui/android/BUILD.gn
index ee71880..f51fbc2 100644
--- a/ui/android/BUILD.gn
+++ b/ui/android/BUILD.gn
@@ -398,6 +398,7 @@
"//base:base_java",
"//base:base_java_test_support",
"//base:base_junit_test_support",
+ "//base/test:test_support_java",
]
}
diff --git a/ui/android/junit/src/org/chromium/ui/resources/dynamics/BitmapDynamicResourceTest.java b/ui/android/junit/src/org/chromium/ui/resources/dynamics/BitmapDynamicResourceTest.java
index b746405..719def28 100644
--- a/ui/android/junit/src/org/chromium/ui/resources/dynamics/BitmapDynamicResourceTest.java
+++ b/ui/android/junit/src/org/chromium/ui/resources/dynamics/BitmapDynamicResourceTest.java
@@ -8,7 +8,7 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
-import static org.chromium.base.util.GarbageCollectionTestUtil.isGarbageCollected;
+import static org.chromium.base.GarbageCollectionTestUtils.canBeGarbageCollected;
import android.graphics.Bitmap;
@@ -47,11 +47,11 @@
WeakReference<Bitmap> bitmapWeakReference = new WeakReference<>(bitmap);
mResource.setBitmap(bitmap);
bitmap = null;
- assertFalse(isGarbageCollected(bitmapWeakReference));
+ assertFalse(canBeGarbageCollected(bitmapWeakReference));
Bitmap bitmap2 = Bitmap.createBitmap(3, 4, Bitmap.Config.ARGB_8888);
mResource.setBitmap(bitmap2);
- assertTrue(isGarbageCollected(bitmapWeakReference));
+ assertTrue(canBeGarbageCollected(bitmapWeakReference));
}
@Test
@@ -60,9 +60,9 @@
WeakReference<Bitmap> bitmapWeakReference = new WeakReference<>(bitmap);
mResource.setBitmap(bitmap);
bitmap = null;
- assertFalse(isGarbageCollected(bitmapWeakReference));
+ assertFalse(canBeGarbageCollected(bitmapWeakReference));
mResource.getBitmap();
- assertTrue(isGarbageCollected(bitmapWeakReference));
+ assertTrue(canBeGarbageCollected(bitmapWeakReference));
}
}
diff --git a/ui/android/junit/src/org/chromium/ui/resources/dynamics/ViewResourceAdapterTest.java b/ui/android/junit/src/org/chromium/ui/resources/dynamics/ViewResourceAdapterTest.java
index 163d762..61cf3b23 100644
--- a/ui/android/junit/src/org/chromium/ui/resources/dynamics/ViewResourceAdapterTest.java
+++ b/ui/android/junit/src/org/chromium/ui/resources/dynamics/ViewResourceAdapterTest.java
@@ -12,7 +12,7 @@
import static org.mockito.Mockito.when;
import static org.mockito.MockitoAnnotations.initMocks;
-import static org.chromium.base.util.GarbageCollectionTestUtil.isGarbageCollected;
+import static org.chromium.base.GarbageCollectionTestUtils.canBeGarbageCollected;
import android.graphics.Bitmap;
import android.graphics.Canvas;
@@ -229,22 +229,22 @@
public void testDropCachedBitmapGCed() {
WeakReference<Bitmap> bitmapWeakReference = new WeakReference<>(mAdapter.getBitmap());
assertNotNull(bitmapWeakReference.get());
- assertFalse(isGarbageCollected(bitmapWeakReference));
+ assertFalse(canBeGarbageCollected(bitmapWeakReference));
mAdapter.dropCachedBitmap();
- assertTrue(isGarbageCollected(bitmapWeakReference));
+ assertTrue(canBeGarbageCollected(bitmapWeakReference));
}
@Test
public void testResizeGCed() {
WeakReference<Bitmap> bitmapWeakReference = new WeakReference<>(mAdapter.getBitmap());
assertNotNull(bitmapWeakReference.get());
- assertFalse(isGarbageCollected(bitmapWeakReference));
+ assertFalse(canBeGarbageCollected(bitmapWeakReference));
mViewWidth += 10;
mAdapter.invalidate(null);
mAdapter.getBitmap();
- assertTrue(isGarbageCollected(bitmapWeakReference));
+ assertTrue(canBeGarbageCollected(bitmapWeakReference));
}
@Test