Bug 1065485: Don't decode-to-encode default favicons. r=rnewman
authorChris Kitching <chriskitching@linux.com>
Wed, 10 Sep 2014 16:10:34 -0700
changeset 233007 1de6161e995bcd1477ff42d7ecd66ec6034fcc5e
parent 233006 9b598425c7cb675c1673ff33c8e1e1a9f15a8b4b
child 233008 fa1f219e3ebaeac89b707d2250a79e53560413a0
push id4187
push userbhearsum@mozilla.com
push dateFri, 28 Nov 2014 15:29:12 +0000
treeherdermozilla-beta@f23cc6a30c11 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs1065485
milestone35.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 1065485: Don't decode-to-encode default favicons. r=rnewman
mobile/android/base/db/LocalBrowserDB.java
mobile/android/base/favicons/LoadFaviconTask.java
mobile/android/base/gfx/BitmapUtils.java
mobile/android/base/moz.build
mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_addons.png
mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_marketplace.png
mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_support.png
mobile/android/base/resources/raw/bookmarkdefaults_favicon_addons.png
mobile/android/base/resources/raw/bookmarkdefaults_favicon_marketplace.png
mobile/android/base/resources/raw/bookmarkdefaults_favicon_support.png
mobile/android/base/util/IOUtils.java
--- a/mobile/android/base/db/LocalBrowserDB.java
+++ b/mobile/android/base/db/LocalBrowserDB.java
@@ -2,16 +2,19 @@
  * 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.db;
 
 import java.io.ByteArrayOutputStream;
 import java.io.File;
+import java.io.InputStream;
+import java.lang.IllegalAccessException;
+import java.lang.NoSuchFieldException;
 import java.lang.reflect.Field;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.regex.Matcher;
@@ -48,22 +51,28 @@ import android.database.ContentObserver;
 import android.database.Cursor;
 import android.database.CursorWrapper;
 import android.graphics.Bitmap;
 import android.graphics.drawable.BitmapDrawable;
 import android.net.Uri;
 import android.provider.Browser;
 import android.text.TextUtils;
 import android.util.Log;
