Re: [chromium-dev] How to reject a promise when document becomes inactive?

54 views
Skip to first unread message

Jeremy Roman

unread,
Jun 18, 2019, 9:10:43 PM6/18/19
to dan...@chromium.org, platform-architecture-dev, Chromium-dev

On Tue, Jun 18, 2019 at 7:23 PM Danyao Wang <dan...@chromium.org> wrote:
Hi chromium-dev,

I have a web API that returns a promise, and I'm trying to make it match the current spec: reject the promise if document stops being fully active while the promise is outstanding [1].

blink::PaymentRequest is the object that implements the API.
blink::PaymentRequest::show() is the method that returns a promise. It creates a ScriptPromiseResolver and saves it in a Member<ScriptPromiseResolver>.

To implement the spec behavior, I want to call accept_resolver_->Reject(...) in blink::PaymentRequest::ContextDestroyed().

However, the problem is ScriptPromiseResolver::Reject() does nothing if the context is not valid. This leaves the promise in a pending state.

It seems that what I need may be a new ContextWillBeDestroyed() method on ContextLifecycleObserver?
Is there a better alternative?
Or is this spec behavior unusual? Should I try to change the spec?

Thanks,
Danyao

[1] See bottom of show() algorithm: https://w3c.github.io/payment-request/#show-method

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAErabb_WeE5pHEQqYhQwzLM0uYSD%3DUaR0Xz1B%2BddhoSQFO8Bvw%40mail.gmail.com.

Daniel Cheng

unread,
Jun 18, 2019, 9:38:24 PM6/18/19
to Jeremy Roman, Danyao Wang, platform-architecture-dev, Chromium-dev, Domenic Denicola
There was previously an internal discussion about the same topic (sorry it wasn't on a mailing list!), because there are other things that are specced to do the same (reject at detach): https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decode

I would honestly prefer to just not reject the promise in this case at all (since it feels like another unload event and could complicate things like bfcache), but it might be helpful to understand the use cases where this reject behavior is useful.

Daniel

You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CACuR13db6pHsZ2Summ_CaBExL%3D%3DnjjpbmXNREDUzgWDCtz0q5A%40mail.gmail.com.

Kouhei Ueno

unread,
Jun 18, 2019, 9:43:26 PM6/18/19
to Daniel Cheng, Jeremy Roman, Danyao Wang, platform-architecture-dev, Chromium-dev, Domenic Denicola
We just had a discussion on a crbug. We ended up using raw V8 API to return a rejected promise.



--
kouhei

Kentaro Hara

unread,
Jun 19, 2019, 4:34:16 AM6/19/19
to Kouhei Ueno, Daniel Cheng, Jeremy Roman, Danyao Wang, platform-architecture-dev, Chromium-dev, Domenic Denicola
I would honestly prefer to just not reject the promise in this case at all (since it feels like another unload event and could complicate things like bfcache), but it might be helpful to understand the use cases where this reject behavior is useful.

+1 to not rejecting a promise on a document detach.

Rejecting a promise on a document detach will end up with executing an arbitrary script after the document is detached. At that point JavaScript can still run but many things are already destructed and most DOM operations are disabled (e.g., throw errors). Also the behavior after the document detach is not speced well and different user agents are doing different things :/

We might want to get inputs from Domenic :)


Daniel Cheng

unread,
Jun 19, 2019, 4:37:38 AM6/19/19
to Kentaro Hara, Kouhei Ueno, Jeremy Roman, Danyao Wang, platform-architecture-dev, Chromium-dev, Domenic Denicola
Actually I was just thinking about this more.

Let's say img.decode() creates a Promise in context1, and then passes this Promise to the same-origin context2.

If context1 is detached, it seems like it'd make sense to reject the previous Promise--but wouldn't it be associated with context1, so rejection would fail?

Danyao Wang

unread,
Jun 19, 2019, 10:07:14 AM6/19/19
to Daniel Cheng, Kentaro Hara, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev, Domenic Denicola
Thanks everyone for the input. I'll find out if there are real-world use cases that motivated the payment spec being written this way.

