1592427 - [jsdbg2] Debugger.Objects for promises should provide reaction records

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jim Blandy :jimb]

Given a Debugger.Object for a promise, the Debugger API should provide a way to obtain Debugger.Objects for chained promises, and Debugger.Frames for any async calls awaiting the resolution of that promise.

Together with bug 1592415, this would give the devtools server the primitives it needs to include asynchronous callers in stack traces.

When we evaluate an expression like the await call in f:

async function f() {
    await g();
}

where g is an asynchronous function, this bug challenges us to find a path from the promise returned by the call g(), representing g's resolution, to the Debugger.Frame for the call to f.

That path is not trivial:

• Let G be the internal generator object representing the call to f.

• Let P1 be the promise returned by g().

• The await expression creates a fresh internal promise, P2, and resolves it to P1. That is, resolving P1 resolves P2 as well.

• The await expression then attaches a promise reaction record to P2 (i.e., it calls P2.then) to awaken G.

Concretely, we should see the following arrangement:

• Bug 1592415 lets us obtain a Debugger.Object for P1, given a Debugger.Frame for the call to g.

• P1 is a PromiseObject. A PromiseObject has a reserved slot, PromiseSlot_ReactionsOrResult, that holds its reaction jobs, represented as PromiseReactionRecords. (There are separate representations for zero, one, and more than one reaction jobs, but those are details.)

• A PromiseReactionRecord has flags indicating what sort of reaction it represents.

  • I believe the REACTION_FLAG_DEFAULT_RESOLVING_HANDLER flag indicates that the reaction record represents another promise resolved to this one, so P1 should have a PromiseReactionRecord with this flag set that points to P2.

  • I believe the REACTION_FLAG_ASYNC_FUNCTION flag indicates that the reaction record represents an asynchronous function awaiting this promise, so P2 should have a PromiseReactionRecord with this flag set that points to G.

• Given G, we should be able to find the Debugger.Frame representing the suspended call to f.

Comment 1

[Jim Blandy :jimb]

Attachment: Bug 1592427: Correct comment for ReactionRecordSlots::ReactionRecordSlot_Promise. DONTBUILD r?arai

Comment 2

[Tooru Fujisawa [:arai]]

