Closed Bug 1426742 Opened 2 years ago Closed 2 years ago

Fix TPS cleanup issues and test_history failures

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

Details

Attachments

(1 file)

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 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?
Attachment #8938483 - Flags: review?(kit) → review+
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.
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a71ff50707d4
Fix TPS test_history and cleanup failures. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/a71ff50707d4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.