Bug 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman
authorAndrzej Hunt <ahunt@mozilla.com>
Tue, 01 Mar 2016 14:00:22 -0800
changeset 322765 82e8f730b3e18f3be117c5a202f9fead8e7a9e78
parent 322764 a0978b299ee5ec86c1e8ed982dbe04145c7a8b0f
child 322766 a10be60e15925625be398dda0c8db1c168d7fd8d
push id5913
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 16:57:49 +0000
treeherdermozilla-beta@dcaf0a6fa115 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs1252501
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 1252501 - Coalesce topsites position with rowid to ensure a valid position r=rnewman MozReview-Commit-ID: BFcs3sUT0Ff
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -7,16 +7,17 @@ package org.mozilla.gecko.db;
 
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
 import org.mozilla.gecko.AboutPages;
 import org.mozilla.gecko.GeckoProfile;
+import org.mozilla.gecko.R;
 import org.mozilla.gecko.db.BrowserContract.Bookmarks;
 import org.mozilla.gecko.db.BrowserContract.Combined;
 import org.mozilla.gecko.db.BrowserContract.FaviconColumns;
 import org.mozilla.gecko.db.BrowserContract.Favicons;
 import org.mozilla.gecko.db.BrowserContract.History;
 import org.mozilla.gecko.db.BrowserContract.Schema;
 import org.mozilla.gecko.db.BrowserContract.Tabs;
 import org.mozilla.gecko.db.BrowserContract.Thumbnails;
@@ -689,82 +690,79 @@ public class BrowserProvider extends Sha
         // tricks to generate these:
         // 1. The list of free ids is small, hence we can do a self-join to generate rowids.
         // 2. The topsites list is larger, hence we use a temporary table, which automatically provides rowids.
 
         final SQLiteDatabase db = getWritableDatabase(uri);
 
         final String TABLE_TOPSITES = "topsites";
 
-        final String totalLimit = uri.getQueryParameter(BrowserContract.PARAM_LIMIT);
-        final String suggestedGridLimit = uri.getQueryParameter(BrowserContract.PARAM_SUGGESTEDSITES_LIMIT);
+        final String limitParam = uri.getQueryParameter(BrowserContract.PARAM_LIMIT);
+        final String gridLimitParam = uri.getQueryParameter(BrowserContract.PARAM_SUGGESTEDSITES_LIMIT);
 
-        final String[] suggestedGridLimitArgs = new String[] {
-                suggestedGridLimit
-        };
+        final int totalLimit;
+        final int suggestedGridLimit;
 
-        final String[] totalLimitArgs = new String[] {
-                totalLimit
-        };
+        if (limitParam == null) {
+            totalLimit = 50;
+        } else {
+            totalLimit = Integer.parseInt(limitParam, 10);
+        }
 
-        final String pinnedSitesFromClause = "FROM " + TABLE_BOOKMARKS + " WHERE " + Bookmarks.PARENT + " == ?";
-        final String[] pinnedSitesArgs = new String[] {
-                String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID)
-        };
+        if (gridLimitParam == null) {
+            suggestedGridLimit = getContext().getResources().getInteger(R.integer.number_of_top_sites);
+        } else {
+            suggestedGridLimit = Integer.parseInt(gridLimitParam, 10);
+        }
+
+        final String pinnedSitesFromClause = "FROM " + TABLE_BOOKMARKS + " WHERE " + Bookmarks.PARENT + " == " + Bookmarks.FIXED_PINNED_LIST_ID;
 
         // Ideally we'd use a recursive CTE to generate our sequence, e.g. something like this worked at one point:
         // " WITH RECURSIVE" +
         // " cnt(x) AS (VALUES(1) UNION ALL SELECT x+1 FROM cnt WHERE x < 6)" +
         // However that requires SQLite >= 3.8.3 (available on Android >= 5.0), so in the meantime
         // we use a temporary numbers table.
         // Note: SQLite rowids are 1-indexed, whereas we're expecting 0-indexed values for the position. Our numbers
         // table starts at position = 0, which ensures the correct results here.
         final String freeIDSubquery =
                 " SELECT count(free_ids.position) + 1 AS rowid, numbers.position AS " + Bookmarks.POSITION +
                 " FROM (SELECT position FROM numbers WHERE position NOT IN (SELECT " + Bookmarks.POSITION + " " + pinnedSitesFromClause + ")) AS numbers" +
                 " LEFT OUTER JOIN " +
                 " (SELECT position FROM numbers WHERE position NOT IN (SELECT " + Bookmarks.POSITION + " " + pinnedSitesFromClause + ")) AS free_ids" +
                 " ON numbers.position > free_ids.position" +
                 " GROUP BY numbers.position" +
                 " ORDER BY numbers.position ASC" +
