1339340 - Repair items the bookmark validator reports as missing on the client or server

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

To reproduce this validator error, use SQL to delete an item locally from places.sqlite. The validator then correctly reports the item is missing from the client, but we still never download that item. We should consider doing so. 

Thom, are there any other validation results you think are similar (ie, that could be fixed simply by downloading and reapplying a record from the server without needing to perform the full-blown multi-device repair dance?)

(Aside: Interestingly (and probably correctly, hence this bug), the current state of our "repair" work also doesn't try and handle that. To make this error be something repairable I also had to delete the item from the server)

Comment 1

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

heh - obvious other candidate is one we can simply upload :)

Comment 2

[Edouard Oger [:eoger]]

Attachment: Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server.

Review commit: https://reviewboard.mozilla.org/r/120654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120654/

Comment 3

[Edouard Oger [:eoger]]

Comment on attachment 8847728 [details]
Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120654/diff/1-2/

Comment 4

[Edouard Oger [:eoger]]

Updated my patch after 1:1 with mark yesterday:
- Don't sync after the repair is done, the repair will be done on next sync
- Don't re-run the validator, abort the current repair if we did some server-only repairs. I would be happier if we could raise the probability of a repair next sync though, since we are not fixing everything here and might leave a slightly improved mess.
- Mark server-missing records for weak re-upload instead of "touch"ing them.

Comment 5

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

Comment on attachment 8847728 [details]
Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server.

This is using the "weak" stuff added by Kit to the repair patch, which I think is probably the right way to go. Thom, do you mind verifying that and taking the review?

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8847728 [details]
Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server.

https://reviewboard.mozilla.org/r/120654/#review124466

I'd like to take another look after my issues (which are all small) are addressed but good otherwise.

::: services/sync/modules/bookmark_repair.js:178
(Diff revision 2)
>  
>      // XXX - any others we should consider?
>      return ids;
>    }
>  
> +  tryServerOnlyRepairs(validationInfo) {

serverDeleted (e.g. we have the item locally, but it's marked as deleted on the server) could be treated identically to clientMissing in this function.

While conceptually different, it's a "server-only" repair that is fixed by fetching.

(I realize now that I didn't notice markh's question along these lines, directed at me)

::: services/sync/modules/bookmark_repair.js:185
(Diff revision 2)
> +         validationInfo.problems.serverMissing.length) == 0) {
> +      return false;
> +    }
> +    let engine = this.service.engineManager.get("bookmarks");
> +    for (let id of validationInfo.problems.serverMissing) {
> +      engine._modified.setWeak(id, { tombstone: false });

Do we need to ensure that the id doesn't refer to a record we aren't allowed to upload to the server? E.g. bookmark roots, or children of non-roots?

Is anything in place that will prevent that?

(There's also a potential for a race condition if the client deletes the record between when you set this and when we upload it, I think, but I think that should be addressed by bug 1337978 and we shouldn't bother with it here)

::: services/sync/modules/bookmark_repair.js:187
(Diff revision 2)
> +    }
> +    let engine = this.service.engineManager.get("bookmarks");
> +    for (let id of validationInfo.problems.serverMissing) {
> +      engine._modified.setWeak(id, { tombstone: false });
> +    }
> +    engine.toFetch = engine.toFetch.concat(validationInfo.problems.clientMissing);

We probably want to avoid duplicates here. Easy solution is `Array.from(new Set(...))` around what gets assigned to engine.toFetch.

