Bug 1351678: Handle resolved conflicts correctly, r=kmag

The elements of the SyncResultObject#conflicts field are {local,
remote} pairs.  However, the elements of the SyncResultObject#resolved
field are just the resolutions, and trying to access "local" and
"remote" fields of those resolutions causes a crash. Add a test that
demonstrates the error, and then fix it.

MozReview-Commit-ID: LF0NBw5VKBC

--HG--
extra : rebase_source : 7711111d35cfe962f57c7d08d74bad2e37090b7c

toolkit/components/extensions/ExtensionStorageSync.jsm

@@ -808,13 +808,18 @@ class ExtensionStorageSync {
       };
     }
     for (const conflict of syncResults.resolved) {
-      // FIXME: Should we even send a notification? If so, what
-      // best values for "old" and "new"? This might violate
-      // client code's assumptions, since from their perspective,
-      // we were in state L, but this diff is from R -> L.
-      changes[conflict.remote.key] = {
-        oldValue: conflict.local.data,
-        newValue: conflict.remote.data,
+      // FIXME: We can't send a "changed" notification because
+      // kinto.js only provides the newly-resolved value. But should
+      // we even send a notification? We use CLIENT_WINS so nothing
+      // has really "changed" on this end. (The change will come on
+      // the other end when it pulls down the update, which is handled
+      // by the "updated" case above.) If we are going to send a
+      // notification, what best values for "old" and "new"?  This
+      // might violate client code's assumptions, since from their
+      // perspective, we were in state L, but this diff is from R ->
+      // L.
+      changes[conflict.key] = {
+        newValue: conflict.data,
       };
     }
     if (Object.keys(changes).length > 0) {

toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js

@@ -1114,6 +1114,75 @@ add_task(function* test_storage_sync_pushes_changes() {
   });
 });
 
+add_task(function* test_storage_sync_pulls_conflicts() {
+  const extensionId = uuid();
+  const extension = {id: extensionId};
+  yield* withContextAndServer(function* (context, server) {
+    yield* withSignedInUser(loggedInUser, function* (extensionStorageSync, fxaService) {
+      const cryptoCollection = new CryptoCollection(fxaService);
+      let transformer = new CollectionKeyEncryptionRemoteTransformer(cryptoCollection, extensionId);
+      server.installCollection("storage-sync-crypto");
+
+      yield extensionStorageSync.ensureCanSync([extensionId]);
+      const collectionId = yield cryptoCollection.extensionIdToCollectionId(extensionId);
+      yield server.encryptAndAddRecord(transformer, {
+        collectionId,
+        data: {
+          "id": "key-remote_2D_key",
+          "key": "remote-key",
+          "data": 6,
+        },
+        predicate: appearsAt(850),
+      });
+      server.etag = 900;
+
+      yield extensionStorageSync.set(extension, {"remote-key": 8}, context);
+
+      let calls = [];
+      yield extensionStorageSync.addOnChangedListener(extension, function() {
+        calls.push(arguments);
+      }, context);
+
+      yield extensionStorageSync.syncAll();
+      const remoteValue = (yield extensionStorageSync.get(extension, "remote-key", context))["remote-key"];
+      equal(remoteValue, 8,
+            "locally set value overrides remote value");
+
+      equal(calls.length, 1,
+            "conflicts manifest in on-changed listener");
+      deepEqual(calls[0][0], {"remote-key": {newValue: 8}});
+      calls = [];
+
+      // Syncing again doesn't do anything
+      yield extensionStorageSync.syncAll();
+
+      equal(calls.length, 0,
+            "syncing again shouldn't call on-changed listener");
+
+      // Updating the server causes us to pull down the new value
+      server.etag = 1000;
+      yield server.encryptAndAddRecord(transformer, {
+        collectionId,
+        data: {
+          "id": "key-remote_2D_key",
+          "key": "remote-key",
+          "data": 7,
+        },
+        predicate: appearsAt(950),
+      });
+
+      yield extensionStorageSync.syncAll();
+      const remoteValue2 = (yield extensionStorageSync.get(extension, "remote-key", context))["remote-key"];
+      equal(remoteValue2, 7,
+            "conflicts do not prevent retrieval of new values");
+
+      equal(calls.length, 1,
+            "syncing calls on-changed listener on update");
+      deepEqual(calls[0][0], {"remote-key": {oldValue: 8, newValue: 7}});
+    });
+  });
+});
+
 add_task(function* test_storage_sync_pulls_deletes() {
   const extension = defaultExtension;
   yield* withContextAndServer(function* (context, server) {