Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed. r?grisha draft
authorSebastian Kaspari <s.kaspari@gmail.com>
Tue, 22 Nov 2016 14:12:16 +0100
changeset 442906 54bf4529da200893a0ffba539d8c769ab709f813
parent 442068 0534254e9a40b4bade2577c631fe4cfa0b5db41d
child 537930 ca8b30240072d22f16744dd354dc6897f0dde5cb
push id36862
push users.kaspari@gmail.com
push dateWed, 23 Nov 2016 14:38:53 +0000
reviewersgrisha
bugs1315076
milestone53.0a1
Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed. r?grisha MozReview-Commit-ID: E6HUKrX37cH
mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/loader/TestIconDownloader.java
--- a/mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java
+++ b/mobile/android/base/java/org/mozilla/gecko/icons/loader/IconDownloader.java
@@ -2,16 +2,18 @@
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.icons.loader;
 
 import android.content.Context;
 import android.graphics.Bitmap;
+import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
 import android.support.annotation.VisibleForTesting;
 import android.util.Log;
 
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.icons.decoders.FaviconDecoder;
 import org.mozilla.gecko.icons.decoders.LoadFaviconResult;
 import org.mozilla.gecko.icons.IconRequest;
 import org.mozilla.gecko.icons.IconResponse;
@@ -77,129 +79,140 @@ public class IconDownloader implements I
     }
 
     /**
      * Download the Favicon from the given URL and pass it to the decoder function.
      *
      * @param targetFaviconURL URL of the favicon to download.
      * @return A LoadFaviconResult containing the bitmap(s) extracted from the downloaded file, or
      *         null if no or corrupt data was received.
-     * @throws IOException If attempts to fully read the stream result in such an exception, such as
-     *                     in the event of a transient connection failure.
-     * @throws URISyntaxException If the underlying call to tryDownload retries and raises such an
-     *                            exception trying a fallback URL.
      */
     @VisibleForTesting
