Bug 919768 - Slowdown and OOM hit visiting top-sites in about:home. r=sriram, a=lsblakk
authorRichard Newman <rnewman@mozilla.com>
Mon, 30 Sep 2013 20:56:09 -0700
changeset 160644 e78a73708c7ac254fb1e6627e5647ffbe62e6d53
parent 160643 968f4dec5f9de6ef146012374e3d00f6fbd3aa6e
child 160645 cf7f2c1c241b573f7995e9e9fbfbe32513615e35
push id2961
push userlsblakk@mozilla.com
push dateMon, 28 Oct 2013 21:59:28 +0000
treeherdermozilla-beta@73ef4f13486f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssriram, lsblakk
bugs919768
milestone26.0a2
Bug 919768 - Slowdown and OOM hit visiting top-sites in about:home. r=sriram, a=lsblakk
mobile/android/base/db/LocalBrowserDB.java
mobile/android/base/home/TopSitesPage.java
--- a/mobile/android/base/db/LocalBrowserDB.java
+++ b/mobile/android/base/db/LocalBrowserDB.java
@@ -848,30 +848,48 @@ public class LocalBrowserDB implements B
         } finally {
             if (c != null)
                 c.close();
         }
 
         return b;
     }
 
+    /**
+     * Query for non-null thumbnails matching the provided <code>urls</code>.
+     * The returned cursor will have no more than, but possibly fewer than,
+     * the requested number of thumbnails.
+     *
+     * Returns null if the provided list of URLs is empty or null.
+     */
     @Override
     public Cursor getThumbnailsForUrls(ContentResolver cr, List<String> urls) {
-        StringBuilder selection = new StringBuilder();
-        String[] selectionArgs = new String[urls.size()];
+        if (urls == null) {
+            return null;
+        }
 
-        for (int i = 0; i < urls.size(); i++) {
-          final String url = urls.get(i);
+        int urlCount = urls.size();
+        if (urlCount == 0) {
+            return null;
+        }
 
-          if (i > 0)
-            selection.append(" OR ");
+        // Don't match against null thumbnails.
+        StringBuilder selection = new StringBuilder(
+                Thumbnails.DATA + " IS NOT NULL AND " +
+                Thumbnails.URL + " IN ("
+        );
 
-          selection.append(Thumbnails.URL + " = ?");
-          selectionArgs[i] = url;
+        // Compute a (?, ?, ?) sequence to match the provided URLs.
+        int i = 1;
+        while (i++ < urlCount) {
+            selection.append("?, ");
         }
+        selection.append("?)");
+
+        String[] selectionArgs = urls.toArray(new String[urlCount]);
 
         return cr.query(mThumbnailsUriWithProfile,
                         new String[] { Thumbnails.URL, Thumbnails.DATA },
                         selection.toString(),
                         selectionArgs,
                         null);
     }
 
--- a/mobile/android/base/home/TopSitesPage.java
+++ b/mobile/android/base/home/TopSitesPage.java
@@ -596,39 +596,62 @@ public class TopSitesPage extends HomeFr
         public View newView(Context context, Cursor cursor, ViewGroup parent) {
             return new TopSitesGridItemView(context);
         }
     }
 
     private class CursorLoaderCallbacks implements LoaderCallbacks<Cursor> {
         @Override
         public Loader<Cursor> onCreateLoader(int id, Bundle args) {
+            Log.d(LOGTAG, "Creating TopSitesLoader: " + id);
             return new TopSitesLoader(getActivity());
         }
 
+        /**
+         * This method is called *twice* in some circumstances.
+         *
+         * If you try to avoid that through some kind of boolean flag,
+         * sometimes (e.g., returning to the activity) you'll *not* be called
+         * twice, and thus you'll never draw thumbnails.
+         *
+         * The root cause is TopSitesLoader.loadCursor being called twice.
+         * Why that is... dunno.
+         */
         @Override
         public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
+            Log.d(LOGTAG, "onLoadFinished: " + c.getCount() + " rows.");
+
             mListAdapter.swapCursor(c);
             mGridAdapter.swapCursor(c);
             updateUiFromCursor(c);
 
+            final int col = c.getColumnIndexOrThrow(URLColumns.URL);
+
             // Load the thumbnails.
-            if (c.getCount() > 0 && c.moveToFirst()) {
-                final ArrayList<String> urls = new ArrayList<String>();
-                do {
-                    final String url = c.getString(c.getColumnIndexOrThrow(URLColumns.URL));
-                    urls.add(url);
-                } while (c.moveToNext());
+            // Even though the cursor we're given is supposed to be fresh,
+            // we get a bad first value unless we reset its position.
+            // Using move(-1) and moveToNext() doesn't work correctly under
+            // rotation, so we use moveToFirst.
+            if (!c.moveToFirst()) {
+                return;
+            }
+            
+            final ArrayList<String> urls = new ArrayList<String>();
+            int i = 1;
+            do {
+                urls.add(c.getString(col));
+            } while (i++ < mMaxGridEntries && c.moveToNext());
 
-                if (urls.size() > 0) {
-                    Bundle bundle = new Bundle();
-                    bundle.putStringArrayList(THUMBNAILS_URLS_KEY, urls);
-                    getLoaderManager().restartLoader(LOADER_ID_THUMBNAILS, bundle, mThumbnailsLoaderCallbacks);
-                }
+            if (urls.isEmpty()) {
+                return;
             }
+
+            Bundle bundle = new Bundle();
+            bundle.putStringArrayList(THUMBNAILS_URLS_KEY, urls);
+            getLoaderManager().restartLoader(LOADER_ID_THUMBNAILS, bundle, mThumbnailsLoaderCallbacks);
         }
 
         @Override
         public void onLoaderReset(Loader<Cursor> loader) {
             if (mListAdapter != null) {
                 mListAdapter.swapCursor(null);
             }
 
@@ -651,39 +674,53 @@ public class TopSitesPage extends HomeFr
         }
 
         @Override
         public Map<String, Thumbnail> loadInBackground() {
             if (mUrls == null || mUrls.size() == 0) {
                 return null;
             }
 
-            final Map<String, Thumbnail> thumbnails = new HashMap<String, Thumbnail>();
-
             // Query the DB for thumbnails.
             final ContentResolver cr = getContext().getContentResolver();
             final Cursor cursor = BrowserDB.getThumbnailsForUrls(cr, mUrls);
 
+            if (cursor == null) {
+                return null;
+            }
+
+            final Map<String, Thumbnail> thumbnails = new HashMap<String, Thumbnail>();
+
             try {
-                if (cursor != null && cursor.moveToFirst()) {
-                    do {
-                        // Try to get the thumbnail, if cursor is valid.
-                        String url = cursor.getString(cursor.getColumnIndexOrThrow(Thumbnails.URL));
-                        final byte[] b = cursor.getBlob(cursor.getColumnIndexOrThrow(Thumbnails.DATA));
-                        final Bitmap bitmap = (b == null ? null : BitmapUtils.decodeByteArray(b));
+                final int urlIndex = cursor.getColumnIndexOrThrow(Thumbnails.URL);
+                final int dataIndex = cursor.getColumnIndexOrThrow(Thumbnails.DATA);
+
+                while (cursor.moveToNext()) {
+                    String url = cursor.getString(urlIndex);
+
+                    // This should never be null, but if it is...
+                    final byte[] b = cursor.getBlob(dataIndex);
+                    if (b == null) {
+                        continue;
+                    }
 
-                        if (bitmap != null) {
-                            thumbnails.put(url, new Thumbnail(bitmap, true));
-                        }
-                    } while (cursor.moveToNext());
+                    final Bitmap bitmap = BitmapUtils.decodeByteArray(b);
+
+                    // Our thumbnails are never null, so if we get a null decoded
+                    // bitmap, it's because we hit an OOM or some other disaster.
+                    // Give up immediately rather than hammering on.
+                    if (bitmap == null) {
+                        Log.w(LOGTAG, "Aborting thumbnail load; decode failed.");
+                        break;
+                    }
+
+                    thumbnails.put(url, new Thumbnail(bitmap, true));
                 }
             } finally {
-                if (cursor != null) {
-                    cursor.close();
-                }
+                cursor.close();
             }
 
             // Query the DB for favicons for the urls without thumbnails.
             for (String url : mUrls) {
                 if (!thumbnails.containsKey(url)) {
                     final Bitmap bitmap = BrowserDB.getFaviconForUrl(cr, url);
                     if (bitmap != null) {
                         // Favicons.scaleImage can return several different size favicons,