1443343 - client commands that affect engine data may result in no client data if processed when only the clients engine is being synced.

Status: NEW

(Firefox :: Sync, defect, P3)

Description

[Kanchan Kumari QA]

Attachment: Archive .zip

Prerequisite:
1. About:sync addon installed
2. services.sync.engine.bookmarks.buffer is set to true

STR:
1. Created new profile
2. Sign in to new created sync account
3. Added some bookmars and syced to second device
4. Exported bookmarks as HTML file on both device
5. Imported Html bookmarks file on both devices and synced (as I remember)
6. Bookmarks got imported 

Observed: After a couple hours on Mac (was not in use ~2 hrs)I observed there were no bookmarks in the library

Attached is the error logged after step 6.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

There are only 2 error logs in the .zip file - they aren't at trace level and it appears no "success" logs are being captured.

Those error logs have a few instances of:

> 1520276946764	Sync.Resource	WARN	GET request to https://sync-597-us-west-2.sync.services.mozilla.com/1.5/93106991/info/collections failed: [Exception... "The connection has timed out"  nsresult: "0x804b000e (NS_ERROR_NET_TIMEOUT)"  location: "<unknown>"  data: no] No traceback available

But apart from that have nothing to help us work out what might have gone wrong. I'm going to close this as there's nothing we can action here, but Kanchan, if you can use about:sync to ensure all the logs are at "trace" level and that you are capturing logs even on success, there might be more actionable information if it can be reproduced in the future.

Comment 2

[Kanchan Kumari QA]

Attachment: logsfromWindows.zip

I encountered this issue again today while trying to reproduce bug 1437153. I followed following STR:
STR:
1. Set up sync on two devices
2. Run the backup script https://gist.github.com/kitcambridge/b08dae1f4f1b5a93f2d30425cdef09d5 and save tree-with-new-guids.json file on device 1.
3. Restore json file on device 1
4. Restore same json file on device 2 
Things worked as expected till here.
5. Export bookmarks to HTML file on device 1.
6. Export bookmarks to HTML file on device 2.
7. Trigger sync on both devices.

Observed: There are no bookmarks in Bookmarks library. Attached are success logged from both devices.

Here device 1 is Windows 10 and device 2 is Mac OS 10.13

After couple minutes I triggered sync again on both devices and all bookmarks reappeared in library.

Comment 3

[Kanchan Kumari QA]

Attachment: traceloggedOnMac

Comment 4

[Mark Hammond [:markh] [:mhammond]]

This seems bad. What we appear to see is:

* client A sends a wipeClient to client B.
* client B sees a push notification due to the command and syncs just the client engine.
* client B wipes all bookmarks.
* next sync is scheduled in 600 seconds - at which point bookmarks are likely to sync.

It's not yet clear to me how we should best handle this.

Comment 5

[Lina Butler [:lina]]

We could change `sendCommand` to accept an `affectedCollections` option, pass those along to `_notifyCollectionChanged`, and pass them as `engines` in https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/services/sync/modules/service.js#426.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

This really is a mismatch - using client commands for collection commands doesn't make sense. But here we are :)

Consider another sad use-case - client gets the wipe command then fails to sync for corruption etc related issues - they stay screwed forever. ISTM that the bookmark mirror can help us here - in theory we could perform the remote commands only after we are confident we can apply the remote data.

But we need to fix this sooner rather than later, so...

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #5)
> We could change `sendCommand` to accept an `affectedCollections` option,
> pass those along to `_notifyCollectionChanged`, and pass them as `engines`

Yeah, that makes sense. We should remove support for resetAll and wipeAll - nuking *everything* makes no sense - engines must be specified. That means "sendTab" is the only command with no engine specified so is done every time. Ideally, we'd arrange for collection commands to be processed immediately before the engine is synced - IOW, we'd process commands multiple times per sync.

The pushbox work seems a good time to do this (although is probably a bit of a PITA for Ed's patch queue ;)

Comment 7

[Julie McCracken (:julie)]

Mark, please clarify.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

What I'm suggesting here is:

1) Remove support for resetAll and wipeAll - this means every command applies to exactly 1 engine.
2) Add a processCommand function to each engine - this should be called as each engine syncs. This also implies that not all commands will be removed from the "command queue" at the same time - eg, imagine "displayURL" and "wipeEngine(bookmarks)" commands - as the clients engine syncs, displayURL (only) will be removed leaving wipeEngine(bookmarks). When bookmarks finally syncs, that last command will be removed.

Making this slightly tricky is that commands for unknown or disabled engines must still be removed even though their engines will never be synced. There are probably a few ways to arrange for this, which I'm happy to discuss, but otherwise I'll leave the actual mechanics to you.

Ed, does that make sense and make this bug actionable?