Closed Bug 1470584 Opened 2 years ago Closed 2 years ago

If the PaymentRequest dialog is closed via the window manager, tell the PaymentUIService

Categories

(Firefox :: WebPayments UI, enhancement, P1)

All
Unspecified
enhancement

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: MattN, Assigned: sfoster)

References

(Depends on 1 open bug)

Details

(Whiteboard: [webpayments])

Attachments

(1 file)

On Windows and Linux it's easy to close the modal PaymentRequest from the window manager (e.g. with an [x] button) but we don't listen for this to tell the PaymentUIService that the dialog is closed. Some DOM code will get in the wrong state (due to trying to enforce one simultaneous PaymentRequest being shown) and break other PaymentRequests from being shown if we don't notify the DOM that the dialog is closed.

It may be possible to trigger this on macOS too but I didn't try off-hand.
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

This is *a* way to accomplish this. Another way would be to watch for "domwindowclosed" from paymentUIService and do essentially the same thing. In that case paymentUIService would need to keep track of active request ids, however it does not get any notification from the onPaymentCancel in paymentDialogWrapper, which made this difficult when I was testing this out.
Attachment #8992503 - Flags: feedback?(MattN+bmo)
Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

Moving to :jaws for feedback as MattN is buried right now.
Attachment #8992503 - Flags: feedback?(MattN+bmo) → feedback?(jaws)
Attachment #8992503 - Flags: feedback?(jaws) → feedback?(MattN+bmo)
Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

https://reviewboard.mozilla.org/r/257348/#review264468

::: browser/components/payments/content/paymentDialogWrapper.js:214
(Diff revision 1)
> +    // watch for unexpected unloading to ensure the PR is cleaned up
> +    window.addEventListener("unload", this);

(In reply to Sam Foster [:sfoster] from comment #2)
> Comment on attachment 8992503 [details]
> Bug 1470584 - (WIP) Send PAYMENT_REJECTED response if window is unexpectedly
> closed
> 
> This is *a* way to accomplish this. Another way would be to watch for
> "domwindowclosed" from paymentUIService and do essentially the same thing.
> In that case paymentUIService would need to keep track of active request
> ids, however it does not get any notification from the onPaymentCancel in
> paymentDialogWrapper, which made this difficult when I was testing this out.

If you were just trying to have this code live in paymentUIService then you can add the same event listener there instead and use requestIdForWindow to know the requestID for the paymentService response.

::: browser/components/payments/content/paymentDialogWrapper.js:503
(Diff revision 1)
> +      this.log.debug("window is unloading");
> +      if (!window.responseSent) {
> +        this.log.debug("unexpected window unload");

This should use an early return rather than adding nesting.

::: browser/components/payments/content/paymentDialogWrapper.js:504
(Diff revision 1)
> +      if (!window.responseSent) {
> +        this.log.debug("unexpected window unload");

This isn't unexpected if the site calls .abort().

::: browser/components/payments/content/paymentDialogWrapper.js:515
(Diff revision 1)
>    onPaymentCancel() {
>      const showResponse = this.createShowResponse({
>        acceptStatus: Ci.nsIPaymentActionResponse.PAYMENT_REJECTED,
>      });
> -    paymentSrv.respondPayment(showResponse);
> +    this.sendPaymentResponse(showResponse);
>      window.close();
>    },

Wouldn't a simpler alternative be to remove this body other than `window.close` then just handle not triggering the event listener when closeWindow is called from the UI service.

I think you could simply remove the event listener in the closeWindow call of paymentUIService. I would also then move the event listener there to make it easier. Let me know if this idea doesn't work.
Attachment #8992503 - Flags: feedback?(MattN+bmo)
Depends on: 1478750
I've filed bug 1478740 for the DOM bug that prevents landing a test to verify closing (rejecting) a payment in one window doesn't interfere with completion of a payment from another window. I've filed a follow-up bug 1478750 to add that test.
Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

https://reviewboard.mozilla.org/r/257348/#review266726

Thanks

::: browser/components/payments/paymentUIService.js:34
(Diff revision 3)
>    });
> +  Services.wm.addListener(this);
>    this.log.debug("constructor");

You may want to do a debug build try push to ensure this doesn't trigger a leak failure. If it doesn't then this is fine but if it does fail then you may need to unregister at shutdown.

::: browser/components/payments/paymentUIService.js:49
(Diff revision 3)
> +    let domWindow = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                      .getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);