+import org.mozilla.gecko.util.IOUtils;
+
+import static org.mozilla.gecko.util.IOUtils.ConsumedInputStream;
+import static org.mozilla.gecko.favicons.LoadFaviconTask.DEFAULT_FAVICON_BUFFER_SIZE;
 
 public class LocalBrowserDB {
     // Calculate these once, at initialization. isLoggable is too expensive to
     // have in-line in each log call.
     private static final String LOGTAG = "GeckoLocalBrowserDB";
-    private static final Integer FAVICON_ID_NOT_FOUND = Integer.MIN_VALUE;
+
+    // Sentinel value used to indicate a failure to locate an ID for a default favicon.
+    private static final int FAVICON_ID_NOT_FOUND = Integer.MIN_VALUE;
 
     private static final boolean logDebug = Log.isLoggable(LOGTAG, Log.DEBUG);
     protected static void debug(String message) {
         if (logDebug) {
             Log.d(LOGTAG, message);
         }
     }
 
@@ -180,40 +189,45 @@ public class LocalBrowserDB {
 
                 final Field urlField = stringsClass.getField(name.replace("_title_", "_url_"));
                 final int urlID = urlField.getInt(null);
                 final String url = context.getString(urlID);
 
                 final ContentValues bookmarkValue = createBookmark(now, title, url, pos++, folderID);
                 bookmarkValues.add(bookmarkValue);
 
-                Bitmap icon = getDefaultFaviconFromPath(context, name);
-                if (icon == null) {
-                    icon = getDefaultFaviconFromDrawable(context, name);
+                ConsumedInputStream faviconStream = getDefaultFaviconFromDrawable(context, name);
+                if (faviconStream == null) {
+                    faviconStream = getDefaultFaviconFromPath(context, name);
                 }
-                if (icon == null) {
+
+                if (faviconStream == null) {
+                    continue;
+                }
+
+                // In the event that truncating the buffer fails, give up and move on.
+                byte[] icon;
+                try {
+                    icon = faviconStream.getTruncatedData();
+                } catch (OutOfMemoryError e) {
                     continue;
                 }
 
                 final ContentValues iconValue = createFavicon(url, icon);
 
                 // Assign a reserved negative _id to each new favicon.
                 // For now, each name is expected to be unique, and duplicate
                 // icons will be duplicated in the DB. See Bug 1040806 Comment 8.
                 if (iconValue != null) {
                     final int faviconID = faviconIDs.get(name);
                     iconValue.put("_id", faviconID);
                     bookmarkValue.put(Bookmarks.FAVICON_ID, faviconID);
                     faviconValues.add(iconValue);
                 }
-            } catch (IllegalAccessException e) {
-                Log.wtf(LOGTAG, "Reflection failure.", e);
-            } catch (IllegalArgumentException e) {
-                Log.wtf(LOGTAG, "Reflection failure.", e);
-            } catch (NoSuchFieldException e) {
+            } catch (IllegalAccessException | IllegalArgumentException | NoSuchFieldException e) {
                 Log.wtf(LOGTAG, "Reflection failure.", e);
             }
         }
 
         if (!faviconValues.isEmpty()) {
             try {
                 cr.bulkInsert(mFaviconsUriWithProfile, faviconValues.toArray(new ContentValues[faviconValues.size()]));
             } catch (Exception e) {
@@ -287,23 +301,23 @@ public class LocalBrowserDB {
 
                 // Return early if there is no icon for this bookmark.
                 if (!bookmark.has("icon")) {
                     continue;
                 }
 
                 try {
                     final String iconData = bookmark.getString("icon");
-                    final Bitmap icon = BitmapUtils.getBitmapFromDataURI(iconData);
+
+                    byte[] icon = BitmapUtils.getBytesFromDataURI(iconData);
                     if (icon == null) {
                         continue;
                     }
 
                     final ContentValues iconValue = createFavicon(url, icon);
-
                     if (iconValue == null) {
                         continue;
                     }
 
                     /*
                      * Find out if this icon is a duplicate. If it is, don't try
                      * to insert it again, but reuse the shared ID.
                      * Otherwise, assign a new reserved negative _id.
@@ -356,31 +370,21 @@ public class LocalBrowserDB {
 
         v.put(Bookmarks.PARENT, parent);
         v.put(Bookmarks.POSITION, pos);
         v.put(Bookmarks.TITLE, title);
         v.put(Bookmarks.URL, url);
         return v;
     }
 
-    private static ContentValues createFavicon(final String url, final Bitmap icon) {
-        ByteArrayOutputStream stream = new ByteArrayOutputStream();
-
+    private static ContentValues createFavicon(final String url, final byte[] icon) {
         ContentValues iconValues = new ContentValues();
         iconValues.put(Favicons.PAGE_URL, url);
+        iconValues.put(Favicons.DATA, icon);
 
-        byte[] data = null;
-        if (icon.compress(Bitmap.CompressFormat.PNG, 100, stream)) {
-            data = stream.toByteArray();
-        } else {
-            Log.w(LOGTAG, "Favicon compression failed.");
-            return null;
-        }
-
-        iconValues.put(Favicons.DATA, data);
         return iconValues;
     }
 
     private static String getLocalizedProperty(final JSONObject bookmark, final String property, final Locale locale) throws JSONException {
         // Try the full locale.
         final String fullLocale = property + "." + locale.toString();
         if (bookmark.has(fullLocale)) {
             return bookmark.getString(fullLocale);
@@ -399,55 +403,63 @@ public class LocalBrowserDB {
         if (bookmark.has(lang)) {
             return bookmark.getString(lang);
         }
 
         // Default to the non-localized property name.
         return bookmark.getString(property);
     }
 
-    private static Bitmap getDefaultFaviconFromPath(Context context, String name) {
-        Class<?> stringClass = R.string.class;
+    private static int getFaviconId(String name) {
         try {
-            // Look for a drawable with the id R.drawable.bookmarkdefaults_favicon_*
-            Field faviconField = stringClass.getField(name.replace("_title_", "_favicon_"));
-            if (faviconField == null) {
-                return null;
-            }
-            int faviconId = faviconField.getInt(null);
-            String path = context.getString(faviconId);
+            Class<?> drawablesClass = R.raw.class;
+
+            // Look for a favicon with the id R.raw.bookmarkdefaults_favicon_*.
+            Field faviconField = drawablesClass.getField(name.replace("_title_", "_favicon_"));
+            faviconField.setAccessible(true);
 
-            String apkPath = context.getPackageResourcePath();
-            File apkFile = new File(apkPath);
-            String bitmapPath = "jar:jar:" + apkFile.toURI() + "!/" + AppConstants.OMNIJAR_NAME + "!/" + path;
-            return GeckoJarReader.getBitmap(context.getResources(), bitmapPath);
-        } catch (java.lang.IllegalAccessException ex) {
-            Log.e(LOGTAG, "[Path] Can't create favicon " + name, ex);
-        } catch (java.lang.NoSuchFieldException ex) {
-            // If the field does not exist, that means we intend to load via a drawable.
+            return faviconField.getInt(null);
+        } catch (IllegalAccessException | NoSuchFieldException  ex) {
+            Log.wtf(LOGTAG, "Reflection error fetching favicon: " + name, ex);
         }
-        return null;
+
+        Log.e(LOGTAG, "Failed to find favicon resource ID for " + name);
+        return FAVICON_ID_NOT_FOUND;
     }
 
-    private static Bitmap getDefaultFaviconFromDrawable(Context context, String name) {
-        Class<?> drawablesClass = R.drawable.class;
-        try {
-            // Look for a drawable with the id R.drawable.bookmarkdefaults_favicon_*
-            Field faviconField = drawablesClass.getField(name.replace("_title_", "_favicon_"));
-            if (faviconField == null) {
-                return null;
-            }
-            int faviconId = faviconField.getInt(null);
-            return BitmapUtils.decodeResource(context, faviconId);
-        } catch (java.lang.IllegalAccessException ex) {
-            Log.e(LOGTAG, "[Drawable] Can't create favicon " + name, ex);
-        } catch (java.lang.NoSuchFieldException ex) {
-            Log.wtf(LOGTAG, "No field, and presumably no drawable, for " + name);
+    /**
+     * Load a favicon from the omnijar.
+     * @return A ConsumedInputStream containing the bytes loaded from omnijar. This must be a format
+     *         compatible with the favicon decoder (most probably a PNG or ICO file).
+     */
+    private static ConsumedInputStream getDefaultFaviconFromPath(Context context, String name) {
+        int faviconId = getFaviconId(name);
+        if (faviconId == FAVICON_ID_NOT_FOUND) {
+            return null;
         }
-        return null;
+
+        String path = context.getString(faviconId);
+
+        String apkPath = context.getPackageResourcePath();
+        File apkFile = new File(apkPath);
+        String bitmapPath = "jar:jar:" + apkFile.toURI() + "!/" + AppConstants.OMNIJAR_NAME + "!/" + path;
+
+        InputStream iStream = GeckoJarReader.getStream(bitmapPath);
+
+        return IOUtils.readFully(iStream, DEFAULT_FAVICON_BUFFER_SIZE);
+    }
+
+    private static ConsumedInputStream getDefaultFaviconFromDrawable(Context context, String name) {
+        int faviconId = getFaviconId(name);
+        if (faviconId == FAVICON_ID_NOT_FOUND) {
+            return null;
+        }
+
+        InputStream iStream = context.getResources().openRawResource(faviconId);
+        return IOUtils.readFully(iStream, DEFAULT_FAVICON_BUFFER_SIZE);
     }
 
     // Invalidate cached data
     public void invalidateCachedState() {
         mDesktopBookmarksExist = null;
     }
 
     private Uri bookmarksUriWithLimit(int limit) {
--- a/mobile/android/base/favicons/LoadFaviconTask.java
+++ b/mobile/android/base/favicons/LoadFaviconTask.java
@@ -15,28 +15,31 @@ import org.apache.http.Header;
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.mozilla.gecko.GeckoAppShell;
 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.URI;
 import java.net.URISyntaxException;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import static org.mozilla.gecko.util.IOUtils.ConsumedInputStream;
+
 /**
  * Class representing the asynchronous task to load a Favicon which is not currently in the in-memory
  * cache.
  * The implementation initially tries to get the Favicon from the database. Upon failure, the icon
  * is loaded from the internet.
  */
 public class LoadFaviconTask {
     private static final String LOGTAG = "LoadFaviconTask";
@@ -44,17 +47,17 @@ public class LoadFaviconTask {
     // Access to this map needs to be synchronized prevent multiple jobs loading the same favicon
     // from executing concurrently.
     private static final HashMap<String, LoadFaviconTask> loadsInFlight = new HashMap<>();
 
     public static final int FLAG_PERSIST = 1;
     private static final int MAX_REDIRECTS_TO_FOLLOW = 5;
     // The default size of the buffer to use for downloading Favicons in the event no size is given
     // by the server.
-    private static final int DEFAULT_FAVICON_BUFFER_SIZE = 25000;
+    public static final int DEFAULT_FAVICON_BUFFER_SIZE = 25000;
 
     private static final AtomicInteger nextFaviconLoadId = new AtomicInteger(0);
     private final Context context;
     private final int id;
     private final String pageUrl;
     private String faviconURL;
     private final OnFaviconLoadedListener listener;
     private final int flags;
@@ -276,52 +279,27 @@ public class LoadFaviconTask {
             // 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;
         } else {
             // No declared size, so guess and reallocate later if it turns out to be too small.
             bufferSize = DEFAULT_FAVICON_BUFFER_SIZE;
         }
 
-        // Allocate a buffer to hold the raw favicon data downloaded.
-        byte[] buffer = new byte[bufferSize];
-
-        // The offset of the start of the buffer's free space.
-        int bPointer = 0;
-
-        // The quantity of bytes the last call to read yielded.
-        int lastRead = 0;
-        InputStream contentStream = entity.getContent();
-        try {
-            // Fully read the entity into the buffer - decoding of streams is not supported
-            // (and questionably pointful - what would one do with a half-decoded Favicon?)
-            while (lastRead != -1) {
-                // Read as many bytes as are currently available into the buffer.
-                lastRead = contentStream.read(buffer, bPointer, buffer.length - bPointer);
-                bPointer += lastRead;
-
-                // If buffer has overflowed, double its size and carry on.
-                if (bPointer == buffer.length) {
-                    bufferSize *= 2;
-                    byte[] newBuffer = new byte[bufferSize];
-
-                    // Copy the contents of the old buffer into the new buffer.
-                    System.arraycopy(buffer, 0, newBuffer, 0, buffer.length);
-                    buffer = newBuffer;
-                }
-            }
-        } finally {
-            contentStream.close();
+        // Read the InputStream into a byte[].
+        ConsumedInputStream result = IOUtils.readFully(entity.getContent(), bufferSize);
+        if (result == null) {
+            return null;
         }
 
         // Having downloaded the image, decode it.
-        return FaviconDecoder.decodeFavicon(buffer, 0, bPointer + 1);
+        return FaviconDecoder.decodeFavicon(result.getData(), 0, result.consumedLength);
     }
 
-    // LoadFavicon tasks are performed on a unique background executor thread
+    // LoadFaviconTasks are performed on a unique background executor thread
     // to avoid network blocking.
     public final void execute() {
         try {
             Favicons.longRunningExecutor.execute(new Runnable() {
                 @Override
                 public void run() {
                     final Bitmap result = doInBackground();
 
--- a/mobile/android/base/gfx/BitmapUtils.java
+++ b/mobile/android/base/gfx/BitmapUtils.java
@@ -336,26 +336,43 @@ public final class BitmapUtils {
      * @param dataURI a Base64-encoded data URI string
      * @return        the decoded bitmap, or null if the data URI is invalid
      */
     public static Bitmap getBitmapFromDataURI(String dataURI) {
         if (dataURI == null) {
             return null;
         }
 
-        final String base64 = dataURI.substring(dataURI.indexOf(',') + 1);
+        byte[] raw = getBytesFromDataURI(dataURI);
+        if (raw == null || raw.length == 0) {
+            return null;
+        }
+
+        return decodeByteArray(raw);
+    }
+
+    /**
+     * Return a byte[] containing the bytes in a given base64 string, or null if this is not a valid
+     * base64 string.
+     */
+    public static byte[] getBytesFromBase64(String base64) {
         try {
-            byte[] raw = Base64.decode(base64, Base64.DEFAULT);
-            return BitmapUtils.decodeByteArray(raw);
+            return Base64.decode(base64, Base64.DEFAULT);
         } catch (Exception e) {
-            Log.e(LOGTAG, "exception decoding bitmap from data URI: " + dataURI, e);
+            Log.e(LOGTAG, "exception decoding bitmap from data URI: " + base64, e);
         }
+
         return null;
     }
 
+    public static byte[] getBytesFromDataURI(String dataURI) {
+        final String base64 = dataURI.substring(dataURI.indexOf(',') + 1);
+        return getBytesFromBase64(base64);
+    }
+
     public static Bitmap getBitmapFromDrawable(Drawable drawable) {
         if (drawable instanceof BitmapDrawable) {
             return ((BitmapDrawable) drawable).getBitmap();
         }
 
         int width = drawable.getIntrinsicWidth();
         width = width > 0 ? width : 1;
         int height = drawable.getIntrinsicHeight();
--- a/mobile/android/base/moz.build
+++ b/mobile/android/base/moz.build
@@ -55,16 +55,17 @@ gujar.sources += [
     'util/GamepadUtils.java',
     'util/GeckoBackgroundThread.java',
     'util/GeckoEventListener.java',
     'util/GeckoJarReader.java',
     'util/GeckoRequest.java',
     'util/HardwareUtils.java',
     'util/INIParser.java',
     'util/INISection.java',
+    'util/IOUtils.java',
     'util/JSONUtils.java',
     'util/MenuUtils.java',
     'util/NativeEventListener.java',
     'util/NativeJSContainer.java',
     'util/NativeJSObject.java',
     'util/NonEvictingLruCache.java',
     'util/PrefUtils.java',
     'util/ProxySelector.java',
rename from mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_addons.png
rename to mobile/android/base/resources/raw/bookmarkdefaults_favicon_addons.png
rename from mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_marketplace.png
rename to mobile/android/base/resources/raw/bookmarkdefaults_favicon_marketplace.png
rename from mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_support.png
rename to mobile/android/base/resources/raw/bookmarkdefaults_favicon_support.png
new file mode 100644
--- /dev/null
+++ b/mobile/android/base/util/IOUtils.java
@@ -0,0 +1,111 @@
+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
+ * 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.util;
+
+import android.util.Log;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+/**
+ * Static helper class containing useful methods for manipulating IO objects.
+ */
+public class IOUtils {
+    private static final String LOGTAG = "GeckoIOUtils";
+
+    /**
+     * Represents the result of consuming an input stream, holding the returned data as well
+     * as the length of the data returned.
+     * The byte[] is not guaranteed to be trimmed to the size of the data acquired from the stream:
+     * hence the need for the length field. This strategy avoids the need to copy the data into a
+     * trimmed buffer after consumption.
+     */
+    public static class ConsumedInputStream {
+        public final int consumedLength;
+        // Only reassigned in getTruncatedData.
+        private byte[] consumedData;
+
+        public ConsumedInputStream(int consumedLength, byte[] consumedData) {
+            this.consumedLength = consumedLength;
+            this.consumedData = consumedData;
+        }
+
+        /**
+         * Get the data trimmed to the length of the actual payload read, caching the result.
+         */
+        public byte[] getTruncatedData() {
+            if (consumedData.length == consumedLength) {
+                return consumedData;
+            }
+
+            consumedData = truncateBytes(consumedData, consumedLength);
+            return consumedData;
+        }
+
+        public byte[] getData() {
+            return consumedData;
+        }
+    }
+
+    /**
+     * Fully read an InputStream into a byte array.
+     * @param iStream the InputStream to consume.
+     * @param bufferSize The initial size of the buffer to allocate. It will be grown as
+     *                   needed, but if the caller knows something about the InputStream then
+     *                   passing a good value here can improve performance.
+     */
+    public static ConsumedInputStream readFully(InputStream iStream, int bufferSize) {
+        // Allocate a buffer to hold the raw data downloaded.
+        byte[] buffer = new byte[bufferSize];
+
+        // The offset of the start of the buffer's free space.
+        int bPointer = 0;
+
+        // The quantity of bytes the last call to read yielded.
+        int lastRead = 0;
+        try {
+            // Fully read the data into the buffer.
+            while (lastRead != -1) {
+                // Read as many bytes as are currently available into the buffer.
+                lastRead = iStream.read(buffer, bPointer, buffer.length - bPointer);
+                bPointer += lastRead;
+
+                // If buffer has overflowed, double its size and carry on.
+                if (bPointer == buffer.length) {
+                    bufferSize *= 2;
+                    byte[] newBuffer = new byte[bufferSize];
+
+                    // Copy the contents of the old buffer into the new buffer.
+                    System.arraycopy(buffer, 0, newBuffer, 0, buffer.length);
+                    buffer = newBuffer;
+                }
+            }
+
+            return new ConsumedInputStream(bPointer + 1, buffer);
+        } catch (IOException e) {
+            Log.e(LOGTAG, "Error consuming input stream.", e);
+        } finally {
+            try {
+                iStream.close();
+            } catch (IOException e) {
+                Log.e(LOGTAG, "Error closing input stream.", e);
+            }
+        }
+
+        return null;
+    }
+
+    /**
+     * Truncate a given byte[] to a given length. Returns a new byte[] with the first length many
+     * bytes of the input.
+     */
+    public static byte[] truncateBytes(byte[] bytes, int length) {
+        byte[] newBytes = new byte[length];
+        System.arraycopy(bytes, 0, newBytes, 0, length);
+
+        return newBytes;
+    }
+}