Closed Bug 1403079 Opened 4 years ago Closed 4 years ago

Categories

(Core :: DOM: Web Payments, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: edenchuang, Assigned: edenchuang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → echuang
Status: NEW → ASSIGNED
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
Attachment #8919129 - Flags: review?(amarchesini) → review?(bugs)
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
Attachment #8919129 - Flags: review?(bugs) → review-
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.
Attachment #8919129 - Attachment is obsolete: true
Attachment #8920918 - Flags: review?(bugs)
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?
Attachment #8920918 - Flags: review?(bugs) → review-
(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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Update the patch according to comment 5.

1. Remove the subdocument iteration
2. Change IsAllowPaymentRequest to AllowPaymentRequest
Attachment #8920918 - Attachment is obsolete: true
Attachment #8921376 - Flags: review?(bugs)
(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 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())
Attachment #8921376 - Flags: review?(bugs) → review+
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.
Attachment #8921376 - Attachment is obsolete: true
Attachment #8922220 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/98d063283487
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.