1403079 - Pass web-platform-test under https://w3c-test.org/payment-request/allowpaymentrequest/

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Eden Chuang[:edenchuang]]

Because the web-platform-test update, https://github.com/w3c/web-platform-tests/pull/6905, our implementation fails with several tests under https://w3c-test.org/payment-request/allowpaymentrequest/.

According to the comments on https://github.com/w3c/web-platform-tests/pull/6905, the allowpaymentrequest attribute should not be applied immediately, the modification should be applied while the context is reloaded or created.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Bug 1403079 - Add a new attribute allowpaymentrequest on nsIDocument for setting allowpayment when setting the parent document. r?baku

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8919129 [details] [diff] [review]
Bug 1403079 - Add a new attribute allowpaymentrequest on nsIDocument for setting allowpayment when setting the parent document. r?baku

Review of attachment 8919129 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +4301,5 @@
>      // aSubDoc is nullptr, remove the mapping
>  
>      if (mSubDocuments) {
> +      // restore the allowpaymentrequest value for binding sub document
> +      GetSubDocumentFor(aElement)->SetAllowPaymentRequest(false);

this can return null.

@@ +4340,5 @@
>      entry->mSubDocument = aSubDoc;
>      NS_ADDREF(entry->mSubDocument);
>  
> +    // setup the allowpaymentrequest for the subdocument when the element is an
> +    // iframe and its allowpaymentrequest attribute is ture.

true

@@ +4343,5 @@
> +    // setup the allowpaymentrequest for the subdocument when the element is an
> +    // iframe and its allowpaymentrequest attribute is ture.
> +    if (aElement->IsHTMLElement(nsGkAtoms::iframe) &&
> +        aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::allowpaymentrequest) &&
> +        aElement->GetBoolAttr(nsGkAtoms::allowpaymentrequest)) {

I'm not sure this is the right place to do this check.

::: dom/payments/test/test_allowpaymentrequest.html
@@ +20,5 @@
> +        if (i === 0) {
> +          ifrr.removeAttribute("allowpaymentrequest");
> +        }
> +        ifrr.contentWindow.postMessage('checking new PaymentRequest', '*');
> +      }); 

extra space

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8919129 [details] [diff] [review]
Bug 1403079 - Add a new attribute allowpaymentrequest on nsIDocument for setting allowpayment when setting the parent document. r?baku

This doesn't seem to work the expected way if one has subsubdocuments.
One could have iframe which doesn't have allowpaymentrequest, but if it has then iframe in it, and that has allowpaymentrequest, then the subsubdocument would get the allowpaymentrequest.
And certainly this is missing a test for that case.

https://w3c.github.io/payment-request/#constructor links to
https://html.spec.whatwg.org/multipage/iframe-embed-object.html#allowed-to-use

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Bug 1403079 - Add mAllowPaymentRequest and related methods on nsIDocument to check if PaymentRequest API is allowed to use. r?olli

In comment 3, Olli mentioned a case as following

> This doesn't seem to work the expected way if one has subsubdocuments.
> One could have iframe which doesn't have allowpaymentrequest, but if it has
> then iframe in it, and that has allowpaymentrequest, then the subsubdocument
> would get the allowpaymentrequest.
> And certainly this is missing a test for that case.

In fact, we handle the case in the PaymentRequest::Constructor() to avoid traversing document tree while nsDocument::SetSubDocumentFor() for performance improvement, but it makes the document's allowpaymentrequest mismatch with the spec. So I implement this patch to let allowpaymentrequest match to spec. And also add a new test case for the nested iframe case.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8920918 [details] [diff] [review]
Bug 1403079 - Add mAllowPaymentRequest and related methods on nsIDocument to check if PaymentRequest API is allowed to use. r?olli

>+bool
>+nsDocument::IsAllowPaymentRequest() const
>+{
>+  return mAllowPaymentRequest;
>+}

Nit, I guess I would call the method just AllowPaymentRequest()

