Closed Bug 1349703 Opened 7 years ago Closed 7 years ago

Fix TPS failures for bookmark tests

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(3 files)

Bug 1349703 - (part 1) Fix issue where sync would fail misreconcile special folders in fresh syncs
59 bytes, text/x-review-board-request
lina
: review+
Details
Bug 1349703 - (part 2) Fix intermittent TPS failures caused by failing cleanup by waiting longer before starting
59 bytes, text/x-review-board-request
lina
: review+
Details
Bug 1349703 - (part 3) Delete a test from TPS that was testing that we made the wrong decision during reconciliation
59 bytes, text/x-review-board-request
lina
: review+
Details 
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: nobody → tchiovoloni
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+
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.
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.
Status: NEW → ASSIGNED
Priority: -- → P1
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
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
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
See Also: → 1352233
See Also: → 1357814
Verified a while back.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: