1717603 - [MacOS] Credit Card autofill does not work

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P1)

Description

[Timea Cernea [:tbabos][inactive]]

Attachment: Screenshot 2021-06-18 at 10.49.39.png

Affected Versions:
All latest Firefox versions:
Release 89.0.1
Beta 90.0b10
Nightly 91.0a1

Affected Platforms:
Seems like MacOS only, reproduced on 10.15, 10.13
Not reproducible on Windows 7/10 or Linux

Steps to reproduce:

1. Have your browser region set to allow CC autofill, ex: US
2. Have at least 1 CC card saved
3. Go to https://luke-chang.github.io/autofill-demo/basic_cc.html
4. Click on any of the fields (except CSC)

Expected Result:
The CC dropdown should be displayed upon clicking on any of the mentioned fields.

Actual Result:
Only a blank white/black square is displayed on the first click and nothing on the second.

Regression-Range:
This is a regression but I can't find a reliable mozregression pushlog, it is always different, and by checking the First releases without builds for the suspected Regressor it did not confirm that it worked before. Tentatively marking regression-windowswanted but this is very shady to mozregress. It definitely used to work but something broke it for macOS.

Comment 1

[Timea Cernea [:tbabos][inactive]]

Attachment: Log

Comment 2

[Dimi Lee [:dimi]]

