Saved login autocomplete shows "null" for the username due to invalid `timePasswordChanged`
Categories
(Firefox :: Migration, defect, P1)
Tracking
()
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:
- Save multiple logins for ebay.com
- Navigate to signin.ebay.com
- 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).
Updated•5 years ago
|
Comment 1•5 years ago
|
||
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?
Reporter | ||
Comment 2•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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.
Reporter | ||
Comment 5•5 years ago
|
||
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:
- Save multiple passwords for eBay with Chrome.
- Export those passwords to a CSV file.
- Edit the CSV file by removing a username from one login.
- Import the CSV passwords into Chrome (you can use the attached CSV with Fake passwords).
- Import the passwords from Chrome into Firefox Nightly.
- Navigate to signin.ebay.com
- 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.
Reporter | ||
Comment 6•5 years ago
|
||
Reporter | ||
Comment 7•5 years ago
|
||
Reporter | ||
Comment 8•5 years ago
|
||
Reporter | ||
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
The priority flag is not set for this bug.
:MattN, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•5 years ago
|
Comment 11•5 years ago
|
||
From the logs the cause is:
RangeError: date value is not finite in DateTimeFormat.format() 2 LoginAutoCompleteResult.jsm:121:41
Can you check the timePasswordChanged
values of the affected logins in logins.json?
Updated•5 years ago
|
Reporter | ||
Comment 12•5 years ago
|
||
timePasswordChanged: 18446732429235952000
This is identical for the all the affected logins.
Comment 13•5 years ago
|
||
(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•5 years ago
|
||
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•5 years ago
|
||
The relevant Chrome migrator code is here if somebody has time to reproduce or fix this.
Updated•5 years ago
|
Comment 16•5 years ago
•
|
||
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...
Updated•5 years ago
|
Assignee | ||
Comment 17•5 years ago
|
||
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 | ||
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
(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•5 years ago
|
||
(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•5 years ago
|
||
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
Comment 22•5 years ago
|
||
bugherder |
Comment 23•5 years ago
|
||
(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.
Assignee | ||
Comment 24•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 25•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 26•5 years ago
|
||
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?
Assignee | ||
Comment 27•5 years ago
|
||
(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•5 years ago
|
||
Agreed that this can skip ESR68 since it's not a new issue.
Comment 29•5 years ago
|
||
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•5 years ago
|
||
bugherder uplift |
Comment 31•5 years ago
•
|
||
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
Updated•5 years ago
|
Description
•