1436903 - Shipping options are passed to the front-end even if requestShipping isn't true

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, defect, P1)

Description

[Matthew N. [:MattN]]

Attachment: Screen Shot showing one shipping option and requestShipping:false

shippingOptions are currently passed to the front-end even if requestShipping isn't true. This seems to be in violation of the spec's constructor algorithm[1]:

> 8. If the requestShipping member of options is present and set to true, process shipping options: 
>    1. Let options be an empty sequence<PaymentShippingOption>. 
>    …
>    3. Set details.shippingOptions to options. 

The same issue probably applies to the "Update a PaymentRequest's details algorithm"[2].

The front-end only wants to receive valid data according to the spec. It seems like bug 1398991 handled this only from the web-exposed side.

[1] https://w3c.github.io/payment-request/#constructor
[2] https://w3c.github.io/payment-request/#update-a-paymentrequest-s-details-algorithm

Comment 1

[Henri Sivonen (:hsivonen)]

Is whatever code you used to arrive at the screenshot shared somewhere?

Comment 2

[Matthew N. [:MattN]]

This screenshot is showing the nsIPaymentRequest object received in the front-end.

STR:
1) Set the following prefs:
* dom.payments.loglevel=Debug
* dom.payments.request.enabled=true
* devtools.debugger.remote-enabled=true
* devtools.chrome.enabled=true
2) Load https://paymentrequest.show/demo/
3) Open the Browser Toolbox
4) Set a breakpoint in `init` after `this.request = paymentSrv.getPaymentRequestById(requestId);` in chrome://payments/content/paymentDialogWrapper.js
5) Add some shipping options on paymentrequest.show by temporarily toggling "Shipping" on. Make sure to add a valid amount.
6) Choose "BUY NOW" with "Shipping" unchecked on paymentrequest.show
7) When the breakpoint is hit you can inspect `this.request` in the Console tab and expand paymentDetails.shippingOptions.

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1436903 - Avoid passing shipping options to the front end when shipping was not requested.

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

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 8956774 [details]
Bug 1436903 - Avoid passing shipping options to the front end when shipping was not requested.

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

Comment 5

[Henri Sivonen (:hsivonen)]

I don't have a good clue about what I'm going with the chrome side of the test case, so I modified an existing test. The result probably isn't minimal.

Comment 6

[Matthew N. [:MattN]]

Just as an idea to potentially simplify the code, did you consider reverting parts of bug 1398991 in order to clear the shipping options in one place rather than separately clear them for the UI and the DOM side?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #6)
> Just as an idea to potentially simplify the code, did you consider reverting
> parts of bug 1398991 in order to clear the shipping options in one place
> rather than separately clear them for the UI and the DOM side?

If I'm reading the spec correctly, the spec doesn't say to set details.shippingOptions to null if requestShipping is false. The shippingOptions just stay there without canonicalization or duplicate checking.

Marcos, is there a reason why details.shippingOptions doesn't get nulled out by spec if requestShipping is false even though it gets modified if requestShipping is true (so there doesn't appear to be a fundamental reason not to modify it)?

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 8956774 [details]
Bug 1436903 - Avoid passing shipping options to the front end when shipping was not requested.

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

Comment 9

[Marcos Caceres [:marcosc]]

