Closed Bug 1590362 Opened 5 years ago Closed 5 years ago

Incorrect web address string is displayed for identical username/password entries for origin and subdomain

Categories

(Toolkit :: Password Manager, defect, P1)

Desktop
Unspecified
defect

Tracking

()

VERIFIED FIXED
mozilla72
Tracking Status
firefox70 --- disabled
firefox71 + verified
firefox72 --- verified

People

(Reporter: tbabos, Assigned: sfoster)

References

Details

Attachments

(4 files, 1 obsolete file)

Attached video First order -> as described in bug (deleted) —

Affected versions
Nightly 72
Beta 71.0b3

Affected platforms
Windows 10 x64
Ubuntu 18.04

Steps to reproduce

Expected result
The autocomplete dropdown should show "From this website" under the suggested credentials for www.facebook.com

Actual Result
The "ro-ro.facebook.com" web address string is displayed under the suggested credentials for www.facebook.com
The "From this website" is displayed on ro-ro.facebook.com

Note
This can be reproduced in reverse order as well. Attached video for both cases and logs as well.

Attached file Console Log ->described scenario (obsolete) (deleted) —
Attached video Reverse Scenario video (deleted) —
Attachment #9103221 - Attachment is obsolete: true
Attached file Console Logs.zip (deleted) —

Great find! The preferredOrigin argument to LoginHelper.dedupeLogins should be handling this but perhaps the timePasswordChanged resolveBy argument that follows it is the problem, or there is just a bug?

[Tracking Requested - why for this release]: New subdomain login suggestion feature in 71.

Flags: qe-verify+
Priority: -- → P1
Hardware: Unspecified → Desktop
Blocks: 1590622
  • The 2 logins get passed into LoginHelper.dedupeLogins; they are in the order "https://www.facebook.com", "https://ro-ro.facebook.com"
  • It uses its internal helper isLoginPreferred to figure out which of the 2 to prefer, using the keys "actionOrigin", "scheme", "subdomain", "timePasswordChanged" to resolve the question.
  • actionOrigin and scheme are the same for both
  • The subdomain comparison logic looks to see if the 2nd login is a better match to the preferred origin than the 1st login.
  • In this case, "ro-ro.facebook.com" is not a better match, so the algorithm continues on to "timePasswordChanged" to break the deadlock.

I looks like we should check the negative case here too. If the 1st origin matches the preferredOrigin and the 2nd doesnt, return false. Its only in the case where either both match the preferredOrigin, or neither do where we should continue.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED

Hey Matt and Sam,

This issue seems to be fixed in the latest Nightly now.
I tried to find the patch in hope for a Beta uplfit but always got irrelevant bugs for auto-complete.

Last affected build: October 21
First good build: October 22

There are some password fixes landed in that timeframe, maybe one of them could've fix this?
Anyway, just wanted to let you know and see if there are any changes or further actions needed here.

Flags: needinfo?(sfoster)
Flags: needinfo?(MattN+bmo)
Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5a546ed4e35f Prefer matched origin logins when deduping. r=MattN

(In reply to Timea Cernea [:tbabos] from comment #7)

Hey Matt and Sam,

This issue seems to be fixed in the latest Nightly now.
I tried to find the patch in hope for a Beta uplfit but always got irrelevant bugs for auto-complete.

There was definitely a bug there, which attachment 9104094 [details] fixes. I'll request a beta uplift for it once it is merged. I was able to reproduce with the steps in comment #0. I'll take a look but I'm not aware of anything that landed recently that would have fixed this.

Flags: needinfo?(sfoster)
Flags: needinfo?(MattN+bmo)

sorry, getting bugs mixed up with the flag changes. This should be right now. The feature is disabled in <=70, currently in 71 and 72 with this bug.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Sam, could you request an uplift to beta? Thanks

Flags: needinfo?(sfoster)

(In reply to Sam Foster [:sfoster] (he/him) from comment #9)

(In reply to Timea Cernea [:tbabos] from comment #7)

Hey Matt and Sam,

This issue seems to be fixed in the latest Nightly now.
I tried to find the patch in hope for a Beta uplfit but always got irrelevant bugs for auto-complete.

There was definitely a bug there, which attachment 9104094 [details] fixes. I'll request a beta uplift for it once it is merged. I was able to reproduce with the steps in comment #0. I'll take a look but I'm not aware of anything that landed recently that would have fixed this.

Hi Timea, the bug was sensitive to the last change time of the two logins so ensure that your are consistent with one or the other being older when testing. Can you try repro it again and verify the fix on Nightly?

Flags: needinfo?(tbabos)

Comment on attachment 9104094 [details]
Bug 1590362 - Prefer matched origin logins when deduping. r?MattN

Beta/Release Uplift Approval Request

  • User impact if declined: In some circumstances, the wrong login is provided as the first suggestion in the password autocomplete menu.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment #0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Change only has impact on sorting and de-duping of logins. Existing logic is otherwise unchanged, we just now also handle this specific case.
  • String changes made/needed: None
Flags: needinfo?(sfoster)
Attachment #9104094 - Flags: approval-mozilla-beta?
QA Whiteboard: [qa-triaged]
QA Contact: tbabos

Reproduced the issue again on affected Nightly 72, Build ID: 20191021215155.

Updated the affected Nightly build to the latest to confirm that the web address strings are correctly displayed now.
Tried it out once more on a new profile with the latest nightly by following the STR and got the same positive results.

Marking it as verified for Nightly 72 (Build ID: 20191028215046) on Windows 10, MacOS 10.15 and Ubuntu 18.04.
Waiting for uplift to Beta.

Flags: needinfo?(tbabos)

Comment on attachment 9104094 [details]
Bug 1590362 - Prefer matched origin logins when deduping. r?MattN

P1 fix for a new 71 feature, covered by tests and verified by our QA in nightly, uplift approved for 71 beta 6, thanks.

Attachment #9104094 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified - fixed on latest Firefox Beta 71.0b6 (20191030153225) - build from Treeherder - on Windows 10, MacOS10.15 and Ubuntu 18.04.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: