Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r=rnewman,nalexander
authorSebastian Kaspari <s.kaspari@gmail.com>
Mon, 07 Mar 2016 12:47:48 +0100
changeset 324925 d8c0657ff5d34dd1a62d63830f967e0d215b9e2b
parent 324924 dddb62ccf997f8b2afa17a9ef9822d4584212972
child 324926 c8d1f83532a7fe4289a473e3528c04b8b2f85a64
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, nalexander
bugs1254089
milestone47.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1254089 - LoadFaviconTask: Use HttpUrlConnection instead of HttpClient. r=rnewman,nalexander HttpUrlConnection supports SNI and LoadFaviconTask does not require fancy HTTP features. MozReview-Commit-ID: IuDyU4xk7qI
mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java
mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java
--- a/mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java
+++ b/mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java
@@ -442,18 +442,16 @@ public class Favicons {
         // Cancel any pending tasks
         synchronized (loadTasks) {
             final int count = loadTasks.size();
             for (int i = 0; i < count; i++) {
                 cancelFaviconLoad(loadTasks.keyAt(i));
             }
             loadTasks.clear();
         }
-
-        LoadFaviconTask.closeHTTPClient();
     }
 
     /**
      * Get the dominant colour of the Favicon at the URL given, if any exists in the cache.
      *
      * @param url The URL of the Favicon, to be used as the cache key for the colour value.
      * @return The dominant colour of the provided Favicon.
      */