>+
>+void
>+nsDocument::SetAllowPaymentRequest(bool aAllowPaymentRequest)
>+{
>+  mAllowPaymentRequest = aAllowPaymentRequest;
>+  // re-calculate AllowPaymentRequest for children documents
>+  if (mSubDocuments) {
>+    for (auto iter = mSubDocuments->Iter(); !iter.Done(); iter.Next()) {
This looks weird. Why do we need to iterate the subdocuments?
mAllowPaymentRequest should have its right value before the document has any subdocument.
Is that not the case?



...
>+      // case 3: AllowPaymentRequest is true in the current document and the
>+      // origin is differnt with the subdocument, and the parent element of the
>+      // subdocument is iframe with allowpaymentrequest flag, set true on the
>+      // subdocument, otherwise set false.
>+      if (entry->mKey->IsHTMLElement(nsGkAtoms::iframe) &&
>+          entry->mKey->HasAttr(kNameSpaceID_None, nsGkAtoms::allowpaymentrequest) &&
>+          entry->mKey->GetBoolAttr(nsGkAtoms::allowpaymentrequest)) {
>+        entry->mSubDocument->SetAllowPaymentRequest(true);
Especially, why we need this code, when we do the same thing in nsDocument::SetSubDocumentFor?

Comment 6

[Eden Chuang[:edenchuang]]

(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #5)
> Comment on attachment 8920918 [details] [diff] [review]
> Bug 1403079 - Add mAllowPaymentRequest and related methods on nsIDocument to
> check if PaymentRequest API is allowed to use. r?olli
> 
> >+bool
> >+nsDocument::IsAllowPaymentRequest() const
> >+{
> >+  return mAllowPaymentRequest;
> >+}
> 
> Nit, I guess I would call the method just AllowPaymentRequest()
> 
> >+
> >+void
> >+nsDocument::SetAllowPaymentRequest(bool aAllowPaymentRequest)
> >+{
> >+  mAllowPaymentRequest = aAllowPaymentRequest;
> >+  // re-calculate AllowPaymentRequest for children documents
> >+  if (mSubDocuments) {
> >+    for (auto iter = mSubDocuments->Iter(); !iter.Done(); iter.Next()) {
> This looks weird. Why do we need to iterate the subdocuments?
> mAllowPaymentRequest should have its right value before the document has any
> subdocument.
> Is that not the case?
> 

The reason of iterating the subdocument is I assume that in some case we could transfer one document tree from one document to another document. And the document tree uses the Payment API in children document. Considering the following

iframe1.setAttribute('allowpaymentrequest', false);
iframe1.appendChild(iframe3);
iframe3.appendChild(iframe4);

   ...
iframe1.removeChild(iframe3);
ifrmae2.setAttribute('allowpaymentrequest', true);
iframe2.appendChild(iframe3);

and in iframe4, new PaymentRequest is called();

Consider that iframe1's mAllowPaymentRequest is false, so iframe3 and iframe4's mAllowPaymentRequest will be set as false while creating. and iframe2's mAllowPaymentRequest is true, so iframe3's mAllowPaymentRequest should be changed to true, but if we don't iterate subDocument, iframe4's mAllowPaymentRequest would not be updated to true.

> 
> 
> ...
> >+      // case 3: AllowPaymentRequest is true in the current document and the
> >+      // origin is differnt with the subdocument, and the parent element of the
> >+      // subdocument is iframe with allowpaymentrequest flag, set true on the
> >+      // subdocument, otherwise set false.
> >+      if (entry->mKey->IsHTMLElement(nsGkAtoms::iframe) &&
> >+          entry->mKey->HasAttr(kNameSpaceID_None, nsGkAtoms::allowpaymentrequest) &&
> >+          entry->mKey->GetBoolAttr(nsGkAtoms::allowpaymentrequest)) {
> >+        entry->mSubDocument->SetAllowPaymentRequest(true);
> Especially, why we need this code, when we do the same thing in
> nsDocument::SetSubDocumentFor?

Comment 7

[Eden Chuang[:edenchuang]]

After tracing the code, I found that the subdocument tree transfer issue would not happen. 
So, I will modify the patch according to the comment 5.

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Bug 1403079 - Add mAllowPaymentRequest and related methods on nsIDocument to check if PaymentRequest API is allowed to use. r?olli

Update the patch according to comment 5.

1. Remove the subdocument iteration
2. Change IsAllowPaymentRequest to AllowPaymentRequest

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Eden Chuang[:edenchuang] from comment #7)
> After tracing the code, I found that the subdocument tree transfer issue
> would not happen. 
> So, I will modify the patch according to the comment 5.

Yeah, moving iframes around destroys their internal browsing context.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8921376 [details] [diff] [review]
Bug 1403079 - Add mAllowPaymentRequest and related methods on nsIDocument to check if PaymentRequest API is allowed to use. r?olli

>      2. The document has the same origin with its parent document and its parent
>         document's mAllowPaymentRequest is true.
The document is same origin with...

>      3. The document is has different origin with its parent document but its
drop 'has'

>+      } else {
>+        if (aElement->IsHTMLElement(nsGkAtoms::iframe) &&
>+            aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::allowpaymentrequest) &&
>+            aElement->GetBoolAttr(nsGkAtoms::allowpaymentrequest)) {
You don't need HasAttr call. GetBoolAttr does the right thing
http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/dom/base/Element.h#1342

>-  // If the node has the same origin as the parent node, the feature is allowed-to-use.
>-  // Otherwise, only allow-to-use this feature when the browsing context container is
>-  // an iframe with "allowpaymentrequest" attribute.
>+
>   nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
>   nsINode* node = static_cast<nsINode*>(doc);
Not about this patch, but why is there this cast? nsIDocument is-an nsINode

>   if (!node) {
>     aRv.Throw(NS_ERROR_UNEXPECTED);
>     return nullptr;
>   }
> 
>+  // Check if AllowPaymentRequest on the owner document
>+  if (!node->OwnerDoc()->AllowPaymentRequest()) {
You have document there as doc variable.
So, just if (!doc->AllowPaymentRequest())

Comment 11

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eec2c4b90f7a9c67353ca4625ceecaa8ce22bfa

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: Bug 1403079 - Add mAllowPaymentRequest and related methods on nsIDocument to check if PaymentRequest API is allowed to use. r=olli

Update the patch according to the comment 10.

>>-  // If the node has the same origin as the parent node, the feature is allowed-to-use.
>>-  // Otherwise, only allow-to-use this feature when the browsing context container is
>>-  // an iframe with "allowpaymentrequest" attribute.
>>+
>>   nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
>>   nsINode* node = static_cast<nsINode*>(doc);
> Not about this patch, but why is there this cast? nsIDocument is-an nsINode

This is the code before we implement mAllowPaymentRequest, and remove this redundant code from the new patch. Instead of using a do-while loop, we use GetTopLevelContentDocument() and NodePrincipal() to get the topLevelPrincipal in the new patch.

Comment 13

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aba3f13931cc29d09b97ef11d4f3266d1a9883e

Comment 14

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81ae94a96fa5c5ac100001f0289761e8b5105800

Comment 15

[Eden Chuang[:edenchuang]]

Attachment: Bug 1403079 - Add mAllowPaymentRequest and related methods on nsIDocument to check if PaymentRequest API is allowed to use. r=olli

Update the patch to fix the debug build fail.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98d063283487
Add mAllowPaymentRequest and related methods on nsIDocument to check if PaymentRequest API is allowed to use. r=smaug

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/98d063283487