1426742 - Fix TPS cleanup issues and test_history failures

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

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

Patch incoming, the cleanup issues were caused by the FormAutofill engines not being initialized, and the history issue is caused assuming handleResult in updatePlaces is something worth looking for.

This also has a few mostly cosmetic improvements to TPS written while debugging it.

Comment 1

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

Attachment: Bug 1426742 - Fix TPS test_history and cleanup failures.

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

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8938483 [details]
Bug 1426742 - Fix TPS test_history and cleanup failures.

https://reviewboard.mozilla.org/r/209182/#review214986

Thanks!

::: browser/extensions/formautofill/FormAutofillSync.jsm:335
(Diff revision 1)
>    _deleteId(id) {
>      this._noteDeletedId(id);
>    },
>  
>    async _resetClient() {
> +    await profileStorage.initialize();

Since we now have https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/services/sync/modules/engines.js#863, I wonder if it makes more sense to add an `async initialize` method to the engine, move this call there, and also remove the `profileStorage.initialize()` call from `_syncStartup`. WDYT?

::: services/sync/tps/extensions/tps/resource/tps.jsm:748
(Diff revision 1)
>          this.quit();
>          return;
>        }
>        this.seconds_since_epoch = Services.prefs.getIntPref("tps.seconds_since_epoch");
>        if (this.seconds_since_epoch)
> -        this._usSinceEpoch = this.seconds_since_epoch * 1000 * 1000;
> +        this._usSinceEpoch = (this.seconds_since_epoch - 60) * 1000 * 1000;

Is this to guard against adding visits in the future? Mind adding a comment?

Comment 3

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

Comment on attachment 8938483 [details]
Bug 1426742 - Fix TPS test_history and cleanup failures.

https://reviewboard.mozilla.org/r/209182/#review214986

> Since we now have https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/services/sync/modules/engines.js#863, I wonder if it makes more sense to add an `async initialize` method to the engine, move this call there, and also remove the `profileStorage.initialize()` call from `_syncStartup`. WDYT?

There's a comment above explaining why we don't do this.

Comment 4

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

Comment on attachment 8938483 [details]
Bug 1426742 - Fix TPS test_history and cleanup failures.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a71ff50707d4
Fix TPS test_history and cleanup failures. r=kitcambridge

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a71ff50707d4