-                " LIMIT ?";
-
-        final String[] freeIDArgs = DBUtils.concatenateSelectionArgs(
-                pinnedSitesArgs,
-                pinnedSitesArgs,
-                suggestedGridLimitArgs);
+                " LIMIT " + suggestedGridLimit;
 
         // Filter out: unvisited pages (history_id == -1) pinned (and other special) sites, deleted sites,
         // and about: pages.
         final String ignoreForTopSitesWhereClause =
                 "(" + Combined.HISTORY_ID + " IS NOT -1)" +
                 " AND " +
                 Combined.URL + " NOT IN (SELECT " +
                 Bookmarks.URL + " FROM bookmarks WHERE " +
-                DBUtils.qualifyColumn("bookmarks", Bookmarks.PARENT) + " < ? AND " +
+                DBUtils.qualifyColumn("bookmarks", Bookmarks.PARENT) + " < " + Bookmarks.FIXED_ROOT_ID + " AND " +
                 DBUtils.qualifyColumn("bookmarks", Bookmarks.IS_DELETED) + " == 0)" +
                 " AND " +
                 "(" + Combined.URL + " NOT LIKE ?)";
 
         final String[] ignoreForTopSitesArgs = new String[] {
-                String.valueOf(Bookmarks.FIXED_ROOT_ID),
                 AboutPages.URL_FILTER
         };
 
-
         // Stuff the suggested sites into SQL: this allows us to filter pinned and topsites out of the suggested
         // sites list as part of the final query (as opposed to walking cursors in java)
         final SuggestedSites suggestedSites = GeckoProfile.get(getContext(), uri.getQueryParameter(BrowserContract.PARAM_PROFILE)).getDB().getSuggestedSites();
 
         StringBuilder suggestedSitesBuilder = new StringBuilder();
         // We could access the underlying data here, however SuggestedSites also performs filtering on the suggested
         // sites list, which means we'd need to process the lists within SuggestedSites in any case. If we're doing
         // that processing, there is little real between us using a MatrixCursor, or a Map (or List) instead of the
         // MatrixCursor.
-        final Cursor suggestedSitesCursor = suggestedSites.get(Integer.parseInt(suggestedGridLimit));
+        final Cursor suggestedSitesCursor = suggestedSites.get(suggestedGridLimit);
 
         String[] suggestedSiteArgs = new String[0];
 
         boolean hasProcessedAnySuggestedSites = true;
 
         final int idColumnIndex = suggestedSitesCursor.getColumnIndexOrThrow(Bookmarks._ID);
         final int urlColumnIndex = suggestedSitesCursor.getColumnIndexOrThrow(Bookmarks.URL);
         final int titleColumnIndex = suggestedSitesCursor.getColumnIndexOrThrow(Bookmarks.TITLE);
@@ -786,21 +784,18 @@ public class BrowserProvider extends Sha
                                                                     suggestedSitesCursor.getString(idColumnIndex),
                                                                     suggestedSitesCursor.getString(urlColumnIndex),
                                                                     suggestedSitesCursor.getString(titleColumnIndex)
                                                             });
         }
 
         // To restrict suggested sites to the grid, we simply subtract the number of topsites (which have already had
         // the pinned sites filtered out), and the number of pinned sites.