I don't think I've ever seen getInterface called with a fallback like this. What's this for? Are you sure you need this? Based on the IDL signature this doesn't seem correct.

::: browser/components/payments/paymentUIService.js:52
(Diff revision 3)
> +    if (requestId && paymentSrv.getPaymentRequestById(requestId)) {
> +      this.log.debug(`onCloseWindow, close of window for active requestId: ${requestId}`);

Nit: Mozilla style is to use early returns instead of nesting where possible.

::: browser/components/payments/paymentUIService.js:90
(Diff revision 3)
> +    responseData.initData({});
> +    rejectResponse.init(requestId,
> +                        Ci.nsIPaymentActionResponse.PAYMENT_REJECTED,
> +                        "", // payment method
> +                        responseData, // payment method data

responseData can simply be `null`

https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/payments/content/paymentDialogWrapper.js#232,235

::: browser/components/payments/paymentUIService.js:98
(Diff revision 3)
> +                        "", // payment method
> +                        responseData, // payment method data
> +                        "", // payer name
> +                        "", // payer email
> +                        "");// payer phone
> +    paymentSrv.respondPayment(rejectResponse.QueryInterface(Ci.nsIPaymentActionResponse));

Hmm… interesting paymentDialogWrapper seems to use nsIPaymentShowActionResponse, which is correct? https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/payments/content/paymentDialogWrapper.js#240
Attachment #8992503 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

https://reviewboard.mozilla.org/r/257348/#review266726

> I don't think I've ever seen getInterface called with a fallback like this. What's this for? Are you sure you need this? Based on the IDL signature this doesn't seem correct.

I'm just cargo-culting this from a similar usage I found elsewhere and had wondered the same. I don't think we need nsIDOMWindowInternal here - will fix

> Hmm… interesting paymentDialogWrapper seems to use nsIPaymentShowActionResponse, which is correct? https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/payments/content/paymentDialogWrapper.js#240

I don't know. I was borrowing from what I found in the DOM tests. nisPaymentShowActionResponse *sounds* more correct but  will investigate.
Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

https://reviewboard.mozilla.org/r/257348/#review266726

> I don't know. I was borrowing from what I found in the DOM tests. nisPaymentShowActionResponse *sounds* more correct but  will investigate.

Looking at the comments in https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/payments/nsIPaymentActionResponse.idl nsIPaymentShowActionResponse should be correct here.
Flags: qe-verify+
QA Contact: hani.yacoub
Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

https://reviewboard.mozilla.org/r/257348/#review266726

> You may want to do a debug build try push to ensure this doesn't trigger a leak failure. If it doesn't then this is fine but if it does fail then you may need to unregister at shutdown.

Try run is looking ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c4a5bd078742035e63f4d9a7a1e61f97dc4ebc8
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f6664786e85
Send PAYMENT_REJECTED response if window is unexpectedly closed. r=MattN
https://hg.mozilla.org/mozilla-central/rev/3f6664786e85
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
I have verified this fix on Nightly v63.0a1 (2018-07-30) on Windows 10 and Ubuntu 16.04. 
The following steps have been performed:
Precondition: Set the following preconditions: dom.payments.request.enabled -> true and dom.payments.loglevel to "Debug".
1. Open https://rsolomakhin.github.io/pr/cc/
2. Click on "Buy".
3. Click on the "x" button from the Web Payment drop-down's bar.
4. Click on the "Buy" button again.

Before fix issue reproduction: 
An error is displayed and Web Payment drop-down is not being shown.

After fix behavior:
The Web Payment drop-down is properly displayed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.