Maybe a naive question: why don't we reject a promise immediately after its associated context is invalid? What is a use case of a promise that is passed to a different context and can still resolve?

Kentaro Hara

unread,
Jun 19, 2019, 10:25:45 AM6/19/19
to Danyao Wang, Daniel Cheng, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev, Domenic Denicola
Maybe a naive question: why don't we reject a promise immediately after its associated context is invalid?

Promises need to be resolved / rejected in microtasks, which are invoked at the end of the event loop. It's too late to reject the promises at that point because the associated context is already shut down. In other words, there's no way to reject promises after its associated context becomes invalid but before the associated context is shut down.
 
What is a use case of a promise that is passed to a different context and can still resolve?

I personally hope that there's no use case. I think that a promise should stop working (i.e., neither resolved or rejected) after its associated context becomes invalid. That is the current implementation of ScriptPromiseResolver::ResolveOrReject.

But Domenic is an expert in this area :)

Domenic Denicola

unread,
Jun 19, 2019, 10:33:51 PM6/19/19
to Kentaro Hara, Danyao Wang, Daniel Cheng, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev, Domenic Denicola
On Wed, Jun 19, 2019 at 11:25 PM Kentaro Hara <har...@chromium.org> wrote:
Maybe a naive question: why don't we reject a promise immediately after its associated context is invalid?

Promises need to be resolved / rejected in microtasks, which are invoked at the end of the event loop. It's too late to reject the promises at that point because the associated context is already shut down. In other words, there's no way to reject promises after its associated context becomes invalid but before the associated context is shut down.
 
What is a use case of a promise that is passed to a different context and can still resolve?

I personally hope that there's no use case. I think that a promise should stop working (i.e., neither resolved or rejected) after its associated context becomes invalid. That is the current implementation of ScriptPromiseResolver::ResolveOrReject.

But Domenic is an expert in this area :)

I think the specs currently are a bit broken. Some of them, like PaymentRequest, were written with a nice behavior of using rejection to signal a failure due to inactivity. But we didn't realize that this would just do nothing, because the event loop on inactive documents is suspended. The result is then that, for documents which become inactive, the promises will stay unsettled, but then if the document becomes active again, the promise will end up rejected. This is weird.

There is a related problem with fetches; there is nothing that really states in the specs that fetches should be aborted when the document becomes inactive, or that you shouldn't be able to start new fetches from inactive documents. (E.g. via `inactiveIframe.contentWindow.eval("fetch('...')")`.)

I haven't thought about a solution to this mess very hard so far, but maybe we should, on a spec level, 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. This feels quite ad-hoc, but I don't really have any better ideas. Thoughts welcome.

Dave Tapuska

unread,
Jun 19, 2019, 10:43:47 PM6/19/19
to Domenic Denicola, Kentaro Hara, Danyao Wang, Daniel Cheng, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
What are we referring to as "inactive" here?

You can't move from a destroyed context to a new context without a reload of the document. So Domenic your reference from inactive to active seems odd to me. 

Dave

Domenic Denicola

unread,
Jun 19, 2019, 10:51:47 PM6/19/19
to Dave Tapuska, Domenic Denicola, Kentaro Hara, Danyao Wang, Daniel Cheng, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
On Thu, Jun 20, 2019 at 11:43 AM Dave Tapuska <dtap...@chromium.org> wrote:
What are we referring to as "inactive" here?


You can't move from a destroyed context to a new context without a reload of the document. So Domenic your reference from inactive to active seems odd to me. 

In the spec, this is not true, I believe mostly (entirely?) because of bfcache.

Domenic Denicola

unread,
Jun 19, 2019, 10:53:20 PM6/19/19
to Domenic Denicola, Dave Tapuska, Kentaro Hara, Danyao Wang, Daniel Cheng, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
On Thu, Jun 20, 2019 at 11:51 AM Domenic Denicola <dom...@chromium.org> wrote:


On Thu, Jun 20, 2019 at 11:43 AM Dave Tapuska <dtap...@chromium.org> wrote:
What are we referring to as "inactive" here?