-        // SQLite completely ignores negative limits, hence we need to manually totalLimit to 0 in this case.
-        final String suggestedLimitClause = " LIMIT MAX(0, (? - (SELECT COUNT(*) FROM " + TABLE_TOPSITES + ") - (SELECT COUNT(*) " + pinnedSitesFromClause + "))) ";
-
-        final String[] suggestedLimitArgs = DBUtils.concatenateSelectionArgs(suggestedGridLimitArgs,
-                                                                             pinnedSitesArgs);
+        // SQLite completely ignores negative limits, hence we need to manually limit to 0 in this case.
+        final String suggestedLimitClause = " LIMIT MAX(0, (" + suggestedGridLimit + " - (SELECT COUNT(*) FROM " + TABLE_TOPSITES + ") - (SELECT COUNT(*) " + pinnedSitesFromClause + "))) ";
 
         db.beginTransaction();
         try {
             db.execSQL("DROP TABLE IF EXISTS " + TABLE_TOPSITES);
 
             db.execSQL("CREATE TEMP TABLE " + TABLE_TOPSITES + " AS" +
                        " SELECT " +
                        Bookmarks._ID + ", " +
@@ -808,20 +803,19 @@ public class BrowserProvider extends Sha
                        Combined.HISTORY_ID + ", " +
                        Bookmarks.URL + ", " +
                        Bookmarks.TITLE + ", " +
                        Combined.HISTORY_ID + ", " +
                        TopSites.TYPE_TOP + " AS " + TopSites.TYPE +
                        " FROM " + Combined.VIEW_NAME +
                        " WHERE " + ignoreForTopSitesWhereClause +
                        " ORDER BY " + BrowserContract.getFrecencySortOrder(true, false) +
-                       " LIMIT ?",
+                       " LIMIT " + totalLimit,
 
-                       DBUtils.appendSelectionArgs(ignoreForTopSitesArgs,
-                                                   totalLimitArgs));
+                       ignoreForTopSitesArgs);
 
             if (!hasProcessedAnySuggestedSites) {
                 db.execSQL("INSERT INTO " + TABLE_TOPSITES +
                            // We need to LIMIT _after_ selecting the relevant suggested sites, which requires us to
                            // use an additional internal subquery, since we cannot LIMIT a subquery that is part of UNION ALL.
                            // Hence the weird SELECT * FROM (SELECT ...relevant suggested sites... LIMIT ?)
                            " SELECT * FROM (SELECT " +
                            Bookmarks._ID + ", " +
@@ -833,29 +827,37 @@ public class BrowserProvider extends Sha
                            TopSites.TYPE_SUGGESTED + " as " + TopSites.TYPE +
                            " FROM ( " + suggestedSitesBuilder.toString() + " )" +
                            " WHERE " +
                            Bookmarks.URL + " NOT IN (SELECT url FROM " + TABLE_TOPSITES + ")" +
                            " AND " +
                            Bookmarks.URL + " NOT IN (SELECT url " + pinnedSitesFromClause + ")" +
                            suggestedLimitClause + " )",
 
-                           DBUtils.concatenateSelectionArgs(suggestedSiteArgs,
-                                                            pinnedSitesArgs,
-                                                            suggestedLimitArgs));
+                           suggestedSiteArgs);
             }
 
+            // If we retrieve more topsites than we have free positions for in the freeIdSubquery,
+            // we will have topsites that don't receive a position when joining TABLE_TOPSITES
+            // with freeIdSubquery. Hence we need to coalesce the position with a generated position.
+            // We know that the difference in positions will be at most suggestedGridLimit, hence we
+            // can add that to the rowid to generate a safe position.
+            // I.e. if we have 6 pinned sites then positions 0..5 are filled, the JOIN results in
+            // the first N rows having positions 6..(N+6), so row N+1 should receive a position that is at
+            // least N+1+6, which is equal to rowid + 6.
             final SQLiteCursor c = (SQLiteCursor) db.rawQuery(
                         "SELECT " +
                         Bookmarks._ID + ", " +
                         TopSites.BOOKMARK_ID + ", " +
                         TopSites.HISTORY_ID + ", " +
                         Bookmarks.URL + ", " +
                         Bookmarks.TITLE + ", " +
-                        Bookmarks.POSITION + ", " +
+                        "COALESCE(" + Bookmarks.POSITION + ", " +
+                            DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") + " + " + suggestedGridLimit +
+                        ")" + " AS " + Bookmarks.POSITION + ", " +
                         Combined.HISTORY_ID + ", " +
                         TopSites.TYPE +
                         " FROM " + TABLE_TOPSITES +
                         " LEFT OUTER JOIN " + // TABLE_IDS +
                         "(" + freeIDSubquery + ") AS id_results" +
                         " ON " + DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") +
                         " = " + DBUtils.qualifyColumn("id_results", "rowid") +
 
@@ -866,22 +868,21 @@ public class BrowserProvider extends Sha
                         Bookmarks._ID + " AS " + TopSites.BOOKMARK_ID + ", " +
                         " -1 AS " + TopSites.HISTORY_ID + ", " +
                         Bookmarks.URL + ", " +
                         Bookmarks.TITLE + ", " +
                         Bookmarks.POSITION + ", " +
                         "NULL AS " + Combined.HISTORY_ID + ", " +
                         TopSites.TYPE_PINNED + " as " + TopSites.TYPE +
                         " FROM " + TABLE_BOOKMARKS +
-                        " WHERE " + Bookmarks.PARENT + " == ? " +
+                        " WHERE " + Bookmarks.PARENT + " == " + Bookmarks.FIXED_PINNED_LIST_ID +
 
                         " ORDER BY " + Bookmarks.POSITION,
 
-                         DBUtils.appendSelectionArgs(freeIDArgs,
-                                                     pinnedSitesArgs));
+                        null);
 
             c.setNotificationUri(getContext().getContentResolver(),
                                  BrowserContract.AUTHORITY_URI);
 
             // Force the cursor to be compiled and the cursor-window filled now:
             // (A) without compiling the cursor now we won't have access to the TEMP table which
             // is removed as soon as we close our connection.
             // (B) this might also mitigate the situation causing this crash where we're accessing