493256 - Reconcile doesn't handle removed items correctly

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[Wladimir Palant]

I am writing a sync engine for Adblock Plus. When an item is removed on one computer, the other only gets the notification about the removal if it *doesn't* have this item. I checked and the problem seems to be reconciliation, more precisely the second step. There Weave checks whether the incoming item is identical to the existing item. It calls SyncEngine._isEqual() which returns true - because it ignores the payload property and all the other properties are equal. I added the following override to my engine class:

  _isEqual: function(item)
  {
    if (!item.payload)
      return false;

    return this.__proto__.__proto__._isEqual.apply(this, arguments);
  }

Note that checking the payload property of the local item is not necessary - it is already known to exist so it should be non-null. This seems to fix the issue (there are still some glitches so I am not 100% sure).

I tested with Weave 0.3 but the code seems unchanged in version 0.3.2.

Comment 1

[Ed Lee :Mardak]

SyncEngine__isEqual should be checking the payload indirectly through cleartext:
Utils.deepEquals(item.cleartext, local.cleartext)

payload is the encrypted version except in the case of a deleted record, it's "".

What does your adblock plus record look like? Are you directly setting the payload or the cleartext? (Data records should have __proto__ set to CryptoWrapper.prototype and set values in cleartext so that CryptoWrapper will encrypt the cleartext data into the payload.)

But I suppose if you didn't want things encrypted, you could inherit from WBORecord (like clients engine).

Either way, this is still a bug because the sync engine incorrectly assumes Crypto records in various places.

Comment 2

[Ed Lee :Mardak]

Attachment: v1

(In reply to comment #0)
>   _isEqual: function(item)
>     if (!item.payload)
>       return false;
It's possible for two machines to delete the same record, so they should be considered equal.

Comment 3

[Dan Mills [:thunder]]

Comment on attachment 377712 [details] [diff] [review]
v1

Won't this catch the deleted case also?

         Utils.deepEquals(item.cleartext, local.cleartext)) {

But either way, r+

Comment 4

[Dan Mills [:thunder]]

(In reply to comment #1)
> But I suppose if you didn't want things encrypted, you could inherit from
> WBORecord (like clients engine).
> 
> Either way, this is still a bug because the sync engine incorrectly assumes
> Crypto records in various places.

Maybe we should bake the crypto stuff into the base class and use other methods for specifying non-encrypted data? (e.g., set a cryptoAlgorithm property to 'none')

Comment 5

[Ed Lee :Mardak]

What would be use cases of engines not wanting to encrypt their data -- especially if in the UI we show that we always encrypt data put on the server? Should clients use cryptowrapper as well?

Comment 6

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/befd9af4df5d

Have engines check if the deleted flag is the same for _isEqual.

Comment 7

[Dan Mills [:thunder]]

It would make some testing a little easier (currently you can't turn it off, and that means you can only check state during a sync when stuff gets logged).

It would also make sense to not encrypt data if I'm using my own server, for example.

Comment 8

[Wladimir Palant]

(In reply to comment #1)
> SyncEngine__isEqual should be checking the payload indirectly through
> cleartext:
> Utils.deepEquals(item.cleartext, local.cleartext)

Yes, I guessed that - but it doesn't work for some reason. local.cleartext is {disabled: false} in this case, maybe Utils.deepEqual() cannot distinguish it from {} (either way, there are other records that don't have any data at all).

Comment 9

[Wladimir Palant]

My guess was correct, filed bug 493363 on Utils.deepEqual() issues.