(In reply to Henri Sivonen (:hsivonen) from comment #7)
> Marcos, is there a reason why details.shippingOptions doesn't get nulled out
> by spec if requestShipping is false even though it gets modified if
> requestShipping is true (so there doesn't appear to be a fundamental reason
> not to modify it)?

D’oh! I accidentally deleted the “Otherwise, set it to null” when I did the update. Thanks, will fix!

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 8956774 [details]
Bug 1436903 - Avoid passing shipping options to the front end when shipping was not requested.

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

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to Marcos Caceres [:marcosc] from comment #9)
> (In reply to Henri Sivonen (:hsivonen) from comment #7)
> > Marcos, is there a reason why details.shippingOptions doesn't get nulled out
> > by spec if requestShipping is false even though it gets modified if
> > requestShipping is true (so there doesn't appear to be a fundamental reason
> > not to modify it)?
> 
> D’oh! I accidentally deleted the “Otherwise, set it to null” when I did the
> update. Thanks, will fix!

Thanks. I'm a bit worried about implementing stuff I can't see in the Editor's Draft, so if the try run is OK, I intend to request review of the current state of the patch.

We can implement the upcoming spec fix in another bug.

Comment 12

[Marcos Caceres [:marcosc]]

(In reply to Henri Sivonen (:hsivonen) from comment #11)
> Thanks. I'm a bit worried about implementing stuff I can't see in the
> Editor's Draft, so if the try run is OK, I intend to request review of the
> current state of the patch.

That's fair. 

> We can implement the upcoming spec fix in another bug.

Yes, that would be preferable. Spec reviews can definitely result in unanticipated spec changes.

Comment 13

[Henri Sivonen (:hsivonen)]

This breaks https://searchfox.org/mozilla-central/source/toolkit/components/payments/test/browser/browser_request_serialization.js#44

Should I just comment out this part of the test for now in the hope that it can be uncommented when the change discussed in comment 9, comment 11 and comment 12 lands?

Comment 14

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

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> This breaks
> https://searchfox.org/mozilla-central/source/toolkit/components/payments/
> test/browser/browser_request_serialization.js#44
> 
> Should I just comment out this part of the test for now in the hope that it
> can be uncommented when the change discussed in comment 9, comment 11 and
> comment 12 lands?

yeah that sounds fine.

Comment 15

[Matthew N. [:MattN]]

Please change it to `todo_is` instead.

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 8956774 [details]
Bug 1436903 - Avoid passing shipping options to the front end when shipping was not requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225736/diff/4-5/

Comment 17

[Henri Sivonen (:hsivonen)]

Comment on attachment 8956774 [details]
Bug 1436903 - Avoid passing shipping options to the front end when shipping was not requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225736/diff/5-6/

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #15)
> Please change it to `todo_is` instead.

Treeherder says todo_is is undefined even though MDN says it should be there:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=339630d5005ec121db2a1cbcdfcfbf28cdba2f93&selectedJob=167613857

Comment 19

[Henri Sivonen (:hsivonen)]

Comment on attachment 8956774 [details]
Bug 1436903 - Avoid passing shipping options to the front end when shipping was not requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225736/diff/6-7/

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8956774 [details]
Bug 1436903 - Avoid passing shipping options to the front end when shipping was not requested.

https://reviewboard.mozilla.org/r/225736/#review233148

::: toolkit/components/payments/test/browser/browser_request_serialization.js:41
(Diff revision 7)
>      let contentWin = Cu.waiveXrays(content);
>      let store = contentWin.document.querySelector("payment-dialog").requestStore;
>      let state = store && store.getState();
>      ok(state, "got request store state");
>  
> -    let expected = details;
> +    // let expected = details;

Change the test or remove this block.

Comment 21

[Henri Sivonen (:hsivonen)]

(In reply to Andrea Marchesini [:baku] from comment #20)
> Comment on attachment 8956774 [details]
> Bug 1436903 - Avoid passing shipping options to the front end when shipping
> was not requested.
> 
> https://reviewboard.mozilla.org/r/225736/#review233148
> 
> :::
> toolkit/components/payments/test/browser/browser_request_serialization.js:41
> (Diff revision 7)
> >      let contentWin = Cu.waiveXrays(content);
> >      let store = contentWin.document.querySelector("payment-dialog").requestStore;
> >      let state = store && store.getState();
> >      ok(state, "got request store state");
> >  
> > -    let expected = details;
> > +    // let expected = details;
> 
> Change the test or remove this block.

Even in the light of comment 13 and comment 14?

Comment 22

[Andrea Marchesini [:baku]]

ah sorry, I haven't read those comments. r+

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e9f6f73c78e
Avoid passing shipping options to the front end when shipping was not requested. r=baku

Comment 24

[Matthew N. [:MattN]]

(In reply to Henri Sivonen (:hsivonen) from comment #18)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #15)
> > Please change it to `todo_is` instead.
> 
> Treeherder says todo_is is undefined even though MDN says it should be there:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=339630d5005ec121db2a1cbcdfcfbf28cdba2f93&selectedJob=1
> 67613857

Oh, yeah, that's because you're in a ContentTask which only has a subset of those.

Comment 25

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0e9f6f73c78e