Closed Bug 1323869 Opened 3 years ago Closed 3 years ago

Consider running bookmark validation when a new device is connected

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: Lina, Unassigned)

Details

(Whiteboard: [sync-metrics][measure-integrity])

Attachments

(1 file)

There's a concern this might be a bad user experience, since validation is expensive, but this is a good time to do it: we've read the entire local tree into memory (thanks to `_buildGUIDMap`), and we'll be downloading all server records.

We should do this carefully after looking at telemetry.
To Chris' point: just because we report a validation problem doesn't mean Sync created it in the first place. This gives us a good baseline.
Priority: -- → P2
Whiteboard: [sync-metrics][measure-integrity]
Assignee: nobody → kit
Status: NEW → ASSIGNED
Comment on attachment 8845131 [details]
Bug 1323869 - Validate bookmarks on first sync.

https://reviewboard.mozilla.org/r/118350/#review120196

::: services/sync/modules/stages/enginesync.js:251
(Diff revision 1)
>      this._log.info("Updating enabled engines: " +
>                      numClients + " clients.");
>  
>      if (meta.isNew || !meta.payload.engines) {
>        this._log.debug("meta/global isn't new, or is missing engines. Not updating enabled state.");
> +      Svc.Prefs.resetBranch("engineStatusChanged.");

This fixes another interesting (likely test-only) issue, where we only reset this pref if we have a `meta/global`.

Without this, reenabling an engine that was disabled before the first sync would disable that engine again on the next sync (http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/services/sync/modules/service.js#461-463 clears the pref, since we didn't clear it before, and then http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/services/sync/modules/stages/enginesync.js#334-337 takes the server's version).
Comment on attachment 8845131 [details]
Bug 1323869 - Validate bookmarks on first sync.

https://reviewboard.mozilla.org/r/118350/#review120382

::: commit-message-58753:9
(Diff revision 1)
> +records for validation after the first sync. The default is 100% and
> +1k records, but we can experiment with the thresholds as needed.
> +
> +I'm not sure if we can reuse the downloaded records, or the tree we
> +built for the GUID map. It's possible for bookmarks to change during
> +the sync, and validation happens long after the engine has finished.

I'll have a better look at this tomorrow, but see also bug 1345754, which was supposed to land as part of the repair. I believe that we must always skip validation when there are pending changes, or we are guaranteed to see validation problems that aren't actual problems - and I believe the same is true here - even if it means we need to skip the validation in that case.

As you mention, the cost does also worry me - the user is already taking a decent hit for that first sync, and re-downloading all records makes me a little uneasy, especially if we intend for validation to end up on the release channel.
Comment on attachment 8845131 [details]
Bug 1323869 - Validate bookmarks on first sync.

https://reviewboard.mozilla.org/r/118350/#review124344

We chatted about this last week, and I think we said that we should try use the records we just downloaded and applied, but also that we don't really think this should be a priority as the benefits are a little questionable in the first place, especially if it has a risk of making users sad. I'm clearing the review request and you should consider unassigning yourself and adding this back to the backlog :)
Attachment #8845131 - Flags: review?(markh)
Assignee: kit → nobody
Status: ASSIGNED → NEW
Priority: P2 → --
From triage: the second sync seems like a better time to do this, since the first one is already expensive...but doesn't really answer the question of whether corruption gets introduced during the first sync. (Which is valuable information, except we don't have information from before the first sync if the server is already corrupt). Alex also brought up the very good point that we don't necessarily want to use the local records, in case they're different than what's on the server.

Also, this isn't riding the trains, since validation is quite expensive. This patch has different limit for the first sync; we could experiment with adjusting those as needed.

Related: we discussed adding a "reason" to the validation. It might be useful to resurrect this and add a `{ "why": "firstSync" }`, so that we can include it in our dashboards.
Priority: -- → P2
We decided not to do this.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.