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

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P1)

Description

[Dan Ceban [:okazki98]]

Attachment: ebay saved login TRIPLE CLICk.png

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).

Comment 1

[Matthew N. [:MattN]]

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?

Comment 2

[Dan Ceban [:okazki98]]

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

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit auto_nag documentation.

Comment 4

[Sam Foster [:sfoster] (he/him)]

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.

Comment 5

[Dan Ceban [: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.

Comment 6

[Dan Ceban [:okazki98]]

Attachment: Chrome EBAY FAKE Passwords.csv

Comment 7

[Dan Ceban [:okazki98]]

Attachment: Console Logs - ebaySignin.txt

Comment 8

[Dan Ceban [:okazki98]]

Attachment: SavedLogins window - Nigthly.png

Comment 9

[Dan Ceban [:okazki98]]

Attachment: ebay filling the form with corrupted logins - DEMO.mp4

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit auto_nag documentation.

Comment 11

[Matthew N. [:MattN]]

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?

Comment 12

[Dan Ceban [:okazki98]]

timePasswordChanged: 18446732429235952000

This is identical for the all the affected logins.

Comment 13

[Matthew N. [:MattN]]

(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.

Comment 14

[Matthew N. [:MattN]]

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.

Comment 15

[Matthew N. [:MattN]]

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

Comment 16

[David Durst [:ddurst]]

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...

Comment 17

[:Gijs (he/him)]

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.

Comment 18

[:Gijs (he/him)]

Attachment: Bug 1557960 - cope with 0 values in Chrome's date/time columns when importing, r?mak

Comment 19

[Matthew N. [:MattN]]

(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.

Comment 20

[David Durst [:ddurst]]

(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)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb8ea5809615
cope with 0 values in Chrome's date/time columns when importing, r=mak

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/bb8ea5809615

Comment 23

[Matthew N. [:MattN]]

(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.

Comment 24

[:Gijs (he/him)]

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

Comment 25

[:Gijs (he/him)]

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.

Comment 26

[Adrian Florinescu [:aflorinescu]]

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?

Comment 27

[:Gijs (he/him)]

(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 .

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 30

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/8fc282145f2e

Comment 31

[Adrian Florinescu [:aflorinescu]]

Attachment: Chrome EBAY FAKE Passwords_samesubdomain.csv

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