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

Status: VERIFIED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

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.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

Review commit: https://reviewboard.mozilla.org/r/257348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257348/

Comment 2

[Sam Foster [:sfoster] (he/him)]

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.

Comment 3

[Sam Foster [:sfoster] (he/him)]

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.

Comment 4

[Matthew N. [:MattN]]

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.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257348/diff/1-2/

Comment 6

[Sam Foster [:sfoster] (he/him)]

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 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257348/diff/2-3/

Comment 8

[Matthew N. [:MattN]]

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

Comment 9

[Sam Foster [:sfoster] (he/him)]

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 10

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8992503 [details]
Bug 1470584 - Send PAYMENT_REJECTED response if window is unexpectedly closed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257348/diff/3-4/

Comment 11

[Sam Foster [:sfoster] (he/him)]

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.

Comment 12

[Sam Foster [:sfoster] (he/him)]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f6664786e85
Send PAYMENT_REJECTED response if window is unexpectedly closed. r=MattN

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3f6664786e85

Comment 15

[Daniel Bodea [:danibodea]]

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.