[IC] Fix null pointer exception when under memory pressure

The problem here is that between CachedImageFetcherImpl's FetchImage
invocation and completion, destroy was called on
InMemoryCachedImageFetcher. This causes an attempt to store the image
in a null bitmap cache. This just checks if the bitmap cache is null
before attempting to store an image.

Bug: 910080
Change-Id: I28b7aaee8588d883cfaf98328e2e58478d452375
Reviewed-on: https://chromium-review.googlesource.com/c/1355770Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612857}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedImageLoader.java

@@ -61,6 +61,7 @@ public class FeedImageLoader implements ImageLoaderApi {
 
     public void destroy() {
         mCachedImageFetcher.destroy();
+        mCachedImageFetcher = null;
     }
 
     @Override
@@ -82,7 +83,7 @@ public class FeedImageLoader implements ImageLoaderApi {
      */
     private void loadDrawableWithIter(
             Iterator<String> urlsIter, int widthPx, int heightPx, Consumer<Drawable> consumer) {
-        if (!urlsIter.hasNext()) {
+        if (!urlsIter.hasNext() || mCachedImageFetcher == null) {
             // Post to ensure callback is not run synchronously.
             ThreadUtils.postOnUiThread(() -> consumer.accept(null));
             return;

chrome/android/java/src/org/chromium/chrome/browser/cached_image_fetcher/InMemoryCachedImageFetcher.java

@@ -47,13 +47,21 @@ public class InMemoryCachedImageFetcher implements CachedImageFetcher {
     @Override
     public void destroy() {
         mCachedImageFetcher.destroy();
+        mCachedImageFetcher = null;
+
         mBitmapCache.destroy();
+        mBitmapCache = null;
     }
 
     @Override
     public void fetchImage(String url, int width, int height, Callback<Bitmap> callback) {
         Bitmap cachedBitmap = tryToGetBitmap(url, width, height);
         if (cachedBitmap == null) {
+            if (mCachedImageFetcher == null) {
+                callback.onResult(null);
+                return;
+            }
+
             mCachedImageFetcher.fetchImage(url, width, height, (Bitmap bitmap) -> {
                 storeBitmap(bitmap, url, width, height);
                 callback.onResult(bitmap);
@@ -76,7 +84,10 @@ public class InMemoryCachedImageFetcher implements CachedImageFetcher {
      * @param height The height (in pixels) of the image.
      * @return The Bitmap stored in memory or null.
      */
-    private Bitmap tryToGetBitmap(String url, int width, int height) {
+    @VisibleForTesting
+    Bitmap tryToGetBitmap(String url, int width, int height) {
+        if (mBitmapCache == null) return null;
+
         String key = encodeCacheKey(url, width, height);
         return mBitmapCache.getBitmap(key);
     }
@@ -89,7 +100,9 @@ public class InMemoryCachedImageFetcher implements CachedImageFetcher {
      * @param height The height (in pixels) of the image.
      */
     private void storeBitmap(Bitmap bitmap, String url, int width, int height) {
-        if (bitmap == null) return;
+        if (bitmap == null || mBitmapCache == null) {
+            return;
+        }
 
         String key = encodeCacheKey(url, width, height);
         mBitmapCache.putBitmap(key, bitmap);

chrome/android/junit/src/org/chromium/chrome/browser/cached_image_fetcher/InMemoryCachedImageFetcherTest.java

@@ -4,16 +4,20 @@
 
 package org.chromium.chrome.browser.cached_image_fetcher;
 
-import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
 import android.graphics.Bitmap;
 import android.support.test.filters.SmallTest;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -23,36 +27,34 @@ import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
 import org.robolectric.annotation.Config;
-import org.robolectric.shadows.ShadowLooper;
 
 import org.chromium.base.Callback;
 import org.chromium.base.DiscardableReferencePool;
-import org.chromium.base.task.test.BackgroundShadowAsyncTask;
 import org.chromium.base.test.BaseRobolectricTestRunner;
 import org.chromium.chrome.browser.BitmapCache;
-import org.chromium.chrome.browser.util.test.ShadowUrlUtilities;
 
 /**
  * Unit tests for InMemoryCachedImageFetcher.
  */
 @RunWith(BaseRobolectricTestRunner.class)
-@Config(manifest = Config.NONE,
-        shadows = {ShadowUrlUtilities.class, BackgroundShadowAsyncTask.class})
+@Config(manifest = Config.NONE)
 public class InMemoryCachedImageFetcherTest {
     private static final String URL = "http://foo.bar";
     private static final int WIDTH_PX = 100;
     private static final int HEIGHT_PX = 200;
     private static final int DEFAULT_CACHE_SIZE = 100;
 
-    private final DiscardableReferencePool mReferencePool = new DiscardableReferencePool();
     private final Bitmap mBitmap =
             Bitmap.createBitmap(WIDTH_PX, HEIGHT_PX, Bitmap.Config.ARGB_8888);
 
     private InMemoryCachedImageFetcher mInMemoryCachedImageFetcher;
     private BitmapCache mBitmapCache;
+    private DiscardableReferencePool mReferencePool;
 
     @Mock
     private CachedImageFetcherImpl mCachedImageFetcherImpl;
+    @Mock
+    private Callback<Bitmap> mCallback;
 
     @Captor
     private ArgumentCaptor<Integer> mWidthCaptor;
@@ -64,32 +66,76 @@ public class InMemoryCachedImageFetcherTest {
     @Before
     public void setUp() {
         MockitoAnnotations.initMocks(this);
-
+        mReferencePool = new DiscardableReferencePool();
         mBitmapCache = new BitmapCache(mReferencePool, DEFAULT_CACHE_SIZE);
+    }
+
+    @After
+    public void tearDown() {
+        mBitmapCache.destroy();
+        mReferencePool.drain();
+    }
+
+    public void answerFetch(Bitmap bitmap, CachedImageFetcher cachedImageFetcher,
+            boolean deleteBitmapCacheOnFetch) {
         mInMemoryCachedImageFetcher =
-                new InMemoryCachedImageFetcher(mBitmapCache, mCachedImageFetcherImpl);
+                spy(new InMemoryCachedImageFetcher(mBitmapCache, cachedImageFetcher));
         // clang-format off
         doAnswer((InvocationOnMock invocation) -> {
-            mCallbackCaptor.getValue().onResult(mBitmap);
+            if (deleteBitmapCacheOnFetch) {
+                mInMemoryCachedImageFetcher.destroy();
+                mReferencePool.drain();
+            }
+
+            mCallbackCaptor.getValue().onResult(bitmap);
             return null;
         }).when(mCachedImageFetcherImpl)
-          .fetchImage(eq(URL), mWidthCaptor.capture(), mHeightCaptor.capture(),
-                      mCallbackCaptor.capture());
+                .fetchImage(eq(URL), mWidthCaptor.capture(), mHeightCaptor.capture(),
+                        mCallbackCaptor.capture());
         // clang-format on
     }
 
+    @SuppressWarnings("unchecked")
     @Test
     @SmallTest
     public void testFetchImageCachesFirstCall() throws Exception {
-        mInMemoryCachedImageFetcher.fetchImage(
-                URL, WIDTH_PX, HEIGHT_PX, (Bitmap bitmap) -> { assertEquals(bitmap, mBitmap); });
-        BackgroundShadowAsyncTask.runBackgroundTasks();
-        ShadowLooper.runUiThreadTasks();
-        mInMemoryCachedImageFetcher.fetchImage(
-                URL, WIDTH_PX, HEIGHT_PX, (Bitmap bitmap) -> { assertEquals(bitmap, mBitmap); });
-        BackgroundShadowAsyncTask.runBackgroundTasks();
-        ShadowLooper.runUiThreadTasks();
+        answerFetch(mBitmap, mCachedImageFetcherImpl, false);
+        mInMemoryCachedImageFetcher.fetchImage(URL, WIDTH_PX, HEIGHT_PX, mCallback);
+        verify(mCallback).onResult(eq(mBitmap));
+
+        reset(mCallback);
+        mInMemoryCachedImageFetcher.fetchImage(URL, WIDTH_PX, HEIGHT_PX, mCallback);
+        verify(mCallback).onResult(eq(mBitmap));
+
         verify(mCachedImageFetcherImpl, /* Should only go to native the first time. */ times(1))
                 .fetchImage(eq(URL), eq(WIDTH_PX), eq(HEIGHT_PX), any());
     }
+
+    @Test
+    @SmallTest
+    public void testFetchImageReturnsNullWhenFetcherIsNull() throws Exception {
+        answerFetch(mBitmap, null, false);
+        doReturn(null)
+                .when(mInMemoryCachedImageFetcher)
+                .tryToGetBitmap(eq(URL), eq(WIDTH_PX), eq(HEIGHT_PX));
+
+        mInMemoryCachedImageFetcher.fetchImage(URL, WIDTH_PX, HEIGHT_PX, mCallback);
+        verify(mCallback).onResult(eq(null));
+
+        verify(mCachedImageFetcherImpl, /* Shouldn't make the call at all. */ times(0))
+                .fetchImage(eq(URL), eq(WIDTH_PX), eq(HEIGHT_PX), any());
+    }
+
+    @Test
+    @SmallTest
+    public void testFetchImageDoesNotCacheAfterDestroy() {
+        try {
+            answerFetch(mBitmap, mCachedImageFetcherImpl, true);
+
+            // No exception should be thrown here when bitmap cache is null.
+            mInMemoryCachedImageFetcher.fetchImage(URL, WIDTH_PX, HEIGHT_PX, (Bitmap bitmap) -> {});
+        } catch (Exception e) {
+            fail("Destroy called in the middle of execution shouldn't throw");
+        }
+    }
 }
\ No newline at end of file

chrome/android/junit/src/org/chromium/chrome/browser/cached_image_fetcher/OWNERS

+file://components/image_fetcher/OWNERS

chrome/android/junit/src/org/chromium/chrome/browser/feed/FeedImageLoaderTest.java

@@ -88,8 +88,12 @@ public class FeedImageLoaderTest {
     @Before
     public void setUp() throws Exception {
         MockitoAnnotations.initMocks(this);
+        setUpWithImageFetcher(mCachedImageFetcher);
+    }
+
+    public void setUpWithImageFetcher(CachedImageFetcher cachedImageFetcher) {
         mImageLoader = Mockito.spy(
-                new FeedImageLoader(ContextUtils.getApplicationContext(), mCachedImageFetcher));
+                new FeedImageLoader(ContextUtils.getApplicationContext(), cachedImageFetcher));
     }
 
     private void answerFetchImage(String url, Bitmap bitmap) {
@@ -138,6 +142,17 @@ public class FeedImageLoaderTest {
         verify(mConsumer, times(1)).accept(eq(null));
     }
 
+    @Test
+    @SmallTest
+    public void testLoadDrawableWithNullFetcher() {
+        setUpWithImageFetcher(null);
+        loadDrawable(HTTP_STRING1);
+        verify(mConsumer, times(1)).accept(eq(null));
+        verify(mImageLoader, times(0))
+                .fetchImage(eq(HTTP_STRING1), eq(ImageLoaderApi.DIMENSION_UNKNOWN),
+                        eq(ImageLoaderApi.DIMENSION_UNKNOWN), any());
+    }
+
     @Test
     @SmallTest
     public void testLoadDrawableMultiple() {