--- a/mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java
+++ b/mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java
@@ -1,35 +1,31 @@
 /* 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.favicons;
 
-
 import android.content.ContentResolver;
 import android.content.Context;
 import android.graphics.Bitmap;
-import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
 import android.text.TextUtils;
 import android.util.Log;
-import ch.boye.httpclientandroidlib.Header;
-import ch.boye.httpclientandroidlib.HttpEntity;
-import ch.boye.httpclientandroidlib.HttpResponse;
-import ch.boye.httpclientandroidlib.client.methods.HttpGet;
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.GeckoProfile;
 import org.mozilla.gecko.db.BrowserDB;
 import org.mozilla.gecko.favicons.decoders.FaviconDecoder;
 import org.mozilla.gecko.favicons.decoders.LoadFaviconResult;
 import org.mozilla.gecko.util.GeckoJarReader;
 import org.mozilla.gecko.util.IOUtils;
 import org.mozilla.gecko.util.ThreadUtils;
 
 import java.io.IOException;
+import java.io.InputStream;
+import java.net.HttpURLConnection;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -73,37 +69,41 @@ public class LoadFaviconTask {
     private final OnFaviconLoadedListener listener;
     private final int flags;
     private final BrowserDB db;
 
     private final boolean onlyFromLocal;
     volatile boolean mCancelled;
 
     // Assuming square favicons, judging by width only is acceptable.
-    protected int targetWidth;
+    protected int targetWidthAndHeight;
     private LinkedList<LoadFaviconTask> chainees;
     private boolean isChaining;
 
-    static DefaultHttpClient httpClient = new DefaultHttpClient();
+    private static class Response {
+        public final int contentLength;
+        public final InputStream stream;
 
-    public LoadFaviconTask(Context context, String pageURL, String faviconURL, int flags, OnFaviconLoadedListener listener) {
-        this(context, pageURL, faviconURL, flags, listener, -1, false);
+        private Response(InputStream stream, int contentLength) {
+            this.stream = stream;
+            this.contentLength = contentLength;
+        }
     }
 
     public LoadFaviconTask(Context context, String pageURL, String faviconURL, int flags, OnFaviconLoadedListener listener,
                            int targetWidth, boolean onlyFromLocal) {
         id = nextFaviconLoadId.incrementAndGet();
 
         this.context = context;
         db = GeckoProfile.get(context).getDB();
         this.pageUrl = pageURL;
         this.faviconURL = faviconURL;
         this.listener = listener;
         this.flags = flags;
-        this.targetWidth = targetWidth;
+        this.targetWidthAndHeight = targetWidth;
         this.onlyFromLocal = onlyFromLocal;
     }
 
     // Runs in background thread
     private LoadFaviconResult loadFaviconFromDb(final BrowserDB db) {
         ContentResolver resolver = context.getContentResolver();
         return db.getFaviconForUrl(resolver, faviconURL);
     }
@@ -122,83 +122,63 @@ public class LoadFaviconTask {
         db.updateFaviconForUrl(resolver, pageUrl, encodedFavicon, faviconURL);
     }
 
     /**
      * 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 HttpResponse tryDownload(URI faviconURI) throws URISyntaxException, IOException {
+    private Response tryDownload(URI faviconURI) throws URISyntaxException, IOException {
         HashSet<String> visitedLinkSet = new HashSet<>();
         visitedLinkSet.add(faviconURI.toString());
         return tryDownloadRecurse(faviconURI, visitedLinkSet);
     }
-    private HttpResponse tryDownloadRecurse(URI faviconURI, HashSet<String> visited) throws URISyntaxException, IOException {
+    private Response tryDownloadRecurse(URI faviconURI, HashSet<String> visited) throws URISyntaxException, IOException {
         if (visited.size() == MAX_REDIRECTS_TO_FOLLOW) {
             return null;
         }
 
-        HttpGet request = new HttpGet(faviconURI);
-        request.setHeader("User-Agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());
-        HttpResponse response = httpClient.execute(request);
-        if (response == null) {
+        HttpURLConnection connection = (HttpURLConnection) faviconURI.toURL().openConnection();
+        connection.setRequestProperty("User-Agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());
+
+        connection.connect();
+
+        // Was the response a failure?
+        int status = connection.getResponseCode();
+
+        // Handle HTTP status codes requesting a redirect.
+        if (status >= 300 && status < 400) {
+            final String newURI = connection.getHeaderField("Location");
+
+            // Handle mad webservers.
+            try {
+                if (newURI == null || newURI.equals(faviconURI.toString())) {
+                    return null;
+                }
+
+                if (visited.contains(newURI)) {
+                    // Already been redirected here - abort.
+                    return null;
+                }
+
+                visited.add(newURI);
+            } finally {
+                connection.disconnect();
+            }
+
+            return tryDownloadRecurse(new URI(newURI), visited);
+        }
+
+        if (status >= 400) {
+            connection.disconnect();
             return null;
         }
 
-        if (response.getStatusLine() != null) {
-
-            // Was the response a failure?
-            int status = response.getStatusLine().getStatusCode();
-
-            // Handle HTTP status codes requesting a redirect.
-            if (status >= 300 && status < 400) {
-                Header header = response.getFirstHeader("Location");
-
-                // Handle mad webservers.
-                final String newURI;
-                try {
-                    if (header == null) {
-                        return null;
-                    }
-
-                    newURI = header.getValue();
-                    if (newURI == null || newURI.equals(faviconURI.toString())) {
-                        return null;
-                    }
-
-                    if (visited.contains(newURI)) {
-                        // Already been redirected here - abort.
-                        return null;
-                    }
-
-                    visited.add(newURI);
-                } finally {
-                    // Consume the entity before recurse or exit.
-                    try {
-                        response.getEntity().consumeContent();
-                    } catch (Exception e) {
-                        // Doesn't matter.
-                    }
-                }
-
-                return tryDownloadRecurse(new URI(newURI), visited);
-            }
-
-            if (status >= 400) {
-                // Consume the entity and exit.
-                try {
-                    response.getEntity().consumeContent();
-                } catch (Exception e) {
-                    // Doesn't matter.
-                }
-                return null;
-            }
-        }
-        return response;
+        return new Response(connection.getInputStream(), connection.getHeaderFieldInt("Content-Length", -1));
     }
 
     /**
      * Retrieve the specified favicon from the JAR, returning null if it's not
      * a JAR URI.
      */
     private Bitmap fetchJARFavicon(String uri) {
         if (uri == null) {
@@ -251,60 +231,54 @@ public class LoadFaviconTask {
      *         null if no or corrupt data ware 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.
      */
     private LoadFaviconResult downloadAndDecodeImage(URI targetFaviconURL) throws IOException, URISyntaxException {
         // Try the URL we were given.
-        HttpResponse response = tryDownload(targetFaviconURL);
+        Response response = tryDownload(targetFaviconURL);
         if (response == null) {
             return null;
         }
 
-        HttpEntity entity = response.getEntity();
-        if (entity == null) {
-            return null;
-        }
-
         // Decode the image from the fetched response.
         try {
-            return decodeImageFromResponse(entity);
+            return decodeImageFromResponse(response);
         } finally {
             // Close the stream and free related resources.
-            entity.consumeContent();
+            IOUtils.safeStreamClose(response.stream);
         }
     }
 
     /**
      * Copies the favicon stream to a buffer and decodes downloaded content  into bitmaps using the
      * FaviconDecoder.
      *
-     * @param entity HttpEntity containing the favicon stream to decode.
+     * @param response Response containing the favicon stream to decode.
      * @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.
      */
-    private LoadFaviconResult decodeImageFromResponse(HttpEntity entity) throws IOException {
+    private LoadFaviconResult decodeImageFromResponse(Response response) throws IOException {
         // This may not be provided, but if it is, it's useful.
-        final long entityReportedLength = entity.getContentLength();
         int bufferSize;
-        if (entityReportedLength > 0) {
+        if (response.contentLength > 0) {
             // The size was reported and sane, so let's use that.
             // Integer overflow should not be a problem for Favicon sizes...
-            bufferSize = (int) entityReportedLength + 1;
+            bufferSize = response.contentLength + 1;
         } else {
             // No declared size, so guess and reallocate later if it turns out to be too small.
             bufferSize = DEFAULT_FAVICON_BUFFER_SIZE;
         }
 
         // Read the InputStream into a byte[].
-        ConsumedInputStream result = IOUtils.readFully(entity.getContent(), bufferSize);
+        ConsumedInputStream result = IOUtils.readFully(response.stream, bufferSize);
         if (result == null) {
             return null;
         }
 
         // Having downloaded the image, decode it.
         return FaviconDecoder.decodeFavicon(result.getData(), 0, result.consumedLength);
     }
 
@@ -469,17 +443,17 @@ public class LoadFaviconTask {
                 final List<Integer> sizes = new ArrayList<>();
 
                 while (loadedBitmaps.getBitmaps().hasNext()) {
                     final Bitmap b = loadedBitmaps.getBitmaps().next();
                     iconMap.put(b.getWidth(), b);
                     sizes.add(b.getWidth());
                 }
 
-                int bestSize = Favicons.selectBestSizeFromList(sizes, targetWidth);
+                int bestSize = Favicons.selectBestSizeFromList(sizes, targetWidthAndHeight);
                 return iconMap.get(bestSize);
             }
         }
 
         if (isUsingDefaultURL) {
             Favicons.putFaviconInFailedCache(faviconURL);
             return null;
         }
@@ -523,18 +497,17 @@ public class LoadFaviconTask {
      * This call is certain to succeed, provided there was enough memory to decode this favicon.
      *
      * @param loadedBitmaps LoadFaviconResult to store.
      * @return The optimal favicon available to satisfy this LoadFaviconTask's request, or null if
      *         we are under extreme memory pressure and find ourselves dropping the cache immediately.
      */
     private Bitmap pushToCacheAndGetResult(LoadFaviconResult loadedBitmaps) {
         Favicons.putFaviconsInMemCache(faviconURL, loadedBitmaps.getBitmaps());
-        Bitmap result = Favicons.getSizedFaviconFromCache(faviconURL, targetWidth);
-        return result;
+        return Favicons.getSizedFaviconFromCache(faviconURL, targetWidthAndHeight);
     }
 
     private static boolean imageIsValid(final Bitmap image) {
         return image != null &&
                image.getWidth() > 0 &&
                image.getHeight() > 0;
     }
 
@@ -571,19 +544,19 @@ public class LoadFaviconTask {
     }
 
     private void processResult(Bitmap image) {
         Favicons.removeLoadTask(id);
         final Bitmap scaled;
 
         // Notify listeners, scaling if required.
         if ((flags & FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS) != 0) {
-            scaled = Bitmap.createScaledBitmap(image, targetWidth, targetWidth, true);
-        } else if (targetWidth != -1 && image != null &&  image.getWidth() != targetWidth) {
-            scaled = Favicons.getSizedFaviconFromCache(faviconURL, targetWidth);
+            scaled = Bitmap.createScaledBitmap(image, targetWidthAndHeight, targetWidthAndHeight, true);
+        } else if (targetWidthAndHeight != -1 && image != null &&  image.getWidth() != targetWidthAndHeight) {
+            scaled = Favicons.getSizedFaviconFromCache(faviconURL, targetWidthAndHeight);
         } else {
             scaled = image;
         }
 
         Favicons.dispatchResult(pageUrl, faviconURL, scaled, listener);
     }
 
     void onCancelled() {
@@ -624,28 +597,9 @@ public class LoadFaviconTask {
         }
 
         chainees.add(aChainee);
     }
 
     int getId() {
         return id;
     }
-
-    static void closeHTTPClient() {
-        // This work must be done on a background thread because it shuts down
-        // the connection pool, which typically involves closing a connection --
-        // which counts as network activity.
-        if (ThreadUtils.isOnBackgroundThread()) {
-            if (httpClient != null) {
-                httpClient.close();
-            }
-            return;
-        }
-
-        ThreadUtils.postToBackgroundThread(new Runnable() {
-            @Override
-            public void run() {
-                LoadFaviconTask.closeHTTPClient();
-            }
-        });
-    }
 }