1929595 - Promise rejections aren't considered as exception and doesn't break the debugger when pause on exception is enabled

Status: NEW

(DevTools :: Debugger, enhancement)

Description

[Alexandre Poirot [:ochameau]]

For example, when running this:
data:text/html,<script>window.onclick=()=>{fetch(null)};</script>

The call to fetch with an invalid parameter does trigger an exception, but won't be identified as one by the debugger.

We can see in the console that the exception is slighly special as fetch involves a promise:
Uncaught (in promise) TypeError: Window.fetch: null is not a valid URL.

The debugger currently relies on Spidermonkey Debugger API onExceptionUnwind:
https://searchfox.org/mozilla-central/rev/aecb006bbb135d707ca4b8cea7572dd8abab6817/devtools/docs/user/debugger-api/debugger/index.rst#98
which doesn't seem to handle promise rejections.

The error message logged by the console is coming from a ConsoleService error, sent from here:
https://searchfox.org/mozilla-central/rev/aecb006bbb135d707ca4b8cea7572dd8abab6817/dom/promise/Promise.cpp#677
That DOM code is hooked on Spidermonkey, via PromiseRejectionTrackerCallback
https://searchfox.org/mozilla-central/rev/aecb006bbb135d707ca4b8cea7572dd8abab6817/xpcom/base/CycleCollectedJSContext.cpp#347-350

It isn't clear to be if you should either:
a) have devtools to register into PromiseDebugging to observe uncaught and caught promise rejections ?
+ probably easily actionable, just need to wire DevTools backend with PromiseDebugging
b) have spidermonkey to handle async exception via onExceptionUnwind, or some new API ?
+ would benefit to other projects using spidermonkey and relying on Debugger API

Arai, any opinion about a more spidermonkey approach rather than a PromiseDebugging one?

Comment 1

[Tooru Fujisawa [:arai]]

I think, both PromiseRejectionTrackerCallback and onExceptionUnwind don't fit the purpose.
Also, we'll first need to define the exact set of events that we want to detect, or, we want to ignore.

Details below:

onExceptionUnwind and error

