Bug 1065485: Don't decode-to-encode default favicons. r=rnewman

--HG--
rename : mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_addons.png => mobile/android/base/resources/raw/bookmarkdefaults_favicon_addons.png
rename : mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_marketplace.png => mobile/android/base/resources/raw/bookmarkdefaults_favicon_marketplace.png
rename : mobile/android/base/resources/drawable-mdpi/bookmarkdefaults_favicon_support.png => mobile/android/base/resources/raw/bookmarkdefaults_favicon_support.png

mobile/android/base/db/LocalBrowserDB.java

@@ -7,6 +7,9 @@ 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;
@@ -53,12 +56,18 @@ 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) {
@@ -185,11 +194,20 @@ public class LocalBrowserDB {
                 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;
                 }
 
@@ -204,11 +222,7 @@ public class LocalBrowserDB {
                     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);
             }
         }
@@ -292,13 +306,13 @@ public class LocalBrowserDB {
 
                 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;
                     }
@@ -361,21 +375,11 @@ public class LocalBrowserDB {
         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;
     }
 
@@ -404,45 +408,53 @@ public class LocalBrowserDB {
         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;
 
-            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.
+            // Look for a favicon with the id R.raw.bookmarkdefaults_favicon_*.
+            Field faviconField = drawablesClass.getField(name.replace("_title_", "_favicon_"));
+            faviconField.setAccessible(true);
+
+            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

mobile/android/base/favicons/LoadFaviconTask.java

@@ -20,6 +20,7 @@ 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;
@@ -32,6 +33,8 @@ 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.
@@ -49,7 +52,7 @@ public class LoadFaviconTask {
     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;
@@ -281,42 +284,17 @@ public class LoadFaviconTask {
             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 {

mobile/android/base/gfx/BitmapUtils.java

@@ -341,16 +341,33 @@ public final class BitmapUtils {
             return null;
         }
 
-        final String base64 = dataURI.substring(dataURI.indexOf(',') + 1);
-        try {
-            byte[] raw = Base64.decode(base64, Base64.DEFAULT);
-            return BitmapUtils.decodeByteArray(raw);
-        } catch (Exception e) {
-            Log.e(LOGTAG, "exception decoding bitmap from data URI: " + dataURI, e);
+        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 {
+            return Base64.decode(base64, Base64.DEFAULT);
+        } catch (Exception 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();

mobile/android/base/moz.build

@@ -60,6 +60,7 @@ gujar.sources += [
     'util/HardwareUtils.java',
     'util/INIParser.java',
     'util/INISection.java',
+    'util/IOUtils.java',
     'util/JSONUtils.java',
     'util/MenuUtils.java',
     'util/NativeEventListener.java',

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;
+    }
+}