1083359 - Allow C++ code to provide an async stack when calling a JS function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Paolo Amadini]

For Promises and Tasks, when calling "then" callbacks or Task generators, invocations of "new Error().stack" inside the callback should display a modified stack provided by the Promise or Task implementation.

Currently, a small stack from Promise.jsm or Task.jsm is displayed, and no stack is displayed at all in case of DOM Promise callbacks.

A marker in the stack trace should show where the stack is rewritten and the cause, for example "Promise*" indicating that the rest of the stack is related to where the promise was created. For example, this test added to Promise.jsm xpcshell tests...

tests.push(
  make_promise_test(function test_promise_stack() {
    return Promise.resolve().then(value => { // Line 1105
      return (() => { // Line 1106
        return new Promise(resolve => do_execute_soon(resolve)); // Line 1107
      })();
    }).then(() => {
      dump(new Error().stack); // Line 1110
    });
  })
);

...would print (paths simplified):

test_promise_stack/<@test_Promise.js:1110:12
Promise*test_promise_stack/</<@test_Promise.js:1107:16
test_promise_stack/<@test_Promise.js:1106:1
Promise*test_promise_stack@test_Promise.js:1105:12
runtest@test_Promise.js:43:20
loop@test_Promise.js:33:18
run_promise_tests/loop/<@test_Promise.js:30:9
do_execute_soon/<[e-mail]:570:9
[e-mail]:191:5
[e-mail]:405:5
@-e:1:1

Instead of the current:

test_promise_stack/<@test_Promise.js:1110:12
[e-mail]:912:23
[e-mail]:789:7
[e-mail]:191:5
[e-mail]:405:5
@-e:1:1

Comment 1

[Alex Vincent [:WeirdAl]]

What API would you propose for this?  Say, Function.prototype.applyWithStack(thisObj, argArray, stack) ?

Comment 2

[:Paolo Amadini]

(In reply to Alex Vincent [:WeirdAl] from comment #1)
> What API would you propose for this?  Say,
> Function.prototype.applyWithStack(thisObj, argArray, stack) ?

Yes, seems like what we need for the JS case. We may have an optional "cause" argument as well (for example, "Promise").

Comment 3

[:Paolo Amadini]

Attachment: Experiment

This is something I've been working on at the Promises Debugging work week.

It actually doesn't work, nor it is supposed to be the right way to do this, but I'm trying to create a proof of concept that can then be extended.

Comment 4

[Alex Vincent [:WeirdAl]]

A couple more thoughts:
* The stack trace might be reformed over the next couple of years over at ecmascript.org.  When it does, the matching code here will need updates like everything else.
* We really shouldn't expose any .applyWithStack() method to unprivileged JS code, I think.
* For privileged JS code, should we warn developers that the method is non-standard and could change in the future?  (This would be a warning through devmo, not through source code.)

Comment 5

[Jim Blandy :jimb]

I had a little difficulty reading the example, so I stretched it out like so. Is this the sort of stack you're looking for? (Constructed by hand, so possibly contains typos.)

--- stretched.js
tests.push(make_promise_test(test_promise_stack));

function test_promise_stack() {
  return Promise.resolve("firstValue")                          // line 4
    .then(firstThen)
    .then(secondThen);
}

function firstThen(firstValue) {
  firstThenInner();                                             // line 10

  function firstThenInner() {
    return new Promise(function secondExecutor(resolve) {       // line 13
      do_execute_soon(() => resolve("secondValue"));
    });
  }
}

function secondThen(secondValue) {
  dump(new Error().stack);                                      // line 20
}


// [e-mail]:20
// Promise*[e-mail]:13
// [e-mail]:10
// Promise*[e-mail]:4
// runtest@test_Promise.js:43:20
// loop@test_Promise.js:33:18
// run_promise_tests/loop/<@test_Promise.js:30:9
// do_execute_soon/<[e-mail]:570:9
// [e-mail]:191:5
// [e-mail]:405:5
// @-e:1:1

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8506402 [details] [diff] [review]
Experiment

Review of attachment 8506402 [details] [diff] [review]:
-----------------------------------------------------------------

We probably want another slot in SavedStack to store the "mark" indicating where the two stacks were artificially joined together.

::: js/src/jsapi.cpp
@@ +5119,5 @@
> +        cx->setFakeStack(nullptr);
> +        return;
> +    }
> +
> +    SavedFrame *fakeStack = &stackp->as<SavedFrame>();

JSObject::as always returns a non-null ptr. It asserts if 'this' has the wrong class. So you can just say:

cx->setFakeStack(&stackp->as<SavedFrame>());

::: js/src/jsapi.h
@@ +4104,5 @@
>  extern JS_PUBLIC_API(void)
>  JS_RestoreFrameChain(JSContext *cx);
>  
> +extern JS_PUBLIC_API(void)
> +JS_SetFakeStack(JSContext *cx, JSObject *stackp);

It would be better to have a MOZ_STACK_CLASS class whose constructor establishes the fake stack, and whose destructor restores the prior fake stack, if any. That way, we could be sure the fake stack will always be cleaned up.

::: js/src/vm/SavedStacks.cpp
@@ +532,5 @@
> +            if (parentFrame.get() && parentFrame.get() != fakeStack) {
> +                break;
> +            }
> +            parentFrame.set(fakeStack);
> +        }

This doesn't seem right. When you walk off the old end of an Activation that has a fake stack, you want to substitute that as the value of the parent frame.

