Bug 713524 - Batch bookmark inserts. r=rnewman, a=android-only

mobile/android/base/sync/Utils.java

@@ -1,40 +1,6 @@
-/* ***** BEGIN LICENSE BLOCK *****
- * Version: MPL 1.1/GPL 2.0/LGPL 2.1
- *
- * The contents of this file are subject to the Mozilla Public License Version
- * 1.1 (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- * http://www.mozilla.org/MPL/
- *
- * Software distributed under the License is distributed on an "AS IS" basis,
- * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
- * for the specific language governing rights and limitations under the
- * License.
- *
- * The Original Code is Android Sync Client.
- *
- * The Initial Developer of the Original Code is
- * the Mozilla Foundation.
- * Portions created by the Initial Developer are Copyright (C) 2011
- * the Initial Developer. All Rights Reserved.
- *
- * Contributor(s):
- * Jason Voll <jvoll@mozilla.com>
- * Richard Newman <rnewman@mozilla.com>
- *
- * Alternatively, the contents of this file may be used under the terms of
- * either the GNU General Public License Version 2 or later (the "GPL"), or
- * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
- * in which case the provisions of the GPL or the LGPL are applicable instead
- * of those above. If you wish to allow use of your version of this file only
- * under the terms of either the GPL or the LGPL, and not to allow others to
- * use your version of this file under the terms of the MPL, indicate your
- * decision by deleting the provisions above and replace them with the notice
- * and other provisions required by the GPL or the LGPL. If you do not delete
- * the provisions above, a recipient may use your version of this file under
- * the terms of any one of the MPL, the GPL or the LGPL.
- *
- * ***** END LICENSE BLOCK ***** */
+/* 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.sync;
 
@@ -46,6 +12,7 @@ import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.security.SecureRandom;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
@@ -309,4 +276,26 @@ public class Utils {
     }
     return out;
   }
+
+  // Because TextUtils.join is not stubbed.
+  public static String toDelimitedString(String delimiter, Collection<String> items) {
+    if (items == null || items.size() == 0) {
+      return "";
+    }
+
+    StringBuilder sb = new StringBuilder();
+    int i = 0;
+    int c = items.size();
+    for (String string : items) {
+      sb.append(string);
+      if (++i < c) {
+        sb.append(delimiter);
+      }
+    }
+    return sb.toString();
+  }
+
+  public static String toCommaSeparatedString(Collection<String> items) {
+    return toDelimitedString(", ", items);
+  }
 }

mobile/android/base/sync/repositories/Server11RepositorySession.java

@@ -223,6 +223,8 @@ public class Server11RepositorySession extends RepositorySession {
   }
 
   private String flattenIDs(String[] guids) {
+    // Consider using Utils.toDelimitedString if and when the signature changes
+    // to Collection<String> guids.
     if (guids.length == 0) {
       return "";
     }

mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java

@@ -5,6 +5,7 @@
 package org.mozilla.gecko.sync.repositories.android;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -29,15 +30,20 @@ import org.mozilla.gecko.sync.repositories.delegates.RepositorySessionWipeDelega
 import org.mozilla.gecko.sync.repositories.domain.BookmarkRecord;
 import org.mozilla.gecko.sync.repositories.domain.Record;
 
+import android.content.ContentUris;
 import android.content.Context;
 import android.database.Cursor;
+import android.net.Uri;
 
-public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepositorySession {
+public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepositorySession
+  implements BookmarksInsertionManager.BookmarkInserter {
 
   public static final int DEFAULT_DELETION_FLUSH_THRESHOLD = 50;
+  public static final int DEFAULT_INSERTION_FLUSH_THRESHOLD = 50;
+
   // TODO: synchronization for these.
-  private HashMap<String, Long> guidToID = new HashMap<String, Long>();
-  private HashMap<Long, String> idToGuid = new HashMap<Long, String>();
+  private HashMap<String, Long> parentGuidToIDMap = new HashMap<String, Long>();
+  private HashMap<Long, String> parentIDToGuidMap = new HashMap<Long, String>();
 
   /**
    * Some notes on reparenting/reordering.
@@ -101,6 +107,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
   private AndroidBrowserBookmarksDataAccessor dataAccessor;
 
   protected BookmarksDeletionManager deletionManager;
+  protected BookmarksInsertionManager insertionManager;
 
   /**
    * An array of known-special GUIDs.
@@ -227,13 +234,13 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
   }
 
   private String getGUIDForID(long androidID) {
-    String guid = idToGuid.get(androidID);
+    String guid = parentIDToGuidMap.get(androidID);
     trace("  " + androidID + " => " + guid);
     return guid;
   }
 
   private long getIDForGUID(String guid) {
-    Long id = guidToID.get(guid);
+    Long id = parentGuidToIDMap.get(guid);
     if (id == null) {
       Logger.warn(LOG_TAG, "Couldn't find local ID for GUID " + guid);
       return -1;
@@ -419,7 +426,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     if (androidParentGUID == null) {
       Logger.debug(LOG_TAG, "No parent GUID for record " + recordGUID + " with parent " + androidParentID);
       // If the parent has been stored and somehow has a null GUID, throw an error.
-      if (idToGuid.containsKey(androidParentID)) {
+      if (parentIDToGuidMap.containsKey(androidParentID)) {
         Logger.error(LOG_TAG, "Have the parent android ID for the record but the parent's GUID wasn't found.");
         throw new NoGuidForIdException(null);
       }
@@ -480,7 +487,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
       return null;
     }
 
-    long androidID = guidToID.get(recordGUID);
+    long androidID = parentGuidToIDMap.get(recordGUID);
     JSONArray childArray = getChildrenArray(androidID, persist);
     if (childArray == null) {
       return null;
@@ -512,7 +519,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     Logger.debug(LOG_TAG, "Ignoring record with guid: " + bmk.guid + " and type: " + bmk.type);
     return true;
   }
-  
+
   @Override
   public void begin(RepositorySessionBeginDelegate delegate) throws InvalidSessionTransitionException {
     // Check for the existence of special folders
@@ -534,7 +541,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
       delegate.onBeginFailed(e);
       return;
     }
-    
+
     // To deal with parent mapping of bookmarks we have to do some
     // hairy stuff. Here's the setup for it.
 
@@ -542,15 +549,15 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
 
     // Fake our root.
     Logger.debug(LOG_TAG, "Tracking places root as ID 0.");
-    idToGuid.put(0L, "places");
-    guidToID.put("places", 0L);
+    parentIDToGuidMap.put(0L, "places");
+    parentGuidToIDMap.put("places", 0L);
     try {
       cur.moveToFirst();
       while (!cur.isAfterLast()) {
         String guid = getGUID(cur);
         long id = RepoUtils.getLongFromCursor(cur, BrowserContract.Bookmarks._ID);
-        guidToID.put(guid, id);
-        idToGuid.put(id, guid);
+        parentGuidToIDMap.put(guid, id);
+        parentIDToGuidMap.put(id, guid);
         Logger.debug(LOG_TAG, "GUID " + guid + " maps to " + id);
         cur.moveToNext();
       }
@@ -558,14 +565,88 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
       cur.close();
     }
     deletionManager = new BookmarksDeletionManager(dataAccessor, DEFAULT_DELETION_FLUSH_THRESHOLD);
+
+    // We just crawled the database enumerating all folders; we'll start the
+    // insertion manager with exactly these folders as the known parents (the
+    // collection is copied) in the manager constructor.
+    insertionManager = new BookmarksInsertionManager(DEFAULT_INSERTION_FLUSH_THRESHOLD, parentGuidToIDMap.keySet(), this);
+
     Logger.debug(LOG_TAG, "Done with initial setup of bookmarks session.");
     super.begin(delegate);
   }
 
+  /**
+   * Implement method of BookmarksInsertionManager.BookmarkInserter.
+   */
+  @Override
+  public boolean insertFolder(BookmarkRecord record) {
+    // A folder that is *not* deleted needs its androidID updated, so that
+    // updateBookkeeping can re-parent, etc.
+    Record toStore = prepareRecord(record);
+    try {
+      Uri recordURI = dbHelper.insert(toStore);
+      if (recordURI == null) {
+        delegate.onRecordStoreFailed(new RuntimeException("Got null URI inserting folder with guid " + toStore.guid + "."));
+        return false;
+      }
+      toStore.androidID = ContentUris.parseId(recordURI);
+      Logger.debug(LOG_TAG, "Inserted folder with guid " + toStore.guid + " as androidID " + toStore.androidID);
+
+      updateBookkeeping(toStore);
+    } catch (Exception e) {
+      delegate.onRecordStoreFailed(e);
+      return false;
+    }
+    trackRecord(toStore);
+    delegate.onRecordStoreSucceeded(toStore);
+    return true;
+  }
+
+  /**
+   * Implement method of BookmarksInsertionManager.BookmarkInserter.
+   */
+  @Override
+  public void bulkInsertNonFolders(Collection<BookmarkRecord> records) {
+    // All of these records are *not* deleted and *not* folders, so we don't
+    // need to update androidID at all!
+    // TODO: persist records that fail to insert for later retry.
+    ArrayList<Record> toStores = new ArrayList<Record>(records.size());
+    for (Record record : records) {
+      toStores.add(prepareRecord(record));
+    }
+
+    try {
+      int stored = dataAccessor.bulkInsert(toStores);
+      if (stored != toStores.size()) {
+        // Something failed; most pessimistic action is to declare that all insertions failed.
+        // TODO: perform the bulkInsert in a transaction and rollback unless all insertions succeed?
+        for (Record failed : toStores) {
+          delegate.onRecordStoreFailed(new RuntimeException("Possibly failed to bulkInsert non-folder with guid " + failed.guid + "."));
+        }
+        return;
+      }
+    } catch (NullCursorException e) {
+      delegate.onRecordStoreFailed(e); // TODO: include which records failed.
+      return;
+    }
+
+    // Success For All!
+    for (Record succeeded : toStores) {
+      try {
+        updateBookkeeping(succeeded);
+      } catch (Exception e) {
+        Logger.warn(LOG_TAG, "Got exception updating bookkeeping of non-folder with guid " + succeeded.guid + ".", e);
+      }
+      trackRecord(succeeded);
+      delegate.onRecordStoreSucceeded(succeeded);
+    }
+  }
+
   @Override
   public void finish(RepositorySessionFinishDelegate delegate) throws InactiveSessionException {
-    // Allow this to be GCed.
+    // Allow these to be GCed.
     deletionManager = null;
+    insertionManager = null;
 
     // Override finish to do this check; make sure all records
     // needing re-parenting have been re-parented.
@@ -671,8 +752,8 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
    * @param bmk
    */
   private void handleParenting(BookmarkRecord bmk) {
-    if (guidToID.containsKey(bmk.parentID)) {
-      bmk.androidParentID = guidToID.get(bmk.parentID);
+    if (parentGuidToIDMap.containsKey(bmk.parentID)) {
+      bmk.androidParentID = parentGuidToIDMap.get(bmk.parentID);
 
       // Might as well set a basic position from the downloaded children array.
       JSONArray children = parentToChildArray.get(bmk.parentID);
@@ -684,7 +765,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
       }
     }
     else {
-      bmk.androidParentID = guidToID.get("unfiled");
+      bmk.androidParentID = parentGuidToIDMap.get("unfiled");
       ArrayList<String> children;
       if (missingParentToChildren.containsKey(bmk.parentID)) {
         children = missingParentToChildren.get(bmk.parentID);
@@ -719,8 +800,8 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     // Mappings between ID and GUID.
     // TODO: update our persisted children arrays!
     // TODO: if our Android ID just changed, replace parents for all of our children.
-    guidToID.put(bmk.guid,      bmk.androidID);
-    idToGuid.put(bmk.androidID, bmk.guid);
+    parentGuidToIDMap.put(bmk.guid,      bmk.androidID);
+    parentIDToGuidMap.put(bmk.androidID, bmk.guid);
 
     JSONArray childArray = bmk.children;
 
@@ -742,6 +823,15 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     }
   }
 
+  @Override
+  protected void insert(Record record) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
+    try {
+      insertionManager.enqueueRecord((BookmarkRecord) record);
+    } catch (Exception e) {
+      throw new NullCursorException(e);
+    }
+  }
+
   @Override
   protected void storeRecordDeletion(final Record record, final Record existingRecord) {
     if (SPECIAL_GUIDS_MAP.containsKey(record.guid)) {
@@ -755,10 +845,18 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
     deletionManager.deleteRecord(bookmarkRecord.guid, isFolder, parentGUID);
   }
 
-  protected void flushDeletions() {
+  protected void flushQueues() {
+    long now = now();
+    Logger.debug(LOG_TAG, "Applying remaining insertions.");
+    try {
+      insertionManager.finishUp();
+      Logger.debug(LOG_TAG, "Done applying remaining insertions.");
+    } catch (Exception e) {
+      Logger.warn(LOG_TAG, "Unable to apply remaining insertions.", e);
+    }
+
     Logger.debug(LOG_TAG, "Applying deletions.");
     try {
-      long now = now();
       untrackGUIDs(deletionManager.flushAll(getIDForGUID("unfiled"), now));
       Logger.debug(LOG_TAG, "Done applying deletions.");
     } catch (Exception e) {
@@ -769,7 +867,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
   @SuppressWarnings("unchecked")
   private void finishUp() {
     try {
-      flushDeletions();
+      flushQueues();
       Logger.debug(LOG_TAG, "Have " + parentToChildArray.size() + " folders whose children might need repositioning.");
       for (Entry<String, JSONArray> entry : parentToChildArray.entrySet()) {
         String guid = entry.getKey();
@@ -824,6 +922,7 @@ public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepo
       try {
         // Clear our queued deletions.
         deletionManager.clear();
+        insertionManager.clear();
         super.run();
       } catch (Exception ex) {
         delegate.onWipeFailed(ex);

mobile/android/base/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java

@@ -18,7 +18,6 @@ import org.mozilla.gecko.sync.repositories.domain.Record;
 
 import android.content.ContentValues;
 import android.content.Context;
-import android.database.Cursor;
 import android.net.Uri;
 
 public class AndroidBrowserHistoryDataAccessor extends
@@ -108,17 +107,15 @@ public class AndroidBrowserHistoryDataAccessor extends
    * This inserts all the records (using <code>ContentProvider.bulkInsert</code>),
    * then inserts all the visit information (using the data extender's
    * <code>bulkInsert</code>, which internally uses a single database
-   * transaction), and then optionally updates the <code>androidID</code> of
-   * each record.
+   * transaction).
    *
    * @param records
-   *          The records to insert.
-   * @param fetchFreshAndroidIDs
-   *          <code>true</code> to update the <code>androidID</code> of each
-   *          record; <code>false</code> to invalidate them all.
+   *          the records to insert.
+   * @return
+   *          the number of records actually inserted.
    * @throws NullCursorException
    */
-  public void bulkInsert(ArrayList<HistoryRecord> records, boolean fetchFreshAndroidIDs) throws NullCursorException {
+  public int bulkInsert(ArrayList<HistoryRecord> records) throws NullCursorException {
     if (records.isEmpty()) {
       Logger.debug(LOG_TAG, "No records to insert, returning.");
     }
@@ -149,37 +146,6 @@ public class AndroidBrowserHistoryDataAccessor extends
     }
     // Then update the history visits.
     dataExtender.bulkInsert(records);
-
-    // And finally patch up the androidIDs.
-    if (!fetchFreshAndroidIDs) {
-      return;
-    }
-
-    // We do this here to save a few loops.
-    String guidIn = RepoUtils.computeSQLInClause(guids.length, BrowserContract.History.GUID);
-    Cursor cursor = queryHelper.safeQuery("", GUID_AND_ID, guidIn, guids, null);
-    int guidIndex = cursor.getColumnIndexOrThrow(BrowserContract.History.GUID);
-    int androidIDIndex = cursor.getColumnIndexOrThrow(BrowserContract.History._ID);
-
-    try {
-      cursor.moveToFirst();
-      while (!cursor.isAfterLast()) {
-        String guid = cursor.getString(guidIndex);
-        int androidID = cursor.getInt(androidIDIndex);
-        cursor.moveToNext();
-
-        Record record = guidToRecord.get(guid);
-        if (record == null) {
-          // Should never happen!
-          Logger.warn(LOG_TAG, "Failed to update androidID for record with guid " + guid + ".");
-          continue;
-        }
-        record.androidID = androidID;
-      }
-    } finally {
-      if (cursor != null) {
-        cursor.close();
-      }
-    }
+    return inserted;
   }
 }

mobile/android/base/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java

@@ -127,13 +127,19 @@ public class AndroidBrowserHistoryRepositorySession extends AndroidBrowserReposi
 
   @Override
   public void abort() {
-    ((AndroidBrowserHistoryDataAccessor) dbHelper).closeExtender();
+    if (dbHelper != null) {
+      ((AndroidBrowserHistoryDataAccessor) dbHelper).closeExtender();
+      dbHelper = null;
+    }
     super.abort();
   }
 
   @Override
   public void finish(final RepositorySessionFinishDelegate delegate) throws InactiveSessionException {
-    ((AndroidBrowserHistoryDataAccessor) dbHelper).closeExtender();
+    if (dbHelper != null) {
+      ((AndroidBrowserHistoryDataAccessor) dbHelper).closeExtender();
+      dbHelper = null;
+    }
     super.finish(delegate);
   }
 
@@ -148,17 +154,10 @@ public class AndroidBrowserHistoryRepositorySession extends AndroidBrowserReposi
    *
    * @param record
    *          A <code>Record</code> with a GUID that is not present locally.
-   * @return The <code>Record</code> to be inserted. <b>Warning:</b> the
-   *         <code>androidID</code> is not valid! It will be set after the
-   *         records are flushed to the database.
    */
   @Override
-  protected Record insert(Record record) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
-    HistoryRecord toStore = (HistoryRecord) prepareRecord(record);
-    toStore.androidID = -111; // Hopefully this special value will make it easy to catch future errors.
-    updateBookkeeping(toStore); // Does not use androidID -- just GUID -> String map.
-    enqueueNewRecord(toStore);
-    return toStore;
+  protected void insert(Record record) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
+    enqueueNewRecord((HistoryRecord) prepareRecord(record));
   }
 
   /**
@@ -198,7 +197,33 @@ public class AndroidBrowserHistoryRepositorySession extends AndroidBrowserReposi
     recordsBuffer = new ArrayList<HistoryRecord>();
     Logger.debug(LOG_TAG, "Flushing " + outgoing.size() + " records to database.");
     // TODO: move bulkInsert to AndroidBrowserDataAccessor?
-    ((AndroidBrowserHistoryDataAccessor) dbHelper).bulkInsert(outgoing, false); // Don't need to update any androidIDs.
+    int inserted = ((AndroidBrowserHistoryDataAccessor) dbHelper).bulkInsert(outgoing);
+    if (inserted != outgoing.size()) {
+      // Something failed; most pessimistic action is to declare that all insertions failed.
+      // TODO: perform the bulkInsert in a transaction and rollback unless all insertions succeed?
+      for (HistoryRecord failed : outgoing) {
+        delegate.onRecordStoreFailed(new RuntimeException("Failed to insert history item with guid " + failed.guid + "."));
+      }
+      return;
+    }
+
+    // All good, everybody succeeded.
+    for (HistoryRecord succeeded : outgoing) {
+      try {
+        // Does not use androidID -- just GUID -> String map.
+        updateBookkeeping(succeeded);
+      } catch (NoGuidForIdException e) {
+        // Should not happen.
+        throw new NullCursorException(e);
+      } catch (ParentNotFoundException e) {
+        // Should not happen.
+        throw new NullCursorException(e);
+      } catch (NullCursorException e) {
+        throw e;
+      }
+      trackRecord(succeeded);
+      delegate.onRecordStoreSucceeded(succeeded); // At this point, we are really inserted.
+    }
   }
 
   @Override
@@ -209,7 +234,7 @@ public class AndroidBrowserHistoryRepositorySession extends AndroidBrowserReposi
         synchronized (recordsBufferMonitor) {
           try {
             flushNewRecords();
-          } catch (NullCursorException e) {
+          } catch (Exception e) {
             Logger.warn(LOG_TAG, "Error flushing records to database.", e);
           }
         }

mobile/android/base/sync/repositories/android/AndroidBrowserRepositoryDataAccessor.java

@@ -4,6 +4,8 @@
 
 package org.mozilla.gecko.sync.repositories.android;
 
+import java.util.List;
+
 import org.mozilla.gecko.db.BrowserContract;
 import org.mozilla.gecko.sync.Logger;
 import org.mozilla.gecko.sync.repositories.NullCursorException;
@@ -177,4 +179,55 @@ public abstract class AndroidBrowserRepositoryDataAccessor {
     }
     Logger.warn(LOG_TAG, "Unexpectedly updated " + updated + " rows for guid " + guid);
   }
+
+  /**
+   * Insert records.
+   * <p>
+   * This inserts all the records (using <code>ContentProvider.bulkInsert</code>),
+   * but does <b>not</b> update the <code>androidID</code> of each record.
+   *
+   * @param records
+   *          the records to insert.
+   * @return
+   *          the number of records actually inserted.
+   * @throws NullCursorException
+   */
+  public int bulkInsert(List<Record> records) throws NullCursorException {
+    if (records.isEmpty()) {
+      Logger.debug(LOG_TAG, "No records to insert, returning.");
+    }
+
+    int size = records.size();
+    ContentValues[] cvs = new ContentValues[size];
+    int index = 0;
+    for (Record record : records) {
+      try {
+        cvs[index] = getContentValues(record);
+        index += 1;
+      } catch (Exception e) {
+        Logger.warn(LOG_TAG, "Got exception in getContentValues for record with guid " + record.guid, e);
+      }
+    }
+
+    if (index != size) {
+      // bulkInsert treats null ContentValues as blank rows, which we don't want
+      // to insert into the database.
+      // We expect exceptions in getContentValues to be exceedingly rare, so we
+      // re-allocate in the (rare) error case and maintain a fast path for the
+      // success case.
+      size = index;
+      ContentValues[] temp = new ContentValues[size];
+      System.arraycopy(cvs, 0, temp, 0, size); // No java.util.Arrays.copyOf in older Android SDKs.
+    }
+
+    int inserted = context.getContentResolver().bulkInsert(getUri(), cvs);
+    if (inserted == size) {
+      Logger.debug(LOG_TAG, "Inserted " + inserted + " records, as expected.");
+    } else {
+      Logger.debug(LOG_TAG, "Inserted " +
+                   inserted + " records but expected " +
+                   size     + " records.");
+    }
+    return inserted;
+  }
 }

mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java

@@ -22,6 +22,7 @@ import org.mozilla.gecko.sync.repositories.Repository;
 import org.mozilla.gecko.sync.repositories.StoreTrackingRepositorySession;
 import org.mozilla.gecko.sync.repositories.delegates.RepositorySessionBeginDelegate;
 import org.mozilla.gecko.sync.repositories.delegates.RepositorySessionFetchRecordsDelegate;
+import org.mozilla.gecko.sync.repositories.delegates.RepositorySessionFinishDelegate;
 import org.mozilla.gecko.sync.repositories.delegates.RepositorySessionGuidsSinceDelegate;
 import org.mozilla.gecko.sync.repositories.delegates.RepositorySessionWipeDelegate;
 import org.mozilla.gecko.sync.repositories.domain.Record;
@@ -53,9 +54,9 @@ import android.net.Uri;
  *
  */
 public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepositorySession {
+  public static final String LOG_TAG = "BrowserRepoSession";
 
   protected AndroidBrowserRepositoryDataAccessor dbHelper;
-  public static final String LOG_TAG = "BrowserRepoSession";
   private HashMap<String, String> recordToGuid;
 
   public AndroidBrowserRepositorySession(Repository repository) {
@@ -148,6 +149,13 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
     deferredDelegate.onBeginSucceeded(this);
   }
 
+  @Override
+  public void finish(RepositorySessionFinishDelegate delegate) throws InactiveSessionException {
+    dbHelper = null;
+    recordToGuid = null;
+    super.finish(delegate);
+  }
+
   protected abstract String buildRecordString(Record record);
 
   protected void checkDatabase() throws ProfileDatabaseException, NullCursorException {
@@ -353,6 +361,8 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
     this.fetchSince(0, delegate);
   }
 
+  protected int storeCount = 0;
+
   @Override
   public void store(final Record record) throws NoStoreDelegateException {
     if (delegate == null) {
@@ -363,6 +373,9 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
       throw new IllegalArgumentException("Null record passed to AndroidBrowserRepositorySession.store().");
     }
 
+    storeCount += 1;
+    Logger.debug(LOG_TAG, "Storing record with GUID " + record.guid + " (stored " + storeCount + " records this session).");
+
     // Store Runnables *must* complete synchronously. It's OK, they
     // run on a background thread.
     Runnable command = new Runnable() {
@@ -457,9 +470,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
           if (existingRecord == null) {
             // The record is new.
             trace("No match. Inserting.");
-            Record inserted = insert(record);
-            trackRecord(inserted);
-            delegate.onRecordStoreSucceeded(inserted);
+            insert(record);
             return;
           }
 
@@ -531,16 +542,19 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos
     delegate.onRecordStoreSucceeded(record);
   }
 
-  protected Record insert(Record record) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
+  protected void insert(Record record) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
     Record toStore = prepareRecord(record);
     Uri recordURI = dbHelper.insert(toStore);
-    long id = ContentUris.parseId(recordURI);
-    Logger.debug(LOG_TAG, "Inserted as " + id);
+    if (recordURI == null) {
+      throw new NullCursorException(new RuntimeException("Got null URI inserting record with guid " + record.guid));
+    }
+    toStore.androidID = ContentUris.parseId(recordURI);
 
-    toStore.androidID = id;
     updateBookkeeping(toStore);
-    Logger.debug(LOG_TAG, "insert() returning record " + toStore.guid);
-    return toStore;
+    trackRecord(toStore);
+    delegate.onRecordStoreSucceeded(toStore);
+
+    Logger.debug(LOG_TAG, "Inserted record with guid " + toStore.guid + " as androidID " + toStore.androidID);
   }
 
   protected Record replace(Record newRecord, Record existingRecord) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {

mobile/android/base/sync/repositories/android/BookmarksInsertionManager.java

@@ -0,0 +1,298 @@
+/* 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.sync.repositories.android;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Set;
+
+import org.mozilla.gecko.sync.Logger;
+import org.mozilla.gecko.sync.Utils;
+import org.mozilla.gecko.sync.repositories.domain.BookmarkRecord;
+
+/**
+ * Queue up insertions:
+ * <ul>
+ * <li>Folder inserts where the parent is known. Do these immediately, because
+ * they allow other records to be inserted. Requires bookkeeping updates. On
+ * insert, flush the next set.</li>
+ * <li>Regular inserts where the parent is known. These can happen whenever.
+ * Batch for speed.</li>
+ * <li>Records where the parent is not known. These can be flushed out when the
+ * parent is known, or entered as orphans. This can be a queue earlier in the
+ * process, so they don't get assigned to Unsorted. Feed into the main batch
+ * when the parent arrives.</li>
+ * </ul>
+ * <p>
+ * Deletions are always done at the end so that orphaning is minimized, and
+ * that's why we are batching folders and non-folders separately.
+ * <p>
+ * Updates are always applied as they arrive.
+ * <p>
+ * Note that this class is not thread safe. This should be fine: call it only
+ * from within a store runnable.
+ */
+public class BookmarksInsertionManager {
+  public static final String LOG_TAG = "BookmarkInsert";
+  public static boolean DEBUG = false;
+
+  protected final int flushThreshold;
+  protected final BookmarkInserter inserter;
+
+  /**
+   * Folders that have been successfully inserted.
+   */
+  private final Set<String> insertedFolders = new HashSet<String>();
+
+  /**
+   * Non-folders waiting for bulk insertion.
+   * <p>
+   * We write in insertion order to keep things easy to debug.
+   */
+  private final Set<BookmarkRecord> nonFoldersToWrite = new LinkedHashSet<BookmarkRecord>();
+
+  /**
+   * Map from parent folder GUID to child records (folders and non-folders)
+   * waiting to be enqueued after parent folder is inserted.
+   */
+  private final Map<String, Set<BookmarkRecord>> recordsWaitingForParent = new HashMap<String, Set<BookmarkRecord>>();
+
+  /**
+   * Create an instance to be used for tracking insertions in a bookmarks
+   * repository session.
+   *
+   * @param flushThreshold
+   *        When this many non-folder records have been stored for insertion,
+   *        an incremental flush occurs.
+   * @param insertedFolders
+   *        The GUIDs of all the folders already inserted into the database.
+   * @param inserter
+   *        The <code>BookmarkInsert</code> to use.
+   */
+  public BookmarksInsertionManager(int flushThreshold, Collection<String> insertedFolders, BookmarkInserter inserter) {
+    this.flushThreshold = flushThreshold;
+    this.insertedFolders.addAll(insertedFolders);
+    this.inserter = inserter;
+  }
+
+  protected void addRecordWithUnwrittenParent(BookmarkRecord record) {
+    Set<BookmarkRecord> destination = recordsWaitingForParent.get(record.parentID);
+    if (destination == null) {
+      destination = new LinkedHashSet<BookmarkRecord>();
+      recordsWaitingForParent.put(record.parentID, destination);
+    }
+    destination.add(record);
+  }
+
+  /**
+   * If <code>record</code> is a folder, insert it immediately; if it is a
+   * non-folder, enqueue it. Then do the same for any records waiting for this record.
+   *
+   * @param record
+   *          the <code>BookmarkRecord</code> to enqueue.
+   */
+  protected void recursivelyEnqueueRecordAndChildren(BookmarkRecord record) {
+    if (record.isFolder()) {
+      if (!inserter.insertFolder(record)) {
+        Logger.warn(LOG_TAG, "Folder with known parent with guid " + record.parentID + " failed to insert!");
+        return;
+      }
+      Logger.debug(LOG_TAG, "Folder with known parent with guid " + record.parentID + " inserted; adding to inserted folders.");
+      insertedFolders.add(record.guid);
+    } else {
+      Logger.debug(LOG_TAG, "Non-folder has known parent with guid " + record.parentID + "; adding to insertion queue.");
+      nonFoldersToWrite.add(record);
+    }
+
+    // Now process record's children.
+    Set<BookmarkRecord> waiting = recordsWaitingForParent.remove(record.guid);
+    if (waiting == null) {
+      return;
+    }
+    for (BookmarkRecord waiter : waiting) {
+      recursivelyEnqueueRecordAndChildren(waiter);
+    }
+  }
+
+  /**
+   * Enqueue a folder.
+   *
+   * @param record
+   *          the folder to enqueue.
+   */
+  protected void enqueueFolder(BookmarkRecord record) {
+    Logger.debug(LOG_TAG, "Inserting folder with guid " + record.guid);
+
+    if (!insertedFolders.contains(record.parentID)) {
+      Logger.debug(LOG_TAG, "Folder has unknown parent with guid " + record.parentID + "; keeping until we see the parent.");
+      addRecordWithUnwrittenParent(record);
+      return;
+    }
+
+    // Parent is known; add as much of the tree as this roots.
+    recursivelyEnqueueRecordAndChildren(record);
+    flushNonFoldersIfNecessary();
+  }
+
+  /**
+   * Enqueue a non-folder.
+   *
+   * @param record
+   *          the non-folder to enqueue.
+   */
+  protected void enqueueNonFolder(BookmarkRecord record) {
+    Logger.debug(LOG_TAG, "Inserting non-folder with guid " + record.guid);
+
+    if (!insertedFolders.contains(record.parentID)) {
+      Logger.debug(LOG_TAG, "Non-folder has unknown parent with guid " + record.parentID + "; keeping until we see the parent.");
+      addRecordWithUnwrittenParent(record);
+      return;
+    }
+
+    // Parent is known; add to insertion queue and maybe write.
+    Logger.debug(LOG_TAG, "Non-folder has known parent with guid " + record.parentID + "; adding to insertion queue.");
+    nonFoldersToWrite.add(record);
+    flushNonFoldersIfNecessary();
+  }
+
+  /**
+   * Enqueue a bookmark record for eventual insertion.
+   *
+   * @param record
+   *          the <code>BookmarkRecord</code> to enqueue.
+   */
+  public void enqueueRecord(BookmarkRecord record) {
+    if (record.isFolder()) {
+      enqueueFolder(record);
+    } else {
+      enqueueNonFolder(record);
+    }
+    if (DEBUG) {
+      dumpState();
+    }
+  }
+
+  /**
+   * Flush non-folders; empties the insertion queue entirely.
+   */
+  protected void flushNonFolders() {
+    inserter.bulkInsertNonFolders(nonFoldersToWrite); // All errors are handled in bulkInsertNonFolders.
+    nonFoldersToWrite.clear();
+  }
+
+  /**
+   * Flush non-folder insertions if there are many of them; empties the
+   * insertion queue entirely.
+   */
+  protected void flushNonFoldersIfNecessary() {
+    int num = nonFoldersToWrite.size();
+    if (num < flushThreshold) {
+      Logger.debug(LOG_TAG, "Incremental flush called with " + num + " < " + flushThreshold + " non-folders; not flushing.");
+      return;
+    }
+    Logger.debug(LOG_TAG, "Incremental flush called with " + num + " non-folders; flushing.");
+    flushNonFolders();
+  }
+
+  /**
+   * Insert all remaining folders followed by all remaining non-folders,
+   * regardless of whether parent records have been successfully inserted.
+   */
+  public void finishUp() {
+    // Iterate through all waiting records, writing the folders and collecting
+    // the non-folders for bulk insertion.
+    int numFolders = 0;
+    int numNonFolders = 0;
+    for (Set<BookmarkRecord> records : recordsWaitingForParent.values()) {
+      for (BookmarkRecord record : records) {
+        if (!record.isFolder()) {
+          numNonFolders += 1;
+          nonFoldersToWrite.add(record);
+          continue;
+        }
+
+        numFolders += 1;
+        if (!inserter.insertFolder(record)) {
+          Logger.warn(LOG_TAG, "Folder with known parent with guid " + record.parentID + " failed to insert!");
+          continue;
+        }
+
+        Logger.debug(LOG_TAG, "Folder with known parent with guid " + record.parentID + " inserted; adding to inserted folders.");
+        insertedFolders.add(record.guid);
+      }
+    }
+    recordsWaitingForParent.clear();
+    flushNonFolders();
+
+    Logger.debug(LOG_TAG, "finishUp inserted " +
+        numFolders + " folders without known parents and " +
+        numNonFolders + " non-folders without known parents.");
+    if (DEBUG) {
+      dumpState();
+    }
+  }
+
+  public void clear() {
+    this.insertedFolders.clear();
+    this.nonFoldersToWrite.clear();
+    this.recordsWaitingForParent.clear();
+  }
+
+  // For debugging.
+  public boolean isClear() {
+    return nonFoldersToWrite.isEmpty() && recordsWaitingForParent.isEmpty();
+  }
+
+  // For debugging.
+  public void dumpState() {
+    ArrayList<String> readies = new ArrayList<String>();
+    for (BookmarkRecord record : nonFoldersToWrite) {
+      readies.add(record.guid);
+    }
+    String ready = Utils.toCommaSeparatedString(new ArrayList<String>(readies));
+
+    ArrayList<String> waits = new ArrayList<String>();
+    for (Set<BookmarkRecord> recs : recordsWaitingForParent.values()) {
+      for (BookmarkRecord rec : recs) {
+        waits.add(rec.guid);
+      }
+    }
+    String waiting = Utils.toCommaSeparatedString(waits);
+    String known = Utils.toCommaSeparatedString(insertedFolders);
+
+    Logger.debug(LOG_TAG, "Q=(" + ready + "), W = (" + waiting + "), P=(" + known + ")");
+  }
+
+  public interface BookmarkInserter {
+    /**
+     * Insert a single folder.
+     * <p>
+     * All exceptions should be caught and all delegate callbacks invoked here.
+     *
+     * @param record
+     *          the record to insert.
+     * @return
+     *          <code>true</code> if the folder was inserted; <code>false</code> otherwise.
+     */
+    public boolean insertFolder(BookmarkRecord record);
+
+    /**
+     * Insert many non-folders. Each non-folder's parent was already present in
+     * the database before this <code>BookmarkInsertionsManager</code> was
+     * created, or had <code>insertFolder</code> called with it as argument (and
+     * possibly was not inserted).
+     * <p>
+     * All exceptions should be caught and all delegate callbacks invoked here.
+     *
+     * @param record
+     *          the record to insert.
+     */
+    public void bulkInsertNonFolders(Collection<BookmarkRecord> records);
+  }
+}