Right now, we've made pref account for one of the fallback conditions, but still lack a test case for credit card field in test_basic_autocomplete_form.html.

Not only for pref, we will need all equivalents of what we've done for address as well.

Comment on attachment 8904294 [details] Bug 1393374 - Part 2. Add a credit card basic autofill mochitest. https://reviewboard.mozilla.org/r/176068/#review182692 Looks good. ::: browser/extensions/formautofill/test/mochitest/test_basic_creditcard_autocomplete_form.html:71 (Diff revision 1) > + > +function checkFormFilled(creditCard) { > + let promises = []; > + for (let prop in creditCard) { > + let element = document.getElementById(prop); > + let converted = "" + creditCard[prop]; // Convert potential number to string nit: `String(creditCard[prop])` ::: browser/extensions/formautofill/test/mochitest/test_basic_creditcard_autocomplete_form.html:199 (Diff revision 1) > + // Explicitly focus on the other field to receive "change" event from the original focused input. > + await setInput("#cc-csc", ""); That makes sense but I wonder why we didn't do this in the address' version?

Comment on attachment 8903626 [details] Bug 1393374 - Part 1. Introduce credit card helper functions in formautofill mochitest. https://reviewboard.mozilla.org/r/175398/#review183682 ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:53 (Diff revision 3) > } > > function checkMenuEntries(expectedValues, isFormAutofillResult = true) { > let actualValues = getMenuEntries(); > // Expect one more item would appear at the bottom as the footer if the result is from form autofill. > - let expectedLength = isFormAutofillResult ? expectedValues.length + 1 : expectedValues.length; > + let expectedLength = expectedValues.length + (+!!isFormAutofillResult); nit: I feel the original one looks clearer. ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:61 (Diff revision 3) > for (let i = 0; i < expectedValues.length; i++) { > is(actualValues[i], expectedValues[i], " Checking menu entry #" + i); > } > } > > -async function addAddress(address) { > +function invokeAsyncChromeTask(msg, res, payload = {}) { Not sure what `res` stands for. (response?) I'd prefer to use full names (including "message" for `msg`). ::: browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js:54 (Diff revision 3) > - if (data != type) { > - return; > + switch (collection | OPS[type]) { > + case (COLLECTIONS.address | OPS.add): { I kind of feel it's overkill since those combinations won't appear elsewhere and won't be reused. Maybe we can discuss it face to face.

Comment on attachment 8903626 [details] Bug 1393374 - Part 1. Introduce credit card helper functions in formautofill mochitest. https://reviewboard.mozilla.org/r/175398/#review183682 > nit: I feel the original one looks clearer. reverted to the original one. > Not sure what `res` stands for. (response?) I'd prefer to use full names (including "message" for `msg`). fixed > I kind of feel it's overkill since those combinations won't appear elsewhere and won't be reused. Maybe we can discuss it face to face. Yeah, agree. Seems no other place will re-use it and also no new operator or collection will be added in the near future. Thanks.

Comment on attachment 8903626 [details] Bug 1393374 - Part 1. Introduce credit card helper functions in formautofill mochitest. https://reviewboard.mozilla.org/r/175398/#review185392 Looks good. Thanks.

A leaked issue on try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d551d4e9a5536d074b6ba4b4c5138e7f883851e I'm looking into it.

Got the same error on other mochitests with local debug build, so I suspect that the problem is something rather than this patch. Will try to re-build on top of earlier commit and test again.

I've tried different reduced cases, but the part2 patch still cause a permafail with nsTArray_base leaked. Since Sean need those utils to write credit card mochitests, I'm thinking to split the part 1 off to another bug and land it first. Let's discuss in person next week.

Even a test of the simple form is with one input only, `elem.focus()` still makes the memory leak. with focus: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f24ba39a993052cc9828849d587fbe4d914082 without focus: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bbc850f799f6e4d17819c3dbfa4b4c69a715b67 These tests are without `addEventListener("focusin", this);` in FormAutofillFrameScript, so I guess this issue is related to `focus()` or other `focusin` handlers.

Does anyone have any idea that which focus related code causes this leak?

I'd like to give it a shot on autoland since it seems unlikely to be our problem from Sean's investigation.

Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5033a269b87f Part 1. Introduce credit card helper functions family in formautofill mochitest. r=lchang https://hg.mozilla.org/integration/autoland/rev/ed09c905bce8 Part 2. Add a credit card basic autofill mochitest. r=lchang

