1433876 - form submit does not work due to "keys is undefined" from FormAutofillUtils.jsm

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, defect, P1)

Description

[Randy Sieber]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180125163041

Steps to reproduce:

submit form

<form id="kontaktverwaltungForm" name="kontaktverwaltungForm" action="/kundenbereich/bauvorhaben/kontaktverwaltung/neuedit/" method="post" enctype="text/html" class="normalForm">
<input id="iMobilnr" type="text" name="iMobilnr" size="15" maxlength="25" value=""  />
<input name="mySubmit" id="mySubmit" type="submit" value="Kontakt erstellen"/>
</form>


Actual results:

NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "keys is undefined" {file: "resource://formautofill/FormAutofillUtils.jsm" line: 779}]'[JavaScript Error: "keys is undefined" {file: "resource://formautofill/FormAutofillUtils.jsm" line: 779}]' when calling method: [nsIFormSubmitObserver::notify]


Expected results:

submit the form to server

Comment 1

[Matthew N. [:MattN]]

This seems bad… we should probably wrap our `notify` implementation in a try…catch

Comment 2

[Matthew N. [:MattN]]

https://dxr.mozilla.org/mozilla-central/rev/0d806b3230fe4767fa70cdee57db87d1e9a5ba49/browser/extensions/formautofill/FormAutofillContent.jsm#389,391

Comment 3

[Matthew N. [:MattN]]

Hi Randy, I can't reproduce the problem with the form you pasted. I think it has to do with other parts of the page outside this one <form>. Is it possible to provide the link to the form? If not, can you provide the HTML for the whole page? Please attach it as a file: https://bugzilla.mozilla.org/attachment.cgi?bugid=1433876&action=enter

Also, could you provide the value of the preference `browser.search.countryCode` from about:config?

Thank you

Comment 4

[Matthew N. [:MattN]]

Nevermind… I figured out a test case. It requires a <select> which we determine to be address-level1 and the country needs to be DE.

Use https://luke-chang.github.io/autofill-demo/textarea_select.html and the following:

addressLevel2: Berlin
addressLevel1: Alabama (any nonsense state/province). The key thing is that this is a dropdown.
postalCode: 12345
country: Germany

Comment 5

[Matthew N. [:MattN]]

browser.search.countryCode may also need to be DE

Comment 6

[Matthew N. [:MattN]]

Full stack:
        keys is undefined FormAutofillUtils.jsm:779
	identifyValue resource://formautofill/FormAutofillUtils.jsm:779:9
	getAbbreviatedSubregionName resource://formautofill/FormAutofillUtils.jsm:612:31
	computeFillingValue resource://formautofill/FormAutofillHandler.jsm:690:17
	createRecord/< resource://formautofill/FormAutofillHandler.jsm:483:15
	forEach self-hosted:271:13
	createRecord resource://formautofill/FormAutofillHandler.jsm:479:5
	createRecords resource://formautofill/FormAutofillHandler.jsm:1110:25
	notify resource://formautofill/FormAutofillContent.jsm:411:21

Comment 7

[Matthew N. [:MattN]]

Attachment: Bug 1433876 - Test form submission with autofill in country without sub_keys.

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

Comment 8

[Matthew N. [:MattN]]

Attachment: Bug 1433876 - Don't assume countries have sub_keys in FormAutofillUtils.

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

Comment 9

[Matthew N. [:MattN]]

Attachment: Bug 1433876 - Wrap earlyformsubmit observer in try…catch to ensure autofill never stops form submission.

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

Comment 10

[Matthew N. [:MattN]]

Sorry to ask you again for an autofill review but in this case at least it's relevant to you being in DE.

For reference, sub_keys for DE aren't defined at https://dxr.mozilla.org/mozilla-central/rev/4fe6f6560083f8c8257282bef1d4e0ced9d1b975/browser/extensions/formautofill/addressmetadata/addressReferences.js#21 since the region (e.g. province) isn't used for mailing purposes. This is why we also don't show the region/province field in prefs if the country is Germany. The code was assuming that every country had sub_keys (US and CA do) and threw and exception when then broke form submission since the "earlyformsubmit" observer MUST return true.

To prevent this type of issue from happening again, I've wrapped the observer body in a try…catch like it should have been.

Comment 11

[Randy Sieber]

Attachment: firefox_form_submit_does_not_work.zip

here is the complete html page, where the error occurs.
(but all forms, on every pages are affected)

Comment 12

[Randy Sieber]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3)
> Hi Randy, I can't reproduce the problem with the form you pasted. I think it
> has to do with other parts of the page outside this one <form>. Is it
> possible to provide the link to the form? If not, can you provide the HTML
> for the whole page? Please attach it as a file:
> https://bugzilla.mozilla.org/attachment.cgi?bugid=1433876&action=enter
> 
> Also, could you provide the value of the preference
> `browser.search.countryCode` from about:config?
> 
> Thank you

