1349709 - TPS failure: test_sync.js: ASSERTION FAILED! Validation error for "passwords" of type "serverMissing"

Status: VERIFIED FIXED

(Testing Graveyard :: TPS, defect)

Description

[Karl Thiessen [:kthiessen, he/him]]

Attachment: TPS output

This is the bug Kit and I saw when preparing the "light" run file to run TPS as a smoke check against prod.

TPS stdout is attached.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1349709 - Ensure `PasswordEngine` conforms to the new `pullAllChanges` interface.

This was a regression from bug 1317223. This commit also adds a
password engine sync test to catch future regressions.

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

Comment 2

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

Comment on attachment 8850266 [details]
Bug 1349709 - Ensure `PasswordEngine` conforms to the new `pullAllChanges` interface.

https://reviewboard.mozilla.org/r/122902/#review125504

::: services/sync/modules/engines/passwords.js:104
(Diff revision 1)
>    },
>  
>    pullAllChanges() {
> -    let changeset = new Changeset();
> +    let changes = {};
>      for (let [id, info] of Object.entries(this._store.getAllIDs())) {
> -      changeset.set(id, info.timePasswordChanged);
> +      changes[id] = info.timePasswordChanged / 1000;

Glad you took this bug because I didn't realize there was a unit mismatch!

::: services/sync/tests/unit/test_password_engine.js:11
(Diff revision 1)
> +  "@mozilla.org/login-manager/loginInfo;1", Ci.nsILoginInfo, "init");
> +
> +const PropertyBag = Components.Constructor(
> +  "@mozilla.org/hash-property-bag;1", Ci.nsIWritablePropertyBag);
> +
> +function serverForEngine(engine) {

At some point we should move this function into one of the headers... But probably not this bug.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88542bb64b97
Ensure `PasswordEngine` conforms to the new `pullAllChanges` interface. r=tcsc

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8850266 [details]
Bug 1349709 - Ensure `PasswordEngine` conforms to the new `pullAllChanges` interface.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1317223.
[User impact if declined]: Password syncing is completely broken for new users.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet. Once this patch lands, it can be verified using the TPS test suite.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Three-line change to fix a change introduced in bug 1317223; includes xpcshell test.
[String changes made/needed]: None.

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/88542bb64b97

Comment 6

[Gerry Chang [:gchang]]

Comment on attachment 8850266 [details]
Bug 1349709 - Ensure `PasswordEngine` conforms to the new `pullAllChanges` interface.

Fix a broken password syncing. Aurora54+.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bff0c493536

Comment 8

[Carsten Book [:Tomcat]]

backed out for causing problems like https://treeherder.mozilla.org/logviewer.html#?job_id=86165032&repo=mozilla-aurora

Comment 9

[Lina Butler [:lina]]

Re-landed in https://hg.mozilla.org/releases/mozilla-aurora/rev/426f26d1a43efcbf9bed49a01a52afb4b8907c79, tests should pass now that bug 1345005 has been uplifted, too.

Comment 10

[Karl Thiessen [:kthiessen, he/him]]

Verified.