You can't move from a destroyed context to a new context without a reload of the document. So Domenic your reference from inactive to active seems odd to me. 

In the spec, this is not true, I believe mostly (entirely?) because of bfcache.

Oh, and also JavaScript can just hold on to the Document (and/or Window) object even after its parent WindowProxy has been navigated. So those Documents still exist and are not reloaded, e.g. you can inspect their DOM trees. But they are inactive.

Ben Kelly

unread,
Jun 19, 2019, 10:55:50 PM6/19/19
to Domenic Denicola, Dave Tapuska, Kentaro Hara, Danyao Wang, Daniel Cheng, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
On Wed, Jun 19, 2019, 10:51 PM Domenic Denicola <dom...@chromium.org> wrote:
On Thu, Jun 20, 2019 at 11:43 AM Dave Tapuska <dtap...@chromium.org> wrote:
You can't move from a destroyed context to a new context without a reload of the document. So Domenic your reference from inactive to active seems odd to me. 

In the spec, this is not true, I believe mostly (entirely?) because of bfcache.

Maybe also if you remove an iframe from the DOM (making it inactive) and then add it back to the DOM (making it active) again?

Daniel Cheng

unread,
Jun 20, 2019, 12:07:16 AM6/20/19
to Ben Kelly, Domenic Denicola, Dave Tapuska, Kentaro Hara, Danyao Wang, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
On Wed, Jun 19, 2019 at 7:55 PM Ben Kelly <wande...@chromium.org> wrote:
On Wed, Jun 19, 2019, 10:51 PM Domenic Denicola <dom...@chromium.org> wrote:
On Thu, Jun 20, 2019 at 11:43 AM Dave Tapuska <dtap...@chromium.org> wrote:
You can't move from a destroyed context to a new context without a reload of the document. So Domenic your reference from inactive to active seems odd to me. 

In the spec, this is not true, I believe mostly (entirely?) because of bfcache.

Maybe also if you remove an iframe from the DOM (making it inactive) and then add it back to the DOM (making it active) again?

When an iframe is removed from the DOM, the document becomes inactive. When it's reattached to the DOM, we perform a new load and create an entirely separate, new Document, from the loaded content.

Daniel

Dave Tapuska

unread,
Jun 20, 2019, 9:15:50 AM6/20/19
to Daniel Cheng, Ben Kelly, Domenic Denicola, Kentaro Hara, Danyao Wang, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
For bfcache you transition into "frozen" state but the document is still "fully active".  Yes I agree that you could grab a reference to a document and then navigate the frame and then that old reference to the document would be in the kStopped state.

I was just curious what we were referring to as inactive. Because blink's model is kInactive -> [Running states, there are a bunch of them] -> kStopped.

Is the payment request spec requiring a transition when the page becomes "frozen" or is it when it is actually "detached" ? It isn't clear to me.

dave.

Kentaro Hara

unread,
Jun 20, 2019, 8:52:36 PM6/20/19
to Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Danyao Wang, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
As Dave pointed out, inactive => active does not happen in BFcache. IIUC the transition should not happen anywhere.

I haven't thought about a solution to this mess very hard so far, but maybe we should, on a spec level, 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. This feels quite ad-hoc, but I don't really have any better ideas. Thoughts welcome.

+1 to this proposal :)

Danyao Wang

unread,
Jul 15, 2019, 5:29:34 PM7/15/19
to Kentaro Hara, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
maybe we should, on a spec level, 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.

+1 to this proposal :)

What does "make resolve/reject a no-op" mean? I'm not following how it solves the unsettled promise problem in the original thread...

Danyao Wang

unread,
Jul 16, 2019, 11:58:51 AM7/16/19
to Kentaro Hara, Dave Tapuska, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
Chewing on Domenic's suggestion a bit more, I don't think "make resolve/reject a no-op" when the promise's associated document is not fully-active will solve the payment payment use case. Here's a concrete scenario:
  • Top-level frame embeds an iframe that creates a PaymentRequest object, e.g. |request|
  • 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.
  • iframe navigates so the document associated with |acceptPromise| is no longer a fully active document
  • Top-level frame code hangs because |acceptPromise| stays unsettled
