Settings Sync Provider with support for syncing Email Protection

[ayoy]

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/0/1205254652502303/f
iOS PR: duckduckgo/iOS#1963
macOS PR: duckduckgo/macos-browser#1565
What kind of version bump will this require?: Major

Optional:

Tech Design URL: https://app.asana.com/0/481882893211075/1205248329666560/f

Description:
Updated SyncMetadata Core Data model with a new version that adds SyncableSettingsMetadata entity.
This entity keeps last modification timestamps for each syncable setting (identified by its Sync key).
Added SettingsProvider that is composed of multiple per-setting providers with their respective handlers.
Handlers conform to SettingsSyncHandling protocol that provides a unified interface for setting and retrieving
values, as well as allows for fine-tuning deduplication logic. Currently there's only one handler – for Email
Protection auth state (EmailProtectionSyncHandler). It own an instance of EmailManager, which means that
now we have multiple (at least 2) EmailManagers that can modify Email Protection state concurrently.
I've added a static lock to EmailManager to avoid data races. Also some of EmailManager APIs got updated
to throw errors instead of only notifying requestDelegate.

Steps to test this PR:

• Initial sync: test merge strategy according to this Asana task.
• Regular sync: test that updates to Email Protection state are correctly synced to other devices.
• Verify that in-context Email Protection signup triggers sync:
  • disable Email Protection
  • try to sign up to some website and click Dax logo in the email field
  • follow instructions to enable Email Protection
  • verify that Sync is triggered after enabling Email Protection

OS Testing:

• iOS 14
• iOS 15
• iOS 16
• macOS 10.15
• macOS 11
• macOS 12

---

Internal references:

Software Engineering Expectations
Technical Design Template

[ayoy]

Setting nil value upon deletion is good for Email Protection, but we will see if other settings follow this logic too. We can always abstract this to the handler protocol.

[ayoy]

We don't nullify timestamps anymore (this change was part of enabling selective sync – DataProvider.registerFeature takes care of it now). Bookmarks Provider doesn't have this line of code, and I just forgot to remove it from CredentialsProvider, so I'm doing it now.

[bwaresiak]

I'm not sure about this. It seems to be prone to timing issues, if user logs out while response is being processed. I know, rather rare scenario, but I wonder if we could make the state of the "pending deletion" more clear and definitive?

[ayoy]

Great catch. I think we can only limit the potential damage here. How about fetching values for all settings at once, right after the call to SyncableSettingsMetadataUtils.fetchSettingsMetadata(for: keys, in: context)? I'm not sure it would help much as the rest of this function body is really fast anyway, but it will at least look better and make the intention clear (i.e. "here is the place where we fetch all data, and later we only check values").

[ayoy]

I've added this change in b6fa811. Let me know in case you have a better idea. Now that I think of it, settings could be read before creating SettingsResponseHandler (inside handleSyncResponse) to even better align the timing with the moment of creating/resetting context.

[bwaresiak]

In fact this: if let lastModified = metadata.lastModified, lastModified > clientTimestamp should actually be a good guard agaisnt what I wrote - that feels like the best effort to avoid issues here, given that we can't guarantee transcationality of the changes.

[ayoy]

If I understood your original concern correctly, this line wouldn't guard us:

• timestamps are fetched a few lines above (and are fetched from the context created/refreshed in the calling function – handleSyncResponse),
• in an unmodified code (before b6fa811) fetching individual settings values is happening after fetching timestamps,
• hence we could get a lastModified that is still old (as far as a few-ms old context is concerned), and a new value,
• so I looked for a solution that brings fetching settings values as close to creating/refreshing the context as possible.

Ideally this could be done here in handleSyncResponse (but this may be hair-splitting):

context.performAndWait {
    while true {
        do {
            let originalValues: [Setting: String?] = try settingsHandlers.reduce(into: .init()) { partialResult, handler in
                partialResult[handler.key] = try handler.value.getValue()
            }
            let responseHandler = try SettingsResponseHandler( // ...

[bwaresiak]

in an unmodified code (before b6fa811) fetching individual settings values is happening after fetching timestamps

I think array of settings were fetched, but not loaded into memory (by accessing property or unfaulting them in ay way), that's why I've assumed it is ok. But it is better to make it more explicit rather than rely on nuance when given object is being loaded into memory, especially as these operations should be pretty efficient. :)

[ayoy]

Thanks for the insight about faulting, it slipped my mind!

[bwaresiak]

Is this delete intentional? Wouldn't that affect any update, not only deletes?

[ayoy]

This function clears timestamps from all sent items. In settings, clearing timestamp is equivalent to deleting its respective metadata object.

It looks like this for bookmarks:

if bookmark.isPendingDeletion, !isLocalChangeRejectedBySync {
    context.delete(bookmark)
} else {
    bookmark.modifiedAt = nil
    idsOfItemsToClearModifiedAt.insert(uuid)
}

I should put context.delete(metadata) outside of the if statement because it's a common part of deleting an object and clearing a timestamp on a locally updated object.

[bwaresiak]

I see. So metadata is only present if there's something to send right? It probably would be simpler to just always keep one instance around (and update timetamp if needed) but that approach is also fine. Since we are removing/adding objects I think:

• it is posible that setting new timestamp value will fail (metadata object could be deleted by sync)
• It is possible we have more that one metadata created (as there is no synchornization on which context or logic flow could lead to creation of metadata).

Both are unlikely, but would be good to know if that happens.

[ayoy]

Yes, the idea was to delete metadata objects when there's nothing to sync.

Setting new timestamp value should always work because if the object wasn't present, we create one (see this function). This can of course lead to creation of multiple objects per key. Let me see if I can rewrite it to keep a single metadata object around.

[ayoy]

I've updated logic to keep metadata objects around (once they're created in prepareForFistSync), and only clear timestamps after sync. I added SyncError.settingsMetadataNotPresent and a self-healing mechanism of creating a new object in this scenario.

Please have a look at 8de5b4a.

[bwaresiak]

Great job, I went through the code, algorithm and tested various scenarios, without any issues.