1480880 - Changing defaults preferences does not update the checkbox for saving address and credit cards in the payments UI

Status: VERIFIED FIXED

(Firefox :: WebPayments UI, defect, P1)

Description

[Sam Foster [:sfoster] (he/him)]

This is a follow-up from bug 1477106. Although this functionality is covered by automated tests and those are passing as expected, in manual testing the pref values for dom.payments.defaults.saveCreditCard and dom.payments.defaults.saveAddress are not being correctly passed through to the dialog content, so the initial checked state does not reflect the pref value.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1480880 - Fix exposing of payment defaults prefs object.

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

Comment 2

[Sam Foster [:sfoster] (he/him)]

I've done some initial investigation into this. There is an actual bug here - the object returned by PaymentDialogUtils.getDefaultPreferences() was not being cloneInto'd  so it appeared to the unprivileged code as an empty object, meaning each property evaluated as false. This is fixed in the patch, but the false positive from the test is not. 

When the same code is run in the test harness, this object is apparently populated with the correct values with or without the cloneInto fix. We have explicit test coverage for the pref values driving the checked state of the "Save credit card to Firefox" checkbox in the browser_card_edit.js test. I've verified that the correct value is being read and also that the expected value is correct. I tried putting the defaults into the state object to make it easier to inspect from tests and at runtime with our debugging tools. That confirmed that there is some difference between the test environment and when opening the payment dialog manually. I don't have any insight into where or why this is.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8997614 [details]
Bug 1480880 - Fix exposing of payment defaults prefs object.

https://reviewboard.mozilla.org/r/261306/#review268396

::: browser/components/payments/content/paymentDialogFrameScript.js:99
(Diff revision 1)
>            saveCreditCardDefaultChecked:
>              Services.prefs.getBoolPref(SAVE_CREDITCARD_DEFAULT_PREF, false),
>            saveAddressDefaultChecked:
>              Services.prefs.getBoolPref(SAVE_ADDRESS_DEFAULT_PREF, false),
>          };
> -        return prefValues;
> +        return Cu.cloneInto(prefValues, waivedContent);

If you can't figure out how to update a m-bc test then you can throw an error in the shipping code if `typeof` of the pref property on the object isn't a boolean and that should cause tests to fail due to the console watching.

Comment 4

[Sam Foster [:sfoster] (he/him)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3)
> Comment on attachment 8997614 [details]

> If you can't figure out how to update a m-bc test then you can throw an
> error in the shipping code if `typeof` of the pref property on the object
> isn't a boolean and that should cause tests to fail due to the console
> watching.

I can add the assert in the shipping code - in basic-card-form.js, but this doesn't actually help with the false positive test results, as when running the test the object has the correctly typed values even without the cloneInto fix, so the typeof check will pass and no exception is logged. I'll update the patch to be sure we're talking about the same thing.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8997614 [details]
Bug 1480880 - Fix exposing of payment defaults prefs object.

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

Comment 6

[Sam Foster [:sfoster] (he/him)]

I guess that should have been a need-info. See comment 4.

Comment 7

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] from comment #4)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #3)
> > Comment on attachment 8997614 [details]
> 
> > If you can't figure out how to update a m-bc test then you can throw an
> > error in the shipping code if `typeof` of the pref property on the object
> > isn't a boolean and that should cause tests to fail due to the console
> > watching.
> 
> I can add the assert in the shipping code - in basic-card-form.js, but this
> doesn't actually help with the false positive test results, as when running
> the test the object has the correctly typed values even without the
> cloneInto fix, so the typeof check will pass and no exception is logged.
> I'll update the patch to be sure we're talking about the same thing.

OK, but I guess you see the errors in a regular build in the console without the cloneInto fix? I think the custom elements shim may hide them unless you disable it with dom.webcomponents.customelements.enabled=true

Comment 8

[Sam Foster [:sfoster] (he/him)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #7)

> OK, but I guess you see the errors in a regular build in the console without
> the cloneInto fix? I think the custom elements shim may hide them unless you
> disable it with dom.webcomponents.customelements.enabled=true

With a regular build & run, once I get to the card add/edit screen, I get this in the browser console: "Security wrapper denied access to property "saveCreditCardDefaultChecked" on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ has been removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported." 

That is with or without dom.webcomponents.customelements.enabled.  But I get nothing like this when doing the same in a test. Its not just suppressing the error, the values are actually getting passed through without the cloneInto() fix. I don't want to keep spinning on this - we have other priorities and I can just land the patch as-is. But as this is a common (for me at least) source of errors and we rely on these tests to catch the kind of corner cases which are tedious/impractical to test manually  - like the intersection of user actions with preference values, I don't want to defer creating a more robust test for this case too long.

Comment 9

[Matthew N. [:MattN]]

OK, I guess it's fine to land this then without a test. I think what you're seeing is the issue from bug 1397513 comment 1 (I think he meant ContentTask there).

Comment 10

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 327ef6a046ada97c5487f41331b90c4c93e7e24d -d 1a03fc28195e: rebasing 477457:327ef6a046ad "Bug 1480880 - Fix exposing of payment defaults prefs object. r=MattN" (tip)
merging browser/components/payments/content/paymentDialogFrameScript.js
merging browser/components/payments/res/containers/basic-card-form.js
warning: conflicts while merging browser/components/payments/content/paymentDialogFrameScript.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 11

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8997614 [details]
Bug 1480880 - Fix exposing of payment defaults prefs object.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/554e0fbeec46
Fix exposing of payment defaults prefs object. r=MattN

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/554e0fbeec46

Comment 14

[Timea Cernea [:tbabos][inactive]]

Verified - Fixed on the latest Firefox Nightly 63.0a1 (2018-08-09) on Windows 10 x64, Windows 7 x64, Ubuntu 16.04 and Mac 10.13.

Changing default preferences in about:config will update the checkbox for saving address and credit cards in the payments UI.