Reject promise if document becomes not fully active

[danyao]

There are multiple places in the spec today that requires any outstanding promises to be rejected if the document becomes not fully active, e.g. show(), retry(), complete().

In Blink, this turns out to be difficult to implement because there is no hook to inform the blink::PaymentRequest object that its associated context is about to be destroy. The only hook is after the context is destroyed. This is too late, as any rejection queue then will not actually be executed. So from web developer's perspective, the promise gets into a limbo state, and never rejects nor resolves (http://crbug.com/941271).

Where this causes a real problem for web developers is when a show() promise is created from a PaymentRequest in one context (e.g. an iframe) and passed to a second context (e.g. a parent frame), and the first context navigates away. If the parent frame relies on the promise to resolve or reject to control its UI, it will hang.

@domenic you suggested the following on the platform-architecture-dev thread regarding this issue:

make resolve/reject a no-op if the promise's relevant Realm's global object is a Window, and that Window's associated Document is not fully-active. And then, I guess, remove all uses of rejected promises to signal failures due to document inactivity, like the one opening this thread.

I'm not sure I fully follow what you mean. Can you clarify how this solves the problem?

In the end, I think there are two options out of this problem:

1. Change Payment Request API spec to not require outstanding promises be rejected when the document becomes not fully active. I'm not sure how to address the parent frame in limbo question.
2. Figure out a way to solve this problem in Blink. Based on a discussion I had with @aestes, WebKit doesn't have this problem because there is a hook just before the context is destroyed. So perhaps Blink can introduce a similar hook.

Any thoughts? @marcoscaceres @domenic @aestes @rsolomakhin @ianbjacobs

[marcoscaceres]

We have a notification system that monitors the document state associated with the payment request: https://searchfox.org/mozilla-central/source/dom/payments/PaymentRequest.cpp#1158

If the document becomes !InFullyActiveDocument(), it rejects the appropriate promises: https://searchfox.org/mozilla-central/source/dom/payments/PaymentRequest.cpp#1164

We also have checks throughout the implementation that call PaymentRequest::InFullyActiveDocument(): https://searchfox.org/mozilla-central/source/dom/payments/PaymentRequest.cpp#1117

[rsolomakhin]

@marcoscaceres : Do you mind if we just copy-paste your code? k, thx, bye! :1st_place_medal:

[marcoscaceres]

Heh, open source for a reason:)

[danyao]

Thanks @marcoscaceres for the pointers!

I'm getting some push back from Blink architecture owners for adding a new hook without a real world use case. After thinking about this a bit more, it seems that the only scenario where the spec'ed behavior is critical is if a reference to the promise is retained in a different context after the document associated with the promise becomes inactive.

Concretely, I can only think of one scenario where this would be the case:

1. Top-level frame embeds a same-origin iframe that creates a PaymentRequest object, e.g. request
2. request.show() is called inside the iframe and the returned Promise (e.g. acceptPromise) is passed back to the top-level frame, e.g. to update UI after the promise resolves.
3. iframe navigates so the document associated with acceptPromise is no longer a fully active document
4. Top-level frame code hangs because acceptPromise stays unsettled.

If the iframe was not same-origin as the top-level frame, it wouldn't have been able to pass acceptPromise in step 2, which is needed to get ourselves into a scenario where a reference to acceptPromise exists but its associated document is no longer active.

I suspect almost all of the PaymentRequests made from iframes are from cross-origin iframes. I'm going to add some telemetry to verify this.

If the same-origin use case turns out to be non-existent, then it seems to me that we don't have a real world use case that would depend on the current spec behavior. Then can we remove the spec that the promise must be rejected if the document becomes inactive after the promise is created?

[marcoscaceres]

If the same-origin use case turns out to be non-existent, then it seems to me that we don't have a real world use case that would depend on the current spec behavior.

In Firefox at least, this causes the payment sheet closes if the iframe or top-level browsing context that created the request navigates. That’s obviously quite important for us, otherwise the sheet would continue to show if the underlying document is navigated to a new origin.

Chromium clearly uses a different mechanism for detecting and dealing with this, which is ok I would say. You are correct that from third-party iframes this won’t be observable via script.

Without changing the tests, we could make the argument to the W3C Director that this is interoperable, because at least the payment sheet closes when the creator context navigates (even if promises are left in a pending state, which is not very clean but 🤷‍♂️ ).

If it’s not too painful to get some telemetry data, then great. But I agree with you: I very much doubt there will be many same-origin iframes using the API this way, and then randomly navigating away... so no stress if you don’t want actually gather usage stats.

@domenic, do you have any concerns wrt this?

[domenic]

My concern is that basically I messed up pushing for us to reject the promise when the document became inactive. I am sorry 😢. See https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/BszBZqcPcuk/42KLPiHCDAAJ for a bit more background.

[danyao]

If the same-origin use case turns out to be non-existent, then it seems to me that we don't have a real world use case that would depend on the current spec behavior.

In Firefox at least, this causes the payment sheet closes if the iframe or top-level browsing context that created the request navigates. That’s obviously quite important for us, otherwise the sheet would continue to show if the underlying document is navigated to a new origin.

I'm thinking we can just remove step 3 from the "If document stops being fully active..." section, leaving the following:

If document stops being fully active while the user interface is being shown, or no longer is by the time this step is reached, then: 

1. Close down the user interface.
2. Set request's payment-relevant browsing context's payment request is showing boolean to false.

This way Firefox can still close the payment sheet. In fact, Chromium needs this as well, the only thing we can't do is reject the promise because the event loop is already stopped.

Without changing the tests, we could make the argument to the W3C Director that this is interoperable, because at least the payment sheet closes when the creator context navigates (even if promises are left in a pending state, which is not very clean but ).

What about changing the test such that instead of waiting for the promise to reject, the test will create another payment request in the main frame? We can use whether the main frame request succeeds in calling show() to check if the payment sheet triggered from the iframe is dismissed as expected.

@domenic, do you have any concerns wrt this?

Copying over what @domenic said on the chromium-dev thread:

My message was based on the premise that the spec as-is is broken. It's asking to do something that's basically impossible (*), because you can't reject a promise in an inactive document (since the event loop, which is responsible for promise rejections, does not run).

So my suggestion was that specs should: (1) stop doing impossible things purposefully, i.e. remove the reject-after-inactive requirements; (2) also get a blanket update so that even if they try to do impossible things accidentally, nothing bad happens.

To expand on (2): you can imagine that there are tons of conditions throughout specs like "If the fetch fails, then reject the promise". To be more correct, we would have to edit every spec to say "If the fetch fails, and the promise's global object's associated Document is active, then reject the promise." This is burdensome to do, and even more burdensome to remember to do, for all future spec updates. Instead we should just make it automatic that "If the fetch fails, then reject the promise" implicitly does nothing when it doesn't work.

(*) 'basically' impossible: I am still pretty sure a document could become active again after transitioning out of bfcache...

I think this is consistent with my suggestion above.