Closed Bug 1205901 Opened 7 years ago Closed 5 years ago

Two-way Synchronization History Data Adapter

Categories

(Firefox OS Graveyard :: Sync, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED WONTFIX
2.6 S6 - 1/29

People

(Reporter: selee, Assigned: selee)

References

Details

Attachments

(4 files, 1 obsolete file)

The read-only history adapter is implemented for synchronizing History data from Firefox Sync Service in bug 1191773.

This bug will provide a two-way synchronization feature for pushing the records browsed in FxOS to FxSync Service.
Assignee: nobody → selee
Blocks: fxos-sync
Attached file WIP - Two-way sync feature (obsolete) —
Target Milestone: --- → FxOS-S8 (02Oct)
Depends on: 1208352
Depends on: 1211370
Target Milestone: FxOS-S8 (02Oct) → FxOS-S9 (16Oct)
Target Milestone: FxOS-S9 (16Oct) → FxOS-S11 (13Nov)
Depends on: 1217340
Target Milestone: FxOS-S11 (13Nov) → FxOS-S12 (27Nov)
Priority: -- → P2
Target Milestone: FxOS-S12 (27Nov) → 2.6 S2 - 12/4
After having a discussion with Michiel, we come out a new design based on the first prototype (attachment 8671998 [details] [review]).

Step 1: sync-engine fetches changes from remote (DataAdapter#handleConflict may be called), and DataAdapter#update is called
Step 2: Retrieve the changes in Places Data Store by using checkIfClearedSince [1] (rename to getChanges for this, and add a callback for handleTask), and if task.operation === 'add' or 'create' ->
         Kinto Collection.get
         2.1 No FxSync ID exists, and it means it's a new record -> Kinto Collection#create (without `id` field), write back fxsyncId into DS
         2.2 FxSync ID exists -> Kinto Collection#update (merge visits array, check title field)

Step 3:
      Write History Collection records to PlacesDS. That`s a read-only part and we`ve implemented. (Kinto Collection -> Local DS).
      Return true or false to DataAdapter#update

Step 4: (handled by sync-engine; DataAdapter#handleConflict may be called)
      Push all waiting-for-syncing records to FxSync and update last synced revision id.

[1] https://github.com/michielbdejong/gaia/blob/1224183-DataAdapter-refactor/shared/js/places_model.js#L351
Yes! :) Cool that we have a clear plan for this now.

Just found a typo in our etherpad text:
* s/task.operation === 'add' or 'create'/task.operation === 'add' or 'update'

And we forgot to write down that DataAdapter#handleConflict should do the same thing as when a DS add or update triggers a Kinto Collection#update, namely merge visits array, and pick newest title field.
Target Milestone: 2.6 S2 - 12/4 → 2.6 S4 - 1/1
Status: NEW → ASSIGNED
Hey Michiel,


I got the following error [1] at [2] when the history record is tried to create in KintoColleciton.
Could you help to give me a suggestion about this? Thank you.

attachment 8671998 [details] [review] has my latest patch.

[1] {
  "ok": false,
  "lastModified": 1451361144080,
  "errors": [
    {
      "path": "/v1/buckets/611c7e96a16778e40016504c1928fc57/collections/history/records/MYAiun7IIj8x",
      "sent": {
        "id": "MYAiun7IIj8x",
        "payload": "{\"hmac\":\"39577f7792e88a7a527cbdf4cda35c71486def154b7dcc2de380711fcdd1846b\",\"ciphertext\":\"0Z51Jq1RM7AkTi1k+mCL3HbuuE36rWURHLoCpYCy86O5V95vCJ7KDPqFe8+fEjI2MhZAKXzcW+0btfLduyDe0SpC3aMClTq4arsgVMoP1epdCkW61MWsTv6zjA77BvsnDi1ML9NtaPkj0nlFcSjQHafjgU1MaK19AofMCpK/HsAOuTpo3EWKWV8mTBl/NuOB7wYYOUBTFdXR0zB1qoNwCw==\",\"IV\":\"CRutp5QNWNgicxYHrz9mWw==\"}"
      },
      "error": {
        "errno": 999,
        "message": "Failed batch subrequest",
        "code": 400,
        "error": "Bad Request"
      },
      "type": "outgoing"
    }
  ],
  "created": [],
  "updated": [],
  "deleted": [],
  "published": [],
  "conflicts": [],
  "skipped": [],
  "resolved": []
}
[2] https://github.com/weilonge/gaia/blob/648a120df01795c26b885bc4611ae3d86b1a84f2/shared/js/sync/engine.js#L282
Flags: needinfo?(mbdejong)
Which Syncto server are you using? Make sure it's configured to allow read-write traffic (the dev/prod Syncto instances don't).
Flags: needinfo?(mbdejong)
Target Milestone: 2.6 S4 - 1/1 → 2.6 S5 - 1/15
Attachment #8662692 - Attachment is obsolete: true
Target Milestone: 2.6 S5 - 1/15 → 2.6 S6 - 1/29
Hi Natim, Michiel,

When a new record is tried to sync-up to remote service, the record can be uploaded successfully. The process doing to Collection is [create -> update -> sync], and it's verified by using Kinto.js library directly or SyncEngine.

When the existing and modified records are tried to sync-up, Sync Engine will exit accidentally with this error [1]: (see the attachment)
HTTP 412; Resource was modified meanwhile: Resource was modified meanwhile

It's shown the same result when using Collection to [update -> sync].

Do you have any idea for this issue? Thank you.


[1] {
    "errno": 114,
    "message": "Resource was modified meanwhile",
    "code": 412,
    "error": "Precondition Failed"
}
Flags: needinfo?(rhubscher)
Flags: needinfo?(mbdejong)
Sounds like a client-side issue, will try to replicate.
Flags: needinfo?(rhubscher)
After giving strategy: Kinto.syncStrategy.CLIENT_WINS, it still shows the same error:
Error: HTTP 412; Resource was modified meanwhile: Resource was modified meanwhile
Found the issue, it was server-side after all, created  https://github.com/mozilla-services/syncto/issues/78
Flags: needinfo?(mbdejong)
Besides, MANUAL is what SyncEngine wants. SyncEngine will handle the conflict with this:
 https://github.com/mozilla-b2g/gaia/blob/master/shared/js/sync/engine.js#L263-L268
Issue now fixed in Syncto, try updating to latest master.
Flags: needinfo?(selee)
Ok so I just released Cliquet 2.15.0 with the bugfix.

I am ready to do the Syncto 1.5.0 release: https://github.com/mozilla-services/syncto/pull/80

Please tell me if the latest master works for you guys and I will do the release.
Hi Natim,

Thanks a lot for your update!

After applying prepare-1.5.0 branch of Syncto, 412 error is gone! There is a conflict object in SyncResult when an existing record tried to be sync-up, but the conflict object contains a 'null' remote object.
So I use local-win strategy manually. Here is another error message from Kinto.js: 

Error Message:  "e.remote is null"

Stack:
"[208]</Collection</<.value@app://sync.gaiamobile.org/shared/js/sync/ext/kinto.min.js:632:10604SyncEngine.prototype._resolveConflicts/SyncEngine</<@app://sync.gaiamobile.org/shared/js/sync/engine.js:266:15"

Could you help to give some suggestions? Thank you.
Flags: needinfo?(selee) → needinfo?(rhubscher)
That probably comes from https://github.com/mozilla-b2g/gaia/pull/32263/files#diff-4d73ff90f2479c9b5d1f53457e85e25dR591 can you show the exact conflict object that is logged by the two console.log lines directly above that? Let's chat about it on irc!
Flags: needinfo?(rhubscher) → needinfo?(selee)
It's the conflict object:

{
  "type": "outgoing",
  "local": {
    "id": "ifNjxr2SZZ2r",
    "payload": "{\"hmac\":\"116348c8f601b3fcde88cf0cd43309581b63a88aad409df7f51aa917161be740\",\"ciphertext\":\"u1eoar3cy310k8OvRH1MovftYTaUKYFACAiUce/I3tQdHf1pWMnRKVZz4oAh9jsK52bGzjROM6+iGTqjBrL0Ywgqllbrxsn+bliBLyLIZJ+orHVfL3GBZhBvCvlYqljjoPDe/swMh7czavOkS3cLOjWTRZHa4wfNP+Xs9YLNgSeZ4dC91dhRF6tawITvi06sAp+IdIQ9aD4WBeoTZ5ofd3LqyoN5wvQD7bu9WnYPbBU=\",\"IV\":\"36xgz+KRxN6g3DpFFkkopQ==\"}"
  },
  "remote": null
}
Flags: needinfo?(selee) → needinfo?(rhubscher)
Sean, were you able to intercept the server response for that fetch operation? I'm really surprised with the remote property being null, as we mostly forward what we received from the server here.
Flags: needinfo?(selee)
It is quite possible that the Firefox Sync server is returning a 412 without returning the previous record.

I am pretty sure you should not pass the ``If-None-Match`` header for this records.
Kinto.js seems to think that you are creating a new record when you are actually trying to update it.
It looks like the last_modified value is not kept when you are updating the record.

Could this be a problem with the way the encryption/decryption transformer works?
Flags: needinfo?(rhubscher)
Ah, makes sense, if there's a conflict during PUT, there will be no response body to the 412 response. So would need to do an extra GET after a 412 (I think we already do that anyway?).

But because pullChanges runs before pushChanges, it surprises me that the data on the server changes inbetween the pull and the push. It seems much more likely that pullChanges triggers an incoming conflict than that pushChanges triggers an outgoing conflict, right?
Attached file sync_traffic.json
Hey Niko,

All sync traffic is included in this attachment.
In the last entry, it's a POST request at line 2720 .

And then SyncResult contains this conflict object:
{
  "type": "outgoing",
  "local": {
    "id": "ifNjxr2SZZ2r",
    "payload": "{\"hmac\":\"3ba109d63b84900a8c6d34533cda7a0f05f5a2514b2f38320f9274718f5410e1\",\"ciphertext\":\"ffJy21yQTYeKCfQnsus9/C29Au/wEF2BY4OwzNok0Dn91OjrwY2QyH6fnmD4ACpNatyjTqtgxaRvGjchmUR6aakuqmtpwZG1qhmDZnQ97vNSeTbdEkq0P+STahpL3kx5H3ympEQ4WIOIIzieLpO2rYTtke9hZ5xJEE2ZCBGkJtXu0UBtZ2DbUKEbcoLIWs07yh0SkXHpdd/+psTsYgCvh0Go/meubd5AQD6f3n5c5ww9bmTQNSHmfy6tKTTAA/ARx9FajgFcXe4pcLrRTq0//Q==\",\"IV\":\"FKjTmUM62rVWNInRHUDtjg==\"}"
  },
  "remote": null
}
Flags: needinfo?(selee)
So it's confirmed it's the server returning a 412 batch response with errno 114 "Resource was modified meanwhile" with no conflicting record attached to its json body, that's why we get a null client side.

We should now investigate why is that, could be a server issue or a malformed client request. I think :michielbdejong has a clue it's the latter and is currently working on a patch, am I right Michiel?
Flags: needinfo?(mbdejong)
Yes, try with the code from https://github.com/Kinto/kinto.js/pull/309 I think that should stop the 412s from happening.
Flags: needinfo?(mbdejong) → needinfo?(selee)
Hey Michiel,

After applying the latest kinto.js, it still shows me the following error:
{
  lineNumber: 8480, // [1]
  message: "conflict.remote is null",
  stack: "resolve@app://sync.gaiamobile.org/shared/js/sync/ext/kinto.min.js:8480:9SyncEngine.prototype._resolveConflicts/SyncEngine</<@app://sync.gaiamobile.org/shared/js/sync/engine.js:266:15"
}

You can see the last POST request at line 2208 in this attachment.

kinto.js is built from 'npm run build' command with this commit of Kinto.js : 
commit 701019790d01722769e78ee06b3117b293565f3c
Merge: 3cc9b38 a5c8553
Author: Mathieu Leplatre <mathieu@leplat.re>
Date:   Mon Feb 1 23:42:14 2016 +0100

    Merge pull request #181 from Kinto/181-subresource-integrity

    Add Sub-Resource Integrity information on dist/ files

[1] https://github.com/weilonge/gaia/blob/ab6add8b024d7413cffb629eefc9f8a43a503a46/shared/js/sync/ext/kinto.min.js#L8480
Flags: needinfo?(selee) → needinfo?(mbdejong)
Looks like you're now running into https://github.com/Kinto/kinto.js/issues/306 (although, did you `make clean` to remove your profile from earlier tests?)
Flags: needinfo?(mbdejong)
(In reply to Michiel de Jong [:michielbdejong] from comment #26)
> Looks like you're now running into
> https://github.com/Kinto/kinto.js/issues/306 (although, did you `make clean`
> to remove your profile from earlier tests?)

I always run 'make clean && make && launch b2g'
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.