In this case, we actually need |acceptPromise| to be rejected for the top-level frame to not hang.

I noticed that WebKit has an ActiveDOMObject concept that offers a stop() hook that is called in Document::prepareForDestruction() just before the event queue is stopped. This allows the PaymentRequest, which is an ActiveDOMObject, to clean up in time.

Can we do something similar? Just as a strawman:
  • Introduce a blink::ActiveDOMObject class
  • Store a list of blink::ActiveDOMObjects in blink::ExecutionContext
  • Call stop() on each execution context in Document::Shutdown() in the DocumentLifecycle::kStopping phase
  • PaymentRequest will inherit from blink::ActiveDOMObject and settle promises in stop()
I'm new to Blink so this may be a terrible idea. I'm eager to hear suggestions on how we can solve this problem better. Thanks in advance!

Is the payment request spec requiring a transition when the page becomes "frozen" or is it when it is actually "detached" ? It isn't clear to me.

The payment request spec doesn't currently differentiates between the two (and I don't think it should). The desired behavior is if there's any outstanding promise when the documented associated with the promise is no longer the active document, then we should consider the payment request a failure and reject. Here's the specific wording:

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: 

 tests: 1

  1. Close down the user interface.
  2. Set request's payment-relevant browsing context's payment request is showing boolean to false.
  3. Reject acceptPromise with an "AbortErrorDOMException.

Dave Tapuska

unread,
Jul 16, 2019, 12:12:19 PM7/16/19
to Danyao Wang, Kentaro Hara, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
Just thinking out loud....

So you are trying to chain a promise from one execution context to another. It seems that we'd probably have some type of wrapper in blink that when you bind two a promise together with another promise from another execution context it would reject on the other execution context being destroyed.

Does this really happen in practice? ie; the payment request origin is same origin with the top level frame?  Or that payment requests don't happen in sandboxed iframes?

dave.

Danyao Wang

unread,
Jul 16, 2019, 2:31:22 PM7/16/19
to Dave Tapuska, Kentaro Hara, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, Jeremy Roman, platform-architecture-dev, Chromium-dev
On Tue, Jul 16, 2019 at 12:12 PM Dave Tapuska <dtap...@chromium.org> wrote:
Just thinking out loud....

So you are trying to chain a promise from one execution context to another. It seems that we'd probably have some type of wrapper in blink that when you bind two a promise together with another promise from another execution context it would reject on the other execution context being destroyed. 

Does this really happen in practice? ie; the payment request origin is same origin with the top level frame? 

Hmm I see your point. Promises can only be passed between same-origin frames, so the use case I described doesn't apply to payment request made in cross-origin iframes.

I believe that it's probably uncommon, but I'm not confident that it's non-existent. I'll add a UMA counter to check. If this number is small, perhaps it's sufficient reason to not spec the behavior when document becomes inactive at all.
 
Or that payment requests don't happen in sandboxed iframes?

No. Payment requests are only allowed in an iframe if it's explicitly allowed via the iframe's Feature Policy.

Jeremy Roman

unread,
Jul 16, 2019, 2:49:54 PM7/16/19
to Danyao Wang, Kentaro Hara, Dave Tapuska, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, platform-architecture-dev, Chromium-dev
On Tue, Jul 16, 2019 at 11:58 AM Danyao Wang <dan...@chromium.org> wrote:
Chewing on Domenic's suggestion a bit more, I don't think "make resolve/reject a no-op" when the promise's associated document is not fully-active will solve the payment payment use case. Here's a concrete scenario:
  • Top-level frame embeds an iframe that creates a PaymentRequest object, e.g. |request|
  • 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.
  • iframe navigates so the document associated with |acceptPromise| is no longer a fully active document
  • Top-level frame code hangs because |acceptPromise| stays unsettled
In this case, we actually need |acceptPromise| to be rejected for the top-level frame to not hang.

