Reconcile doesn't handle removed items correctly

RESOLVED FIXED in 0.4

Status

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: gaubugzilla, Assigned: Mardak)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.
Posted patch v1Splinter Review
(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.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #377712 - Flags: review?(thunder)
Attachment #377712 - Flags: review?(thunder) → review+
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+
(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')
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?
http://hg.mozilla.org/labs/weave/rev/befd9af4df5d

Have engines check if the deleted flag is the same for _isEqual.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: -- → 0.4
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.
(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).
My guess was correct, filed bug 493363 on Utils.deepEqual() issues.
Component: Weave → General
Product: Mozilla Labs → Weave
Version: 0.3 → unspecified
QA Contact: weave → general
You need to log in before you can comment on or make changes to this bug.