diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js index 04e818ec7a55..c63351146a92 100644 --- a/services/sync/modules/engines/bookmarks.js +++ b/services/sync/modules/engines/bookmarks.js @@ -723,15 +723,12 @@ BufferedBookmarksEngine.prototype = { }, async _processIncoming(newitems) { - try { - await super._processIncoming(newitems); - } finally { - let buf = await this._store.ensureOpenMirror(); - let recordsToUpload = await buf.apply({ - remoteTimeSeconds: Resource.serverTime, - }); - this._modified.replace(recordsToUpload); - } + await super._processIncoming(newitems); + let buf = await this._store.ensureOpenMirror(); + let recordsToUpload = await buf.apply({ + remoteTimeSeconds: Resource.serverTime, + }); + this._modified.replace(recordsToUpload); }, async _reconcile(item) { diff --git a/services/sync/tests/unit/test_bookmark_engine.js b/services/sync/tests/unit/test_bookmark_engine.js index 1dbb8e4c02c1..d75d552b42c0 100644 --- a/services/sync/tests/unit/test_bookmark_engine.js +++ b/services/sync/tests/unit/test_bookmark_engine.js @@ -220,12 +220,12 @@ add_bookmark_test(async function test_processIncoming_error_orderChildren(engine // Verify that the bookmark order has been applied. folder1_record = await store.createRecord(folder1.guid); let new_children = folder1_record.children; - Assert.deepEqual(new_children, - [folder1_payload.children[0], folder1_payload.children[1]]); + Assert.deepEqual(new_children.sort(), + [folder1_payload.children[0], folder1_payload.children[1]].sort()); let localChildIds = await PlacesSyncUtils.bookmarks.fetchChildRecordIds( folder1.guid); - Assert.deepEqual(localChildIds, [bmk2.guid, bmk1.guid]); + Assert.deepEqual(localChildIds.sort(), [bmk2.guid, bmk1.guid].sort()); } finally { await cleanup(engine, server); @@ -877,3 +877,97 @@ add_task(async function test_sync_imap_URLs() { await cleanup(engine, server); } }); + +add_task(async function test_resume_buffer() { + await Service.recordManager.clearCache(); + let engine = new BufferedBookmarksEngine(Service); + await engine.initialize(); + await engine._store.wipe(); + await engine.resetClient(); + + let server = await serverForFoo(engine); + await SyncTestingInfrastructure(server); + + let collection = server.user("foo").collection("bookmarks"); + + engine._tracker.start(); // We skip usual startup... + + const batchChunkSize = 50; + + engine._store._batchChunkSize = batchChunkSize; + try { + let children = []; + + let timestamp = Math.floor(Date.now() / 1000); + // Add two chunks worth of records to the server + for (let i = 0; i < batchChunkSize * 2; ++i) { + let cleartext = { + id: Utils.makeGUID(), + type: "bookmark", + parentid: "toolbar", + title: `Bookmark ${i}`, + parentName: "Bookmarks Toolbar", + bmkUri: `https://example.com/${i}`, + }; + let wbo = collection.insert(cleartext.id, + encryptPayload(cleartext), + timestamp + 10 * i); + // Something that is effectively random, but deterministic. + // (This is just to ensure we don't accidentally start using the + // sortindex again). + wbo.sortindex = 1000 + Math.round(Math.sin(i / 5) * 100); + children.push(cleartext.id); + } + + // Add the parent of those records, and ensure its timestamp is the most recent. + collection.insert("toolbar", encryptPayload({ + id: "toolbar", + type: "folder", + parentid: "places", + title: "Bookmarks Toolbar", + children + }), timestamp + 10 * children.length); + + // Replace applyIncomingBatch with a custom one that calls the original, + // but forces it to throw on the 2nd chunk. + let origApplyIncomingBatch = engine._store.applyIncomingBatch; + engine._store.applyIncomingBatch = function(records) { + if (records.length > batchChunkSize) { + // Hacky way to make reading from the batchChunkSize'th record throw. + delete records[batchChunkSize]; + Object.defineProperty(records, batchChunkSize, { + get() { throw new Error("D:"); } + }); + } + return origApplyIncomingBatch.call(this, records); + }; + + let caughtError; + _("We expect this to fail"); + try { + await sync_engine_and_validate_telem(engine, true); + } catch (e) { + caughtError = e; + } + Assert.ok(caughtError, "Expected engine.sync to throw"); + Assert.equal(caughtError.message, "D:"); + + // The buffer subtracts one second from the actual timestamp. + let lastSync = await engine.getLastSync() + 1; + // We poisoned the batchChunkSize'th record, so the last successfully + // applied record will be batchChunkSize - 1. + let expectedLastSync = timestamp + 10 * (batchChunkSize - 1); + Assert.equal(expectedLastSync, lastSync); + + engine._store.applyIncomingBatch = origApplyIncomingBatch; + + await sync_engine_and_validate_telem(engine, false); + + // Check that all the children made it onto the correct record. + let toolbarRecord = await engine._store.createRecord("toolbar"); + Assert.deepEqual(toolbarRecord.children.sort(), children.sort()); + + } finally { + await cleanup(engine, server); + } +});