-    LoadFaviconResult downloadAndDecodeImage(Context context, String targetFaviconURL) throws IOException, URISyntaxException {
+    @Nullable
+    LoadFaviconResult downloadAndDecodeImage(Context context, String targetFaviconURL) {
         // Try the URL we were given.
         final HttpURLConnection connection = tryDownload(targetFaviconURL);
         if (connection == null) {
             return null;
         }
 
         InputStream stream = null;
 
         // Decode the image from the fetched response.
         try {
             stream = connection.getInputStream();
             return decodeImageFromResponse(context, stream, connection.getHeaderFieldInt("Content-Length", -1));
+        } catch (IOException e) {
+            Log.d(LOGTAG, "IOException while reading and decoding ixon", e);
+            return null;
         } finally {
             // Close the stream and free related resources.
             IOUtils.safeStreamClose(stream);
             connection.disconnect();
         }
     }
 
     /**
      * Helper method for trying the download request to grab a Favicon.
      *
      * @param faviconURI URL of Favicon to try and download
      * @return The HttpResponse containing the downloaded Favicon if successful, null otherwise.
      */
-    private HttpURLConnection tryDownload(String faviconURI) throws URISyntaxException, IOException {
+    @Nullable
+    private HttpURLConnection tryDownload(String faviconURI) {
         final HashSet<String> visitedLinkSet = new HashSet<>();
         visitedLinkSet.add(faviconURI);
         return tryDownloadRecurse(faviconURI, visitedLinkSet);
     }
 
     /**
      * Try to download from the favicon URL and recursively follow redirects.
      */
-    private HttpURLConnection tryDownloadRecurse(String faviconURI, HashSet<String> visited) throws URISyntaxException, IOException {
+    @Nullable
+    private HttpURLConnection tryDownloadRecurse(String faviconURI, HashSet<String> visited) {
         if (visited.size() == MAX_REDIRECTS_TO_FOLLOW) {
             return null;
         }
 
-        final HttpURLConnection connection = connectTo(faviconURI);
+        HttpURLConnection connection = null;
+
+        try {
+            connection = connectTo(faviconURI);
 
-        // Was the response a failure?
-        final int status = connection.getResponseCode();
+            // Was the response a failure?
+            final int status = connection.getResponseCode();
+
+            // Handle HTTP status codes requesting a redirect.
+            if (status >= 300 && status < 400) {
+                final String newURI = connection.getHeaderField("Location");
 
-        // Handle HTTP status codes requesting a redirect.
-        if (status >= 300 && status < 400) {
-            final String newURI = connection.getHeaderField("Location");
+                // Handle mad web servers.
+                try {
+                    if (newURI == null || newURI.equals(faviconURI)) {
+                        return null;
+                    }
 
-            // Handle mad web servers.
-            try {
-                if (newURI == null || newURI.equals(faviconURI)) {
-                    return null;
+                    if (visited.contains(newURI)) {
+                        // Already been redirected here - abort.
+                        return null;
+                    }
+
+                    visited.add(newURI);
+                } finally {
+                    connection.disconnect();
                 }
 
-                if (visited.contains(newURI)) {
-                    // Already been redirected here - abort.
-                    return null;
-                }
+                return tryDownloadRecurse(newURI, visited);
+            }
 
-                visited.add(newURI);
-            } finally {
+            if (status >= 400) {
+                // Client or Server error. Let's not retry loading from this URL again for some time.
+                FailureCache.get().rememberFailure(faviconURI);
+
+                connection.disconnect();
+                return null;
+            }
+        } catch (IOException | URISyntaxException e) {
+            if (connection != null) {
                 connection.disconnect();
             }
-
-            return tryDownloadRecurse(newURI, visited);
-        }
-
-        if (status >= 400) {
-            // Client or Server error. Let's not retry loading from this URL again for some time.
-            FailureCache.get().rememberFailure(faviconURI);
-
-            connection.disconnect();
             return null;
         }
 
         return connection;
     }
 
     @VisibleForTesting
-    HttpURLConnection connectTo(String faviconURI) throws URISyntaxException, IOException {
+    @NonNull
+    HttpURLConnection connectTo(String uri) throws URISyntaxException, IOException {
         final HttpURLConnection connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(
-                new URI(faviconURI));
+                new URI(uri));
 
         connection.setRequestProperty("User-Agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());
 
         // We implemented or own way of following redirects back when this code was using HttpClient.
         // Nowadays we should let HttpUrlConnection do the work - assuming that it doesn't follow
         // redirects in loops forever.
         connection.setInstanceFollowRedirects(false);
 
-        connection.connect();
-
         return connection;
     }
 
     /**
      * Copies the favicon stream to a buffer and decodes downloaded content into bitmaps using the
      * FaviconDecoder.
      *
      * @param stream to decode
      * @param contentLength as reported by the server (or -1)
      * @return A LoadFaviconResult containing the bitmap(s) extracted from the downloaded file, or
      *         null if no or corrupt data were received.
      * @throws IOException If attempts to fully read the stream result in such an exception, such as
      *                     in the event of a transient connection failure.
      */
+    @Nullable
     private LoadFaviconResult decodeImageFromResponse(Context context, InputStream stream, int contentLength) throws IOException {
         // This may not be provided, but if it is, it's useful.
         final int bufferSize;
         if (contentLength > 0) {
             // The size was reported and sane, so let's use that.
             // Integer overflow should not be a problem for Favicon sizes...
             bufferSize = contentLength + 1;
         } else {
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/loader/TestIconDownloader.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/loader/TestIconDownloader.java
@@ -11,21 +11,23 @@ import org.junit.runner.RunWith;
 import org.mozilla.gecko.background.testhelpers.TestRunner;
 import org.mozilla.gecko.icons.IconDescriptor;
 import org.mozilla.gecko.icons.IconRequest;
 import org.mozilla.gecko.icons.IconResponse;
 import org.mozilla.gecko.icons.Icons;
 import org.mozilla.gecko.icons.storage.FailureCache;
 import org.robolectric.RuntimeEnvironment;
 
+import java.io.IOException;
 import java.net.HttpURLConnection;
 
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 
 @RunWith(TestRunner.class)
 public class TestIconDownloader {
     /**
@@ -104,9 +106,37 @@ public class TestIconDownloader {
         final IconDownloader downloader = spy(new IconDownloader());
         doReturn(mockedConnection).when(downloader).connectTo(anyString());
         IconResponse response = downloader.load(request);
 
         Assert.assertNull(response);
 
         Assert.assertTrue(FailureCache.get().isKnownFailure(faviconUrl));
     }
+
+    /**
+     * Scenario: Connected to successfully to server but reading the response code throws an exception.
+     *
+     * Verify that:
+     *  * disconnect() is called on HttpUrlConnection
+     */
+    @Test
+    public void testConnectionIsClosedWhenReadingResponseCodeThrows() throws Exception {
+        final IconRequest request = Icons.with(RuntimeEnvironment.application)
+                .pageUrl("http://www.mozilla.org")
+                .icon(IconDescriptor.createFavicon(
+                        "https://www.mozilla.org/media/img/favicon.52506929be4c.ico",
+                        32,
+                        "image/x-icon"))
+                .build();
+
+        HttpURLConnection mockedConnection = mock(HttpURLConnection.class);
+        doThrow(new IOException()).when(mockedConnection).getResponseCode();
+
+        final IconDownloader downloader = spy(new IconDownloader());
+        doReturn(mockedConnection).when(downloader).connectTo(anyString());
+        IconResponse response = downloader.load(request);
+
+        Assert.assertNull(response);
+
+        verify(mockedConnection).disconnect();
+    }
 }