# HG changeset patch
# User Mark Hammond <mhammond@skippinet.com.au>
# Date 1512352146 -39600
# Node ID 0d2cfb31201992ae4ddb5a21c4abc35b41e16bd7
# Parent  eba3cedfe84baa8340ccb7b4be34f0108b647588
Bug 1415560 - ignore insertMany result and add an onError handler to log errors. r=tcsc

MozReview-Commit-ID: 8aJkRL8WCPr

diff --git a/services/sync/modules/engines/history.js b/services/sync/modules/engines/history.js
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -212,17 +212,29 @@ HistoryStore.prototype = {
       if (shouldApply) {
         k += 1;
       }
     }
     records.length = k; // truncate array
 
     if (records.length) {
       for (let chunk of this._generateChunks(records)) {
-        await PlacesUtils.history.insertMany(chunk);
+        // Per bug 1415560, we ignore any exceptions returned by insertMany
+        // as they are likely to be spurious. We do supply an onError handler
+        // and log the exceptions seen there as they are likely to be
+        // informative, but we still never abort the sync based on them.
+        try {
+          await PlacesUtils.history.insertMany(chunk, null, failedVisit => {
+            this._log.info("Failed to insert a history record", failedVisit.guid);
+            this._log.trace("The record that failed", failedVisit);
+            failed.push(failedVisit.guid);
+          });
+        } catch (ex) {
+          this._log.info("Failed to insert history records", ex);
+        }
       }
     }
 
     return failed;
   },
 
   /**
    * Returns a generator that splits records into sanely sized chunks suitable
@@ -252,16 +264,23 @@ HistoryStore.prototype = {
         count += record.visits.length;
       } while (curIndex < records.length &&
                count + records[curIndex].visits.length <= this.MAX_VISITS_PER_INSERT);
       this._log.trace(`adding ${toAdd.length} items in this chunk`);
       yield toAdd;
     }
   },
 
+  /* An internal helper to determine if we can add an entry to places.
+     Exists primarily so tests can override it.
+   */
+  _canAddURI(uri) {
+    return PlacesUtils.history.canAddURI(uri);
+  },
+
   /**
    * Converts a Sync history record to a mozIPlaceInfo.
    *
    * Throws if an invalid record is encountered (invalid URI, etc.),
    * returns true if the record is to be applied, false otherwise
    * (no visits to add, etc.),
    */
   async _recordToPlaceInfo(record) {
@@ -270,17 +289,17 @@ HistoryStore.prototype = {
     record.uri = CommonUtils.makeURI(record.histUri);
 
     if (!Utils.checkGUID(record.id)) {
       this._log.warn("Encountered record with invalid GUID: " + record.id);
       return false;
     }
     record.guid = record.id;
 
-    if (!PlacesUtils.history.canAddURI(record.uri)) {
+    if (!this._canAddURI(record.uri)) {
       this._log.trace("Ignoring record " + record.id + " with URI "
                       + record.uri.spec + ": can't add this URI.");
       return false;
     }
 
     // We dupe visits by date and type. So an incoming visit that has
     // the same timestamp and type as a local one won't get applied.
     // To avoid creating new objects, we rewrite the query result so we
diff --git a/services/sync/tests/unit/test_history_store.js b/services/sync/tests/unit/test_history_store.js
--- a/services/sync/tests/unit/test_history_store.js
+++ b/services/sync/tests/unit/test_history_store.js
@@ -241,16 +241,36 @@ add_task(async function test_invalid_rec
   await applyEnsureNoFailures([
     {id: Utils.makeGUID(),
      histUri: "http://getfirebug.com",
      title: "Get Firebug!",
      visits: []}
   ]);
 });
 
+add_task(async function test_unknowingly_invalid_records() {
+  _("Make sure we handle rejection of records by places gracefully.");
+  let oldCAU = store._canAddURI;
+  store._canAddURI = () => true;
+  try {
+    _("Make sure that when places rejects this record we record it as failed");
+    let guid = Utils.makeGUID();
+    let result = await store.applyIncomingBatch([
+      {id: guid,
+       histUri: "javascript:''",
+       title: "javascript:''",
+       visits: [{date: TIMESTAMP3,
+                 type: Ci.nsINavHistoryService.TRANSITION_EMBED}]}
+    ]);
+    deepEqual(result, [guid]);
+  } finally {
+    store._canAddURI = oldCAU;
+  }
+});
+
 add_task(async function test_clamp_visit_dates() {
   let futureVisitTime = Date.now() + 5 * 60 * 1000;
   let recentVisitTime = Date.now() - 5 * 60 * 1000;
 
   await applyEnsureNoFailures([{
     id: "visitAAAAAAA",
     histUri: "http://example.com/a",
     title: "A",