1156851 - [jsdbg2] Cannot get newest frame within `onPromiseSettled` hook

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[James Long (:jlongster)]

Take this code:

    function foo() {
      let p = new Promise(function(resolve, reject) {
        resolve('yay!');
      });

      p.then(function(v) {
        throw new Error('oh no');
      });

      return p;
    }

    foo();
    console.log('done');

Now implement the `onPromiseSettled` hook on the debugger object with this code:

    onPromiseSettled: function(aPromise) {
      let frame = this.dbg.getNewestFrame();
      console.log(frame);
    }

For some reason `frame` is null most of the time. I thought it always returned null, but actually it does the the frame the first time. Here's the raw output of the hook:

console.log: Frame {}
console.log: null
console.log: null

We should always be able to get the newest frame, and we need it to implement bug 1156474

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looking at that testcase, the following things will happen:

1)  A promise is created and stored in p.  As part of its creation, it settles (when the
    resolve() function is called) and invokes the onPromiseSettled hook.  I assume this is
    the cse when you see a non-null this.dbg.getNewestFrame().

2)  then() is invoked on p.  This creates a new promise and returns it.  This promise is
    not yet settled and won't be settled until the promise callbacks of p run.  Let's call
    this anonymous promise "q".

3)  The promise callbacks of p run.  The function passed to then() is called and throws.
    "q" is rejected with that exception and settles.  Note that at the point when q
    settles there is no JS on the stack at all.  Hence this.dbg.getNewestFrame returns
    null.

I have no idea where the second null is coming from, offhand...

For purposes of bug 1156474, in which code would you expect to end up breaking in this testcase?

Comment 2

[James Long (:jlongster)]

That makes sense. It's a difference in how the promise is rejected: if it's the `Promise.reject` call (which immediately fires the `onPromiseSettled` hook as far as I can know), or throwing an error which the engine will take and settle the promise (there will never be a live frame here).

(In reply to Not doing reviews right now from comment #1)
> 
> For purposes of bug 1156474, in which code would you expect to end up
> breaking in this testcase?

Intuitively I expected it to break on the `throw new Error('oh no');` line but I see now that it won't work. "Break on caught exceptions" will actually do that, so it's just a different case.

fitzgen, for bug 1156474 were you thinking of just breaking on any call on `Promise.reject`? The usefulness of this is getting less and less in my opinion.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Intuitively I expected it to break on the `throw new Error('oh no');` line 

Right, I agree.

We _could_ make this work, I believe.  Specifically, if we have an uncaught exception (though our detection of these is sucky) while inside a promise then callback (we'd need to communicate this information to the JS engine), then we should be able to treat it as a rejection.  Whatever info we communicate about it being a promise then callback can include the promise on the other side of the then(), which can then be used to test whether the rejection is unhandled...

Comment 4

[Boris Zbarsky [:bzbarsky]]

A slightly more serious issue, btw, is that for promises rejected by browser internals there might not be any stack involved anywhere at all.  :(

We could consider pushing down the Promise-internal stack captures further down (so the happen for internal rejections too) and use those in combination with async stacks... that will at least indicate how you got to the non-scripted rejection point.  But I'm not sure what it would mean to break in that case, exactly, nor whether C++ internals would be robust to the event loop spinning at whatever points they reject promises.

Comment 5

[James Long (:jlongster)]

=(In reply to Not doing reviews right now from comment #4)
> A slightly more serious issue, btw, is that for promises rejected by browser
> internals there might not be any stack involved anywhere at all.  :(
> 
> We could consider pushing down the Promise-internal stack captures further
> down (so the happen for internal rejections too) and use those in
> combination with async stacks... that will at least indicate how you got to
> the non-scripted rejection point.  But I'm not sure what it would mean to
> break in that case, exactly, nor whether C++ internals would be robust to
> the event loop spinning at whatever points they reject promises.

Yeah, I don't know what we would even break on in that case. Potentially wherever the browser API was invoked?

There's a lot of various semantics here so we need to nail them down before implementing bug 1156474.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Potentially wherever the browser API was invoked?

That works if the rejection happens sync, but most promise-based APIs are async and would reject at some later point whenever the error condition is discovered...

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to James Long (:jlongster) from comment #2)
> Intuitively I expected it to break on the `throw new Error('oh no');` line
> but I see now that it won't work. "Break on caught exceptions" will actually
> do that, so it's just a different case.
> 
> fitzgen, for bug 1156474 were you thinking of just breaking on any call on
> `Promise.reject`? The usefulness of this is getting less and less in my
> opinion.

Yeah, I didn't put as much forethought into break-on-promise-rejection as I should have.

Ideally, we would break on both

* Promise.reject() calls, and

* uncaught errors inside of a promise handler or function passed to new Promise (resulting in promise rejections).

The onPromiseSettled hook solves the first case (although, it's not useless otherwise: it is perfect for making a general promise tool and keeping track of all promises' state).

For the second, I think we would need to have the following:

* A version of Debugger.Script.prototype.isInCatchScope that is shallow and only applies to one function at a time, not the whole lexical environment (say shallowIsInCatchScope).

* We would then walk up the stack checking shallowIsInCatchScope along the way until we get to the Promise C++ frame. (Side note: Is this always the oldest frame because Promise is always async? Can we ever have multiple activations with the youngest being Promise stuff and older being something else? Maybe with nested event loops?)

  * If we were in a catch scope before the Promise C++ frame, then don't pause.

  * Otherwise, when there is no catch scope before the Promise C++ frame, then we do pause because this error will reject the promise.

That seems like enough work that maybe we should reconsider the urgency of break-on-promise-rejection. Or maybe implement the "dumb" version for now and see if people really want a better version or don't care.

CC'ing ejpbruel, who implemented isInCatchScope, and can maybe help poke holes in the above idea.

Comment 8

[James Long (:jlongster)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> 
> That seems like enough work that maybe we should reconsider the urgency of
> break-on-promise-rejection. Or maybe implement the "dumb" version for now
> and see if people really want a better version or don't care.

Agreed, this doesn't really feel like a bug fixing or polish, so I don't think it should be devedition-40 eligible. I know a few people that use "break on caught exceptions" to debug promise work and they say it works pretty well, so I don't think there's a huge gap to be filled. It's worth looking into your solution though.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Side note: Is this always the oldest frame 

  function f() {
    var p = new Promise(function() { throw "hey"; });
  }
  f();

That's going to have an uncaught rejection when the promise C++ is not the oldest frame, right?

> then we do pause because this error will reject the promise

Pause if the promise has no catch handler, right?  Thought it might get more complicated than that with promise chains...  You want to pause if there is no catch handler, ignoring catch handlers that just pass on the rejection.  Or some such.

Comment 10

[Jim Blandy :jimb]

Closing this bug because Debugger was behaving correctly. If we can identify the feature we want, how it should behave, and what we need to implement it, that can be a new bug.