Closed Bug 1557960 Opened 5 months ago Closed 4 months ago

Saved login autocomplete shows "null" for the username due to invalid `timePasswordChanged`

Categories

(Firefox :: Migration, defect, P1)

69 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 + verified
firefox70 --- verified

People

(Reporter: okazki98, Assigned: Gijs)

References

(Regression)

Details

(Keywords: regression)

Attachments

(7 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

  1. Save multiple logins for ebay.com
  2. Navigate to signin.ebay.com
  3. Triple-click on the "Username" or "Password" form

Actual results:

Clicking once: Only the first row has a "Username" preview. The rows below only have the URLs.
Clicking thrice: "null" displayed on all the rows starting from the 2nd.

Expected results:

Show Username preview for all the rows (and please, no more "nulls" :D).

Component: Untriaged → Password Manager
Product: Firefox → Toolkit

This is potentially caused by bug 589628 but it could have been an existing issue. Can you run mozregression to identify when it started? Did this only start happening recently?

Has Regression Range: --- → no
Flags: needinfo?(okazki98)
Keywords: regression

Here's the mozregression results.

2019-06-12T10:42:46: INFO : Narrowed inbound regression window from [af54b2de, 52b90f48] (3 builds) to [af54b2de, 5150c8c0] (2 builds) (~1 steps left)
2019-06-12T10:42:46: DEBUG : Starting merge handling...
2019-06-12T10:42:46: DEBUG : Using url:
https://hg.mozilla.org/integration/autoland/json-pushes?changeset=5150c8c0a8b7213d0833b73e425c687835fff648&full=1
2019-06-12T10:42:47: DEBUG : Found commit message:
Bug 589628 - Tests for LoginManagerParent._searchAndDedupeLogins.

Differential Revision: https://phabricator.services.mozilla.com/D32557

Flags: needinfo?(okazki98)

The priority flag is not set for this bug.
:MattN, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(MattN+bmo)

Bug 589628 looks like a plausible culprit for this, but I'm not able to reproduce it. If you select one of those "null" entries, does it actually fill a real username and password? It looks to me like these are bad saved logins (and if so we want to figure out how they got that way.) Can you inspect them in the "Saved Logins" dialog from about:preferences#privacy and confirm they look ok there? You can filter by ebay.com to produce the same list as in your screenshot.

If you could enable debug logging and attach the logs produced when you visit ebay.com and get the suggested logins autocomplete menu that might also help us narrow this down more.

Clearing ni? for Matt, but I'm leaving this in triage for now until we know more.

Flags: needinfo?(MattN+bmo) → needinfo?(okazki98)

In Comment #0, the attached screenshot was from my personal account, where those saved passwords are pretty old. Some of them might've been imported from Chrome. I'm not sure if this is what triggered the logins to break, but I've found an alternative scenario to reproduce the issue.
So here's what I did to replicate this issue anew:

  1. Save multiple passwords for eBay with Chrome.
  2. Export those passwords to a CSV file.
  3. Edit the CSV file by removing a username from one login.
  4. Import the CSV passwords into Chrome (you can use the attached CSV with Fake passwords).
  5. Import the passwords from Chrome into Firefox Nightly.
  6. Navigate to signin.ebay.com
  7. Click on the form to trigger the saved logins suggestions.

Observations: When editing the CSV, the username was removed only from one of the saved logins. After importing the passwords from Chrome to Nightly, the "jailfish" logins also became corrupted in the list, supposedly because they are duplicates (although they belong to different subdomains of ebay).
When clicking on a corrupted "jailfish" login the browser actually fills the form with the correct username.

I'm not sure how my REAL passwords on the personal account got corrupted, since I certainly didn't use CSV to edit my personal passwords.
I can only speculate that it might have been corrupted during an import/export from/to another browser or a synchronization in the past and has been saved and synced like this up until now.
Attaching the test data and the console logs.

Flags: needinfo?(okazki98)

The priority flag is not set for this bug.
:MattN, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(MattN+bmo)
Status: UNCONFIRMED → NEW
Has Regression Range: no → yes
Ever confirmed: true
Regressed by: 589628

From the logs the cause is:

RangeError: date value is not finite in DateTimeFormat.format() 2 LoginAutoCompleteResult.jsm:121:41

https://searchfox.org/mozilla-central/rev/0671407b7b9e3ec1ba96676758b33316f26887a4/toolkit/components/passwordmgr/LoginAutoCompleteResult.jsm#121

Can you check the timePasswordChanged values of the affected logins in logins.json?

Flags: needinfo?(MattN+bmo) → needinfo?(okazki98)

timePasswordChanged: 18446732429235952000

This is identical for the all the affected logins.

Flags: needinfo?(okazki98)

(In reply to Dan Ceban [:okazki98] from comment #12)

timePasswordChanged: 18446732429235952000

This is identical for the all the affected logins.

Ok, that is the problem then since it's an invalid date… maybe the Chrome migrator is creating the invalid dates. We should figure out that root cause but then also either migrate the bad data and/or not throw for the label in this case.

Flags: needinfo?(MattN+bmo)

The new cause of this was disabled for non-Nightly in bug 1563325. The underlying issue of incorrect imported dates would still exist though and we also show the dates for empty usernames too so it should be reproducible before bug 589628 too.

The relevant Chrome migrator code is here if somebody has time to reproduce or fix this.

Summary: Saved logins auto-fill list shows only the URL starting with the 2nd row (signin.ebay) → Saved login autocomplete shows "null" for the username due to invalid `timePasswordChanged`

Gijs, your name was floated as someone who might be able to look at this (the migration part, not the pwdmgr part). MattN is slammed...

Flags: needinfo?(gijskruitbosch+bugs)

Chrome's CSV format does not include times for the logins, and rather than setting the time created to the then-current date when importing, it looks like Chrome just leaves the column set to 0. We then import the date, and because Chrome uses date values based on the year 1600, and we use unix epochs, we end up with a negative number (having tried to convert the 0 value as a "real" Chrome-format date from around 1600 CE). The negative number then makes things sad.

Fixing this in the migrator isn't particularly hard, I'll put up a patch in a second.

At the moment, it's unclear if this happens only when importing the CSV format into Chrome first, or if Chrome saves 0 to this column in other cases. It's also unclear if this could affect other date values like those for cookies or history/bookmarks that we import. If the import is the only way this breaks, I think we should untrack for branches, given that the import is behind chrome://flags to begin with (ie by default, it seems there is an export option but not an import option, for reasons that... I can't say I really understand).

Note: I'm putting up a patch here only for the migrator. Given the above, I'm not clear on whether we want to make changes to pw manager as well, but I guess it could happen in another bug.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :Gijs (he/him) from comment #17)

Note: I'm putting up a patch here only for the migrator. Given the above, I'm not clear on whether we want to make changes to pw manager as well, but I guess it could happen in another bug.

Thanks Gijs! I think the other piece would be doing a storage migration (to version 3) in LoginStore.jsm using dataPostProcessor to remove the bad dates so we don't have to deal with the bad data in the future (assuming there isn't another cause). I guess we could also add validation in addLogin or checkLoginValues for this too to prevent the bad data from getting to storage.

Flags: needinfo?(MattN+bmo)
See Also: → 1240278

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #19)

(In reply to :Gijs (he/him) from comment #17)

Note: I'm putting up a patch here only for the migrator. Given the above, I'm not clear on whether we want to make changes to pw manager as well, but I guess it could happen in another bug.

Thanks Gijs! I think the other piece would be doing a storage migration (to version 3) in LoginStore.jsm using dataPostProcessor to remove the bad dates so we don't have to deal with the bad data in the future (assuming there isn't another cause). I guess we could also add validation in addLogin or checkLoginValues for this too to prevent the bad data from getting to storage.

Matt -- do you want to address the pwd mgr side in another bug or here? (I don't think Gijs is dealing with anything but the migrator side)

Flags: needinfo?(MattN+bmo)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bb8ea5809615
cope with 0 values in Chrome's date/time columns when importing, r=mak
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

(In reply to David Durst [:ddurst] from comment #20)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #19)

(In reply to :Gijs (he/him) from comment #17)

Note: I'm putting up a patch here only for the migrator. Given the above, I'm not clear on whether we want to make changes to pw manager as well, but I guess it could happen in another bug.

Thanks Gijs! I think the other piece would be doing a storage migration (to version 3) in LoginStore.jsm using dataPostProcessor to remove the bad dates so we don't have to deal with the bad data in the future (assuming there isn't another cause). I guess we could also add validation in addLogin or checkLoginValues for this too to prevent the bad data from getting to storage.

Matt -- do you want to address the pwd mgr side in another bug or here? (I don't think Gijs is dealing with anything but the migrator side)

I guess another bug at this point. For the reasons in comment 17 though I'm not sure it's even worth it and not sure if this should be uplifted… it does seem to be low risk though to prevent future damage from this specific instance.

Flags: needinfo?(MattN+bmo)

Comment on attachment 9077562 [details]
Bug 1557960 - cope with 0 values in Chrome's date/time columns when importing, r?mak

Beta/Release Uplift Approval Request

  • User impact if declined: Potential breakage in password management (and potentially similar issues with cookies and/or bookmarks) when they were initially imported in Chrome without creation timestamps
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 5. Note that in Chrome, you'll want to use the chrome flags page to turn on password import/export
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a relatively small patch and it's early in beta, and it provides a correctness fix to users trying to migrate from Chrome, so we might as well uplift it at this stage. Plus it gets this bug out of my release tracking emails...
  • String changes made/needed: n/a
Attachment #9077562 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Someone else can make a call for esr68 for this particular patch. I'd tend to agree with Matt agreeing with me that it doesn't seem super high prio.

Component: Password Manager → Migration
Product: Toolkit → Firefox
Target Milestone: mozilla70 → ---
Target Milestone: --- → Firefox 70
Blocks: 1567196
QA Whiteboard: [qa-triaged]

Version:

Reproduced on affected:
69.0a1 20190703162038

Verified fix on:
70.0a1 20190721215935

Platform:

Windows 10 v.1809

On the import/migration subject, I'm pretty sure we're only supporting passwords import only on Windows: :Gijs / :MattN, can you please confirm?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(MattN+bmo)

(In reply to Adrian Florinescu [:adrian_sv] from comment #26)

On the import/migration subject, I'm pretty sure we're only supporting passwords import only on Windows: :Gijs / :MattN, can you please confirm?

Correct. See also https://wiki.mozilla.org/QA/Firefox_migrators#Supported_data_types .

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(MattN+bmo)

Agreed that this can skip ESR68 since it's not a new issue.

Flags: in-testsuite-

Comment on attachment 9077562 [details]
Bug 1557960 - cope with 0 values in Chrome's date/time columns when importing, r?mak

Improved handling for some profile data migrated from Chrome. Approved for 69.0b7.

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

Due to bug 1563325, I couldn't verify the fix out of the box on beta, so I've edited the original bogus import .csv to have the same sub-domain, in order to be able to verify it on beta7 as follows:

Platform:

Windows 10 v.1809

Version:

69.0b7 20190722201635

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.