I noticed that WebKit has an ActiveDOMObject concept that offers a stop() hook that is called in Document::prepareForDestruction() just before the event queue is stopped. This allows the PaymentRequest, which is an ActiveDOMObject, to clean up in time.

Can we do something similar? Just as a strawman:
  • Introduce a blink::ActiveDOMObject class
  • Store a list of blink::ActiveDOMObjects in blink::ExecutionContext
  • Call stop() on each execution context in Document::Shutdown() in the DocumentLifecycle::kStopping phase
  • PaymentRequest will inherit from blink::ActiveDOMObject and settle promises in stop()
This is what ContextLifecycleObserver does. Implementing ContextLifecycleObserver and ActiveScriptWrappable together is a common pattern. There's a comment in context_lifecycle_observer.h that describes this in more detail.

(That we have a similar concept is no coincidence: we inherited ActiveDOMObject from WebKit when we forked; we just like renaming/refactoring things.)

Danyao Wang

unread,
Jul 16, 2019, 3:41:18 PM7/16/19
to Jeremy Roman, Kentaro Hara, Dave Tapuska, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, platform-architecture-dev, Chromium-dev
On Tue, Jul 16, 2019 at 2:49 PM Jeremy Roman <jbr...@chromium.org> wrote:
On Tue, Jul 16, 2019 at 11:58 AM Danyao Wang <dan...@chromium.org> wrote:
Chewing on Domenic's suggestion a bit more, I don't think "make resolve/reject a no-op" when the promise's associated document is not fully-active will solve the payment payment use case. Here's a concrete scenario:
  • Top-level frame embeds an iframe that creates a PaymentRequest object, e.g. |request|
  • 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.
  • iframe navigates so the document associated with |acceptPromise| is no longer a fully active document
  • Top-level frame code hangs because |acceptPromise| stays unsettled
In this case, we actually need |acceptPromise| to be rejected for the top-level frame to not hang.

I noticed that WebKit has an ActiveDOMObject concept that offers a stop() hook that is called in Document::prepareForDestruction() just before the event queue is stopped. This allows the PaymentRequest, which is an ActiveDOMObject, to clean up in time.

Can we do something similar? Just as a strawman:
  • Introduce a blink::ActiveDOMObject class
  • Store a list of blink::ActiveDOMObjects in blink::ExecutionContext
  • Call stop() on each execution context in Document::Shutdown() in the DocumentLifecycle::kStopping phase
  • PaymentRequest will inherit from blink::ActiveDOMObject and settle promises in stop()
This is what ContextLifecycleObserver does. Implementing ContextLifecycleObserver and ActiveScriptWrappable together is a common pattern. There's a comment in context_lifecycle_observer.h that describes this in more detail.

(That we have a similar concept is no coincidence: we inherited ActiveDOMObject from WebKit when we forked; we just like renaming/refactoring things.)

Got it. There is still one thing missing: ContextLifecycleObserver doesn't provide a "context-will-be-destroyed" callback a la WebKit's ActiveDOMObject::stop() method. How about this?
  • Add ExecutionContext::NotifyContextWillBeDestroyed() to call a new pure virtual ContextLifecycleObserver::ContextWillBeDestroyed() method
  • PaymentRequest implements ContextLifecycleObserver::ContextWillBeDestroyed() to settle the promises
  • Call ExecutionContext::NotifyContextWillBeDestroyed() in Document::Shutdown() in the DocumentLifecycle::kStopping phase to allow observers to clean up.

Jeremy Roman

unread,
Jul 16, 2019, 3:53:12 PM7/16/19
to Danyao Wang, Kentaro Hara, Dave Tapuska, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, platform-architecture-dev, Chromium-dev
On Tue, Jul 16, 2019 at 3:41 PM Danyao Wang <dan...@chromium.org> wrote:
On Tue, Jul 16, 2019 at 2:49 PM Jeremy Roman <jbr...@chromium.org> wrote:
On Tue, Jul 16, 2019 at 11:58 AM Danyao Wang <dan...@chromium.org> wrote:
Chewing on Domenic's suggestion a bit more, I don't think "make resolve/reject a no-op" when the promise's associated document is not fully-active will solve the payment payment use case. Here's a concrete scenario:
  • Top-level frame embeds an iframe that creates a PaymentRequest object, e.g. |request|
  • 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.
  • iframe navigates so the document associated with |acceptPromise| is no longer a fully active document
  • Top-level frame code hangs because |acceptPromise| stays unsettled
