Autofill is too aggressive at saving profiles compared to Chrome

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: MattN, Assigned: selee)

Tracking

Trunk
mozilla58
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [form autofill:MVP], URL)

Attachments

(1 attachment)

e.g. Chrome doesn't seem to save just a first name + last name + organization. They also don't seem to save a (first name, last name, city, state, zip, country code) combo. The main issue is that users are getting too many partial addresses it seems.

Two of my profiles just have an email address (each a different one) + zip code since I needed to enter them as a confirmation on a website. I no longer have access to this website but this case was also below the threshold of 3.

I think we need to do somethings to be less aggressive. e.g. don't count first + last name as two fields for the threshold. Any name fields should count as 1 IMO.
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
(Assignee)

Updated

2 years ago
Assignee: nobody → selee
Status: NEW → ASSIGNED
After discussed with devs, I would also suggestion first name, middle name and last name count as 1.
(Assignee)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review188316

::: browser/extensions/formautofill/FormAutofillHandler.jsm:587
(Diff revision 1)
>      this._normalizeAddress(data.address);
>  
> +    let getAddressFieldCount = (record) => {
> +      let hasName = 0;
> +      let length = 0;
> +      for (let key of Object.keys(record)) {

AFAIK, there is no parent or inhertance of `record`, and I don't expect that we would like to look up the key in the prototype chain. This is why `for ... of Object.keys(...)` here rather than `for ... in`.

Comment 4

2 years ago
mozreview-review
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review189058

::: browser/extensions/formautofill/FormAutofillHandler.jsm:584
(Diff revision 1)
>        });
>      });
>  
>      this._normalizeAddress(data.address);
>  
> +    let getAddressFieldCount = (record) => {

I'm thinking to put this function outside `createRecords` because `createRecords` is big enough and this function may grow over time.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:587
(Diff revision 1)
>      this._normalizeAddress(data.address);
>  
> +    let getAddressFieldCount = (record) => {
> +      let hasName = 0;
> +      let length = 0;
> +      for (let key of Object.keys(record)) {

I'm fine with that.

::: browser/extensions/formautofill/test/unit/test_createRecords.js:34
(Diff revision 1)
> -      "family-name": "Doe",
> +      "organization": "Mozilla",
>        country: "United States",
>      },
>      expectedRecord: {
>        address: {
>          "given-name": "John",
> -        "family-name": "Doe",
> +        organization: "Mozilla",

nit: We should choose one style from `"organization"` or `organization` (w/ or w/o quotes).
Attachment #8911708 - Flags: review?(lchang)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review189058

> I'm thinking to put this function outside `createRecords` because `createRecords` is big enough and this function may grow over time.

The latest patch is with this change and moving the length check in the new function `_isAddressRecordCreatable`. I think the actual purpose of this function should not only count the length but also check it.
`_isCreditCardRecordCreatable` is the same purpose but for credit card records.
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review189058

> The latest patch is with this change and moving the length check in the new function `_isAddressRecordCreatable`. I think the actual purpose of this function should not only count the length but also check it.
> `_isCreditCardRecordCreatable` is the same purpose but for credit card records.

hmm, the debug log looks like should change with `_isAddressRecordCreatable` and `_isCreditCardRecordCreatable` functions.
(Assignee)

Comment 8

2 years ago
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

Remove the flag since I need to add more tests for credit card cases.
Attachment #8911708 - Flags: review?(lchang)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
Hey Luke, The latest patch is with some credit card cases now. Please review it again. Thanks. :P

Comment 11

2 years ago
mozreview-review
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review189526

Looks good. Thanks.
Attachment #8911708 - Flags: review?(lchang) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/130a44290c28
All name related fields should be counted as 1 field only when creating the records. r=lchang
Keywords: checkin-needed
Backed out for eslint failure at browser/extensions/formautofill/test/unit/test_createRecords.js:207:8 | Missing trailing comma:

https://hg.mozilla.org/integration/autoland/rev/27bd931e843d46869717e4160704a49e5c13a74e

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=130a44290c28b51b919c3f0d01ababad2a594c1d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133996953&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/formautofill/test/unit/test_createRecords.js:207:8 | Missing trailing comma. (comma-dangle)
Flags: needinfo?(lchang)
Thanks for the notice.
Flags: needinfo?(lchang) → needinfo?(selee)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
Hey Sebastian, the latest patch is with eslint fixes, please help to land it again. Thank you.
Flags: needinfo?(selee)
Keywords: checkin-needed

Comment 17

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f3faf87ed901
All name related fields should be counted as 1 field only when creating the records. r=lchang
Keywords: checkin-needed

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f3faf87ed901
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 19

2 years ago
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

Approval Request Comment
[Feature/Bug causing the regression]: New feature.
[User impact if declined]:
Autofill feature is too aggressive at saving the new profile from user's input.
Actually, we should focus on the address related profile.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]:
[Why is the change risky/not risky?]:
This is a minor change since it's about the criteria of saving address profiles.
[String changes made/needed]: No.
Attachment #8911708 - Flags: approval-mozilla-beta?
Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

Improve a new feature, taking it.
Should be in 57b5
Attachment #8911708 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 21

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/e28be9682521
status-firefox57: affected → fixed
Flags: in-testsuite+
(In reply to Sean Lee [:seanlee][:weilonge] from comment #19)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Sean's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
status-firefox56: affected → wontfix
You need to log in before you can comment on or make changes to this bug.