are you going to post more patches later, or is the problem already solved in other bugs? (I'll check the related code, but asking just to make sure)
I'm wondering the relation between the patch and the goal of this bug.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c8c82641737
Correct comment for ReactionRecordSlots::ReactionRecordSlot_Promise. DONTBUILD r=arai

Comment 4

[Jim Blandy :jimb]

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

are you going to post more patches later, or is the problem already solved in other bugs? (I'll check the related code, but asking just to make sure)
I'm wondering the relation between the patch and the goal of this bug.

I'm going to post more patches later, sorry for the confusion.

Comment 5

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4c8c82641737

Comment 6

[Jim Blandy :jimb]

Attachment: Bug 1592427: Let InternalAwait arg types show which handlers are actually used. r?arai

js/src/builtin/Promise.cpp's InternalAwait function takes onFulfilled and
onRejected handlers, but in all uses these are PromiseHandler enum values, not
actual JavaScript function objects.

It's helpful to readers to know that InternalAwait reaction records always use
one of the stock handlers from PromiseHandler, and never arbitrary code supplied
by the user. It's also easier on the callers to not have to pass Value handles.

Comment 7

[Jim Blandy :jimb]

Here's what I'm planning to implement:

promiseReactions

If the referent is a [Promise][promise], this is an array of objects
describing the reaction records added to the promise. There are several
different sorts of reaction records:

• The array entry for a reaction record added with then or catch has the
  form { then: F, catch: F, result: P }, where each F is a Debugger.Object
  referring to a function object, and P is the promise that will be resolved
  with the result of calling them.

  The then and catch properties may be absent in some cases. A call to
  then can omit the rejection handler, and a call to catch omits the
  fulfillment handler. Furthermore, various promise facilities create records
  like this as internal implementation details, creating handlers that are not
  representable as JavaScript functions.

• When a promise P1 is resolved to another promise P2 (that is, when P2
  is resolved, P1 will be resolved the same way), that adds a reaction
  record to P2. The array entry for that reaction record is simply the
  Debugger.Object representing P1.

• An await expression creates an internal promise A, resolves it to its
  operand, and then adds a reaction record to A that resumes the suspended
  call appropriately. The array entry for that reaction record is a
  Debugger.Frame representing the suspended call.

  If an await's operand is a promise P, this means we can obtain the
  Debugger.Frame for the suspended call by looking through P's reaction
  records for the simple promise A, and then look through A's reaction
  records (there should be only one) for the Debugger.Frame for the call.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f16fced2b955
Let InternalAwait arg types show which handlers are actually used. r=arai

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/f16fced2b955

Comment 10

[Jim Blandy :jimb]

Attachment: Bug 1592427: Introduce ForEachReaction helper function. r?arai

SpiderMonkey Promises use a hybrid representation for the list of reaction
records attached to a promise: an empty list is undefined; a list of one
element is the reaction record itself, and a list of two or more records is an
array.

TriggerPromiseReactions and PromiseObject::dependentPromises duplicate the code
for walking over a reaction record list. Later patches in the series will
introduce a third function that does it, so it seemed like a good time to
abstract out the iteration.

This patch defines the function 'ForEachReaction', which applies a closure to
each reaction in the list, and changes existing code to use it.

Comment 11

[Jim Blandy :jimb]

Attachment: Bug 1592427: Let DebuggerFrame::create build both on-stack and suspended frames. r?loganfsmyth

Subsequent patches introduce Debugger.Object.prototype.promiseReactions, which
will be the first case in which we've ever needed to create a Debugger.Frame for
a suspended generator call. All prior generator support creates Debugger.Frames
only for frames on the stack.

This patch changes DebuggerFrame::create to take both a FrameIter and a
generator object, both optional (one or the other must be present), and create a
frame with whatever data is provided. The sole caller is adjusted accordingly.

Depends on D56835

Comment 12

[Jim Blandy :jimb]

Attachment: Bug 1592427: Don't use an AddPtr to access generatorFrames in Debugger::getFrame. r?loganfsmyth

Responsibility for creating entries in Debugger::generatorFrames was moved to
DebuggerFrame::setGenerator, so Debugger::getFrame is only doing lookups now,
and can use the more restricted Ptr type for the results of its lookups.

Depends on D56836

Comment 13

[Jim Blandy :jimb]

Attachment: Bug 1592427: Remove as<...> casts from Debugger frame handling. r?loganfsmyth

Debugger::generatorFrames and Debugger::frames used to have looser types, and
the code working with them needed to downcast in order to work with more
specific object types. But now the key and value types in those maps are
well-typed, so the downcasts do nothing and can be removed.

Depends on D56837

Comment 14

[Jim Blandy :jimb]

Attachment: Bug 1592427: Implement Debugger.Object.prototype.promiseReactions. r?arai,loganfsmyth

See documentation in js/src/doc/Debugger/Debugger.Object.md.

Depends on D56838

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c8f04b2a6dc
Introduce ForEachReaction helper function. r=arai
https://hg.mozilla.org/integration/autoland/rev/35be3ee203a8
Let DebuggerFrame::create build both on-stack and suspended frames. r=loganfsmyth
https://hg.mozilla.org/integration/autoland/rev/cb76becb05c3
Don't use an AddPtr to access generatorFrames in Debugger::getFrame. r=loganfsmyth
https://hg.mozilla.org/integration/autoland/rev/b3a05fee765f
Remove as<...> casts from Debugger frame handling. r=loganfsmyth
https://hg.mozilla.org/integration/autoland/rev/fb6cac74e451
Implement Debugger.Object.prototype.promiseReactions. r=arai,loganfsmyth

Comment 16

[Arthur Iakab [arthur_iakab]]

Backed out 5 changesets (bug 1592427) for causing build bustages on debugger/Object.cpp CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/a9fe5d6acd655181d94ca4e68c0aed7f34c5053b

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=280987337&resultStatus=testfailed%2Cbusted%2Cexception&revision=fb6cac74e451346b28356abc2e77799e6e0f35fc

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=280987337&repo=autoland

Jim can you please take a look ?

Comment 17

[Jim Blandy :jimb]

Taking a look.

Comment 18

[Jim Blandy :jimb]

The static analysis found a GC rooting problem.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0ca73c538e7
Introduce ForEachReaction helper function. r=arai
https://hg.mozilla.org/integration/autoland/rev/cf8aa85ab3da
Let DebuggerFrame::create build both on-stack and suspended frames. r=loganfsmyth
https://hg.mozilla.org/integration/autoland/rev/595e5d95e650
Don't use an AddPtr to access generatorFrames in Debugger::getFrame. r=loganfsmyth
https://hg.mozilla.org/integration/autoland/rev/2cdf0ee12aaf
Remove as<...> casts from Debugger frame handling. r=loganfsmyth
https://hg.mozilla.org/integration/autoland/rev/496e56e95295
Implement Debugger.Object.prototype.promiseReactions. r=arai,loganfsmyth

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f0ca73c538e7
https://hg.mozilla.org/mozilla-central/rev/cf8aa85ab3da
https://hg.mozilla.org/mozilla-central/rev/595e5d95e650
https://hg.mozilla.org/mozilla-central/rev/2cdf0ee12aaf
https://hg.mozilla.org/mozilla-central/rev/496e56e95295