In this case, we actually need |acceptPromise| to be rejected for the top-level frame to not hang.

I noticed that WebKit has an ActiveDOMObject concept that offers a stop() hook that is called in Document::prepareForDestruction() just before the event queue is stopped. This allows the PaymentRequest, which is an ActiveDOMObject, to clean up in time.

Can we do something similar? Just as a strawman:
  • Introduce a blink::ActiveDOMObject class
  • Store a list of blink::ActiveDOMObjects in blink::ExecutionContext
  • Call stop() on each execution context in Document::Shutdown() in the DocumentLifecycle::kStopping phase
  • PaymentRequest will inherit from blink::ActiveDOMObject and settle promises in stop()
This is what ContextLifecycleObserver does. Implementing ContextLifecycleObserver and ActiveScriptWrappable together is a common pattern. There's a comment in context_lifecycle_observer.h that describes this in more detail.

(That we have a similar concept is no coincidence: we inherited ActiveDOMObject from WebKit when we forked; we just like renaming/refactoring things.)

Got it. There is still one thing missing: ContextLifecycleObserver doesn't provide a "context-will-be-destroyed" callback a la WebKit's ActiveDOMObject::stop() method. How about this?
  • Add ExecutionContext::NotifyContextWillBeDestroyed() to call a new pure virtual ContextLifecycleObserver::ContextWillBeDestroyed() method
  • PaymentRequest implements ContextLifecycleObserver::ContextWillBeDestroyed() to settle the promises
  • Call ExecutionContext::NotifyContextWillBeDestroyed() in Document::Shutdown() in the DocumentLifecycle::kStopping phase to allow observers to clean up.
It's not clear to me that this would solve anything (the promises' handlers still wouldn't run until the next microtask checkpoint).

As Dave brought to my attention, this is an instance of the broader question of whether promises returned by HTML/WebIDL operations should work after the context that produced them has gone away.

I think the bindings team has historically opposed allowing WebIDL interfaces to continue to operate under those conditions (e.g., I think functions that take success/error callbacks or event handlers don't call them on shutdown), so maybe leaving it unresolved is the right choice here. Regardless it probably is something that ought to be done consistently for all promise-returning operations (though I would be unsurprised to learn that it's inconsistent across implementations).

Danyao Wang

unread,
Jul 16, 2019, 4:24:43 PM7/16/19
to Jeremy Roman, Kentaro Hara, Kentaro Hara, Dave Tapuska, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, platform-architecture-dev, Chromium-dev
On Tue, Jul 16, 2019 at 3:53 PM Jeremy Roman <jbr...@chromium.org> wrote:
On Tue, Jul 16, 2019 at 3:41 PM Danyao Wang <dan...@chromium.org> wrote:
On Tue, Jul 16, 2019 at 2:49 PM Jeremy Roman <jbr...@chromium.org> wrote:
On Tue, Jul 16, 2019 at 11:58 AM Danyao Wang <dan...@chromium.org> wrote:
Chewing on Domenic's suggestion a bit more, I don't think "make resolve/reject a no-op" when the promise's associated document is not fully-active will solve the payment payment use case. Here's a concrete scenario:
  • Top-level frame embeds an iframe that creates a PaymentRequest object, e.g. |request|
  • 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.
  • iframe navigates so the document associated with |acceptPromise| is no longer a fully active document
  • Top-level frame code hangs because |acceptPromise| stays unsettled
In this case, we actually need |acceptPromise| to be rejected for the top-level frame to not hang.