::: js/src/vm/Stack-inl.h
@@ +757,4 @@
>      kind_(kind)
>  {
>      cx->perThreadData->activation_ = this;
> +    cx->perThreadData->fakeStack_ = nullptr;

In the Activation destructor, you should put the value back in the perThreadData, in case other Activations are pushed before we remove it.

::: js/src/vm/Stack.h
@@ +1125,5 @@
>      // data structures instead.
>      size_t hideScriptedCallerCount_;
>  
> +    // Need better name
> +    SavedFrame* fakeStack_;

You need to store this pointer in a way that the GC can find it. Perhaps RootedObject? Rooted<SavedFrame*>?

Comment 7

[:Paolo Amadini]

Attachment: Initial patch

Okay, I'm back after a break and will try to move this forward. There are actually several questions and issues highlighted by this preliminary patch, I'll try to keep one per paragraph in this comment.

The Promise.cpp changes are an example of a possible use of the new AutoSetAsyncStackForNewActivations class from C++, but they should probably not be included as part of this bug. The exact stacks expected for Promises, like those illustrated in comment 0 and comment 5, are worth a separate discussion.

However, the Promise change is the only thing that makes this code visible to JavaScript. This in fact can be tested in xpcshell tests, since they use Components.stack, but not in other test suites, maybe because they use "new Error()" as they don't have access to Components. This makes me think that bug 1038238 would be important to fix to make this feature useful.

We may want to file a separate bug to implement Function.applyWithStack. This will make the feature unconditionally available to privileged JavaScript, and may help with regression testing.

This patch also stops walking the stack if the async stack was saved from a different compartment than the one requesting the stack. This is probably quite limiting, but it's not clear to me how to handle principal filtering correctly. Is bug 1036527 required for this to work across different compartments? Is there a simpler solution? Do we need to implement cross-compartment wrappers for SavedFrame? Do we need the public API from bug 1031152 to make this happen?

As mentioned in the review, we need a marker in SavedFrame to indicate where an async stack starts. Given how the stack is presented, I think this marker should be saved on the youngest frame of the async stack rather than the oldest of the normally captured stack. This can simply be the descriptive string that appears before the "*" character in the examples, like "Promise", or null for normal frames. Are SavedFrame instances immutable or should be considered such? If so, we should definitely set the marker at CaptureStack time. We actually may want to do that anyways. But then we may need an equivalent of CaptureStack with marker for privileged JavaScript, in order to save a stack to use with Function.applyWithStack later.

We should definitely limit the number of frames captured for async stacks too, as recursion in the general case can be infinite, for example a setTimeout called from its own callback. We can do this by explicitly iterating over SavedFrame instances of the async stack and building the FrameState lookup structures. This would be a separate function than the current one that builds the FrameState instances from the FrameIter.

We may want to save (or just compute every time) the depth of the SavedFrame chain, so that we don't have to iterate and rebuild it in case it's shorter than the remaining allowed depth. However, if we need to regenerate the SavedFrame instances in the current compartment, we may have to copy the data to the FrameState instances first anyways. I'm not sure whether this optimization just adds unneeded complexity. Should we do it, and if so, should we do it in a separate bug later?

Will devtools inherit the ability to see these async stacks? I ask because they might be using FrameIter directly, and it can't see async stacks. Does it still make sense for other objects (like Error) to have access to the FrameIter API at all, if they can't use it to see async stacks without reimplementing all the complex logic?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> but it's not clear to me how to handle principal filtering correctly.

At this point I don't understand the principal filtering setup in SavedStacks and how it's possibly expected to work...  Nick or Jim might know what the right thing here is, both for now and longer term once we have Xrays to stacks.

Comment 9

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

(In reply to Boris Zbarsky [:bz] from comment #8)
> > but it's not clear to me how to handle principal filtering correctly.
> 
> At this point I don't understand the principal filtering setup in
> SavedStacks and how it's possibly expected to work...  Nick or Jim might
> know what the right thing here is, both for now and longer term once we have
> Xrays to stacks.

I am implementing the xrays in bug 1117242, and then I think everything should be groovy in this regard.

(In reply to :Paolo Amadini from comment #7)
> Do we need to implement
> cross-compartment wrappers for SavedFrame?

SavedFrames belong to the compartment the cx was in when you saved the frame. If you are going to expose them to other compartments, then you will need to wrap them.

> Do we need the public API from
> bug 1031152 to make this happen?

I wouldn't think so, although it might be more verbose than you want it to be.

> Are SavedFrame instances immutable or
> should be considered such?

Absolutely. In fact, they get frozen to ensure this.

Comment 10

[Jim Blandy :jimb]

(In reply to :Paolo Amadini from comment #7)
> Will devtools inherit the ability to see these async stacks? I ask because
> they might be using FrameIter directly, and it can't see async stacks. Does
> it still make sense for other objects (like Error) to have access to the
> FrameIter API at all, if they can't use it to see async stacks without
> reimplementing all the complex logic?

To the extent that devtools sees the saved stacks at all, it will be able to see the additional frames you append. We don't use FrameIter directly; instead, we use Debugger.Frame for live frames, and the SavedFrame chains for saved frames.

Comment 11

[:Paolo Amadini]

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> I am implementing the xrays in bug 1117242

I don't have access to that bug. Do you know its timeline for landing, or do you think you can separate the Xrays part into a different bug?

> and then I think everything should be groovy in this regard.

Can you clarify? If we set the parentFrame to a wrapped SavedFrame from a different compartment, is it OK or do we need to implement principal filtering first?

Comment 12

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

(In reply to :Paolo Amadini from comment #11)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> > I am implementing the xrays in bug 1117242
> 
> I don't have access to that bug. Do you know its timeline for landing, or do
> you think you can separate the Xrays part into a different bug?

Very Soon (tm).

I believe I have it working for the most part, but I'm hitting a seemingly unrelated bug in the test that I will need to fix and/or work around before it lands. CC'd you in the bug, so you should be able to view it now.

> If we set the parentFrame to a wrapped SavedFrame from a
> different compartment, is it OK or do we need to implement principal
> filtering first?

The principal filtering is implemented inside SavedFrame, you would need to either:

(1) Copy the SavedFrame stack, but using your new principals, or

(2) Wrap the parent with a cross compartment wrapper

You would do (1) if you wanted to view the parent stack as your child stack would see it. You would do (2) if you wanted to view the parent stack as the parent would see it.

Comment 13

[:Paolo Amadini]

Attachment: Remove FrameState

General GC rooting-related question illustrated by code.

Not sure whom to ask so putting a few people in the feedback field.

Why do we need to have separate FrameState and Lookup structures with the same information in the first place? Is there a fundamental reason why it can't be the same structure?

If not, what would be the simplest way of unifying the structures? As noted in the code, something looks incorrect as there's a lost call to some curiously named function, but I don't really understand this complex rooting machinery yet...

Maybe nowadays part of this can be done with easier templates?

Comment 14

[:Paolo Amadini]

Comment on attachment 8548965 [details] [diff] [review]
Remove FrameState

Review of attachment 8548965 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SavedStacks.cpp
@@ +92,2 @@
>    private:
> +    SavedFrame::AutoLookupRooter *ref;

Oh, and this used to be "SavedFrame::AutoLookupRooter &ref;" but I changed it as Vector requires a default and copy constructor. Maybe I shouldn't use a Vector in the first place?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8548965 [details] [diff] [review]
Remove FrameState

This is totally Nick/Jim territory.

Comment 16

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

Comment on attachment 8548965 [details] [diff] [review]
Remove FrameState

Review of attachment 8548965 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SavedStacks.cpp
@@ +92,2 @@
>    private:
> +    SavedFrame::AutoLookupRooter *ref;

Its nice when we can use references in classes, since it gives certain lifetime guarantees, but we can't always live within the restrictions imposed, and it looks like this evolution of the code means we have reached that point.

@@ +92,5 @@
>    private:
> +    SavedFrame::AutoLookupRooter *ref;
> +};
> +
> +class SavedFrame::AutoLookupVector : public JS::CustomAutoRooter {

Let's make this MOZ_STACK_CLASS

@@ +542,1 @@
>      // objects instead of a vector of FrameIter objects.

I think it would be nice to explicitly mention that the `SavedFrame::Lookup` objects are partially initialized during the first pass. How does this sound?

"""
To avoid making many copies of FrameIter (whose copy constructor is relatively slow), we use a vector of `SavedFrame::Lookup` objects, which only contain the `FrameIter` data we need. The `SavedFrame::Lookup` objects are partially initialized with everything except its parent pointer on the first pass, and then we fill in the parent pointers as we return in the second pass.
"""

@@ +582,5 @@
> +          location->column,
> +          iter.isNonEvalFunctionFrame() ? iter.functionDisplayAtom() : nullptr,
> +          nullptr,
> +          iter.compartment()->principals
> +        );

Maybe add an `AutoLookupRooter` constructor that takes a `FrameIter`?

@@ +607,3 @@
>      // actual SavedFrame instances.
> +    for (size_t i = stackChain->length(); i != 0; i--) {
> +        // This won't call MarkObjectUnbarriered. Is this bad?

These changes look good, but I'm not sure I understand this question. MarkObjectUnbarriered is only used inside SavedFrame::Lookup::trace, which with this patch is still called for each SavedFrame::Lookup inside SavedFrame::AutoLookupVector::trace. Those functions should only be called if a GC is triggered.

Comment 17

[:Paolo Amadini]

Comment on attachment 8548965 [details] [diff] [review]
Remove FrameState

(In reply to Nick Fitzgerald [:fitzgen] from comment #16)
> Maybe add an `AutoLookupRooter` constructor that takes a `FrameIter`?

I don't think it's worth the duplication, it would only be used once, and having the source of the values visible in the caller makes the code easier to read to me.

> Those functions should only be called if a GC is triggered.

Ah cool, my doubt was about them being called before the assignment and their result used after the assignment, but it seems it's not the case.

I've filed bug 1121973 to land this patch separately. Thanks for the review!

Comment 18

[:Paolo Amadini]

Attachment: Part 1 - Add async stack support

Rebased on top of bug 1121973.

Comment 19

[:Paolo Amadini]

Attachment: Part 4 - Limit the depth of async stacks

Still missing the new SavedFrame field, and possibly implementing Function.applyWithStack now to allow some automated testing.

Comment 20

[:Paolo Amadini]

Attachment: Part 3 - Add the asyncCause property to the native SavedFrame object

Comment 21

[:Paolo Amadini]

Attachment: Part 4 - Set the asyncCause property when capturing stacks

Comment 22

[:Paolo Amadini]

Attachment: Part 5 - Add the asyncCause property to nsIStackFrame

Comment 23

[:Paolo Amadini]

Attachment: Part 6 - Allow JS code to provide an async stack when calling a function

Comment 24

[:Paolo Amadini]

This is the complete patch queue for the functionality. It probably needs a quick first pass to see if there is anything that needs to be changed with the API or implementation, and to check the possible issues or assertions triggered when this is used in different compartments, that is a case I didn't test.

For the JavaScript part, in the end I've kept everything in the Components object, since a stack captured by Components.stack is required anyways. Example:


let myCallback = function () {
  // Components.stack here would see "callbackFn" plus the outer stack
}.bind(myThisObj, myArgument);

let asyncStack = Components.stack;

(function notVisibleInStack() {
  Components.utils.callFunctionWithStack(myCallback, asyncStack, "AsyncCauseExample");
})();


The asyncCause is set at the time of invocation rather than capture, because it may often be set on an intermediate frame (that is, using Components.stack.caller rather than Components.stack). We could expose a function to get a stack frame with a given async cause once, if we want to do that only when the stack is saved instead of every time the callback is invoked.

With this exposed to JavaScript, I can now write automated xpcshell tests. However, I'm not sure where they should reside. This is a JavaScript engine feature, but testing using xpcshell would rely on XPConnect. Not sure is there is a better way of doing this.

Comment 25

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

(In reply to :Paolo Amadini from comment #24)
> With this exposed to JavaScript, I can now write automated xpcshell tests.
> However, I'm not sure where they should reside. This is a JavaScript engine
> feature, but testing using xpcshell would rely on XPConnect. Not sure is
> there is a better way of doing this.

js/xpconnect/test/unit might be a good place.

Comment 26

[:Paolo Amadini]

Nick, maybe you'd like to do a first very quick feedback pass on the patch queue to see if there is something clearly to be changed, while I'm waiting for the reviews from Jim?

Comment 27

[Boris Zbarsky [:bzbarsky]]

I have a really hard time believing the assertSameCompartment(cx, this) in SavedStacks::changeAsyncCause doesn't fail all over the place with the patches here....  I see nothing offhand that would make that be the case.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Also, on the topic of compartments, asyncStackForNewActivations_ should document explicitly what it assumes or doesn't about which compartment it's in.

Comment 29

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

Comment on attachment 8551839 [details] [diff] [review]
Part 6 - Allow JS code to provide an async stack when calling a function

Review of attachment 8551839 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +6190,5 @@
> +{
> +    JSCompartment *compartment = cx->compartment();
> +    MOZ_ASSERT(compartment);
> +
> +    JSAtom *asyncCauseAtom = nullptr;

If the frame needs to be rooted below, then this JSAtom probably needs to be rooted as well, right?

@@ +6197,5 @@
> +        if (!asyncCauseAtom)
> +            return false;
> +    }
> +
> +    Rooted<SavedFrame *> frame(cx, &stackp->as<SavedFrame>());

You can include vm/SavedStacks.h and do RootedSavedFrame, which is a little nicer IMO.

::: js/src/jsapi.h
@@ +5473,5 @@
> + * stack frame with the specified |asyncCause| and the same properties.
> + */
> +extern JS_PUBLIC_API(bool)
> +ChangeStackAsyncCause(JSContext *cx, MutableHandleObject stackp,
> +                      const char *asyncCause);

This is mutating the given SavedFrame stack, but uses a MutableHandle, meaning a different SavedFrame is returned?

SavedFrame instances are supposed to be immutable and frozen, so I am assuming that this actually copies the SavedFrame chain but with a new async cause and sets stackp to that? It would be good to document this stuff here, because it isn't clear to me from looking at this API and its documentation.

Comment 30

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

Comment on attachment 8549682 [details] [diff] [review]
Part 1 - Add async stack support

Review of attachment 8549682 [details] [diff] [review]:
-----------------------------------------------------------------

How does the work in this bug relate to bug 971673?

::: js/src/jsapi.cpp
@@ +4687,5 @@
>      CHECK_REQUEST(cx);
>      cx->restoreFrameChain();
>  }
>  
> +JS::AutoSetAsyncStackForNewActivations::AutoSetAsyncStackForNewActivations(

Don't we have an existing RAII class for entering JS scripts? I thought it was called AutoEntryScript, but maybe that is from the other bug and never landed? Or it got renamed?

If we already have one, I think it makes sense to make it take a stack parameter, so that we can just expand existing callsites rather than force new RAII class usage. It seems like an easier migration path to me, but maybe others feel differently.

::: js/src/vm/SavedStacks.cpp
@@ +568,5 @@
> +                // While walking from the youngest to the oldest frame, we found
> +                // an activation that has an async stack set. We will use the
> +                // youngest frame of the async stack as the parent of the oldest
> +                // frame of this activation. We still need to iterate over other
> +                // frames in this activation before reaching the oldest frame.

I'd prefer not to conflate a frame's parent with an "async parent" if possible. Could we make it a separate property? When stringifying stacks, I think we should make the boundary between async parents clear, as side effects from other tasks/events/etc could happen at those locations, but not between synchronous parent frames.

@@ +616,5 @@
>              maxFrameCount--;
>          }
>      }
>  
> +    // Set the async stack if the compartments match.

Why? The approach we've been taking is to record as much data as possible and filter how much we expose to the caller based on their principals. Why shouldn't chrome privileged callers get to view cross compartment async stacks? Is there a reason we can't just wrap it?

Comment 31

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

Comment on attachment 8549686 [details] [diff] [review]
Part 4 - Limit the depth of async stacks

Review of attachment 8549686 [details] [diff] [review]:
-----------------------------------------------------------------

I think the primary purpose of the maxFrameCount is to avoid walking the whole stack because of the perf cost, but in this case we are walking the async saved stack _and_ copying each frame to maintain this maxFrameCount! Wouldn't the caller just prefer to get extra information for free, rather than paying for less?

If your concern is displaying too much data, we could limit the number of frames on stringification or callers could do it themselves.

Comment 32

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

Comment on attachment 8551836 [details] [diff] [review]
Part 3 - Add the asyncCause property to the native SavedFrame object

Review of attachment 8551836 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/doc/SavedFrame/SavedFrame.md
@@ +24,5 @@
> +`asyncCause`
> +:   If this stack frame indicates the point where an asynchronous call was
> +    started, for example a `Promise` construction, this is set to an arbitrary
> +    string representing the type of call, for example "Promise". In all other
> +    cases, this is `null`.

Is this only non-null on youngest frames? Oldest? Non-null on every frame in the stack if non-null on one of them?

I'd suggest only youngest frame, that way we maximize sharing of older frames. I'd suggest the same for linking to async parent stacks (if we make it a separate property, like I suggested earlier), for the same reason.

Comment 33

[:Paolo Amadini]

Thanks for the great feedback!

(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> How does the work in this bug relate to bug 971673?

Not sure, will have to look into this.

> I'd prefer not to conflate a frame's parent with an "async parent" if
> possible. Could we make it a separate property?

I though about this in the early stages. The fact is that we have various instances of manual stack walking code around the tree. Using the existing "parent" makes it so that the extended stack is available to them with no modification (they may just miss the asyncCause if they read the individual properties instead of stringifying each frame).

Using the same "parent" property also makes things much simpler for us when walking the stack through async invocations in this same patch.

> When stringifying stacks, I
> think we should make the boundary between async parents clear, as side
> effects from other tasks/events/etc could happen at those locations, but not
> between synchronous parent frames.

This is visible, for example, as "Promise*" in async frames. We can write something more visible like "+++Promise+++". I wouldn't actually add a separate line for the boundary, it may be overkill for recursive/iterative invocations.

> > +    // Set the async stack if the compartments match.
> 
> Why? The approach we've been taking is to record as much data as possible
> and filter how much we expose to the caller based on their principals. Why
> shouldn't chrome privileged callers get to view cross compartment async
> stacks? Is there a reason we can't just wrap it?

Correct, this needs to change.

(In reply to Nick Fitzgerald [:fitzgen] from comment #31)
> I think the primary purpose of the maxFrameCount is to avoid walking the
> whole stack because of the perf cost, but in this case we are walking the
> async saved stack _and_ copying each frame to maintain this maxFrameCount!

Consider the case of a setTimeout invoking itself at the end (often seen in the wild instead of setInterval) or a Promise-based queue where we always add a "then" on top of the latest promise.

We need to cap the stack to avoid infinite memory usage.

But Jim made a great point in that, if the recursion pattern is the same, we end up reusing the same SavedFrame instances in the end (i.e. two stacks of the same length that look exactly the same will always be made of the exact same SavedFrame instances).

Comment 34

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

(In reply to :Paolo Amadini from comment #33)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #31)
> > I think the primary purpose of the maxFrameCount is to avoid walking the
> > whole stack because of the perf cost, but in this case we are walking the
> > async saved stack _and_ copying each frame to maintain this maxFrameCount!
> 
> Consider the case of a setTimeout invoking itself at the end (often seen in
> the wild instead of setInterval) or a Promise-based queue where we always
> add a "then" on top of the latest promise.
> 
> We need to cap the stack to avoid infinite memory usage.
> 
> But Jim made a great point in that, if the recursion pattern is the same, we
> end up reusing the same SavedFrame instances in the end (i.e. two stacks of
> the same length that look exactly the same will always be made of the exact
> same SavedFrame instances).

This last point only holds if we have a separate async parent property, right? Then we would have something like this:

    Old
     ^
     |
    Mid
     ^
     |
   /-+------\
   |        |
 sync ---> async

Without this, we would have a cyclical SavedFrame stack, which is definitely not right (and impossible to create because of the SavedFrame immutability):

     /------\
     |      |
    Old     |
     ^      |
     |      |
    Mid     |
     ^      |
     |      |
    Young <-/

Comment 35

[:Paolo Amadini]

Hm, let me try to summarize Jim's reasoning quickly.

a => b();
b => asyncCall(c);
c => { captureStack(); a(); }

Let's say we limit the length to 7 frames. Here are the stack traces we would get, youngest last, where uppercase letter means the asyncCause property is set.

1. aBc
2. aBcaBc
3. caBcaBc
4. caBcaBc

Stack number 2 shares the first three SavedFrame instances with stack number 1.

Stack number 3 is limited and the root is different, so it doesn't share any SavedFrame instance with number 1 or 2.

Stack number 4, however, shares all of the SavedFrame instances with stack number 3, because it is exactly the same stack.

Actually, in this example if we limited the stack to 6 frames instead of 7, then stacks number 2 and 3 would reuse the same instances too. This makes me think that 120 rather than 100 can be a better default limit to support recursions up to 8 frames (excluding 7) more efficiently:

120 = 5*3*2*2*2
8   = 2*2*2
6   = 3*2
5   = 5
4   = 2*2
3   = 3
2   = 2

Comment 36

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #27)
> I have a really hard time believing the assertSameCompartment(cx, this) in
> SavedStacks::changeAsyncCause doesn't fail all over the place with the
> patches here....  I see nothing offhand that would make that be the case.

Why? It seems to work for me. The SavedStacks object is always the one from the compartment of cx, in the single call site for changeAsyncCause.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> The SavedStacks object is always the one from the compartment of cx

Why exactly?

The one callsite of changeAsyncCause is JS::ChangeStackAsyncCause, yes?

That's called from nsXPCComponents_Utils::CallFunctionWithStack.

This gets the cx from whatever called it and the SavedFrame object from the nsIStackFrame.  There's absolutely no reason that the compartments of the two would match.

For example, if the nsIStackFrame comes from a content DOMException, its SavedFrame would be in the content compartment, but CallFunctionWithStack is obviously being called in chrome code, yes?

Comment 38

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #37)
> This gets the cx from whatever called it and the SavedFrame object from the
> nsIStackFrame.  There's absolutely no reason that the compartments of the
> two would match.

Your original comment was about "assertSameCompartment(cx, this);" that actually checks the SavedStacks object rather than the SavedFrame, and those compartments I believe would match.

But you're saying that the SavedFrame may be from a different compartment? Since we're creating the Lookup object from the raw C++ getters, we shouldn't hit assertions, except maybe getFunctionDisplayName? Is JSAtom a per-compartment object?

Anyways you're right that this doesn't seem like the correct way of doing this even if it works. How should we handle reading the properties of SavedFrames from other compartments to create Lookup objects here, and when walking through the async chain? We want to copy the raw chain with no principal checks. Quick code example?

Comment 39

[Boris Zbarsky [:bzbarsky]]

> that actually checks the SavedStacks object rather than the SavedFrame

Oh, of course.  You're totally right that this assert should be fine; I got confused about what "this" was.

> Is JSAtom a per-compartment object?

No, atoms are runtime-wide and live in the atom compartment.

I _think_ what you have here is actually correct.  We just need documentation about how the cx and the frame need not be in the same compartment, right?

The only remaining caveat is what compartment we want to be creating the return value of ChangeStackAsyncCause in.  Maybe we should in fact be entering the compartment of the original stackp we had passed in so that the new stack is created in the same compartment....

Comment 40

[Jim Blandy :jimb]

Comment on attachment 8549682 [details] [diff] [review]
Part 1 - Add async stack support

Review of attachment 8549682 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not able to review Promise.cpp and Promise.h. Adding bz as reviewer.

Comment 41

[Jim Blandy :jimb]

Comment on attachment 8549682 [details] [diff] [review]
Part 1 - Add async stack support

Review of attachment 8549682 [details] [diff] [review]:
-----------------------------------------------------------------

Still reviewing, but I'll post my comments from today:

This needs to have test coverage in js/src/jit-test. You need to define a function in js/src/builtin/TestingFunctions.cpp that is something like:

    withAsyncStack(stack, fun)

    Call |fun|, establishing the saved stack |stack| as the asynchronous stack
    responsible for the call. Pass no arguments to |fun|, and pass |undefined| as
    its |this| value. Return the value |fun| returns (or propagate the exception it
    throws).

For example, this function is doing something similar: https://hg.mozilla.org/mozilla-central/file/650863f6987b/js/src/shell/js.cpp#l4660

Then, add code that exercises it in js/src/jit-test/tests/saved-stacks.

::: dom/promise/Promise.cpp
@@ +82,5 @@
> +      JS::AutoSetAsyncStackForNewActivations sas(cx, asyncStack);
> +      mCallback->Call(cx, value);
> +    } else {
> +      mCallback->Call(cx, value);
> +    }

You could use a Maybe<JS::AutoSetAsyncStackForNewActivations> here, emplace it conditionally, and avoid duplicating the callback invocation. For example, that's what we do with |ac| here:

https://hg.mozilla.org/mozilla-central/file/650863f6987b/js/src/shell/js.cpp#l2548

::: js/src/jsapi.h
@@ +4243,5 @@
> + * async stack instead of the current stack.
> + *
> + * Capturing the stack before a new Activation is created will not be affected.
> + */
> +class MOZ_STACK_CLASS JS_PUBLIC_API(AutoSetAsyncStackForNewActivations)

"Activations" are an internal implementation detail of SpiderMonkey, not part of the public API. The class name and docs should refer to "calls" instead: AutoSetAsyncStackForNewCalls, "... picked up by any new calls performed until..."

@@ +4246,5 @@
> + */
> +class MOZ_STACK_CLASS JS_PUBLIC_API(AutoSetAsyncStackForNewActivations)
> +{
> +    JSContext *cx_;
> +    JSObject *oldSavedFrame_;

This needs to be |RootedObject oldSavedFrame;|. There's nothing preventing a GC from relocating the object between the time we construct this and the time we need to restore it.

Also, in SpiderMonkey, private members should not have names ending in _ unless they have an accessor function by the same name. So these can just be 'cx' and 'oldSavedFrame'.

::: js/src/vm/Runtime.h
@@ +559,5 @@
> +     *
> +     * New activations will reset this to nullptr on construction after getting
> +     * the current value, and will restore the previous value on destruction.
> +     */
> +    js::SavedFrame *asyncStackForNewActivations_;

Move this member after asmJSActivationStack_; the current activation is the most interesting thing being saved here, so we shouldn't bury it under the collection of parameters we might save in it.

::: js/src/vm/SavedStacks.cpp
@@ +568,5 @@
> +                // While walking from the youngest to the oldest frame, we found
> +                // an activation that has an async stack set. We will use the
> +                // youngest frame of the async stack as the parent of the oldest
> +                // frame of this activation. We still need to iterate over other
> +                // frames in this activation before reaching the oldest frame.

I agree with Nick's comment here. The saved stack should be additional information, not a replacement. It's fine to change SavedFrame::toStringMethod to use the saved stack when one is present, but we should retain the true parent as well.

@@ +616,5 @@
>              maxFrameCount--;
>          }
>      }
>  
> +    // Set the async stack if the compartments match.

I agree with Nick's comment here.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8551838 [details] [diff] [review]
Part 5 - Add the asyncCause property to nsIStackFrame

You need to rev the IID here.

Does the "asyncCause" property do principal-based sanitization?  I assume it does....

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8551839 [details] [diff] [review]
Part 6 - Allow JS code to provide an async stack when calling a function

DOM code uses JS::MutableHandle<JS::Value>, not the typedef.

Need to rev the nsIXPCComponents_Utils IID.

Why the weird "v" prefixes on the arguments to the new function?

Why reorder the existing methods?

JS::ChangeStackAsyncCause needs to document that its second arg need not be in the same compartment as cx.

Similar for AutoSetAsyncStackForNewActivations?  I haven't sorted through all the other patches to ensure that having that be in a random compartment is in fact OK.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8549682 [details] [diff] [review]
Part 1 - Add async stack support

>+++ b/dom/promise/Promise.cpp

Please use a Maybe for the AutoSetAsyncStackForNewActivations instead of duplicating the Call().

The AutoSetAsyncStackForNewActivations API needs some documentation around compartments.  In particular, the consumer in insertFrames does some sort of compartment check that is not at all obvious to me in terms of what it means for consumers of this API.  

The use of Rooted in a non-stack-class really deserves some comments too.

r=me for the Promise parts conditional on the documentation about compartments making sense with the way the API is used in Promise there.

Comment 45

[Jim Blandy :jimb]

Comment on attachment 8549682 [details] [diff] [review]
Part 1 - Add async stack support

Review of attachment 8549682 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good in general. However, since I've asked for some major changes I'd like to see this again before approval.

::: js/src/jsapi.cpp
@@ +4687,5 @@
>      CHECK_REQUEST(cx);
>      cx->restoreFrameChain();
>  }
>  
> +JS::AutoSetAsyncStackForNewActivations::AutoSetAsyncStackForNewActivations(

I think we're going to need to tackle this question eventually, but for now this is okay.

::: js/src/jsapi.h
@@ +4253,5 @@
> +    // The savedFrame parameter cannot be nullptr by design, because it would
> +    // be ambiguous whether that would clear any scheduled async stack and make
> +    // the normal stack reappear in the new activations, or just keep the async
> +    // stack already scheduled for the new activations, if any.
> +    AutoSetAsyncStackForNewActivations(JSContext *cx, JSObject *savedFrame);

If at all possible, |savedFrame| should be a JS::HandleObject.

::: js/src/vm/Runtime.h
@@ +559,5 @@
> +     *
> +     * New activations will reset this to nullptr on construction after getting
> +     * the current value, and will restore the previous value on destruction.
> +     */
> +    js::SavedFrame *asyncStackForNewActivations_;

This field is never marked by the GC. You'll need to call MarkObjectRoot on it in js::gc::GCRuntime::markRuntime, probably right after the call to MarkPersistentRootedChains.

::: js/src/vm/SavedStacks.cpp
@@ +568,5 @@
> +                // While walking from the youngest to the oldest frame, we found
> +                // an activation that has an async stack set. We will use the
> +                // youngest frame of the async stack as the parent of the oldest
> +                // frame of this activation. We still need to iterate over other
> +                // frames in this activation before reaching the oldest frame.

Other comments aside, this is a nice way to manage picking up the async parent at the right point. I like it.

@@ +573,5 @@
> +                asyncActivation = activation;
> +            }
> +        } else if (asyncActivation != activation) {
> +            // We found an async stack in the previous activation, and we
> +            // walked past the oldest frame of that activation, we're done.

It's probably not entirely clear from prior comments, but here's how I would like this to behave:

- First, let's add a new slot to SavedFrame, JSSLOT_ASYNC_PARENT. This will want its own accessor, like the other user-visible slots, so we'll have SavedFrame::asyncParent, SavedFrame::getAsyncParent, an entry in SavedFrame::protoAccessors, and a clause in SavedFrame::initFromLookup.

- Then, we have a corresponding member of SavedFrame::Lookup, with all the associated hashing and comparison.

In other words, the async parent becomes just another property that any SavedFrame might or might not have.

Finally, we'd change SavedFrame::toStringMethod to follow the async parent, if present, instead of the normal parent.

(The cleanup you did in bug 1121973 is really very nice, so perhaps I'm going into more detail than you need here; if so, apologies. Given how late I am in this review, I thought I should do everything I could to minimize iteration.)

Comment 46

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #45)
> I think we're going to need to tackle this question eventually, but for now
> this is okay.

Splinter didn't make it clear that this was in reply to Nick's comment 30:

> Don't we have an existing RAII class for entering JS scripts? I thought it was
> called AutoEntryScript, but maybe that is from the other bug and never landed?
> Or it got renamed?

Comment 47

[Jim Blandy :jimb]

Comment on attachment 8551836 [details] [diff] [review]
Part 3 - Add the asyncCause property to the native SavedFrame object

Review of attachment 8551836 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. I see now that you've already gone through the process of adding a slot to SavedFrame.

Again, this should have tests in js/src/jit-test; the 'withAsyncStack' testing function I suggested in comment 41 could take a 'cause' argument, too.

::: js/src/vm/SavedStacks.cpp
@@ +428,2 @@
>          RootedAtom name(cx, frame->getFunctionDisplayName());
> +        if ((asyncCause && !sb.append(asyncCause) && !sb.append('*'))

This should be:

(asyncCause && (!sb.append(asyncCause) || !sb.append('*')))

Comment 48

[Jim Blandy :jimb]

Comment on attachment 8551837 [details] [diff] [review]
Part 4 - Set the asyncCause property when capturing stacks

Review of attachment 8551837 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with comment addressed.

::: js/src/jsapi.cpp
@@ +6169,5 @@
>  {
>      JSCompartment *compartment = cx->compartment();
>      MOZ_ASSERT(compartment);
> +
> +    JSAtom *asyncCauseAtom = nullptr;

This should be a RootedAtom, and it should be passed all the way in to insertFrames as a HandleAtom.

Comment 49

[Jim Blandy :jimb]

Comment on attachment 8551838 [details] [diff] [review]
Part 5 - Add the asyncCause property to nsIStackFrame

Review of attachment 8551838 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not able to approve changes to this code. Passing review to bz.

Comment 50

[Jim Blandy :jimb]

Comment on attachment 8551838 [details] [diff] [review]
Part 5 - Add the asyncCause property to nsIStackFrame

Review of attachment 8551838 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Exceptions.cpp
@@ +462,5 @@
> +    JSAutoCompartment ac(cx, stack);
> +    JS::Rooted<JS::Value> asyncCauseVal(cx);
> +    // asyncCause can be null
> +    if (!JS_GetProperty(cx, stack, "asyncCause", &asyncCauseVal) ||
> +        (!asyncCauseVal.isString() && !asyncCauseVal.isNull())) {

I seem to recall bz asking for a better C++ API for dealing with SavedFrames. Is this still the best we can do? (I don't see anything better...)

Comment 51

[Jim Blandy :jimb]

Comment on attachment 8551839 [details] [diff] [review]
Part 6 - Allow JS code to provide an async stack when calling a function

Review of attachment 8551839 [details] [diff] [review]:
-----------------------------------------------------------------

Flagging bz for the changes outside js/src.

js/src portions  looks good, with some comments; r=me with those addressed.

::: js/src/jsapi.cpp
@@ +6190,5 @@
> +{
> +    JSCompartment *compartment = cx->compartment();
> +    MOZ_ASSERT(compartment);
> +
> +    JSAtom *asyncCauseAtom = nullptr;

Yes, this should definitely be a RootedAtom.

::: js/src/jsapi.h
@@ +5469,5 @@
>                      const char *asyncCause = nullptr);
>  
> +/*
> + * Change the provided stack frame reference to point to a possibly different
> + * stack frame with the specified |asyncCause| and the same properties.

Agree with Nick about the surprising use of |stackp|. Honestly, I think this should return a JSObject *, or nullptr. It looks like that would suit your call site better anyway.

::: js/src/vm/SavedStacks.cpp
@@ +487,5 @@
> +      frame->getFunctionDisplayName(),
> +      asyncCause,
> +      frame->getParent(),
> +      frame->getPrincipals()
> +    );

If Lookup had a constructor that accepted a HandleSavedFrame and initialized itself from that frame's fields (explicit, naturally), then this code, and the code in SavedStacks::sweep, could be nicer. (Just mutate lookup.asyncCause after construction.)

::: js/src/vm/SavedStacks.h
@@ +125,5 @@
>      bool     saveCurrentStack(JSContext *cx, MutableHandleSavedFrame frame,
>                                unsigned maxFrameCount = 0,
>                                JSAtom *asyncCause = nullptr);
> +    bool     changeAsyncCause(JSContext *cx, MutableHandleSavedFrame frame,
> +                              JSAtom *asyncCause = nullptr);

The cause should be passed as a HandleAtom. Also, there's no reason to default to nullptr here, is there?

Comment 52

[Jim Blandy :jimb]

Comment on attachment 8549686 [details] [diff] [review]
Part 4 - Limit the depth of async stacks

Review of attachment 8549686 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see this rebased on the revised part 1. Clearing review until then.

Comment 53

[Boris Zbarsky [:bzbarsky]]

> Finally, we'd change SavedFrame::toStringMethod to follow the async parent, if present,
> instead of the normal parent.

That would mean that things that "manually" walk the stack (e.g. via nsIStackFrame's GetCaller and getting properties off the objects; c.f. console's StackFrameToStackEntry bit) will not end up picking up the async parent bits, no?  Or do we want to change nsIStackFrame to follow the async parent in this case as well?

Comment 54

[Boris Zbarsky [:bzbarsky]]

> I seem to recall bz asking for a better C++ API for dealing with SavedFrames. Is this
> still the best we can do? 

Yep.  See bug 1031152.

Comment 55

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #53)
> That would mean that things that "manually" walk the stack
> will not end up picking up the async
> parent bits, no?  Or do we want to change nsIStackFrame to follow the async
> parent in this case as well?

I think we have these options:
1) Store and retrieve asyncParent independently from the normal parent.
2) Store asyncParent independently but retrieve it instead of the parent when present.
3) Not store asyncParent independently.

I think option (2) wouldn't make a lot of sense if implemented from the start, as then we could do (3) instead.

Option (1) over option (3) has the advantage that it has the lowest compatibility impact - code that expects truncated stacks would still see truncated stacks. I would include toString in that group as well, to keep that expectation.

On the other hand it has the disadvantage that we would have to go to every place that walks the stack or copies stack data around, one by one, and ensure it picks and displays async stacks.

In the vast majority of cases, the sync parent will be null when the async parent is present. The only exception would probably happen when Cu.callFunctionWithStack is used from JavaScript. This will only be used by Promise.jsm (soon to be removed) and Task.jsm (that one day will be replaced by real async functions).

It also appears some stack structures like the one for the Console assume a sequence, and don't support a tree, they would need to drop the sync parent when an async one is present anyways.

So what is the use case for storing both? We could keep the compatibility expectation, if at all needed, by stopping at the first non-null asyncCause, and maybe place this behavior behind a pref just for "new Error().stack" called from content (that currently is on a separate code path anyways).

Storing asyncParent separately may be a good exercise for seeing how well we cope with trees during our deduplication (which I think we would do well, the algorithm is quite clever) but I don't see any practical advantage to it, just additional complexity and work for us.

Comment 56

[:Paolo Amadini]

(In reply to Jim Blandy :jimb from comment #45)
> (The cleanup you did in bug 1121973 is really very nice, so perhaps I'm
> going into more detail than you need here; if so, apologies. Given how late
> I am in this review, I thought I should do everything I could to minimize
> iteration.)

By the way, thanks for the thorough review. And I appreciate this level of detail.

Comment 57

[Boris Zbarsky [:bzbarsky]]

So here is a question.  Will the stack in the console want to indicate the sync/async boundary?  This is basically about console.stack(), iirc.

Comment 58

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #57)
> So here is a question.  Will the stack in the console want to indicate the
> sync/async boundary?  This is basically about console.stack(), iirc.

I think we would want to clearly demarcate this in the various places where we display stacks, like:

http://www.html5rocks.com/en/tutorials/developertools/async-call-stack/

That's what asyncCause is for.

In my opinion, having the demarcation in the UI isn't a blocker for seeing the extra stack frames there, in other words we can easily do this after this patch has landed. And, speaking of landing, I don't plan to land any consumer in this bug yet, the Promise parts will be moved to another bug.

Comment 59

[Boris Zbarsky [:bzbarsky]]

> In my opinion, having the demarcation in the UI isn't a blocker

Sure, but it affects the internal API.  If we want the demarcation that means that console.cpp will need to know about the place where the stack goes async.  And that means that we need API of some sort on nsIStackFrame.  Two plausible approaches:

1)  Separate GetCaller() and GetAsyncCaller() functions, with the latter used either in preference to GetCaller() or when GetCaller() returns null, whichever makes more sense.

2)  A single GetCaller() that will follow the async stack, but a way to indicate whether the caller was actually async.

Option 1 pretty much requires separate accessors on the JS side.  Option 2 may not as long as there is some other way to tell when we're going async, right?

It's all sounding like having separate accessors gives us maximum flexibility, at the cost of having to update all consumers which should be async stack aware.

Comment 60

[:Paolo Amadini]

(In reply to Boris Zbarsky [:bz] from comment #59)
> Option 2 may not as long as there is some other way to tell when we're going async,
> right?

Right, and that way would be asyncCause, which is set on the async parent. In fact we should probably ensure this is non-null and non-empty for async parents, which the current code doesn't.

> It's all sounding like having separate accessors gives us maximum
> flexibility, at the cost of having to update all consumers which should be
> async stack aware.

A flexibility that I think we don't need. But I guess you've figured out my opinion by now ;-)

Comment 61

[Jim Blandy :jimb]

(In reply to Boris Zbarsky [:bz] from comment #53)
> > Finally, we'd change SavedFrame::toStringMethod to follow the async parent, if present,
> > instead of the normal parent.
> 
> That would mean that things that "manually" walk the stack (e.g. via
> nsIStackFrame's GetCaller and getting properties off the objects; c.f.
> console's StackFrameToStackEntry bit) will not end up picking up the async
> parent bits, no?  Or do we want to change nsIStackFrame to follow the async
> parent in this case as well?

Yes, but isn't this okay? We need to mark those transitions specially anyway, with the "asyncCause" string. If we're doing that, we might as well have a sane API while we're at it.

Comment 62

[Boris Zbarsky [:bzbarsky]]

> Yes, but isn't this okay? 

Sure.  I'm not objecting to changes to toStringMethod; I'm pointing out that we need a bit more API on nsIStackFrame.

Comment 63

[Jim Blandy :jimb]

It seems to me that nsIStackFrame::caller should not give async parents to naïve stack walkers, because the result will be stack traces that did not, and perhaps could not, occur. If you're a naïf, you don't know how to handle sync parents, so you shouldn't be told about them.

Paolo's plan from the beginning has included the "async cause" markers, to make it clear where the non-call transitions occur. But walkers need to know those exist in order to include them.

It doesn't seem like it would be that hard to fix the stack-walkers in chrome code to know about async parents. Do we also need to decide how to expose them to content?

I guess the middle way would be for nsIStackFrame::caller to follow async parents if present, and for nsIStackFrame::name (that's the function name, right?) to furtively prepend the async cause marker to the function name, so that it gets included in the frame. That would of course still leave stack walkers that omit function names producing impossible listings, which feels to me like an indication that this isn't the best approach.

Comment 64

[:Paolo Amadini]

(In reply to Jim Blandy :jimb from comment #63)
> If you're a naïf, you don't know how to handle
> [a]sync parents, so you shouldn't be told about them.

Does the fact we shouldn't tell always follow? I think there are two cases here:
- Chrome code, xpcshell, console, debugger... I see nothing wrong in letting them see async frames, even if they do naïve stack walking.
- Content. There might be some stack rewriting code in JS libraries that is affected. I wouldn't let content see any async stack just yet.

> I guess the middle way would be for nsIStackFrame::caller to follow async
> parents if present, and for nsIStackFrame::name (that's the function name,
> right?) to furtively prepend the async cause marker to the function name

Prepending the asyncCause to the function name can simplify our lives, yes. That would mean we require very little changes to code using nsIStackFrame, which is good.

If we settle for this solution, I argue that we have no use case for storing the asyncParent separately on StackFrame. It's not used to generate "new Error().stack" now, and when it does, we can just stop at the first non-null asyncCause.

Comment 65

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #64)
> we can just stop at the first non-null asyncCause.

And it's a DRY API as well. Either this is null or it isn't on a stack frame. This and only this tells you whether you crossed a boundary.

Comment 66

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

(In reply to Boris Zbarsky [:bz] from comment #59)
> 1)  Separate GetCaller() and GetAsyncCaller() functions, with the latter
> used either in preference to GetCaller() or when GetCaller() returns null,
> whichever makes more sense.

This is my preference.

Call sites that don't know about async stacks won't accidentally provide stacks that didn't (or couldn't ever) exist.

We can modify stringification to handle and demarcate async causes. I think this should get the majority of stack consumers on the async stacks/causes bandwagon with relatively little effort.

Finally, the stack consumers that are using the accessors directly: we can incrementally migrate them to handle async causes based on priority and prevalence.

Comment 67

[:Paolo Amadini]

(In reply to Nick Fitzgerald [:fitzgen] from comment #66)
> Call sites that don't know about async stacks won't accidentally provide
> stacks that didn't (or couldn't ever) exist.

At one point, I was wondering why my patch wasn't working. It turned out that xpcshell used custom stack walking code, so I had to make this change:

--- a/testing/xpcshell/head.js
+++ b/testing/xpcshell/head.js
@@ -695,17 +695,19 @@ function _abort_failed_test() {
   throw Components.results.NS_ERROR_ABORT;
 }
 
 function _format_stack(stack) {
   let normalized;
   if (stack instanceof Components.interfaces.nsIStackFrame) {
     let frames = [];
     for (let frame = stack; frame; frame = frame.caller) {
-      frames.push(frame.filename + ":" + frame.name + ":" + frame.lineNumber);
+      let asyncPrefix = frame.asyncCause ? (frame.asyncCause + "*") : "";
+      frames.push(frame.filename + ":" + asyncPrefix + frame.name + ":" +
+                  frame.lineNumber);
     }
     normalized = frames.join("\n");
   } else {
     normalized = "" + stack;
   }
   return _Task.Debugging.generateReadableStack(normalized, "    ");
 }

My suggestion is to go the other way and, as Jim also pointed out, prepending the asyncCause to the function name. With "caller" returning the async parent. Less work for us.

Comment 68

[:Paolo Amadini]

We have a bunch of sensibly named "naive" stack walkers around:

http://mxr.mozilla.org/mozilla-central/search?string=frame.caller

Off the top of my head, I don't see a single one that wouldn't benefit from us giving them async frames and prepending the asyncCause to the function name.

Comment 69

[Boris Zbarsky [:bzbarsky]]

> It's not used to generate "new Error().stack" now,

Though note that it's used for DOMException.stack; we'd need to make sure we fixed that up.

Comment 70

[Jim Blandy :jimb]

(In reply to :Paolo Amadini from comment #67)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #66)
> > Call sites that don't know about async stacks won't accidentally provide
> > stacks that didn't (or couldn't ever) exist.
> 
> At one point, I was wondering why my patch wasn't working. It turned out
> that xpcshell used custom stack walking code, so I had to make this change:

I think the heart of the disagreement is exactly here:

- Nick and I look at that change and say "Great, seems like the right thing. r=me". We're concerned that using the async parent will make us start producing stacks that are incomprehensible to people who aren't expecting to see Promise continuations in there.

- I gather Paolo looks at that change and says, "I wonder how much other stuff we're breaking."

I seriously doubt it would take very much time at all to check every single user of nsIStackFrame, see whether it wants the stack for display (in which case, include async parents) or analysis (in which case, use the real caller), and make the corresponding changes. So I'm less concerned with breaking chrome code.

Comment 71

[Jim Blandy :jimb]

Automatically following the async parent feels to me like changing the meaning of nsIStackFrame::caller in a way which is factually wrong --- the returned frame is *not* the caller --- but which we anticipate, given what we know of how nsIStackFrame is generally used, will get felicitous results out of existing code, if we also tweak nsIStackFrame::name to not return the function name.

Generally, I really dislike this kind of thing. "caller" should mean "caller". The called function name should be the called function name. If we have additional information, let's make it available through additional properties. And let's just change the consumers.

Comment 72

[:Paolo Amadini]

Okay, I'll go ahead and implement these as separate properties. Thanks for the discussion!

Comment 73

[:Paolo Amadini]

I get a strange assertion with only the first patch applied. Ideas?


js/src/jit-test/jit_test.py ~/moz/obj-x86_64-apple-darwin/dist/bin/js saved-stacks/asm-frames.js
Assertion failure: PushedFP == masm.currentOffset() - offsetAtBegin, at /Users/paolo/m-c/js/src/asmjs/AsmJSFrameIterator.cpp:194



This seems related to some code generation and offset calculation.

I've done a full build but I've not tried a clobber, maybe I should?

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8551838 [details] [diff] [review]
Part 5 - Add the asyncCause property to nsIStackFrame

r=me modulo comment 42

Comment 75

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8551839 [details] [diff] [review]
Part 6 - Allow JS code to provide an async stack when calling a function

See review comments in comment 43.

r=me with those fixed, I think.

Comment 76

[Jim Blandy :jimb]

(In reply to :Paolo Amadini from comment #73)
> I get a strange assertion with only the first patch applied. Ideas?
> 
> 
> js/src/jit-test/jit_test.py ~/moz/obj-x86_64-apple-darwin/dist/bin/js
> saved-stacks/asm-frames.js
> Assertion failure: PushedFP == masm.currentOffset() - offsetAtBegin, at
> /Users/paolo/m-c/js/src/asmjs/AsmJSFrameIterator.cpp:194
> 
> 
> 
> This seems related to some code generation and offset calculation.
> 
> I've done a full build but I've not tried a clobber, maybe I should?

Are you sure that these crashes are introduced by your patch? Have you run the jit-tests on an unpatched shell?

Comment 77

[Jan de Mooij [:jandem]]

That assertion failure is expected.

  masm.loadAsmJSActivation(scratch);
  masm.push(Address(scratch, AsmJSActivation::offsetOfFP()));
  MOZ_ASSERT(PushedFP == masm.currentOffset() - offsetAtBegin);

With your patch, AsmJSActivation::offsetOfFP() is probably > 127 so the push instruction is encoded slightly differently.

I think you can just adjust PushedFP.

Comment 78

[:Paolo Amadini]

Attachment: Part 0 - Add the asyncCause and asyncParent properties to the native SavedFrame object

Funnily enough, introducing asyncParent revealed a minor issue with the previous approach that is a much more complex issue now. This is related to principal filtering.

Here is a case that may not be at all uncommon:

content: a => b();
chrome:  b => asyncCall(c, { asyncCause: 1 });
chrome:  c => asyncCall(d, { asyncCause: 2 });
chrome:  d => e();
content: e => captureStack();

Here is an unfiltered view of the stack using the notation from comment 35:

  aBCde

With the previous patch, unprivileged content would see this:

  ae

We lost the asyncCause. Arguably, this should have been:

  Ae

Frame "A" would be programmatically generated from frame "a" when accessing "e.parent", changing only its asyncCause. Probably the asyncCause would have to be a generic one - we don't want to leak chrome implementation details.

Now to discuss the new patch, I'll prepend "<" to indicate when a frame would use asyncParent instead of parent, and I'll put the asyncCause inside parentheses (zero for a generic one). Here is the unfiltered view with the new notation:

  a B(1) <C(2) <d e

How would a filtered view of frame "e" look like? It could be:

  a <e

This would be enough as we have asyncParent for the demarcation. We could also do this:

  A(0) <e

In both cases, the "parent" property of "e" is null, meaning that if content code doesn't want to follow async code, it won't see anything, and if it wants to follow, it can do that properly using asyncParent.



But now the bigger question. How would a filtered view of frame "d" look like, in case chrome gives a pointer to it to content?

Without asyncParent, there was no doubt. We would always follow the parent internally (whether that originated from an async or a sync call) and then we could see whether we had an asyncCause (real or generic) on the resulting frame. In other words we would see "A(0)".

With a separate asyncParent, if I call an accessor (say functionDisplayName) on frame "d" with content principals, should I see function "a" or should I see null, as if the stack was empty?



And now, an optional variation on the exercise for the advanced student :-)

What if we could have both asyncParent and parent set on a stack frame? I get the original idea was that we could have both set.

Let's say both frame "C" and "d" also have a "parent" property pointing to the internal JavaScript implementation of the async callback. Let's call this function "k", and let's say frame "k" has content principals, because the callback is implemented in a content compartment. (The callback being in content, and the async stack being set in chrome, is worst case, but may happen and helps illustrating the next point.)

If I call an accessor (say functionDisplayName) on frame "d" with content principals, should I see function "a", should I see function "k", or something else?

(The difference between a common accessor and the parent/asyncParent accessors is that, when using the former, I'm not making a choice on whether I want to see async stacks or not. The curious thing is that, as soon as we obtain a descendant of "d" in content, we solve this ambiguity because we would see function "a" through "asyncParent.functionDisplayName", and function "k" through "parent.functionDisplayName".)



Jim: if you have anything in mind, feel free to edit code.

Comment 79

[:Paolo Amadini]

(In reply to Jan de Mooij [:jandem] from comment #77)
>   masm.loadAsmJSActivation(scratch);
>   masm.push(Address(scratch, AsmJSActivation::offsetOfFP()));
>   MOZ_ASSERT(PushedFP == masm.currentOffset() - offsetAtBegin);
> 
> With your patch, AsmJSActivation::offsetOfFP() is probably > 127 so the push
> instruction is encoded slightly differently.

Ooooh.

> I think you can just adjust PushedFP.

It's obviously different per target platform:

http://mxr.mozilla.org/mozilla-central/source/js/src/asmjs/AsmJSFrameIterator.cpp#107

Any clue about how much I have to add in each case? Or is there something I can do to have those values calculated on continuous integration?

Comment 80

[:Paolo Amadini]

(In reply to Jim Blandy :jimb from comment #76)
> Are you sure that these crashes are introduced by your patch? Have you run
> the jit-tests on an unpatched shell?

Yes, I've verified that unapplying the patch solved the issue. That's why I thought of a clobber issue initially, but it appears the solution was in the statically encoded offsets.

Comment 81

[Jan de Mooij [:jandem]]

(In reply to :Paolo Amadini from comment #79)
> Any clue about how much I have to add in each case? Or is there something I
> can do to have those values calculated on continuous integration?

I think this only affects x64. The other platforms are 32-bit so the offset should still be a lot less than 127. It's possible some platforms like ARM have a different limit, but I'd just do a Try push and trust the assert to catch issues on other platforms.

Comment 82

[Jim Blandy :jimb]

(In reply to :Paolo Amadini from comment #78)
> Funnily enough, introducing asyncParent revealed a minor issue with the
> previous approach that is a much more complex issue now. This is related to
> principal filtering.

Yes, this is an interesting problem. We have a tree, some of whose nodes are invisible to certain consumers, and we need to think of the most meaningful definition for the child accessors.

Just acknowledging the problem for now. I'll think about solutions tomorrow morning.

Comment 83

[:Paolo Amadini]

(In reply to Jim Blandy :jimb from comment #82)
> Yes, this is an interesting problem. We have a tree, some of whose nodes are
> invisible to certain consumers, and we need to think of the most meaningful
> definition for the child accessors.

Another quite interesting problem is that introducing multiple parents on the same frame makes the DAG more complex. Previously stacks were basically a set of directed trees each with its own independent root. Now we have multiple roots that are part of the same directed graph, and we can also have an unbounded number of paths to the same root.

I was looking at the implementation of limitSavedFrames in part 2. As I've mentioned to you, for enforcing the limit we can just consider the path that follows async parents, because it's unlikely and possibly irrelevant that the path following sync parents would be longer.

But I was wondering what should happen when the async stack lives in another compartment. Currently we store a direct pointer to a SavedFrame in another compartment. Does that keep the compartment alive?

If pointing to a SavedFrame in another compartment is a problem, we could easily copy the frames we need to the current compartment. This is what limitSavedFrames did when a limit was reached, using the Lookup objects.

But with multiple parents, if both can be present on the same frame, copying one frame and its ancestors to another compartment substantially requires walking the graph recursively.

Comment 84

[:Paolo Amadini]

I have a proposal that can simplify the problems in comment 78 and comment 83 while preserving the API that Jim envisions for external consumers.

We can rename the current "parent" property to "treeParent" or "rawParent". This would be the only field physically stored in the slot, and the only one used for performing lookups and walking the tree.

Then the "parent" and "asyncParent" JS property accessors can use this logic:
  if (!this.rawParent) return null;
  let { parentFrame, encounteredAsyncCauseWhileWalking } = GetFirstSubsumedFrame(this.rawParent);
  if (encounteredAsyncCauseWhileWalking || parentFrame.asyncCause) {
    if (requestedParent) return null;
    if (requestedAsyncParent) return parentFrame;
  } else {
    if (requestedParent) return parentFrame;
    if (requestedAsyncParent) return null;
  }

The "asyncCause" accessor can apply this logic:
  let { thisFrame, encounteredAsyncCauseWhileWalking } = GetFirstSubsumedFrame(this);
  if (thisFrame.asyncCause) return thisFrame.asyncCause;
  if (encounteredAsyncCauseWhileWalking) return "Async";
  return null;

This answers comment 78 in this way: if content is given a pointer to frame "d", it would see it as if it was frame "A(0)".

In the cases where nsIStackFrame or even a higher level consumer wants to fully mask async callers, it would still have to check whether asyncCause is set on the current frame, and if so return null or defaults for all other accessors, basically indicating the stack is empty. This works because nsIStackFrame can make the decision (for example based on prefs or specific opt-ins) while the SavedFrame low-level structure by itself cannot do it.

Comment 85

[:Paolo Amadini]

Also, there's the special logic that requires JSSLOT_PRIVATE_PARENT.

If we don't have an additional parent slot, we don't need to duplicate this logic.

Comment 86

[:Paolo Amadini]

Attachment: Part 1 - Add the asyncCause and asyncParent properties to the native SavedFrame object

This is how the public interface would look like.

This is half implemented, there is no rename of the internal slot and no code to pick up async boundaries while walking inaccessible frames.

Comment 87

[:Paolo Amadini]

Attachment: Part 2 - Set the asyncCause property when capturing stacks

Comment 88

[:Paolo Amadini]

Attachment: Part 3 - Allow C++ code to provide an async stack when calling a JS function

There are some changes to the public API here (HandleValue instead of char *) that made testing functions easier to implement and as a side effect I believe now better support non-ASCII asyncCause values.

Testing is still bare-bones. I still have to implement asyncCause/asyncParent/parent getters properly in the first part and add test with various principals.

I also think I have to be careful later in the queue when implementing changeAsyncCause, in order not to return details of a frame to which the caller does not have access.

Comment 89

[:Paolo Amadini]

Attachment: Part 4 - Limit the depth of async stacks and ensure chains are in the same compartment

The limiting logic wouldn't be too different with a single parent slot.

So, is there any value in ensuring that JS::CaptureCurrentStack returns a chain of SavedFrame object that lives entirely in the compartment used for the function call?

Comment 90

[:Paolo Amadini]

Now tackling part 5. There's now a "sanitized" property on nsIStackFrame that follows the "caller" path.

If we have "asyncCaller" on nsIStackFrame as well, what is the behavior that "sanitized" would need, if we want content to be able to follow async parents?

Note that, still using the example in comment 78 and the current patches, the sanitized view that results in unprivileged code seeing the SavedFrame "A(0)" can only be obtained with a pointer to frame "d". As soon as we move the pointer to frame "a", unprivileged code would just see frame "a" as it is.

Comment 91

[:Paolo Amadini]

So, more thoughts on the sanitization case. This is generally relevant when chrome code captures a stack for an exception, and then provides the information to content. I'd say that in this case, whether the exception actually happened before or after an async call in chrome code is of little relevance to content. For example:

function inChrome(case) {
  return new Promise(resolve => {
    if (case == 1) throw new SomeExceptionObject("An error happened.");
  }).then(function () {
    if (case == 2) throw new SomeExceptionObject("An error happened.");
  });
}

function inContent(case) {
  let promise = inChrome(case); // Line 1
  promise.catch(someExceptionObject => {
    dump(someExceptionObject.stack);
    let anotherStack = captureStack();
  });
};

The exception stack, seen from content, should just point to line 1 as its youngest frame.

Both cases report the exception itself after the inContent function has already returned, as part of the promise rejection. The fact is that in case 1 there is no asynchronous stack at the time the exception is generated in chrome, while in case 2 there is an asynchronous stack.

Returning to comment 78 again, if frame "d" is sanitized by nsIStackFrame to point to frame "a", it's fine. Content will see frame "a" in the exception.

If we capture the stack in the content function itself, after chrome invoked it, frame "A(0)" would be seen as the asyncParent of frame "e". That is, the youngest frame captured in content is always a synchronous content frame for the function where the stack capture happened. When walking up from there, we can always reconstruct the asyncCause we need.

Comment 92

[Boris Zbarsky [:bzbarsky]]

> The exception stack, seen from content, should just point to line 1 as its youngest frame.

Or be empty in both cases, right?

But yes, having the line 1 seems nicer than empty stack.  I agree that in this case wer're precisely in your "frame d" case.

Comment 93

[:Paolo Amadini]

The common case for comment 91 is that inChrome is a JS-implemented DOM API (or general chrome API).

Content only knows it is an asynchronous API, in this case Promise-based, but can also be callback-based like a setTimeout. It knows about the function it calls, but shouldn't know about its implementation details.

When we capture the stack after the callback is invoked, we would see the frame for line 1 as the asyncParent. For the asyncCause, we can use the generic string "Async", but as an improvement we can also try to use the functionDisplayName of the chrome frame just before it.

For anonymous functions, the functionDisplayName may contain some details about its surroundings. It is probably not a big deal, and if required this can be completely overridden by giving a proper name to the function.

So, the "0" in frame "A(0)" can actually be the name of the chrome API.

Comment 94

[Boris Zbarsky [:bzbarsky]]

> but as an improvement we can also try to use the functionDisplayName of the chrome frame
> just before it.

We could also have the bindings code set some information in the JS engine when it calls into JS-implemented Web IDL and use that (possibly in a followup).

Comment 95

[Tom Tromey :tromey]

(In reply to Boris Zbarsky [:bz] from comment #94)

> We could also have the bindings code set some information in the JS engine
> when it calls into JS-implemented Web IDL and use that (possibly in a
> followup).

This may be bug 961325.

Comment 96

[Jim Blandy :jimb]

Notes from discussions with Paolo (haven't read above comments):

In the end, after working through some examples of how these captured stacks
will work in practice, I have come around largely to Paolo's point of view:
SavedFrames should have a single parent: sometimes the caller, and at other
times the async caller.

DOM Promises specify that fulfillment and rejection handlers are always invoked
on essentially uninteresting stacks. There is some event loop machinery that
runs all runnable promise handlers as the final phase of a tick; and each
dispatch probably has some frame from the promise implementation itself. These
continuations are never interesting. The async caller is the only interesting
parent for such stacks.

I do think it's important to distinguish "caller/callee" relationships from
"async caller / async callee" relationships. We should use different accessors
to retrieve the StackFrame parent when the parent is a true caller and when it
is an asynchronous caller. Paolo and I agreed to call them "parent" (as now) and
"asyncParent".

Comment 97

[:Paolo Amadini]

Attachment: Part 1 - Add the asyncCause and asyncParent properties to the native SavedFrame object

Comment 98

[:Paolo Amadini]

Attachment: Part 2 - Allow C++ code to provide an async stack when calling a JS function

Comment 99

[:Paolo Amadini]

Attachment: Part 3 - Add the asyncCause and asyncCaller properties to nsIStackFrame

Comment 100

[:Paolo Amadini]

Attachment: Part 4 - Allow JS code to provide an async stack when calling a function

Jim, this is the last patch in the updated queue for review. Tests for principal filtering are important and still missing, but everything else should be updated for the latest design. I may have missed some small comment here and there, feel free to point out again any change that may be required.

I flagged you for an additional review on the XPCOM parts if you want, though these parts already have r+ by bz in the previous iterations, and the main difference is only the asyncCaller separation. Tests for the XPCOM parts are also needed.

Comment 101

[:Paolo Amadini]

Attachment: Part 2 - Allow C++ code to provide an async stack when calling a JS function

I wasn't so confident that everything in part 1 was correct, but these tests seem to prove it is :-)

Comment 102

[:Paolo Amadini]

Attachment: Part 4 - Allow JS code to provide an async stack when calling a function

And here is a test for Components.utils.callFunctionWithAsyncStack as well.

Comment 103

[:Paolo Amadini]

Attachment: Part 2 - Allow C++ code to provide an async stack when calling a JS function

Based on a previous tryserver build, I made these changes:
1) Marked the constructor "explicit" (missed the reminder!)
2) Reduced default limit from 120 to 60 for "--no-baseline" failures
3) As planned, removed the usage of the feature in Promise.cpp

Failure number 2 is just a result of how "async-max-frame-count" is implemented, since it goes over the recursion limit of about 100 calls. The simplest solution is to lower the limit.

Change number 3 was because of one test failure, that is however related to Task.jsm stack rewriting and not to the feature in Promise.cpp itself. It's probably just simpler to remove Task.jsm stack rewriting first.

New tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a4b248b8063

Comment 104

[Jim Blandy :jimb]

When skipping non-subsumed frames, would it make sense to simply carry back the async cause of the oldest frame we skip, and use that if the first visible frame doesn't have its own cause? That might mean that implementation details of chrome code leak out, but those are probably better than "Async". But even better, the entry points of chrome facilities implemented in JS could use callFunctionWithAsyncStack immediately open entry to provide a more meaningful label that would automatically appear on the next visible frame.

Comment 105

[:Paolo Amadini]

(In reply to Jim Blandy :jimb from comment #104)
> When skipping non-subsumed frames, would it make sense to simply carry back
> the async cause of the oldest frame we skip, and use that if the first
> visible frame doesn't have its own cause?

I thought about that as well. I don't exclude we may do this eventually, but that might mean that by default the asyncCause could be, for example, "Promise" or "dispatch" depending on what the chrome implementation does first, and may change if the chrome implementation changes. From the content point of view, I don't believe any of them is interesting.

> But even better, the entry points of chrome facilities
> implemented in JS could use callFunctionWithAsyncStack immediately open
> entry to provide a more meaningful label that would automatically appear on
> the next visible frame.

And this would solve the above, but at this point I'd prefer for the function name to be used, so that we don't force each entry point to add extra code to mask its inner asyncCause.

In either case, I'd rather do this in a follow-up. I believe "Async" is the simplest thing that works for now, with the first consumer to see async stacks being xpcshell backtraces.

Comment 106

[Jim Blandy :jimb]

Comment on attachment 8570073 [details] [diff] [review]
Part 1 - Add the asyncCause and asyncParent properties to the native SavedFrame object

Review of attachment 8570073 [details] [diff] [review]:
-----------------------------------------------------------------

This looks very good, but there are some changes necessary.

This patch will need to be rebased atop fitzgen's new public accessors for SavedFrame properties, like JS::GetSavedFrameParent. You'll need to write analogous JS::GetSavedFrameAsyncCause and JS::GetSavedFrameAsyncParent functions, and then make the JS accessors call those, just as the other accessors do.

::: js/src/doc/SavedFrame/SavedFrame.md
@@ +62,5 @@
> +`asyncCause`
> +:   If this stack frame indicates the point where an asynchronous call was
> +    started, for example a `setTimeout` invocation, this is set to an arbitrary
> +    string representing the type of call, for example "setTimeout". If the
> +    asynchronous call was started in a descendant frame to which the caller does

This could be a bit more specific. Perhaps:

If this stack frame is the `asyncParent` of other stack frames, then this is a string representing the type of asynchronous call by which this frame invoked its children. For example, if this frame's children are calls to handlers for a promise this frame created, this frame's `asyncCause` would be `"Promise"`.

(Since this is really created for Promises, it seems like we should use Promises as the example.)

If I've misunderstood the behavior we're trying to arrive at, I apologize; please set me straight. :)

@@ +78,2 @@
>  `parent`
> +:   The next older synchronous frame, or `null` if there are no more frames. In

"Synchronous frame" is not a term that's used anywhere else. This should say:

"This frame's caller, or `null` if this is the oldest frame on the stack."

::: js/src/vm/SavedStacks.cpp
@@ +344,2 @@
>  {
> +    MOZ_ASSERT(skippedAsync);

SpiderMonkey style is to use references for pointers that must never be null. References need not be const. So skippedAsync should be a bool &, and this assertion can go away.

@@ +372,5 @@
>  }
>  
>  /* static */ bool
>  SavedFrame::checkThis(JSContext *cx, CallArgs &args, const char *fnName,
> +                      MutableHandleSavedFrame frame, bool *skippedAsync)

Similarly, 'bool &' here.

@@ +422,1 @@
>  #define THIS_SAVEDFRAME(cx, argc, vp, fnName, defaultVal, args, frame) \

Note that every variable declared by this macro has its name passed in as argument. Although it will mean changing the other uses of THIS_SAVEDFRAME, please do the same for skippedAsync.

@@ +475,5 @@
> +    RootedAtom asyncCause(cx, frame->getAsyncCause());
> +    if (asyncCause)
> +        args.rval().setString(asyncCause);
> +    else if (skippedAsync)
> +        args.rval().setString(JS_NewStringCopyZ(cx, "Async"));

Go ahead and add an entry to js/src/vm/CommonPropertyNames.h for "Async", and then use cx->names().Async.

@@ +490,5 @@
> +    // Now |frame| points to the physical SavedFrame that is used to provide the
> +    // values for the accessors on this object. Whether we crossed any async
> +    // parent relationship to get here is irrelevant. We are interested in
> +    // whether we would cross any async parents to get from here to the first
> +    // subsumed parent frame instead.

This comment (and its copy in SavedFrame::parentProperty) seems unnecessary. That frame and skippedAsync have the values they do is just partof THIS_SAVEDFRAME's contract.

@@ +498,5 @@
> +
> +    // Even if |parent| is not subsumed, we still want to return a pointer to it
> +    // rather than |subsumedParent| so it can pick up any |asyncCause| from the
> +    // inaccessible part of the chain.
> +    if (subsumedParent && (parent->getAsyncCause() || skippedAsync))

Shouldn't this check subsumedParent->getAsyncCause()? We want asyncParent to have a value even if GetFirstSubsumedFrame skips no async frames but arrives at a frame that has an async cause, right?

@@ +522,5 @@
> +
> +    // Even if |parent| is not subsumed, we still want to return a pointer to it
> +    // rather than |subsumedParent| so it can pick up any |asyncCause| from the
> +    // inaccessible part of the chain.
> +    if (subsumedParent && !(parent->getAsyncCause() || skippedAsync))

Similarly.

@@ +546,5 @@
>  
>          {
> +            RootedString asyncCause(cx, frame->getAsyncCause());
> +            if (!asyncCause && skippedAsync) {
> +                asyncCause.set(JS_NewStringCopyZ(cx, "Async"));

Similarly, use cx->names().Async.

Comment 107

[Jim Blandy :jimb]

Comment on attachment 8571367 [details] [diff] [review]
Part 2 - Allow C++ code to provide an async stack when calling a JS function

Review of attachment 8571367 [details] [diff] [review]:
-----------------------------------------------------------------

These tests are great. That's exactly the kind of thing the JS shell and js/src/jit-tests are supposed to enable.

::: js/src/jit-test/tests/saved-stacks/async-principals.js
@@ +55,5 @@
> +// like this:
> +//
> +//     low.a
> +
> +function a() {

You might want to put a comment up here saying:

// We'll move these functions into the right globals below, before invoking them.

because I was confused for a bit by that. (It's very nicely done, though.)

@@ +76,5 @@
> +    { name: "a", asyncCause: null   },
> +  ]);
> +
> +  // Save the stack in the low compartment.
> +  let stackE = e(saveStack(0, e));

It might be a little clearer to use 'low' and 'high' instead of 'e' and 'b' to designate the compartments in calls to saveStack.

::: js/src/vm/SavedStacks.cpp
@@ +53,5 @@
>      {
>          MOZ_ASSERT(source);
>      }
>  
> +    explicit Lookup(SavedFrame *savedFrame)

SpiderMonkey style says to use references for pointers that can't be null.

@@ +699,4 @@
>      // Accumulate the vector of Lookup objects in |stackChain|.
>      SavedFrame::AutoLookupVector stackChain(cx);
>      while (!iter.done()) {
> +        Activation *activation = iter.activation();

This pointer is never null, either.

@@ +760,5 @@
> +    // SavedFrame instances we use are stored in the same compartment as the
> +    // rest of the synchronous stack chain.
> +    RootedSavedFrame parentFrame(cx, nullptr);
> +    if (asyncStack && !adoptAsyncStack(cx, asyncStack, asyncCause, &parentFrame,
> +                                       maxFrameCount))

SpiderMonkey code (but not comments) is allowed to be 100 columns wide. So this might be able to drop the braces.

Comment 108

[Jim Blandy :jimb]

Comment on attachment 8570078 [details] [diff] [review]
Part 3 - Add the asyncCause and asyncCaller properties to nsIStackFrame

Review of attachment 8570078 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I can approve changes to this code. However, I do have some comments. Once these are addressed, perhaps khuey or bholley?

::: dom/bindings/Exceptions.cpp
@@ +563,5 @@
> +    JS::Rooted<JS::Value> asyncCauseVal(cx);
> +    // asyncCause can be null
> +    if (!JS_GetProperty(cx, stack, "asyncCause", &asyncCauseVal) ||
> +        (!asyncCauseVal.isString() && !asyncCauseVal.isNull())) {
> +      return NS_ERROR_UNEXPECTED;

If I understand correctly, this whole dance should be much simpler now that fitzgen has added the C++-friendly JS::GetSavedFrameNoodle functions to js/src/jsapi.h. Your SavedStacks.cpp patch should add those, and you should use them here.

@@ +603,5 @@
> +    JSAutoCompartment ac(cx, stack);
> +    JS::Rooted<JS::Value> asyncCallerVal(cx);
> +    if (!JS_GetProperty(cx, stack, "asyncParent", &asyncCallerVal) ||
> +        !asyncCallerVal.isObjectOrNull()) {
> +      return NS_ERROR_UNEXPECTED;

Same here.

Comment 109

[Jim Blandy :jimb]

Comment on attachment 8570948 [details] [diff] [review]
Part 4 - Allow JS code to provide an async stack when calling a function

Review of attachment 8570948 [details] [diff] [review]:
-----------------------------------------------------------------

Again, I'm not a reviewer for this area. Suggesting khuey.

Comment 110

[:Paolo Amadini]

Attachment: Part 1 - Add the asyncCause and asyncParent properties to the native SavedFrame object

Comment 111

[:Paolo Amadini]

Attachment: Part 2 - Allow C++ code to provide an async stack when calling a JS function

Comment 112

[:Paolo Amadini]

Comment on attachment 8570948 [details] [diff] [review]
Part 4 - Allow JS code to provide an async stack when calling a function

(In reply to Jim Blandy :jimb from comment #108)
> I don't think I can approve changes to this code.

Don't worry, as I mentioned already I'm confident in carrying over bz's reviews from comment 74 and 75, I made all the requested changes and the main difference is that some code is removed.

In any case, since we now have tests on part 2, I can also land parts 3 and 4 in a separate bug.

> If I understand correctly, this whole dance should be much simpler now that
> fitzgen has added the C++-friendly JS::GetSavedFrameNoodle functions to
> js/src/jsapi.h. Your SavedStacks.cpp patch should add those, and you should
> use them here.

Other accessors in the same file haven't been ported, I can file a bug to migrate everything at once.

Comment 113

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d13e1a4575a7

Comment 114

[Jim Blandy :jimb]

Comment on attachment 8572712 [details] [diff] [review]
Part 1 - Add the asyncCause and asyncParent properties to the native SavedFrame object

Review of attachment 8572712 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 115

[Jim Blandy :jimb]

Comment on attachment 8572714 [details] [diff] [review]
Part 2 - Allow C++ code to provide an async stack when calling a JS function

Review of attachment 8572714 [details] [diff] [review]:
-----------------------------------------------------------------

You can carry forward my r+ from the last review here...

Comment 116

[:Paolo Amadini]

There were some mochitest jetpack failures in the previous try build, not sure if they are related.

Retrying with just parts 1 and 2 on the latest trunk:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e39625f68127

Comment 117

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #116)
> There were some mochitest jetpack failures in the previous try build, not
> sure if they are related.

These are green now, so I can go ahead and land:

https://hg.mozilla.org/integration/fx-team/rev/6f9239fa5ca2
https://hg.mozilla.org/integration/fx-team/rev/99c04843ea7e

I'll let the rest of the tryserver build continue.

Comment 118

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f9239fa5ca2
https://hg.mozilla.org/mozilla-central/rev/99c04843ea7e