In the Streams Specification there are infallible operations (marked with !, like in ECMA262) that we are forced to make fallible to account for the fact that JSAPI is fallible in unexpected places, often purely because of the potential for OOM when allocating a new object.

As a result, there are a large number of error handling paths in the DOM Streams implementation that do not exist in the specification.

These error handling paths are under tested, and will likely end up leaving the Stream in a broken and unusable state anyhow.

We should reconsider non-standard error handling paths; a suggestion from Kagami was to purely crash on OOM, which is the gecko default anyhow.

I'm inclined to agree. However, I do think the SpiderMonkey team ought to have a discussion about how to support this in a nice scalable fashion; it's not always clear whether or not a JSAPI call can only fail because of OOM.

In general Gecko doesn't care about small allocations and crashes happen in case of OOM, but usually larger allocations are fallible and some error is returned to the caller.

The current implementation cares very much about Promise allocations. We can remove them, at least.

(In reply to Kagami :saschanaz from comment #2)

The current implementation cares very much about Promise allocations. We can remove them, at least.

I actually took a look at this, because I thought it would be easy enough to make a Promise::CreateInfallible . Turns out, not the case!

There are actually two ways that Promise::Create can throw:

1. Allocating the JS::PromiseObject; this is the OOM Condition I thought we could avoid handling.
2. Running AutoJSAPI::init.

(Technically, there's a third, if you pass it a nullptr global, but I didn't consider that here)

While I'm reasonably comfortable crashing on OOM, I'm less comfortable crashing on AutoJSAPI::init failure; I'm not entirely sure the circumstances where it could fail (seems like mostly it's if the global can't return its JSObject... but I I don't know where that happens).

Having said that, I still think there's improvement we can do here.

Edit: It's not clear how we can cleanly do this though.

My unexamined gut feeling is that we should crash on these. The case that gives me pause is the case implied by Return the JSObject for this global, if it still has one. It might be worth it to check early on whether we're that far into tearing down the global and either return an error or ignore in that case, then for everything else just crash.

Or in other words: I'm fine with the OOM crashes, but not the shutdown crashes. (For some possibly weak form of "shutdown"?)

Would it make sense to have a no-op promise object when AutoJSAPI::Init fails? So that Promise::MaybeResolve/MaybeReject calls will be completely ignored during teardown, because they surely can't do anything at that point.

(Revisiting since we are getting more and more explicit OOM error handlings e.g. in https://phabricator.services.mozilla.com/D166214)

I'm not 100% sure I follow Kagami; the way I read "no-op promise object" is that we have a pre-allocated (per-global) promise wrapper that we can return in this situation which has no handlers attached.

This seems like a plausible (if potentially involved to implement) solution to the issue. We'd want to ensure this shutdown promise wouldn't complain if it was resolved/rejected multiple times as well.

Arai: Does this sound like a feasible road towards helping us implement a Promise::CreateInfallible with the goal of crashing on OOM, but also relatively gracefully handling shutdown?

Is the no-op promise object about mozilla::dom::Promise instance, or JSObject instance ?

the former makes sense but the latter doesn't much.

If latter, the idea is following, right?:

1. we pre-allocate a promise object which is already rejected with undefined and store it somewhere
2. provide a public function to get that promise
3. use the promise object when the consumer wants to return on not-spec-ed abrupt completion

then, if the expected failure case is that global goes away during shutdown and AutoJSAPI::init fails:

• if we store the pre-allocated promise per global, there's no way to get the pre-allocated promise
• if we store the pre-allocated promise to persistent rooted or something, there's no way to wrap it with correct compartment

Thus, my understanding is that, such failure case should be handled by the mozilla::dom::Promise layer without using JSObject.
Is there any place that needs promise JSObject after such failure?

1. we pre-allocate a promise object which is already rejected with undefined and store it somewhere

I'd like to let it unsettled, but rejecting is maybe fine too? The former sounds simpler though.

Thus, my understanding is that, such failure case should be handled by the mozilla::dom::Promise layer without using JSObject.
Is there any place that needs promise JSObject after such failure?

IMO having a JSObject-less dom::Promise is fine and easier to implement.

(In reply to Kagami [:saschanaz] from comment #8)

1. we pre-allocate a promise object which is already rejected with undefined and store it somewhere

I'd like to let it unsettled, but rejecting is maybe fine too? The former sounds simpler though.

I was thinking that, using single pre-allocated unsettled promise JSObject for all failure case will make the situation more complex,
because resolving/rejecting it in one place will affect all other cases.

Thus, my understanding is that, such failure case should be handled by the mozilla::dom::Promise layer without using JSObject.
Is there any place that needs promise JSObject after such failure?

IMO having a JSObject-less dom::Promise is fine and easier to implement.

If JSObject-less dom::Promise works for the DOM Streams case, that should be better.

(In reply to Tooru Fujisawa [:arai] from comment #9)

I was thinking that, using single pre-allocated unsettled promise JSObject for all failure case will make the situation more complex,
because resolving/rejecting it in one place will affect all other cases.

But in that case it should never be resolved/rejected, should it?

If JSObject-less dom::Promise works for the DOM Streams case, that should be better.

👍

Hi Olli, I think I'll go ahead and make Promise::CreateInfallible that leaves mPromiseObject null, and adjust MaybeReject/Resolve to be no-op when it's null. What do you think?

Reasoning: IMO it's more or less equivalent to have a never-settled promise behavior-wise, because Streams currently just throw when promise creation fails which will probably reach nothing when JS context is already dead.

Hmm, so which method would use this CreateInfallible, and how?
I'm not sure CreateInfallible quite explains that the promise can't be resolved/rejected.

But in general the idea might be reasonable.

(In reply to Olli Pettay [:smaug][[email protected]] from comment #11)

Hmm, so which method would use this CreateInfallible, and how?

IMO it can be used in general DOM implementations as a drop-in replacement.

I'm not sure CreateInfallible quite explains that the promise can't be resolved/rejected.

Should it explain that, though? Resolving a promise when JSContext is dead is already no-op AFAICT. https://searchfox.org/mozilla-central/rev/daf613efc5c358f3a94961d73b90472c00703838/dom/promise/Promise.cpp#323

So, can we split the CreatePromiseInfallible portion of this into a different bug -- I think we have agreement there, but the broader question still applies so I'd like to keep this open.