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 209792 1de6161e995bcd1477ff42d7ecd66ec6034fcc5e
parent 209791 9b598425c7cb675c1673ff33c8e1e1a9f15a8b4b
child 209793 fa1f219e3ebaeac89b707d2250a79e53560413a0
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersrnewman
bugs1065485
milestone35.0a1
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;
+    }
+}