browser.search.countryCode = DE

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8949299 [details]
Bug 1433876 - Wrap earlyformsubmit observer in try…catch to ensure autofill never stops form submission.

https://reviewboard.mozilla.org/r/218686/#review225574

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8949298 [details]
Bug 1433876 - Don't assume countries have sub_keys in FormAutofillUtils.

https://reviewboard.mozilla.org/r/218684/#review225576

Sorry for the delay, this looks good to me!

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8949297 [details]
Bug 1433876 - Test form submission with autofill in country without sub_keys.

https://reviewboard.mozilla.org/r/218682/#review225582

::: browser/extensions/formautofill/FormAutofillUtils.jsm:149
(Diff revision 1)
>    },
>  
>    /**
>     * Return the region metadata with default locale and other locales (if exists).
>     * @param   {string} country
> -   * @param   {string} level1
> +   * @param   {string?} level1

I'm not sure I've ever seen "{string?} level1" as a syntax for optional parameters. I usually use "{string} [optional] level1" though JSDoc apparently tells us it's actually "{string} [level1]".

http://usejsdoc.org/tags-param.html

It's not really important, I guess. You should go with ? if it's consistent.

In any case, the _loadData function has the same optional parameter which is not marked optional in the JSDoc.

::: browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html:23
(Diff revision 1)
> +/* import-globals-from ../../../../../toolkit/components/satchel/test/satchel_common.js */
> +/* import-globals-from formautofill_common.js */
> +
> +"use strict";
> +
> +let TEST_ADDRESSES = [{

nit: const

::: browser/extensions/formautofill/test/mochitest/test_address_level_1_submission.html:37
(Diff revision 1)
> +  let chromeScript = SpecialPowers.loadChromeScript(function test_country_data() {
> +    ChromeUtils.import("resource://formautofill/FormAutofillUtils.jsm");
> +    let data = AddressDataLoader.getData("DE");
> +    addMessageListener("CheckSubKeys", () => {
> +      return !data.defaultLocale.sub_keys;
> +    })

nit: semicolon

Comment 17

[Julien Cristau [:jcristau]]

Matt, is this ready to land?

Comment 18

[Matthew N. [:MattN]]

There is a test failure on try which I haven't had time to fix. This is also no longer an issue outside of Nightly since bug 1438306 landed.

Comment 19

[Jens]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #18)
> This is also no longer an issue outside of Nightly

Hi Matt, I am not sure what the Firefox policy is for such bugs for Nightly?! 

But I regularly work on an form, and this is constantly broken (as I reported in the dupe bug 1434581). Unfortunately, often I forget to switch to another browser before working with this form... Some days ago I wanted to book a flight, and after going through the 5-page-checkout-forms with passports, names and seat selections, I found out, that I should have done this all with Firefox stable instead of Nightly, probably again due to this bug (Clicking the "Book flight" button did just nothing, it worked in FF stable later)

So while I can understand (and accept!) that there are bugs and hiccups on Nightly and I am willing to help testing and reporting these bugs, I would like to question the policy of accepting P1 basic-functionality-breaking bugs in Nightly laying around for two months. This is probably a problem not only to me but to the whole Nightly community.

Let me still thank you all for your work!

Comment 20

[Matthew N. [:MattN]]

Updated try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f80c12ef31b07dc61aaf87ad52c22d090d093935

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8949297 [details]
Bug 1433876 - Test form submission with autofill in country without sub_keys.

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

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8949298 [details]
Bug 1433876 - Don't assume countries have sub_keys in FormAutofillUtils.

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

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8949299 [details]
Bug 1433876 - Wrap earlyformsubmit observer in try…catch to ensure autofill never stops form submission.

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49884f493869
Test form submission with autofill in country without sub_keys. r=johannh
https://hg.mozilla.org/integration/autoland/rev/8473057ddc37
Don't assume countries have sub_keys in FormAutofillUtils. r=johannh
https://hg.mozilla.org/integration/autoland/rev/043ccfe81258
Wrap earlyformsubmit observer in try…catch to ensure autofill never stops form submission. r=johannh

Comment 25

[Matthew N. [:MattN]]

Attachment: Bug 1433876 - Follow-up eslint fix to handle Bug 1454813 rename of SpawnTask.js.

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

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8970053 [details]
Bug 1433876 - Follow-up eslint fix to handle Bug 1454813 rename of SpawnTask.js.

https://reviewboard.mozilla.org/r/238810/#review244492

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bad9ca5f4c9b
Follow-up eslint fix to handle Bug 1454813 rename of SpawnTask.js. r=MattN

Comment 28

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/49884f493869
https://hg.mozilla.org/mozilla-central/rev/8473057ddc37
https://hg.mozilla.org/mozilla-central/rev/043ccfe81258
https://hg.mozilla.org/mozilla-central/rev/bad9ca5f4c9b

Comment 29

[Julien Cristau [:jcristau]]

bug 1438306 disabled this in 60