1392947 - [Form Autofill] Misdetect birthday fields as cc-exp-year and cc-exp-month

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P3)

Description

[Sean Lee [:seanlee][:weilonge]]

The heuristics should be fixed to prevent detecting the birthday fields as cc-exp-year and cc-exp-month. This happens in QVC and Sears.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Since this issue is not in the web site already, this bug is removed from [form autofill:MVP].

Comment 2

[Ray Lin[:ralin]]

We went over the top websites in these days, and the problem is still unsolved on QVC. I'll investigate how we can improve this, maybe either adding matching pattern for BOD or enhancing the accuracy of cc-exp-* parser.

Comment 3

[Ray Lin[:ralin]]

wups... s/BOD/DOB/

Comment 4

[Ray Lin[:ralin]]

I just had a quick look to chromium field types, it doesn't define any related type to birthday fields. And it make sense as there's no fields in preferences ask you to fill out birthday information. Therefore, I guess the proper way would be enhancing the prediction for cc-exp field,  which would rather abandon uncertain field than fallback to cc-exp when we're not confident enough to tell the field set is exact cc-exp or cc-exp-* [0]. We need to take the risk in terms of breaking some false positive tests, however, for long term I think it's worthy.

[0] http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/browser/extensions/formautofill/FormAutofillHeuristics.jsm#516-520

Comment 5

[Ray Lin[:ralin]]

I would start from cribbing some useful helper functions from Chromium, or somehow make our flow easier to adapt its logic.

Comment 6

[Ray Lin[:ralin]]

Attachment: Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Still got 4 unexpected failures in unit tests, and few missing comments.

Review commit: https://reviewboard.mozilla.org/r/199706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199706/

Comment 7

[Ray Lin[:ralin]]

Even those patterns are added, I would expect some subtle differences will be spotted comparing with Chromium's implementation, our rules would be a bit looser. The reason is that if we're going to do some fundamental re-design or refactoring, I think it's preferable not to bring too much changes here, like input type filtering or some stateful variable.

Comment 8

[Ray Lin[:ralin]]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199706/diff/1-2/

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

https://reviewboard.mozilla.org/r/199706/#review205926

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:799
(Diff revision 2)
> +   * @param {RegExp} regexp
> +   *
> +   * @returns {boolean}
> +   */
> +  _matchRegexp(element, regexp) {
> +    const elemStrigns = this._getElementStrings(element);

Looks like `_getElementStrings` will be called more than once during single `getFormInfo` process. Is it possible to cache the result of `extractLabelStrings` in `_getElementStrings` so that we won't access the same DOM elements multiple times?

Comment 10

[Ray Lin[:ralin]]

Attachment: Bug 1392947 - (experimental) Use memoize function to help caching previous extracted label result.

Review commit: https://reviewboard.mozilla.org/r/201142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201142/

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199706/diff/2-3/

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8929974 [details]
Bug 1392947 - (experimental) Use memoize function to help caching previous extracted label result.

In this patch, I just want to introduce the memoize concept. I think it would be useful in some contexts, for example, remember the previous computed results in certain scope which lives along with its life cycle . Hence, we can reduce some unnecessary declarations and if-condition for caching, which looks bad somehow. Feel free give me some feedback about this.

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

https://reviewboard.mozilla.org/r/199706/#review206270

The patch looks good! However, I would like to read the patch again with the changes I addressed. Thank you.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:572
(Diff revision 3)
> +    }
> +    fieldScanner.parsingIndex = savedIndex;
> +
> +    // Set current field name to null as it failed to match any patterns.
> +    fieldScanner.updateFieldName(fieldScanner.parsingIndex, null);
> +    return false;

Since `_parseCreditCardExpirationDateFields` only cares about the interesting field types only, `return true` would be reasonable to finish the parsing for this element even there is no matching rule.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:805
(Diff revision 3)
> +   * @param {HTMLElement} element
> +   * @param {RegExp} regexp
> +   *
> +   * @returns {boolean}
> +   */
> +  _matchRegexp(element, regexp) {

`element` here can be replaced with the result of `this._getElementStrings` which is invoked by the caller e.g. `_parseCreditCardExpirationDateFields`. In addition,  `FormAutofillHeuristics.extractedLabelStrings` cache can be removed as well. The result of `_getElementStrings` would be cached in the caller of `_matchRegexp`.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:806
(Diff revision 3)
> +   * @param {RegExp} regexp
> +   *
> +   * @returns {boolean}
> +   */
> +  _matchRegexp(element, regexp) {
> +    const elemStrigns = this._getElementStrings(element);

nit: elemStrigns > elemStrings

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199706/diff/3-4/

Comment 15

[Ray Lin[:ralin]]

Thanks for review! The issues are fixed in latest patch. I ended up persisting the cached label strings within the return object, and theoretically it will be destroyed once the caller popping out the call stack, so we are free from the difficulty managing the cached results in Heuristic singleton.

Comment 16

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

https://reviewboard.mozilla.org/r/199706/#review206300

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:747
(Diff revision 4)
>     * @param {HTMLElement} element
>     * @returns {ElementStrings}
>     */
>    _getElementStrings(element) {
>      return {
> +      extractedLabelStrings: null,

nit: `extractedLabelStrings` is only used in `_getElementStrings` in the current patch.
Do you expect it will be used by the caller? If not, give it an underscore to mark its private scope.

Comment 17

[Sean Lee [:seanlee][:weilonge]]

LGTM! Thank you.

Comment 18

[Ray Lin[:ralin]]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199706/diff/4-5/

Comment 19

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

https://reviewboard.mozilla.org/r/199706/#review206318

Comment 20

[Ray Lin[:ralin]]

Thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48d015dec45a
Add more credit card expiration date matching patterns to enhance prediction. r=lchang,seanlee

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/48d015dec45a

Comment 23

[Ray Lin[:ralin]]

Hi Gabi,

Could you help us verify credit card expiration date can be filled correctly on QVC.com? 

STR:
1. create a credit card profile in Preferences
2. go to QVC checkout page
3. choose "Enter New Card" as payment method
4. use form autofill to fill the credit card data
5. verify if "Expiration Date" field has been filled with correct data

Thank you!

Comment 24

[Ray Lin[:ralin]]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Approval Request Comment
[Feature/Bug causing the regression]: Form Autofill Heuristics 
[User impact if declined]: Birthday fields will be recognized as a credit card expiration fields on QVC and Sears which are our target websites.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, in comment 23
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: only enhance the accuracy of fields prediction, doesn't break any existing expected result in our unit tests.
[String changes made/needed]: none

Thanks!

Comment 25

[Gabi Cheta [:Gabi] Desktop QA]

Issue was verified as fixed on 59.0a1 20171121100129 on Ubuntu 14.4, Windows 10x64 and MacOS 10.12.6.

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Take this to support shield study in beta. Beta58+.

Comment 27

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a8ed5d6e5ff3

Comment 28

[Gabi Cheta [:Gabi] Desktop QA]

Verified as fixed with 58.0b6 20171123161455 on Win 10 x64, Ubuntu 14.4 and MacOS 10.12.6