Check passwords engine handles incoming records with null or missing username fields
Categories
(Firefox :: Sync, defect, P2)
Tracking
()
People
(Reporter: markh, Assigned: lougenia)
Details
Attachments
(1 file)
|
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
MattN thinks we don't and will end up throwing when trying to update the login
|
||
Comment 1•5 years ago
|
||
Yeah, I'm fairly sure we don't since on every sync I get an error (see the last line)
LoginManager: Searching for logins LoginManager.jsm:461:9
Login storage: _searchLogins: returning 0 logins for
Object { guid: "nVIUBkGysOzy" }
with options
Object { }
storage-json.js:556:10
LoginManager: Searching for logins matching origin: https://auth.mozilla.auth0.com formActionOrigin: https://auth.mozilla.auth0.com httpRealm: null LoginManager.jsm:442:9
Login storage: _searchLogins: returning 2 logins for
Object { origin: "https://auth.mozilla.auth0.com", formActionOrigin: "https://auth.mozilla.auth0.com", httpRealm: null }
with options
Object { schemeUpgrades: false, acceptDifferentSubdomains: false }
storage-json.js:556:10
Login storage: _findLogins: returning 2 logins storage-json.js:600:10
Invalid chrome URI: /
LoginManager: Searching for logins LoginManager.jsm:461:9
Login storage: _searchLogins: returning 0 logins for
Object { guid: "nVIUBkGysOzy" }
with options
Object { }
storage-json.js:556:10
Error: Can't add a login with a null username. LoginManager.jsm:264:13
Since the GUID is a shorter one this must have come from a non-desktop client.
When I look at the record in about:sync on the Passwords => Validation tab, it has no username property.
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 2•5 years ago
|
||
Link to diff https://phabricator.services.mozilla.com/D49805
|
|
Assignee | |
Comment 3•5 years ago
|
||
|
|
Assignee | |
Updated•5 years ago
|
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b300b58faf47
Check passwords engine handles incoming records with null or missing username fields r=lina
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f1b618576af6 Backed out changeset b300b58faf47 for browser chrome failures on test_basic_form_autocomplete.html.
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3ad6ab02d033 Check passwords engine handles incoming records with null or missing username fields r=lina
|
||
Comment 7•5 years ago
|
||
Relanded this cause the failure turned out to be an intertemittent one: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linux%2Cx64%2Cquantumrender%2Cdebug%2Cmochitests%2Ctest-linux64-qr%2Fdebug-mochitest-e10s-15%2Cm%2815%29&revision=efee2d5a87089a80b066571b5aac5aaebba9a6e0&selectedJob=272662726
Filed Bug 1590928 instead. Sorry if it caused any trouble.
|
||
Comment 8•5 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 9•5 years ago
|
||
Comment on attachment 9103622 [details]
Bug 1588948 - Check passwords engine handles incoming records with null or missing username fields
Beta/Release Uplift Approval Request
- User impact if declined: Sync users whose mobile client provides a null value in the login username field will encounter sync errors if this fix is not uplifted.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This change is not risky because it is a small change that will only affect sync users whose login username is submitted as null, which is a subset of users. Additionally, xpcshell unit tests have been added to cover this change, which is small and isolated in the codebase.
- String changes made/needed: none
|
||
Comment 10•5 years ago
|
||
Comment on attachment 9103622 [details]
Bug 1588948 - Check passwords engine handles incoming records with null or missing username fields
Fix for a well-understood functional issue with sync, patch has tests and has been on nightly for a couple days, le's take it in 71 beta 5, thanks!
|
||
Comment 11•5 years ago
|
||
| bugherder uplift | ||
Description
•