Closed
Bug 1442453
Opened 7 years ago
Closed 6 years ago
A TypeError is thrown and the payment service can get in a broken state if two requests are used in one document
Categories
(Core :: DOM: Web Payments, defect, P1)
Core
DOM: Web Payments
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: MattN, Assigned: mrbkap)
References
(Blocks 2 open bugs)
Details
(Keywords: testcase, Whiteboard: [webpayments])
Attachments
(6 files)
1.82 KB,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
There are issues related to two PaymentRequests being used in the same document even if only one is shown. Internally we get an NS_ERROR_FAILURE inside PaymentRequestManager::GetPaymentChild[1] after a comment saying: > // Only one payment request can interact with user at the same time. but IIUC, the code isn't actually checking if the existing request is interacting with the user. In the case of my test case, one is interacting with the user (via .show) and the other is just a .canMakePayment() call. STR: In the attached test case: 1. Click Buy 2. Click Cancel in the dialog Expected result: > INFO: Can make payment > INFO: Can make payment > ERROR: AbortError: The operation was aborted. Actual result: > ERROR: TypeError: The expression cannot be converted… > INFO: Can make payment The dialog UI is now in a deadlock There are a few issues: A) It's confusing that we assume all internal errors in this path should be exposed as a TypeError. This adds confusion about whether this is an internal error or we are implementing some spec algorithm. I would suggest we use a different error or exception.[2] B) AFAICT, we shouldn't be throwing an error in this case since only one request is interacting with the user. Even when we want to throw for two requests needing to show in the same tab we should use an AbortError to match what the `show` algorithm defines for showing multiple requests. ** Note that we want to eventually intentionally deviate from the spec and allow multiple requests dialogs at the same time in different tabs. C) The user is not able to abort the shown request dialog because of an NS_ERROR_FAILURE from nsIPaymentRequestService.respondPayment Side note: The test case uses a canMakePayment while a request is showing which is a bit unusual but I don't know of a specific reason to forbid this currently. Marking as P1 as this breaks end-to-end testing with various demo sites such as https://rsolomakhin.github.io/pr/ko/fail/ Chrome handles this as expected. [1] https://dxr.mozilla.org/mozilla-central/rev/b996cabc7ef54bbe050d647494bf00d668ec52e6/dom/payments/PaymentRequestManager.cpp#270 [2] https://dxr.mozilla.org/mozilla-central/rev/b996cabc7ef54bbe050d647494bf00d668ec52e6/dom/payments/PaymentRequest.cpp#594-595 [3] https://w3c.github.io/payment-request/#show-method
Flags: in-testsuite?
Updated•7 years ago
|
Priority: P1 → P2
Whiteboard: [webpayments]
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee | ||
Comment 2•6 years ago
|
||
I think I know what's going on here and will try to have a patch tomorrow.
Assignee | ||
Comment 3•6 years ago
|
||
There are at least two different bugs found by the testcase here (also, if I try the testcase in one tab, open it again in another tab in a different process, and run it again, that tab crashes). The first is that we refuse to create a PaymentRequest if another payment is communicating with the parent process. In this case, the first payment is waiting for `show()` to return, but I don't see any reason that `canMakePayment()` wouldn't trigger the same problem. The second is that the `canMakePayment()` and `show()` calls race with each other. For each request from the child, we try to create a PaymentRequestChild to communicate with the parent. We hold onto each PaymentRequestChild until we get its response from the parent (except for the CreateRequest message, which doesn't expect one). However, the map that allows us to get back to the proper PaymentRequestChild from the IPC message from the parent is only keyed by the PaymentRequest. In order to protect against having to have multiple entries in the hashtable, we'll occasionally reuse a PaymentRequestChild, though none of the code expects that, meaning the second message (which reuses the object) will be unable to process the response. This is ultimately the cause of the testcase in this bug not working. I'm leery of the code as written. IPDL, for all of its faults, excels at creating reusable channels for communications between processes. It feels the code goes out of its way to not use that feature. In addition, creating a PaymentRequest, calling `canMakePayment()` and doing nothing else will cause at least 6 IPC messages. Also, because we only ever hold onto strings, we're constantly hitting hashtables to find objects (with some of them backwards, causing us to have to linearly search the hashtable to get to the object we want). IPDL already maintains most of this info for us, so I don't see the need to not use it. I have a somewhat large patch that I've pushed to try at [1] that fixes the testcase here, establishes a 1:1 relationship between PaymentRequest and PaymentRequestChild objects, and makes the code in the child use objects instead of strings. I like the result. Baku, are you opposed to this? This could also be fixed in other ways but this seems much more clear to me than the existing code. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b6f36df25671011023140cad7ac452a18a36a1
Flags: needinfo?(amarchesini)
Comment 4•6 years ago
|
||
> Baku, are you opposed to this? This could also be fixed in other ways but
> this seems much more clear to me than the existing code.
Absolutely not. If you patch is ready, or if you want somebody to finish it, let me know.
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
I've done some additional cleanup as part of this patch, but there's more simplification to be had here. In particular, we can de-COM-ify a bunch of code, avoiding going through do_CreateInstance in favor of simple 'new' and avoiding unnecessary refcounting. I'll file a followup to look into that after this bug.
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8983192 [details] Bug 1442453 - Don't enforce "one interactive request" in the wrong place. https://reviewboard.mozilla.org/r/249044/#review257762 Do we have tests to covert this change?
Attachment #8983192 -
Flags: review?(amarchesini) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8983193 [details] Bug 1442453 - Fix a typo in this member name. https://reviewboard.mozilla.org/r/249046/#review257764
Attachment #8983193 -
Flags: review?(amarchesini) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8983194 [details] Bug 1442453 - Fix nits and get rid of needless QIs. https://reviewboard.mozilla.org/r/249048/#review257768
Attachment #8983194 -
Flags: review?(amarchesini) → review+
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8983196 [details] Bug 1442453 - Pass objects around instead of string IDs in the child. https://reviewboard.mozilla.org/r/249052/#review257770
Attachment #8983196 -
Flags: review?(amarchesini) → review+
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8983195 [details] Bug 1442453 - Create a single IPC actor per PaymentRequest. https://reviewboard.mozilla.org/r/249050/#review257772 ::: dom/payments/PaymentRequestManager.cpp:316 (Diff revision 1) > +PaymentRequestManager::NotifyRequestDone(PaymentRequest* aRequest) > +{ > + auto entry = mActivePayments.Lookup(aRequest); > + MOZ_ASSERT(entry); > + > + int32_t count = --entry.Data(); uint32_t plus, add a MOZ_ASSERT(count > 0)
Attachment #8983195 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #11) > Comment on attachment 8983192 [details] > Bug 1442453 - Don't enforce "one interactive request" in the wrong place. > Do we have tests to covert this change? We don't! I was surprised that this passed all of the existing tests when I wrote this. Part of the reason for the lack of tests is that we don't actually know what the correct behavior is going to be here (the spec, as I understand it, is in a bit of limbo at this point).
Assignee | ||
Comment 17•6 years ago
|
||
I filed bug 1469419 on comment 16.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cea1719a866e Don't enforce "one interactive request" in the wrong place. r=baku https://hg.mozilla.org/integration/autoland/rev/a509c5193aaa Fix a typo in this member name. r=baku https://hg.mozilla.org/integration/autoland/rev/4be750b97241 Fix nits and get rid of needless QIs. r=baku https://hg.mozilla.org/integration/autoland/rev/057eef48e70b Create a single IPC actor per PaymentRequest. r=baku https://hg.mozilla.org/integration/autoland/rev/df705bf3c62f Pass objects around instead of string IDs in the child. r=baku
Assignee | ||
Comment 24•6 years ago
|
||
I noticed a couple of assertions and had to re-introduce a couple of QIs.
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cea1719a866e https://hg.mozilla.org/mozilla-central/rev/a509c5193aaa https://hg.mozilla.org/mozilla-central/rev/4be750b97241 https://hg.mozilla.org/mozilla-central/rev/057eef48e70b https://hg.mozilla.org/mozilla-central/rev/df705bf3c62f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
Flags: qe-verify+
Comment 26•6 years ago
|
||
Have checked with Firefox 62.0b8, 63.0a1 (2018-07-15) on win 10x64,and 62.0b8 on Ubuntu 16.04LTS. Getting the following results: ERROR: ReferenceError: PaymentRequest is not defined ERROR: TypeError: request is undefined ERROR: ReferenceError: PaymentRequest is not defined
Flags: needinfo?(mrbkap)
Reporter | ||
Comment 27•6 years ago
|
||
You probably didn't enable the feature with: dom.payments.request.enabled=true
Flags: needinfo?(mrbkap) → needinfo?(cristian.fogel)
Comment 28•6 years ago
|
||
That helped out with Nightly. However, for 62.0b8 the behavior is still the same even with the pref set on true.
Flags: needinfo?(cristian.fogel)
Assignee | ||
Comment 29•6 years ago
|
||
(In reply to Cristi Fogel [:cfogel] from comment #28) > That helped out with Nightly. > However, for 62.0b8 the behavior is still the same even with the pref set on > true. This feature is disabled on beta (the pref won't have any effect there).
Flags: needinfo?(cristian.fogel)
Comment 30•6 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #29) > This feature is disabled on beta (the pref won't have any effect there). Thank you for the reply. Based on your previous comment, clearing the qe flag since the feature is disabled on beta.
Flags: qe-verify+
Flags: needinfo?(cristian.fogel)
You need to log in
before you can comment on or make changes to this bug.
Description
•