Bug 1315076 - IconDownloader: Catch exceptions inside tryDownloadRecurse() and close connection if needed. r?grisha
MozReview-Commit-ID: E6HUKrX37cH
--- 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();
+ }
}