LoginManagerParent.updateLoginAnchor mistake converting from countLogins to searchLoginsWithObject

RESOLVED FIXED in Firefox 49

Status

()

Toolkit
Password Manager
P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

({regression})

49 Branch
mozilla51
regression
Points:
---

Firefox Tracking Flags

(firefox48 unaffected, firefox49 fixed, firefox50 fixed, firefox51 fixed)

Details

(URL)

MozReview Requests

()

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

Attachments

(1 attachment)

The change below was incorrect since countLogins would ignore the "" for formSubmitURL but searchLogins does not and therefore this only returns form logins that literally have "" for the saved action.

https://hg.mozilla.org/mozilla-central/diff/da82605a18d6/toolkit/components/passwordmgr/LoginManagerParent.jsm#l1.120

The search should specify `httpRealm: null` instead of `formSubmitURL: ""` but this reveals an issue in that the MP prompt appears when a login is found. This is probably a real user-facing issue and is caught by test_master_password.html with this fix.
Created attachment 8777102 [details]
Bug 1291346 - Fix updateLoginAnchor's search for form logins and delay it until the pref check.

It's possible to write a test for this but that's already covered by bug 1164018 and this code should be removed in bug 1286718.

Review commit: https://reviewboard.mozilla.org/r/68690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68690/
Attachment #8777102 - Flags: review?(dolske)
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Comment on attachment 8777102 [details]
Bug 1291346 - Fix updateLoginAnchor's search for form logins and delay it until the pref check.

https://reviewboard.mozilla.org/r/68690/#review66128

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:520
(Diff revision 1)
> +    // Once this preference is removed, this version of the fill doorhanger
> +    // should be enabled for Desktop only, and not for Android or B2G.
> +    if (!Services.prefs.getBoolPref("signon.ui.experimental")) {
> +      return;

Any reason not to just move the pref check to the very top of the function?
Attachment #8777102 - Flags: review?(dolske) → review+
https://reviewboard.mozilla.org/r/68690/#review66128

> Any reason not to just move the pref check to the very top of the function?

Nope, I guess not. Fixed.
Comment on attachment 8777102 [details]
Bug 1291346 - Fix updateLoginAnchor's search for form logins and delay it until the pref check.

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

Comment 5

a year ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/fx-team/rev/f54fea6078dd
Fix updateLoginAnchor's search for form logins and delay it until the pref check. r=dolske

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f54fea6078dd
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

a year ago
Depends on: 1293513
Matt, can you request uplift for this fix? Thanks!
Flags: needinfo?(MattN+bmo)
Comment on attachment 8777102 [details]
Bug 1291346 - Fix updateLoginAnchor's search for form logins and delay it until the pref check.

Approval Request Comment
[Feature/regressing bug #]: Bug 667233
[User impact if declined]: Useless work done when a non-default feature is disabled. Non-default feature would be missing most logins if it was enabled (not recommended)
[Describe test coverage new/current, TreeHerder]: It's possible to write a test for this but that's already covered by bug 1164018 and this code should be removed in bug 1286718 so I didn't write a new test. It's also a trivial change.
[Risks and why]: Low risk trivial changes and automated tests for other functionality didn't seem to regress.
[String/UUID change made/needed]: N/A
Flags: needinfo?(MattN+bmo)
Attachment #8777102 - Flags: approval-mozilla-beta?
Attachment #8777102 - Flags: approval-mozilla-aurora?
Comment on attachment 8777102 [details]
Bug 1291346 - Fix updateLoginAnchor's search for form logins and delay it until the pref check.

Fixes a regression, Aurora50+, Beta49+
Attachment #8777102 - Flags: approval-mozilla-beta?
Attachment #8777102 - Flags: approval-mozilla-beta+
Attachment #8777102 - Flags: approval-mozilla-aurora?
Attachment #8777102 - Flags: approval-mozilla-aurora+

Comment 10

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6fcbbeaddff
status-firefox50: affected → fixed

Comment 11

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b985e6deb20d
status-firefox49: affected → fixed
No longer depends on: 1293513
Duplicate of this bug: 1283994
Version: unspecified → 49 Branch
You need to log in before you can comment on or make changes to this bug.