PaymentRequest: fixes allowpaymentrequest test errors #6418
Conversation
Pull from upstream
1) The error in removing-allowpaymentrequest.https.sub.html is that the test expects the PaymentRequest Constructor to fail before allowpaymentrequest has been turned off. 2) The error in setting-allowpaymentrequest.https.sub.html is that the test expects the PaymentRequest Constructor to succeed before allowpaymentrequest has ever been turned on.
This test currently fails in Chrome and Edge. From the comments in the test it appears that the test expects to turn on the allowpaymentrequest asynchronously before the iframe can receive the postmessage. However, the error in the test is that it expects a security error to be thrown after the allowpaymentrequest attribute has been turned on.
Firefox (nightly)Testing web-platform-tests at revision 2b23009 |
Sauce (safari)Testing web-platform-tests at revision 2b23009 |
Chrome (unstable)Testing web-platform-tests at revision 2b23009 |
Sauce (MicrosoftEdge)Testing web-platform-tests at revision 2b23009 |
| @@ -13,7 +13,7 @@ | |||
| const path = location.pathname.substring(0, location.pathname.lastIndexOf('/') + 1); | |||
| iframe.src = "https://{{domains[www1]}}:{{ports[https][0]}}" + path + "echo-PaymentRequest.html"; | |||
| iframe.onload = t.step_func(() => { | |||
| if (i === 0) { | |||
| if (i === 1) { | |||
There was a problem hiding this comment.
I'm confused. When onload is called, the expectation is still that i should be 0. Otherwise iframe.allowPaymentRequest = false; is never set? I think the same applies for the rest?
@zcorpan, you wrote these tests, right? Can you confirm?
There was a problem hiding this comment.
(fixed typo, s/exception/expectation... grrr... autocorrect)
There was a problem hiding this comment.
@marcoscaceres I certainly understand that the flow of this test is easy to mistake.
My northstar assumption is that this test file is trying to verify that removing the allowpaymentrequest attribute from iframe causes the PaymentRequest constructor inside of the iframe to start throwing SecurityExceptions
Keep in mind that the iframe src is echo-PaymentRequest.html and this test relies on the script defined in that file as well.
Execution starting from the parent Window:
- Initially iframe.allowPaymentRequest = true;
- First time the iframe loads, script in iframe receives postmessage and calls the PaymentRequest constructor. Because
allowpaymentrequestis still enabled, Iframe should postmessage to parent window indicating success. - parentWindow receives message from iframe and does the following:
- increments counter
i++to 1, - (parent window should expect iframe to report success because we haven't turned off
allowpaymentrequestyet) Parent window validates success result. (without my change the test fails here because it would instead be expecting a SecurityError at this point) - parent window removes
allowpaymentrequestfrom the iframe - parent window triggers iframe onload to fire again via navigation.
^This resets the entire communication loop, replaying the iframeonload& parent windowonmessagehandlers a second time.
^Once again iframe will try to construct another paymentrequest, only this time a SecurityError should be raised. This time when receiving the iframe's response, the parent window onmessage handler will increment counteri++to 2, leading the rest of the function to then correctly expect a Security Error result from the Iframe.
There was a problem hiding this comment.
edited the above for clarity.
There was a problem hiding this comment.
@marcoscaceres @zcorpan, let me know if any further clarification is needed
There was a problem hiding this comment.
@AmazingJaze thanks for the clarification.
@zcorpan (and @sideshowbarker, assuming you reviewed this), we should avoid tests like this - it's too easy to screw them up for both the reviewer and the person writing them.
There was a problem hiding this comment.
I disagree we should avoid tests like this. This needs to be interoperably implemented to not open up security problems...
There was a problem hiding this comment.
Sorry, I was not clear, I meant more with the style in which the tests was written. Its was difficult to decipher what is going on.
|
Rerunning the tests. Hopefully they pass now.... |
|
Hmm. I believe the test was correct before this change. https://w3c-test.org/payment-request/allowpaymentrequest/removing-allowpaymentrequest.https.sub.html now fails in Chrome Canary with experimental web platform features enabled.
Note that the state of whether paymentrequest is allowed is set when the browsing context is created, and again when it is navigated, but it is not updated directly in response to changing the attribute. I know this is quite confusing, but the design is intentional, and it is important that it is tested correctly that the state is set exactly as specified. See whatwg/html#2195 The test can do a better job at documenting what it's doing, of course. cc @rsolomakhin |
…atform-tests#6418)" (web-platform-tests#6905) * Revert "PaymentRequest: fixes allowpaymentrequest test errors (web-platform-tests#6418)" This reverts commit 9a6a1f4. * Document what removing-allowpaymentrequest.https.sub.html is doing * Explain expected result in setting-allowpaymentrequest-timing.https.sub.html
…atform-tests#6418)" (web-platform-tests#6905) * Revert "PaymentRequest: fixes allowpaymentrequest test errors (web-platform-tests#6418)" This reverts commit 9a6a1f4. * Document what removing-allowpaymentrequest.https.sub.html is doing * Explain expected result in setting-allowpaymentrequest-timing.https.sub.html
Fixes test errors causing test files to fail in Edge and Chrome.
Edit: Also fixes an error in setting-allowpaymentrequest-timing.https.sub.html
3) From the comments in the test it appears that the test expects to turn on the allowpaymentrequest asynchronously before the iframe can receive the postmessage. However, the error in the test is that it expects a security error to be thrown after the allowpaymentrequest attribute has been turned on.
//CC @marcoscaceres