I noticed that WebKit has an ActiveDOMObject concept that offers a stop() hook that is called in Document::prepareForDestruction() just before the event queue is stopped. This allows the PaymentRequest, which is an ActiveDOMObject, to clean up in time.

Can we do something similar? Just as a strawman:
  • Introduce a blink::ActiveDOMObject class
  • Store a list of blink::ActiveDOMObjects in blink::ExecutionContext
  • Call stop() on each execution context in Document::Shutdown() in the DocumentLifecycle::kStopping phase
  • PaymentRequest will inherit from blink::ActiveDOMObject and settle promises in stop()
This is what ContextLifecycleObserver does. Implementing ContextLifecycleObserver and ActiveScriptWrappable together is a common pattern. There's a comment in context_lifecycle_observer.h that describes this in more detail.

(That we have a similar concept is no coincidence: we inherited ActiveDOMObject from WebKit when we forked; we just like renaming/refactoring things.)

Got it. There is still one thing missing: ContextLifecycleObserver doesn't provide a "context-will-be-destroyed" callback a la WebKit's ActiveDOMObject::stop() method. How about this?
  • Add ExecutionContext::NotifyContextWillBeDestroyed() to call a new pure virtual ContextLifecycleObserver::ContextWillBeDestroyed() method
  • PaymentRequest implements ContextLifecycleObserver::ContextWillBeDestroyed() to settle the promises
  • Call ExecutionContext::NotifyContextWillBeDestroyed() in Document::Shutdown() in the DocumentLifecycle::kStopping phase to allow observers to clean up.
It's not clear to me that this would solve anything (the promises' handlers still wouldn't run until the next microtask checkpoint).

I see. Thanks for pointing it out. I wonder how WebKit's ActiveDOMObject::stop() works... But maybe I'm looking at the wrong spot. All I know is that their implementation does reject the promise in this scenario...

Is there a good resource you'd recommend for learning more about microtasks and scheduling?
 
As Dave brought to my attention, this is an instance of the broader question of whether promises returned by HTML/WebIDL operations should work after the context that produced them has gone away.

I think the bindings team has historically opposed allowing WebIDL interfaces to continue to operate under those conditions (e.g., I think functions that take success/error callbacks or event handlers don't call them on shutdown), so maybe leaving it unresolved is the right choice here.

I can see the reason for this. However, this makes it impossible to implement payment request spec as is and there seem to be other specs using the same pattern (e.g. dom-img-decode). It seems bad that specs are asked to follow a pattern that's not implementable in Blink...

I think the current spec'ed behavior is critical if there is a real-world use case as mentioned earlier. How about this for next steps?
  • I'll add a UMA counter to see if there's real-world use cases of payment request being created from same-origin iframes. If this is not negligible, perhaps we can revisit the decision to leaving the promise unresolved.
  • If the UMA count is negligible, then I'll work with the payment request editors to update the spec.
Domenic's earlier comment seems to suggest that "mak(ing) resolve/reject a no-op if the promise's...associated Document is not fully-active" should be a prerequisite to removing "reject-after-inactive" requirement from the spec. I don't really understand the connection here. @Domenic Denicola @Kentaro Hara Can you help clarify? I'd be happy to do the leg work if I understand what needs to be done...

Thanks!

Domenic Denicola

unread,
Jul 16, 2019, 4:44:23 PM7/16/19
to Danyao Wang, Jeremy Roman, Kentaro Hara, Kentaro Hara, Dave Tapuska, Dave Tapuska, Daniel Cheng, Ben Kelly, Domenic Denicola, Kouhei Ueno, platform-architecture-dev, Chromium-dev
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...

Danyao Wang

unread,
Jul 16, 2019, 6:07:22 PM7/16/19
to Domenic Denicola, Jeremy Roman, Kentaro Hara, Kentaro Hara, Dave Tapuska, Dave Tapuska, Daniel Cheng, Ben Kelly, Kouhei Ueno, platform-architecture-dev, Chromium-dev
This all makes sense now. Thanks! I'll rely these comments on https://github.com/w3c/payment-request/issues/872 to update the spec.
Reply all
Reply to author
Forward
0 new messages