::: services/sync/modules/collection_repair.js:78
(Diff revision 2)
> +
> +     @param   validationInfo       {Object}
> +              The validation info as returned by the collection's validator.
> +
> +  */
> +  tryServerOnlyRepairs(validationInfo) {

Are we sure this is the right default? I think maybe logging and returning false makes more sense, especially given that the initial implementation of bookmarks didn't contain this, and it makes some sense that other collections might be the same way.

Comment 7

[Edouard Oger [:eoger]]

Comment on attachment 8847728 [details]
Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120654/diff/2-3/

Comment 8

[Edouard Oger [:eoger]]

(Most of the) comments addressed, thanks Thom.

> Do we need to ensure that the id doesn't refer to a record we aren't allowed to upload to the server? E.g. bookmark roots, or children of non-roots?
> 
>Is anything in place that will prevent that?

I don't feel confident enough to answer that question, Kit would you mind chiming in?

Comment 9

[Lina Butler [:lina]]

(In reply to Edouard Oger [:eoger] from comment #8)
> I don't feel confident enough to answer that question, Kit would you mind
> chiming in?

If I understand these checks correctly, non-syncable roots and their children should never end up in `serverMissing`, `clientMissing`, or `serverDeleted`: http://searchfox.org/mozilla-central/source/services/sync/modules/bookmark_validator.js#25-29,285-289,292,715-717,720-722 But Thom would be the expert here. :-)

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

Yeah, I was more arguing that we should add sanity checks, since that logic is fairly convoluted, but we probably should wait for actual evidence that bad things are happening before we complicate the code with possibly unnecessary stuff.

So, sounds fine to me.

Comment 11

[Edouard Oger [:eoger]]

Comment on attachment 8847728 [details]
Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120654/diff/3-4/

Comment 12

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8847728 [details]
Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server.

https://reviewboard.mozilla.org/r/120654/#review125660

LGTM!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16d562074791
Repair items the bookmark validator reports as missing on the client or server. r=tcsc

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing xpcshell test services/sync/tests/unit/test_doctor.js:

https://hg.mozilla.org/integration/autoland/rev/52a505ea3c6c85ea1774ec0decfdbead6e2e2c29

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=16d5620747919e3c0cab25f901053eb8c8196d60&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86064274&repo=autoland

16:38:27     INFO -  TEST-PASS | services/sync/tests/unit/test_doctor.js | test_repairs_start - [test_repairs_start : 95] "test-engine" == "test-engine"
16:38:27     INFO -  PID 6562 | 1490312307304	Sync.Doctor	ERROR	Failed to run validation on test-engine!: TypeError: requestor.tryServerOnlyRepairs is not a function (resource://services-sync/doctor.js:204:11) JS Stack trace: [e-mail]:204:11 < [e-mail]:182:15 < [e-mail]:211:5 < [e-mail]:546:5 < @-e:1:1
16:38:27     INFO -  PID 6562 | 1490312307304	Sync.Doctor	ERROR	Failed to run validation on test-engine!: TypeError: requestor.tryServerOnlyRepairs is not a function (resource://services-sync/doctor.js:204:11) JS Stack trace: [e-mail]:204:11 < [e-mail]:182:15 < [e-mail]:211:5 < [e-mail]:546:5 < @-e:1:1
16:38:27     INFO -  PID 6562 | 1490312307305	Sync.Telemetry	TRACE	observed weave:engine:validate:error test-engine
16:38:27     INFO -  PID 6562 | 1490312307305	Sync.Telemetry	WARN	Observed notification weave:engine:validate:error but no current sync is being recorded.
16:38:27  WARNING -  TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_doctor.js | test_repairs_start - [test_repairs_start : 105] false == true
16:38:27     INFO -  /builds/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/test_doctor.js:test_repairs_start:105
16:38:27     INFO -  /builds/slave/test/build/tests/xpcshell/head.js:_do_main:211
16:38:27     INFO -  /builds/slave/test/build/tests/xpcshell/head.js:_execute_test:546
16:38:27     INFO -  -e:null:1
16:38:27     INFO -  exiting test

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8847728 [details]
Bug 1339340 - Repair items the bookmark validator reports as missing on the client or server.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120654/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/920dd09fefa7
Repair items the bookmark validator reports as missing on the client or server. r=tcsc

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/920dd09fefa7