Fix TPS failures for bookmark tests

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
a month ago
8 days ago

People

(Reporter: tcsc, Assigned: tcsc)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

a month ago
There are a number of these, but for example running test_sync.js I get

Error: ASSERTION FAILED! places item not found; Item not found at the expected index, got 4, expected 7 for folder (location: menu, folder: folderb)

during phase 2. 

I suspect this is simply a consequence of not running TPS often enough, and it using the oldest and most deprecated possible methods of figuring out if there are conflicts, but it's possible these are real bugs in sync (in which case it's possible other bugs will be filed to address them instead of fixing them all here).
(Assignee)

Updated

a month ago
Assignee: nobody → tchiovoloni
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a month ago
The first of these is more important than the rest, as it represents a real bug in sync, and this is a... probably incomplete fix, although IMO worth doing despite the fact it's not complete. I'm not sure if it should have been part of this bug, but I can move it elsewhere if so.

The other patches are more along the lines I expected to fix here.

Part 2 is a bit hacky and it delays running the first actual TPS test. AFAICT the only time this actually matters is in cleanup phases, when we actually depend on the rest of initialization to happen first.  

Really this should be a little more granular, and I tried a couple times to find the right set of "things that TPS expects to be done within the next tick" and wait for all of them, but it wasn't obvious what these were (my initial guess of _executeTestPhase wasn't enough), and with the way TPS is structured (or the way it *isn't* structured), it's far easier to replace nextTick with an extremely generous setTimeout, as I did.

Part 3 deletes a failing test that, as far as I can tell, is *totally irrelevant* now with the new tracker, and doesn't seem like it ever should have been tested in the first place. E.g. shouldn't we remember we deleted those items? Maybe I misunderstood it though!

With this and the fix in bug 1349709, all TPS pass except for test_history_collisions, which A) is not obvious what's happening here and B) isn't bookmark related so I punted on it and will file a separate bug for it.
Comment on attachment 8850743 [details]
Bug 1349703 - (part 1) Fix issue where sync would fail misreconcile special folders in fresh syncs

https://reviewboard.mozilla.org/r/123268/#review125764

LGTM. I think you're right: we'll need to be more clever about how we handle reconciliation for first syncs, and, generally, cases where we lose child order because the local parent is chronologically newer. But this fix is valuable by itself, so ship it.

::: services/sync/modules/engines.js:1579
(Diff revision 1)
>      // We resolve conflicts by record age, where the newest one wins. This does
>      // result in data loss and should be handled by giving the engine an
>      // opportunity to merge the records. Bug 720592 tracks this feature.
>      this._log.warn("DATA LOSS: Both local and remote changes to record: " +
>                     item.id);
> +    if (!remoteIsNewer) {

What do you think about overriding `_reconcile` in the bookmarks engine instead of adding a separate function? Feel free to drop this if you'd prefer to keep `beforeRecordDiscard` separate; I'd be OK with either.

::: services/sync/modules/engines/bookmarks.js:1140
(Diff revision 1)
>  
> +  getStatus(id) {
> +    let change = this.changes[id];
> +    if (!change) {
> +      return PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN;
> +    } else {

Style nit: no `else` after return.
Attachment #8850743 - Flags: review?(kit) → review+
Comment on attachment 8850744 [details]
Bug 1349703 - (part 2) Fix intermittent TPS failures caused by failing cleanup by waiting longer before starting

https://reviewboard.mozilla.org/r/123270/#review125996

::: services/sync/tps/extensions/tps/resource/tps.jsm:180
(Diff revision 1)
>            break;
>  
>          case "sessionstore-windows-restored":
> -          Utils.nextTick(this.RunNextTestAction, this);
> +          // This is a terrible hack, but fixes cases where tps (usually cleanup)
> +          // fails because of sessionstore restoring windows before tps is able
> +          // to initialize. This used to take only 1 tick, but at some point

Hooray for race conditions! Just out of interest, what kinds of failures does this cause?
Attachment #8850744 - Flags: review?(kit) → review+
Comment on attachment 8850745 [details]
Bug 1349703 - (part 3) Delete a test from TPS that was testing that we made the wrong decision during reconciliation

https://reviewboard.mozilla.org/r/123272/#review126126
Attachment #8850745 - Flags: review?(kit) → review+
(Assignee)

Comment 8

a month ago
mozreview-review-reply
Comment on attachment 8850744 [details]
Bug 1349703 - (part 2) Fix intermittent TPS failures caused by failing cleanup by waiting longer before starting

https://reviewboard.mozilla.org/r/123270/#review125996

> Hooray for race conditions! Just out of interest, what kinds of failures does this cause?

It's a failure during cleanup so it generally causes the next test to randomly fail.
(Assignee)

Comment 9

a month ago
mozreview-review-reply
Comment on attachment 8850743 [details]
Bug 1349703 - (part 1) Fix issue where sync would fail misreconcile special folders in fresh syncs

https://reviewboard.mozilla.org/r/123268/#review125764

> What do you think about overriding `_reconcile` in the bookmarks engine instead of adding a separate function? Feel free to drop this if you'd prefer to keep `beforeRecordDiscard` separate; I'd be OK with either.

I think this would end up being kind of complex or depending on the internals of `_reconcile` less explicitly, so I'd rather not.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Status: NEW → ASSIGNED
Priority: -- → P1

Comment 13

29 days ago
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61654eb3874c
(part 1) Fix issue where sync would fail misreconcile special folders in fresh syncs r=kitcambridge
https://hg.mozilla.org/integration/autoland/rev/33568586e1e8
(part 2) Fix intermittent TPS failures caused by failing cleanup by waiting longer before starting r=kitcambridge
https://hg.mozilla.org/integration/autoland/rev/1d822e7126fe
(part 3) Delete a test from TPS that was testing that we made the wrong decision during reconciliation r=kitcambridge

Comment 14

28 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61654eb3874c
https://hg.mozilla.org/mozilla-central/rev/33568586e1e8
https://hg.mozilla.org/mozilla-central/rev/1d822e7126fe
Status: ASSIGNED → RESOLVED
Last Resolved: 28 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
See Also: → bug 1352233
(Assignee)

Updated

8 days ago
See Also: → bug 1357814
You need to log in before you can comment on or make changes to this bug.