onExceptionUnwind is not about the exception/error, but it is about the JavaScript execution context (so, "unwind" is the actual event).
onExceptionUnwind is called only when the exception is exposed as a throw completion in JavaScript execution and that results in unwinding the execution context (call stack).
(to be clear, it's called before actually unwinding)
If the C++ implementation internally creates a pending exception and then immediately steals and clears it, onExceptionUnwind is not called for the case.
(JS_ReportError* + JS_GetPendingException + JS_ClearPendingException)
This means, onExceptionUnwind is not called for "an error is thrown" event, but only about the consequence of the error, against the JavaScript execution.

Then, a promise rejection can be considered as a kind of error, but the rejection itself doesn't directly change the execution.
If the promise is used with await inside an async function, the rejection is converted into a throw completion, and then it results in unwinding, but that's already handled by onExceptionUnwind.
So, extending onExceptionUnwind for "a promise is rejected" event will result in duplicate handling, for separate 2 events.

How PromiseRejectionTrackerCallback works

If we want to directly detect "a promise is rejected" event, then it needs to be handled by a dedicate hook for it.

PromiseRejectionTrackerCallback provides similar thing, but the callback is for tracking unhandled rejections, not for tracking all rejections.
The mechanism is the following:

• (a) the callback is called when a promise is rejected while the promise doesn't yet have onRejected handler
• (b) the callback is called when a onRejected handler is added to (a)'s promises
• (c) Promises that called with (a) but not called with (b) are unhandled

So, the callback is called only for the subset of "a promise is rejected" events. it's not called for the following case:

• a promise is rejected while the promise already has onRejected handler

Thus, using the PromiseRejectionTrackerCallback infrastructure doesn't cover all "a promise is rejected" cases.

What events do we want to detect?

If we really need to detect all "a promise is rejected" events, that needs a new API.
It can be similar thing to PromiseRejectionTrackerCallback, with some different behaviors.

But I'm not sure if that's really what we want, given we don't have the equivalent "an error is thrown" events.
(Or perhaps we want the "an error is thrown" event too? something that can also detect exceptions thrown internally?)

If the set of events that we actually want to detect is only a subset of all "a promise is rejected" events, or if the event is actually a consequence of "a promise is rejected", then we'll need to look into that details, and that will need different consideration.

For example, if we want to detect all unhandled rejections, that's already exposed to the web content as "unhandledrejection" event,
which is dispatched by the PromiseRejectionTrackerCallback.
The example's case is actually unhandled rejection, and that should be caught by "unhandledrejection" event.

So, the question would be, do we want to detect the following cases as well?

• (1) An onRejected handler is added after the rejection (fetch(null).catch(() => {}))
• (2) An onRejected handler is added before the rejection (fetch("URL-that-asynchronously-causes-network-error").catch(() => {}))
• (3) A promise is rejected in the content global, but the promise is never exposed to the web content (some internal promise used inside Web API etc)
• (4) A promise created by user-defined code is rejected by user-defined code (Promise.reject(1), new Promise((_, reject) => reject(1)))

Also, another question would be whether we want to immediately pause the execution on rejection or not.
The "unhandledrejection" event is not immediately dispatched on the rejection, but only after the microtask (ref), because it's not possible to know whether the rejection will never be handled, at the point of rejection.

What's the requirement from the DevTools' side?

Comment 2

[Alexandre Poirot [:ochameau]]

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

Thanks a lot for your insight!

Then, a promise rejection can be considered as a kind of error, but the rejection itself doesn't directly change the execution.
If the promise is used with await inside an async function, the rejection is converted into a throw completion, and then it results in unwinding, but that's already handled by onExceptionUnwind.

Oh. I didn't realize that onExceptionUnwind was actually fired on the following case:
data:text/html,<script>window.onclick=async ()=>{await fetch(null);}</script>
But... for some reason Debugger.Frame.isInCatchScope() returns true over there. Is that expected?
Are async functions considered as having a try/catch by default?
This requires to enable the "pause on caught exception" option to be able to pause on this.

Note that onExceptionUnwind no longer fires when catching the async exception:
data:text/html,<script>window.onclick=async ()=>{try {await fetch(null);}catch(e){console.log("caught")}}</script>

So, extending onExceptionUnwind for "a promise is rejected" event will result in duplicate handling, for separate 2 events.

Anyway, given what you are saying here. May be we should somehow manage to ignore async exceptions and instead work on something to expose all promise rejections reliably?

So, the question would be, do we want to detect the following cases as well?

• (1) An onRejected handler is added after the rejection (fetch(null).catch(() => {}))
• (2) An onRejected handler is added before the rejection (fetch("URL-that-asynchronously-causes-network-error").catch(() => {}))
• (3) A promise is rejected in the content global, but the promise is never exposed to the web content (some internal promise used inside Web API etc)
• (4) A promise created by user-defined code is rejected by user-defined code (Promise.reject(1), new Promise((_, reject) => reject(1)))

We want to pause on all these scenarios, except 3, which would be hard to explain to the user.
You can see in chrome that the debugger pause on all these usecases (with pausing on caught and uncaught exceptions):
data:text/html,<script>window.onclick=async ()=>{fetch(null);}</script>
data:text/html,<script>window.onclick=async ()=>{await fetch(null);}</script>
data:text/html,<script>window.onclick=async ()=>{try {await fetch(null);}catch(e){console.log("caught")}}</script>
data:text/html,<script>window.onclick=async ()=>{Promise.reject("foo")}</script>

Also, another question would be whether we want to immediately pause the execution on rejection or not.
The "unhandledrejection" event is not immediately dispatched on the rejection, but only after the microtask (ref), because it's not possible to know whether the rejection will never be handled, at the point of rejection.

I imagine it is fine pausing with some delay for unhandled rejections.

Comment 3

[Tooru Fujisawa [:arai]]

(In reply to Alexandre Poirot [:ochameau] from comment #2)

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

Thanks a lot for your insight!

Then, a promise rejection can be considered as a kind of error, but the rejection itself doesn't directly change the execution.
If the promise is used with await inside an async function, the rejection is converted into a throw completion, and then it results in unwinding, but that's already handled by onExceptionUnwind.

Oh. I didn't realize that onExceptionUnwind was actually fired on the following case:
data:text/html,<script>window.onclick=async ()=>{await fetch(null);}</script>
But... for some reason Debugger.Frame.isInCatchScope() returns true over there. Is that expected?
Are async functions considered as having a try/catch by default?
This requires to enable the "pause on caught exception" option to be able to pause on this.

Yes, there's non-syntactic try-catch used in the bytecode, to convert the throw completion into a rejection.
And it sounds like we need a new different query on the Frame object, which returns whether given offset is inside a syntactic try-catch.

Given this is different issue, I'll file a separate bug.

Note that onExceptionUnwind no longer fires when catching the async exception:
data:text/html,<script>window.onclick=async ()=>{try {await fetch(null);}catch(e){console.log("caught")}}</script>

I observe the debugger pause with the following:

1. open the URL in a new tab
2. open debugger
3. check "Pause on exceptions"
4. check "Puase on caught exceptions"
5. click the empty area

Can you verify?

Anyway, given what you are saying here. May be we should somehow manage to ignore async exceptions and instead work on something to expose all promise rejections reliably?

Exposing all promise rejections is possible.
(maybe we need to look into the performance cost etc tho)

Then, the probelm would be how to filter out rejections on promises that's not exposed to the web content.

So, the question would be, do we want to detect the following cases as well?

• (1) An onRejected handler is added after the rejection (fetch(null).catch(() => {}))
• (2) An onRejected handler is added before the rejection (fetch("URL-that-asynchronously-causes-network-error").catch(() => {}))
• (3) A promise is rejected in the content global, but the promise is never exposed to the web content (some internal promise used inside Web API etc)
• (4) A promise created by user-defined code is rejected by user-defined code (Promise.reject(1), new Promise((_, reject) => reject(1)))

We want to pause on all these scenarios, except 3, which would be hard to explain to the user.
You can see in chrome that the debugger pause on all these usecases (with pausing on caught and uncaught exceptions):
data:text/html,<script>window.onclick=async ()=>{fetch(null);}</script>
data:text/html,<script>window.onclick=async ()=>{await fetch(null);}</script>
data:text/html,<script>window.onclick=async ()=>{try {await fetch(null);}catch(e){console.log("caught")}}</script>
data:text/html,<script>window.onclick=async ()=>{Promise.reject("foo")}</script>

Okay, so it sounds like the this is really specific to "rejection", and it's not about "handled or not".
In that case, the delay around the "unhandled rejection" no longer matters,
but we just need to call the hook whenever the rejection happens.

I'll file a SpiderMonkey-side bug for this as well.

Comment 4

[Alexandre Poirot [:ochameau]]

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

Note that onExceptionUnwind no longer fires when catching the async exception:
data:text/html,<script>window.onclick=async ()=>{try {await fetch(null);}catch(e){console.log("caught")}}</script>

I observe the debugger pause with the following:

1. open the URL in a new tab
2. open debugger
3. check "Pause on exceptions"
4. check "Puase on caught exceptions"
5. click the empty area

Can you verify?

Oh yes, there is something buggy in DevTools...
It doesn't pause if the settings for step 3 & 4 are already set. But pauses if I toggle them off/on again.
I'll investigate.

Okay, so it sounds like the this is really specific to "rejection", and it's not about "handled or not".
In that case, the delay around the "unhandled rejection" no longer matters,
but we just need to call the hook whenever the rejection happens.

Note that while we want to pause on all rejections when "pause on caught exception" is enabled,
when only "pause on exception" is enabled, we would like to pause only on unhandled rejections and ignore the handed ones.
If that helps, DevTools code can rely on different APIs to observe the two usecases.

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)

Okay, so it sounds like the this is really specific to "rejection", and it's not about "handled or not".
In that case, the delay around the "unhandled rejection" no longer matters,
but we just need to call the hook whenever the rejection happens.

Note that while we want to pause on all rejections when "pause on caught exception" is enabled,
when only "pause on exception" is enabled, we would like to pause only on unhandled rejections and ignore the handed ones.
If that helps, DevTools code can rely on different APIs to observe the two usecases.

Okay, if we want to ignore "handled" rejections in that mode, then it's indeed the "unhandledrejection" event, which is already provided, and it's intrinsically asynchronous, except for some special case (see below).
So, that part should already be solved on the SpiderMonkey side, and the debugger just need to hook into unhandledrejection event.

--

About the special case:

There can be special case where the rejection is known to be unhandled at the point of rejection,
that's the ()=>{fetch(null)} case in the first example.

SpiderMonkey tracks whether a function call's return value is used or not (see JSOp::CallIgnoresRv), and each native function can opt into the behavior for the unused case. (JS::detail::CallArgsBase::ignoresReturnValue)
For example, Promise.prototype.then behaves differently for the unused case, where it doesn't create the promise object.

So, theoretically, each API can opt into the unused case, and treat all rejections on the return value as "unhandled" immediately.

But I'm not sure if this is something really helpful, because:

• The return value promise (especially the fulfilled case) should be meaningful in most cases, in addition to just being "rejected or not"
• Getting different behavior (synchronous pause vs asynchronous pause) depending on the return value usage can be confusing