I also encountered this issue today. This is bad :(
I'll see if i can figure this out soon.

Comment 5

[Neal Dave]

this happens with name/address also, not just CC info except it wont even show a blank box; it will pretend like the field isnt a autofill field

Comment 6

[Dimi Lee [:dimi]]

okay, this is because when we enumerate window in the FormAutofillParent, if the init happens too early, sometimes the windowlist is still empty. In that case, we don't load the customElements.js script and causes this bug.

From the bugs we have so far, this seems to only happen in MAC (also from the comment here). I'm not so sure if this can be fixed by the API and if someone knows how to fix it. I guess an easier solution would be delaying enumerate the window by listen to a certain event, or just retry enumerating if fail(theoretically we should eventually find the current window at least).

Comment 7

[Tim Giles [:tgiles]]

I've run into this bug on tchibdo.de and https://luke-chang.github.io/autofill-demo/basic_cc.html on Windows but instead of a black box appearing, no dropdown appears. I'm getting the same error message of item._adjustAcItem is not a function. I guess this is not an OS specific bug in this case. Restarting the browser seems to fix it. Not sure how the browser got into the bad state.

Not sure if this complicates the bug or simplifies it, but wanted to chime in

Comment 8

[Dimi Lee [:dimi]]

Hi gijs, smaug,
I’m not sure how to best solve this problem, so I would like to know if you have any suggestions on this bug (or point me to someone I can reach out to). If you are not the right person to ask, please feel free to remove the needinfo,

The issue happens when FormAutofillParent doesn’t inject the customElements.js script.
The script is injected duing FormAutofillParent initialization. We use Services.wm.getEnumerator("navigator:browser") to get the DOM window to inject the script, however, in some cases, FormAutofillParent is inited too early (it is inited while we load extensions), and the DOM window is not yet ready at the moment (so Services.wm.getEnumerator("navigator:browser") gets an empty list).

Do you know if there is any event FormAutofillParent can listen to to ensure the DOM window is ready? or is there a better timing to inject the script?

Comment 9

[:Gijs (he/him)]

(In reply to Dimi Lee [:dimi] from comment #8)

The issue happens when FormAutofillParent doesn’t inject the customElements.js script.
The script is injected duing FormAutofillParent initialization. We use Services.wm.getEnumerator("navigator:browser") to get the DOM window to inject the script, however, in some cases, FormAutofillParent is inited too early (it is inited while we load extensions), and the DOM window is not yet ready at the moment (so Services.wm.getEnumerator("navigator:browser") gets an empty list).

I don't really understand. What currently takes care of adding this script to new windows, that are created after startup?

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #9)

(In reply to Dimi Lee [:dimi] from comment #8)

The issue happens when FormAutofillParent doesn’t inject the customElements.js script.
The script is injected duing FormAutofillParent initialization. We use Services.wm.getEnumerator("navigator:browser") to get the DOM window to inject the script, however, in some cases, FormAutofillParent is inited too early (it is inited while we load extensions), and the DOM window is not yet ready at the moment (so Services.wm.getEnumerator("navigator:browser") gets an empty list).

I don't really understand. What currently takes care of adding this script to new windows, that are created after startup?

Actually, it looks like this uses a window mediator listener, see https://searchfox.org/mozilla-central/rev/70a94bffbb73d2b0ba751fb3905428fdbcd4d402/toolkit/components/formautofill/FormAutofillParent.jsm#97 .

The simplest way of doing the tracking is via browser/modules/BrowserWindowTracker.jsm , which has guarantees that a window always ends up being tracked. But that's a browser module, and for some reason form autofill lives in toolkit? I don't really know why that is, or if we use this on android or any other non-desktop-firefox consumers.

If we can't use BrowserWindowTracker, I would make the enumerator code use "" as the windowtype (so you get all windows) and then checking if the window is loaded, and if not, add an event listener for that. See https://searchfox.org/mozilla-central/rev/70a94bffbb73d2b0ba751fb3905428fdbcd4d402/toolkit/components/extensions/parent/ext-tabs-base.js#1484-1493 for how the extensions code itself does this.

If this is implemented as an extension, perhaps a simpler option is to use the webextension window APIs instead?

Comment 11

[Dimi Lee [:dimi]]

(In reply to :Gijs (he/him) from comment #10)

I don't really understand. What currently takes care of adding this script to new windows, that are created after startup?

Actually, it looks like this uses a window mediator listener, see https://searchfox.org/mozilla-central/rev/70a94bffbb73d2b0ba751fb3905428fdbcd4d402/toolkit/components/formautofill/FormAutofillParent.jsm#97 .

Yes, as you commented. We use window mediator listener for newly created window, and we also iterate though all the windows to inject script to the existing window. The problem of this issue is we have an existing window but the window doesn't have a DOM window yet (the window mediator onOpenWindow is not triggred in this case because the window is already opened while registering the listener).

The simplest way of doing the tracking is via browser/modules/BrowserWindowTracker.jsm , which has guarantees that a window always ends up being tracked. But that's a browser module, and for some reason form autofill lives in toolkit? I don't really know why that is, or if we use this on android or any other non-desktop-firefox consumers.

Yes, it was moved to toolkit to support geckoview in Bug 1691821

If we can't use BrowserWindowTracker, I would make the enumerator code use "" as the windowtype (so you get all windows) and then checking if the window is loaded, and if not, add an event listener for that. See https://searchfox.org/mozilla-central/rev/70a94bffbb73d2b0ba751fb3905428fdbcd4d402/toolkit/components/extensions/parent/ext-tabs-base.js#1484-1493 for how the extensions code itself does this.

I did try this approach before asking the question, but somehow I can't get any window when using "" as the windowtype (maybe I didn't use it correctly). I didn't dig deeper to see what caused the problem. Since you also suggested this, I'll try this approach again, thanks!

If this is implemented as an extension, perhaps a simpler option is to use the webextension window APIs instead?

okay, I'll also give this a try. Thanks!

Comment 12

[:Gijs (he/him)]

(In reply to Dimi Lee [:dimi] from comment #11)

(In reply to :Gijs (he/him) from comment #10)

If we can't use BrowserWindowTracker, I would make the enumerator code use "" as the windowtype (so you get all windows) and then checking if the window is loaded, and if not, add an event listener for that. See https://searchfox.org/mozilla-central/rev/70a94bffbb73d2b0ba751fb3905428fdbcd4d402/toolkit/components/extensions/parent/ext-tabs-base.js#1484-1493 for how the extensions code itself does this.

I did try this approach before asking the question, but somehow I can't get any window when using "" as the windowtype (maybe I didn't use it correctly). I didn't dig deeper to see what caused the problem. Since you also suggested this, I'll try this approach again, thanks!

Huh, interesting. Array.from(Services.wm.getEnumerator("")) should be an easy way to get all the windows you have open, and seems to WFM - though if they have not finished loading, you may need to wait before accessing certain properties/elements. Let me know if you still have issues with this going forward. :-)

Comment 13

[Pascal Chevrel (relman team) -> :pascalc]

Dimi, this is marked as P1/S2, should I track it for a potential uplift to beta or would a potential patch have to ride the trains? Thanks

Comment 14

[Dimi Lee [:dimi]]

(In reply to Pascal Chevrel:pascalc from comment #13)

Dimi, this is marked as P1/S2, should I track it for a potential uplift to beta or would a potential patch have to ride the trains? Thanks

I think I'm not able to request uplift to 91 in time, I'll see if I can fix it in 92 and let the patch ride the train.

Comment 15

[Dimi Lee [:dimi]]

(In reply to :Gijs (he/him) from comment #12)

Huh, interesting. Array.from(Services.wm.getEnumerator("")) should be an easy way to get all the windows you have open, and seems to WFM - though if they have not finished loading, you may need to wait before accessing certain properties/elements. Let me know if you still have issues with this going forward. :-)

ok, I think i was using Services.wm.getAppWindowEnumerator(""), using Services.wm.getEnumerator("") works just fine.

Comment 16

[Dimi Lee [:dimi]]

Attachment: Bug 1717603 - Ensure FormAutofill injects the custom for all the opened window.

This patch uses Services.wm.getEnumerator("") instead of Services.wm.getEnumerator("navigator:browser")
to ensure we get all the opened window during FormAutofill initialization. See WindowTrackerBase#*browserWindows
in ext-tabs-base.js for more details.

Comment 17

[Dimi Lee [:dimi]]

Attachment: Bug 1717603 - Ensure FormAutofill loads its script on all the opened window. r=gijs

This patch uses Services.wm.getEnumerator("") instead of Services.wm.getEnumerator("navigator:browser")
to ensure we get all the opened window during FormAutofill initialization. See WindowTrackerBase#*browserWindows
in ext-tabs-base.js for more details.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a65c1f8f12c
Ensure FormAutofill loads its script on all the opened window. r=Gijs

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3a65c1f8f12c

Comment 20

[Alin Ilea, Desktop Test Engineering]

Verified - Fixed in latest Nightly 92.0a1 (2021-08-01) using macOS 10.15. The CC dropdown is displayed accordingly.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

I assume we're going to want to backport this fix to the ESR91 branch at some point, feel free to wontfix if not.

Comment 22

[Ethan Tseng [:ethan]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

I assume we're going to want to backport this fix to the ESR91 branch at some point, feel free to wontfix if not.

Hi Ryan, this is not a low-risk patch. I wanna clarify with you - can we bake the patch for a while and backport the fix in a dot releases of ESR91, such as ESR91.1 (release on 2021-09-07)?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Yes, that was exactly what I meant. We can absolutely bake this before backporting to ESR. This just doesn't feel like a bug we want to live with for the entire 1+ year lifecycle of the new release.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR91 approval when you get a chance.

Comment 25

[Dimi Lee [:dimi]]

Comment on attachment 9233965 [details]
Bug 1717603 - Ensure FormAutofill loads its script on all the opened window. r=gijs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: For some users, credit card autofill will not work
• User impact if declined: For some users, credit card autofill will not work
• Fix Landed on Version: 92
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch doesn't change the logic for most of the cases, it only handles an edge case when we don't get the opened window during initialization.
• String or UUID changes made by this patch: None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9233965 [details]
Bug 1717603 - Ensure FormAutofill loads its script on all the opened window. r=gijs

Approved for 91.1esr.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/fe6e71bec6aa

Comment 29

[Timea Cernea [:tbabos][inactive]]

Verified - Fixed in latest 91.1.0esr (20210830184617 - treeherder build) using macOS 10.15. The CC dropdown is displayed accordingly, tested throughout multiple restarts.