709323 - Handle key changes in info/collections

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, defect, P1)

Comment 1

[Richard Newman [:rnewman]]

Not really relevant until Bug 709324 is done, and we actually save them!

Comment 2

[Nick Alexander :nalexander [he/him]]

Bug 709324 implemented caching crypto/keys and testing info/collections to see if we need to re-fetch.  Is that what this ticket is?

Or is this to handle the case where a client generates a new collection key, updates the server with the new key, and we need to do something other than just "get new crypto/keys and sync as usual"?

Comment 3

[Richard Newman [:rnewman]]

This bug is: if we see keys have changed, and we download them and find that they're different, what do we do?

The answer should be to do something similar to desktop, ensuring consistency. That is, if keys have changed, we need to make sure that all the data on the server is encrypted with those keys (on a per-collection basis).

This is, IIRC, handled by wiping the server when we change keys, and handling *that* correctly.

Comment 4

[Nick Alexander :nalexander [he/him]]

https://github.com/mozilla-services/android-sync/pull/164

Comment 5

[Nick Alexander :nalexander [he/him]]

Superceded by

https://github.com/mozilla-services/android-sync/pull/166

Comment 6

[Damon Sicore (:damons)]

What exactly is this bug for?  Can we get a detailed description?  Why do we need it?  Really strange seeing a blocker with no content.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Damon Sicore (:damons) from comment #6)
> What exactly is this bug for?  Can we get a detailed description?  Why do we
> need it?  Really strange seeing a blocker with no content.

I think https://bugzilla.mozilla.org/show_bug.cgi?id=709323#c3 is pretty clear, but let me add that we need this ticket to be a feature complete sync client that can continue operating when another client changes a collection key or when the server is in an illegal/partially initialized state and needs to be reset.

Comment 8

[Damon Sicore (:damons)]

(In reply to Nick Alexander :nalexander from comment #7)
> (In reply to Damon Sicore (:damons) from comment #6)
> > What exactly is this bug for?  Can we get a detailed description?  Why do we
> > need it?  Really strange seeing a blocker with no content.
> 
> I think https://bugzilla.mozilla.org/show_bug.cgi?id=709323#c3 is pretty
> clear, but let me add that we need this ticket to be a feature complete sync

Firefox on Android is in trouble if we don't already have a feature complete sync *right now*.  

Is the current list of Fennec blockers *THE* list of features that must be implemented for a feature complete sync that is required for fennec to ship?


> client that can continue operating when another client changes a collection
> key or when the server is in an illegal/partially initialized state and
> needs to be reset.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Damon Sicore (:damons) from comment #8)

> Firefox on Android is in trouble if we don't already have a feature complete
> sync *right now*.  

The target until this point has been feature complete and data integrity for the common case. That was explicitly called out to me as the goal for beta readiness, and we are at that point (modulo one outstanding beta blocker, of course).

We don't support creating a new Sync account, choosing what to sync, or a multitude of other things that you could call "feature complete".

(Those have all been explicit tradeoffs, because we've attempted to compress twelve months of work into four. If we wanted to be feature complete and polished by release, we should have started in August, rather than me burning out over Christmas to get a MSU working.)


"The common case" also excludes this set of protocol scenarios: when the user is reassigned to a new node, their keys change on the server, or they otherwise encounter a server state that isn't "business as usual". Again, that was an explicit tradeoff for beta readiness, trading demoability for robustness.


I (and the mobile drivers agree) that correctly handling the full set of server situations is a requirement for release, because enough users will hit those situations that we have to address them, and the consequences are unknown and probably bad.


This should not come as a surprise to anyone who's been following along, because I've been quite vocal about these things since January. I have raised concerns about robustness and completeness prior to initial Aurora uplift, and mentioned it during Beta expectation setting. We have simply run out of phases to push this back to, so now we've got a few release blockers.


> Is the current list of Fennec blockers *THE* list of features that must be
> implemented for a feature complete sync that is required for fennec to ship?

I will be happy releasing with Bug 745430, Bug 745431, Bug 713542, and Bug 714304 (which can slip at the cost of degraded user experience, but is a small task that we have free hands to do), which is why I have argued for those bugs to be release blockers.

It won't be anywhere near perfect, but that's what I'm told a 1.0 is for.

Comment 10

[Richard Newman [:rnewman]]

Last iteration through the review cycle.

Comment 11

[Richard Newman [:rnewman]]

Reviewed and queued to land once it can be tested with other work as part of Bug 745430, Bug 745431.

See <https://bugzilla.mozilla.org/show_bug.cgi?id=709321#c8> for testing tips.

Comment 12

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae35d533711

Let's plan on uplifting this along with the rest of Bug 745430/Bug 745431.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7ae35d533711

Comment 14

[Allison Naaktgeboren :ally]

Sync team: Definitely want to uplift to beta, but not during the first one. We are considering uplifting  709321/323 all together with other dependencies of 745430/431.

Comment 15

[Richard Newman [:rnewman]]

Attachment: Aurora uplift.

Comment 16

[Allison Naaktgeboren :ally]

Comment on attachment 624877 [details] [diff] [review]
Aurora uplift.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

part of the meta/global implementation bundle of release blockers

Comment 17

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa61eedd2337

Comment 18

[Tracy Walker [:tracy]]

verified by - delete crypto keys using
Components.utils.import("resource://services-sync/main.js");
Components.utils.import("resource://services-sync/resource.js");
function deletePath(path) {
  let resource = new Resource(Weave.Service.storageURL + path);
  resource.setHeader("X-Confirm-Delete", "1");
  return resource.delete();
}
deletePath("crypto/keys");

Sync desktop, then sync device.

should get:
05-21 12:17:09.256: W/ServerSyncStage(2518): Remote engine syncID different from local engine syncID: resetting local engine and assuming remote engine syncID.

bug 756549 sometimes is getting in the way.