Backed out for the leak on OS X 10.10 debug mentioned earlier: https://hg.mozilla.org/integration/autoland/rev/9ebff732a7d1bb19852d7627de76cc2f767b7d33 https://hg.mozilla.org/integration/autoland/rev/686694b9544cae9393ff72a563a37d28741ddc0c Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ed09c905bce8ad46649e7fa53946a5bc2ad1a21e&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=131924679&repo=autoland > TEST-UNEXPECTED-FAIL | leakcheck | default process: 8 bytes leaked (nsStringBuffer) Needinfo an XPCOM peer if necessary, this looks like an implementation issue somewhere in the backend.

Hi Eric, nice to meet you over bugzilla :D I'm sorry to bother you, but we've encountered an memory leak issue and been having hard time to find the cause. We don't have too many clues of potential reasons, the closest one after times of narrow down is on comment 16 that Sean made a reduced test case that even only .focus() on an input element would introduce 8 bytes leaks on default process. Could you help us to point out the possible cause if you got some times? also not sure those are informative enough, we're willing to provide more context if needed. Thank you very much!

I've filed bug 1401454 to follow-up the memory leak, and I think this should not be the blocker on the way for other more important patches like 1398101, perhaps, we should go with skip-if=debug first until figuring out the cause of failure. Thanks.

Tree herder result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40d6cc36174b315e16f1b19c17d1d2c34b406e19

Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0735f44b742a Part 1. Introduce credit card helper functions in formautofill mochitest. r=lchang https://hg.mozilla.org/integration/autoland/rev/a182935e2bd4 Part 2. Add a credit card basic autofill mochitest. r=lchang

https://hg.mozilla.org/mozilla-central/rev/0735f44b742a https://hg.mozilla.org/mozilla-central/rev/a182935e2bd4

Thank you Luke, Sebastian :D

This is permafailing on Beta. Please fix the test or backout immediately.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31) > This is permafailing on Beta. Please fix the test or backout immediately. checking, thanks

Presumably the test assumes extensions.formautofill.creditCards.enabled will be set and that isn't currently the case for Beta?

Sorry, I should have included a link to the log too: https://treeherder.mozilla.org/logviewer.html#?job_id=132215741&repo=mozilla-beta

Thanks for the information, I'll attach a patch to explicitly pref on: http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/browser/app/profile/firefox.js#1706-1710

I meant in the test :D

I ended up adding a condition in mochitest.ini to stop the test from running on other releases.

Per IRC discussion with Luke, we're just going to backout for now. https://hg.mozilla.org/mozilla-central/rev/469eb992a9d166004f2601ce725786f671219054

BTW, you may find the XPCOM_MEM_LEAK_LOG env var useful for tracking down that debug leak.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #39) > Per IRC discussion with Luke, we're just going to backout for now. No problem, far enough we should not hurriedly get into a wallpaper fix for this. I'll have more discussion with Luke. > BTW, you may find the XPCOM_MEM_LEAK_LOG env var useful for tracking down > that debug leak. Will try, thank you for your help :-)

Andrew do you think you help :ralin out (see comment 23)?

If you set the XPCOM_MEM_LOG_CLASSES variable to nsStringBuffer it will show you the allocation stack for the string buffer. That might help you figure out what is going wrong. There's some information about how to set that when running locally and on TreeHerder here: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/BloatView#Bloat_Statistics_on_Tinderbox Let me know if you need further help.

Comment on attachment 8910279 [details] [diff] [review] Bug-1393374-Only-run-form-autofill-credit-card-mochitest.patch As patches have backout'ed, let's take our time to fix it properly in the original patch.

It looks like now it's able to set pref before running mochitests, though still no precedents did that. Credit card related tests now can run w/o caring about which release it's in, we'll have to keep the setting until no features hides behind prefs. Will flag checkin-needed once Luke agreed with this approach. Thanks.

(In reply to Ray Lin[:ralin] from comment #47) > It looks like now it's able to set pref before running mochitests, though still no precedents did that. We did the same thing in our browser chrome test already. > Will flag checkin-needed once Luke agreed with this approach. Thanks. I'm fine with that since the credit-card code is quite coupled with address' code. It seems a bit hard to disable credit-card tests one by one manually. Thanks for investigating this.

Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c7fd224bfe0 Part 1. Introduce credit card helper functions in formautofill mochitest. r=lchang https://hg.mozilla.org/integration/autoland/rev/61af19cbe6bf Part 2. Add a credit card basic autofill mochitest. r=lchang

Comment on attachment 8910279 [details] [diff] [review] Bug-1393374-Only-run-form-autofill-credit-card-mochitest.patch no longer need it.

https://hg.mozilla.org/mozilla-central/rev/2c7fd224bfe0 https://hg.mozilla.org/mozilla-central/rev/61af19cbe6bf