Closed Bug 1433876 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Form Autofill, defect, P1)

59 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- disabled
firefox60 --- disabled
firefox61 --- fixed

People

(Reporter: undercover-man, Assigned: MattN)

References

Details

(Keywords: regression)

Attachments

(5 files)

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
Component: Untriaged → HTML: Form Submission
Product: Firefox → Core
Component: HTML: Form Submission → Form Autofill
Product: Core → Toolkit
This seems bad… we should probably wrap our `notify` implementation in a try…catch
Assignee: nobody → MattN+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
Priority: -- → P1
Summary: form submit does not work → form submit does not work due to "keys is undefined" from FormAutofillUtils.jsm
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
Flags: needinfo?(undercover-man)
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
Flags: needinfo?(undercover-man)
browser.search.countryCode may also need to be DE
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
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.
here is the complete html page, where the error occurs.
(but all forms, on every pages are affected)
Attachment #8949327 - Flags: feedback+
(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 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
Attachment #8949299 - Flags: review?(jhofmann) → review+
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!
Attachment #8949298 - Flags: review?(jhofmann) → review+
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
Attachment #8949297 - Flags: review?(jhofmann) → review+
Matt, is this ready to land?
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
(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!
Pushed by mozilla@noorenberghe.ca:
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 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
Attachment #8970053 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/bad9ca5f4c9b
Follow-up eslint fix to handle Bug 1454813 rename of SpawnTask.js. r=MattN
Depends on: 1456148
You need to log in before you can comment on or make changes to this bug.