961326 - [jsdbg2] Debugger should report why JavaScript was called by the embedding

Status: ASSIGNED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Debugger should be able to report why the embedding invoked JavaScript code --- callbacks, event handlers, timeouts, etc. It should essentially report the information provided by the API requested in bug 961325.

Comment 1

[Jim Blandy :jimb]

Attachment: Give SpiderMonkey an API that embeddings can use to safely construct values for Debugger clients.

Comment 2

[Jim Blandy :jimb]

Attachment: Implement Debugger.Frame.prototype.scriptEntryReason, with tests and docs.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8451786 [details] [diff] [review]
Give SpiderMonkey an API that embeddings can use to safely construct values for Debugger clients.

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

Clearing r?. Still a fan of the API generally.

Bikeshedding: "Trusted" is iffy in names, because it's so hard for a name alone not to mislead, re: what is trusted, by whom, and why; and how paranoid you should be about it.  I could go for DebuggerFriendlyObjectBuilder. Or, you know, something like that, but shorter...

If you still think "Trusted" is OK, I can maybe live with it.

::: js/public/DebugAPI.h
@@ +101,5 @@
> +//     }
> +//
> +// Note that TrustedBuilder and Object handle errors on their side internally,
> +// so that if one operation fails, all subsequent operations silently do
> +// nothing. The exception value, if any, is saved in the TrustedBuilder's

When I first looked at this I was a lot more sanguine about the new error-handling style. Now I'm skeptical. With this API style, it's possible to write code like:

    result = builder.newObject();

    // Set the "eventType" property.
    result.setProperty("eventType", SafelyFetchType(eventObject));

    // Set the "event" property.
    JSObject *eventObject = ... obtain debuggee event object somehow ...;
    if (!eventObject)
        return false;
    result.setProperty("event", eventObject);

    return true;

which looks fine but is subtly wrong: the "..." parts may call into the JSAPI even though an exception is already pending on cx.

Right now there's just one error-handling rule to remember: Always check for and propagate false returns. It leads to unsightly code, and people do make mistakes---but those mistakes are not as subtle as this.

Also I don't think having |okay| flags on API-provided classes is a pattern we want to establish here and use in lots of different classes. Then using them together would become cumbersome.

@@ +106,5 @@
> +// internal JSContext. Debugger checks for failure and propagates it
> +// appropriately. So the code above will correctly handler errors in trusted
> +// object allocation or property initialization. The embedding can also report
> +// its own errors; Debugger will give the embedding's error priority over the
> +// builder's.

This doesn't sound right. Whichever one happened last would end up having priority, right?

@@ +119,5 @@
> +class TrustedBuilderOrigin;
> +
> +class TrustedBuilder {
> +    // The context in which we build objects.
> +    JSContext *context;

Because of rooting, I wasn't able to doodle much sample code before wanting a public getter for the JSContext.

@@ +208,5 @@
> +        explicit Object(TrustedBuilder &owner) : Base(owner, nullptr) { }
> +        Object(const Object &rhs) : Base(rhs) { }
> +
> +        // We can inherit our assignment operator from our base class, since we
> +        // add no data members. Our automatically-generated assignment operator

FWIW, I think the autogenerated copy assignment operator works even if the derived class does add (assignable) member variables.

@@ +231,5 @@
> +        // flag. All subsequent construction and initialization will silently do
> +        // nothing.
> +        Object &setProperty(const char *name, JS::HandleValue value);
> +        Object &setProperty(const char *name, JS::HandleObject value);
> +        Object &setProperty(const char *name, Object &value);

I think these should be defineProperty rather than setProperty, at least in terms of what they do, if not their names (but probably their names too).

Properties defined this way should automatically be configurable, enumerable, and writable.

In other words it should be like
     return {type: v1, event: v2};
rather than like
     var obj = {};
     obj.type = v1;
     obj.event = v2;
     return obj;
which can call prototype setters etc.

Comment 4

[Jim Blandy :jimb]

I'm not sure the magical error handling is a great idea, either. If you look at the second patch in the series, the way we "merge" errors on the two contexts is worrisome.

The problem is, if we share a context, then that means switching back and forth between compartments on every call.

Would it work to allocate a fresh JSContext for the builder's exclusive use, whose current compartment is set appropriately? That way, the builder could know that any errors on that context really were its own.

Comment 5

[Jim Blandy :jimb]

If the entire idea of an 'okay' flag is the sticking point, then I think that can be managed...

Comment 6

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> This doesn't sound right. Whichever one happened last would end up having
> priority, right?

The truth is, I was assuming they'd be different JSContexts. But I didn't ensure that that was the case. So this is a problem that must be fixed.


> @@ +119,5 @@
> > +class TrustedBuilderOrigin;
> > +
> > +class TrustedBuilder {
> > +    // The context in which we build objects.
> > +    JSContext *context;
> 
> Because of rooting, I wasn't able to doodle much sample code before wanting
> a public getter for the JSContext.

Here too: the idea is that the embedding is using one JSContext, and the builder is using another. We *definitely* do not want to hand out the builder's context, because it'd be in the wrong compartment.

Comment 7

[Jim Blandy :jimb]

Attachment: Give SpiderMonkey an API that embeddings can use to safely construct values for Debugger clients.

Revised per comments and IRC discussion:

- Error handling is made to work just like everything else.
- A single JSContext is used, and each builder method uses AutoCompartment.

Comment 8

[Jim Blandy :jimb]

Oh, no, that revised JS::dbg::Builder patch doesn't implement the setProperty / defineProperty change requested. I'll get to that in a bit.

Comment 9

[Jim Blandy :jimb]

Attachment: Give SpiderMonkey an API that embeddings can use to safely construct values for Debugger clients.

Okay, I believe this implements all review suggestions.

Comment 10

[Jim Blandy :jimb]

Attachment: Implement Debugger.Frame.prototype.scriptEntryReason, with tests and docs.

Comment 11

[Jim Blandy :jimb]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=f732f55a6c82

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8454910 [details] [diff] [review]
Give SpiderMonkey an API that embeddings can use to safely construct values for Debugger clients.

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

Whoo!

::: js/public/DebugAPI.h
@@ +136,5 @@
> +
> +  protected:
> +    // A reference to a trusted object or value. At the moment, we only use it
> +    // with JSObject *, but if we start building more complex trusted values
> +    // (Debugger.Script instances, perhaps?), then we'll need more

Comment nit: It'd be OK to dispense with the whole "but" clause. I think it goes without saying.

@@ +172,5 @@
> +            return *this;
> +        }
> +
> +        operator ConvertibleToBool() const {
> +            return (value.get() != js::GCMethods<T>::initial()) ? &BuiltThing::nonNull : 0;

Can this be shortened to
    return value ? &BuiltThing::nonNull : 0;
?

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8454911 [details] [diff] [review]
Implement Debugger.Frame.prototype.scriptEntryReason, with tests and docs.

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

Looks good. I agree with the commit message's comments about tests needed. I would add:
- reasons are propagated from one frame to the next (via plain old function calls)
- native function frames get reasons too
- a generator frame inherits the caller's reason, which may differ from the reason when the generator was previously running.

I thought that reentering content from the debugger might set the reason to "debugger" or something like that. After all, the caller in those cases (the debugger itself) is JS and can't do it because the API is C++-only. Your call though.

::: js/public/DebugAPI.h
@@ +420,5 @@
> +    // case of failure.
> +    virtual bool explain(JSContext *cx, Builder &builder, Builder::Object &reason) = 0;
> +
> +  protected:
> +    // A utility function for simple-minded ScriptEntryReason subclasses. Return

"simple-minded"?  :-\

@@ +426,5 @@
> +    // string |propertyValue|. On failure, report the problem on |cx| and return
> +    // a false Object.
> +    Builder::Object simpleReason(JSContext *cx, Builder &builder,
> +                                 const char *propertyName,
> +                                 const char *propertyValue);

I think this would be easier to use if it took a Builer::Object & out parameter and returned bool.

::: js/src/doc/Debugger/ScriptEntry.md
@@ +44,5 @@
> +that can initiate execution in the debuggee establish script entry reasons;
> +calls to `eval` are instigated by the debuggee.
> +
> +
> +## The `Debugger.Frame.prototype.scriptEntryReason` accessor

I can't vouch for the usefulness of this particular set of properties.

I wonder if it'd be more useful to provide a .scriptEntryReason.type property for callers to switch on.

@@ +50,5 @@
> +A [`Debugger.Frame`][frame] instance's `scriptEntryReason` accessor returns
> +a value having one of the forms described here.
> +
> +If the embedding provided no explanation of why it invoked this stack frame,
> +then the `scriptEntryReason` accessor returns `undefined`. Otherwise, it returns

This particular language around `undefined` makes it sound like scriptEntryReasons aren't inherited; maybe it could be clearer?

::: js/src/jit-test/tests/debug/Frame-scriptEntryReason-02.js
@@ +27,5 @@
> +
> +  checkNiceProperty(reason, 'dynamic');
> +  checkNiceProperty(reason, 'reason');
> +  checkNiceProperty(reason, 'nullUntrustedObject');
> +  checkNiceProperty(reason, 'nullTrustedObject');

I wonder how long until someone wants a real JS-language API for what class ScriptEntryReason does.

::: js/src/jscntxt.h
@@ +575,5 @@
> +    // Copy the exception state of |ancestor| into this JSContext, and clear
> +    // |ancestor|'s exception.
> +    //
> +    // This JSContext must not be throwing, itself, to avoid dropping errors.
> +    void inheritException(JSContext *ancestor) {

This doesn't seem to be used anymore, thank goodness! Please drop it.

::: js/src/shell/js.cpp
@@ +4582,5 @@
> +        // This function is effectively part of JS::dbg::Builder's test suite,
> +        // since it's the only code in the SpiderMonkey tree that exercises its
> +        // full API, and it's only called by test scripts. But it'd be terribly
> +        // complex to somehow front Builder and Builder::Object to JS. This is
> +        // less trouble.

:-\

::: js/src/vm/Debugger.cpp
@@ +4239,5 @@
> +}
> +
> +ScriptEntryReason::~ScriptEntryReason()
> +{
> +    MOZ_ASSERT(perThreadData->scriptEntryReason == this);

I don't know that we can assert this. There could be user code like

    ScriptEntryReason foo(cx);
    if (trySomethingFallible())
        return false;
    if (!foo.Init())
        return false;

Isn't that permitted?

But we can assert that perThreadData->scriptEntryReason is either this (Init was called) or this->older (Init was not called).

@@ +4527,5 @@
>  
> +static bool
> +DebuggerFrame_getScriptEntryReason(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    THIS_FRAME_ITER(cx, argc, vp, "get this", args, thisobj, _, iter);

The string (used in error messages) should be "get scriptEntryReason", and please change the one in getOlder too.

(Whenever I read "getOlder" the Matthew Sweet song plays in my head.)

@@ +4541,5 @@
> +    Builder::Object reasonObject(cx, builder);
> +
> +    {
> +        // On our calling context, enter the compartment of the frames we're
> +        // explaining. Propagate errors from buildReason in a meaningful way.

The last sentence here is mysterious to me.

"On our calling context" is mysterious too.

Comment 14

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> Comment nit: It'd be OK to dispense with the whole "but" clause. I think it
> goes without saying.

Dropped.

> Can this be shortened to
>     return value ? &BuiltThing::nonNull : 0;
> ?

Yes; I'm worried that might just be because it's a BuiltThing<T *> and won't work with a BuiltThing<Value>; but we can change that when we run into problems.

Comment 15

[Jim Blandy :jimb]

Fresh Try push for the builder patch; the last one ran into some build problems with clang.
https://tbpl.mozilla.org/?tree=Try&rev=655a07408876

Comment 16

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> I wonder if it'd be more useful to provide a .scriptEntryReason.type property
> for callers to switch on.

So, that's an interesting question. I wrote my thoughts on the matter down here:
https://bugzilla.mozilla.org/show_bug.cgi?id=961325#c43

    We should carefully consider what sort of metadata is going to work best in our
    context.

    In principle, the 'type' field serves as a discriminant for the other properties
    of the object. In practice, however, providers of execution reasons and
    consumers of execution reasons are going to grow and evolve independently of
    each other, and so the situation will play out like this:

    - Consumers, knowing that the set of types is open-ended, will handle the 'type'
      values they know something about, and have a default fallback behavior for the
      rest.

    - New providers will be reluctant to introduce new 'type' values, knowing that
      they would elicit the fallback behaviors from their consumers.

    In the end, I think what makes the most sense is to carefully define meanings of
    particular properties that may or may not appear on execution reason objects ---
    eventName, for example, or moduleURL, or whatever --- and then let consumers use
    duck typing to glean what they can. Then, consumers can be written to understand
    whatever properties they find, trusting that their meanings won't change; and
    new providers can set those properties that make sense for their use of JS, and
    add new properties to cover nuance that naive consumers can safely ignore.

Comment 17

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> I thought that reentering content from the debugger might set the reason to
> "debugger" or something like that. After all, the caller in those cases (the
> debugger itself) is JS and can't do it because the API is C++-only. Your
> call though.

That's a good point. I think it should; Debugger-invoked evaluation is certainly "out of the blue".

> "simple-minded"?  :-\

Replaced with "simple".

> I think this would be easier to use if it took a Builer::Object & out
> parameter and returned bool.

Changed. And actually implemented and tested.


> I can't vouch for the usefulness of this particular set of properties.

It's what we need for categorizing object allocations, at least.


> @@ +50,5 @@
> > +A [`Debugger.Frame`][frame] instance's `scriptEntryReason` accessor returns
> > +a value having one of the forms described here.
> > +
> > +If the embedding provided no explanation of why it invoked this stack frame,
> > +then the `scriptEntryReason` accessor returns `undefined`. Otherwise, it returns
> 
> This particular language around `undefined` makes it sound like
> scriptEntryReasons aren't inherited; maybe it could be clearer?

I changed it to:

> If the embedding provided no explanation when it began the JavaScript execution
> of which this frame is a part, then the `scriptEntryReason` accessor returns
> `undefined`. Otherwise, it returns a JavaScript object with some set of the
> properties described in this section.

Is that any better?


> ::: js/src/jit-test/tests/debug/Frame-scriptEntryReason-02.js
> @@ +27,5 @@
> > +
> > +  checkNiceProperty(reason, 'dynamic');
> > +  checkNiceProperty(reason, 'reason');
> > +  checkNiceProperty(reason, 'nullUntrustedObject');
> > +  checkNiceProperty(reason, 'nullTrustedObject');
> 
> I wonder how long until someone wants a real JS-language API for what class
> ScriptEntryReason does.

That should be pretty easy; js.cpp is a simple example.


> ::: js/src/jscntxt.h
> > +    void inheritException(JSContext *ancestor) {
> 
> This doesn't seem to be used anymore, thank goodness! Please drop it.

Oh, thanks! Sorry about that.


> ::: js/src/vm/Debugger.cpp
> @@ +4239,5 @@
> > +}
> > +
> > +ScriptEntryReason::~ScriptEntryReason()
> > +{
> > +    MOZ_ASSERT(perThreadData->scriptEntryReason == this);
> 
> I don't know that we can assert this. There could be user code like
> 
>     ScriptEntryReason foo(cx);
>     if (trySomethingFallible())
>         return false;
>     if (!foo.Init())
>         return false;
> 
> Isn't that permitted?

It seems bad. Why shouldn't the declaration of foo be moved later? Why would anyone want to operate on a non-Initted ScriptEntryReason? I've changed Init's documentation:

    // Establish this ScriptEntryReason as the newest on cx's thread. The
    // destructor will automatically remove this from the stack. No fallible
    // operations should occur between this ScriptEntryReason's construction and
    // the call to its 'Init' member function.


> But we can assert that perThreadData->scriptEntryReason is either this (Init
> was called) or this->older (Init was not called).

This would work; but I think it's undesirable.


> @@ +4527,5 @@
> >  
> > +static bool
> > +DebuggerFrame_getScriptEntryReason(JSContext *cx, unsigned argc, Value *vp)
> > +{
> > +    THIS_FRAME_ITER(cx, argc, vp, "get this", args, thisobj, _, iter);
> 
> The string (used in error messages) should be "get scriptEntryReason", and
> please change the one in getOlder too.
> 
> (Whenever I read "getOlder" the Matthew Sweet song plays in my head.)

Argh. Fixed.


> @@ +4541,5 @@
> > +    Builder::Object reasonObject(cx, builder);
> > +
> > +    {
> > +        // On our calling context, enter the compartment of the frames we're
> > +        // explaining. Propagate errors from buildReason in a meaningful way.
> 
> The last sentence here is mysterious to me.
> 
> "On our calling context" is mysterious too.

That was a remnant of the multi-context design. Replaced with:

        // Enter the compartment of the frames we're explaining. The builder
        // will take care of switching back to our compartment for constructing
        // the trusted portion of the reason values.

Comment 18

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #17)
> (In reply to Jason Orendorff [:jorendorff] from comment #13)
> > I thought that reentering content from the debugger might set the reason to
> > "debugger" or something like that. After all, the caller in those cases (the
> > debugger itself) is JS and can't do it because the API is C++-only. Your
> > call though.
> 
> That's a good point. I think it should; Debugger-invoked evaluation is
> certainly "out of the blue".

I've implemented this now.

Comment 19

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> Looks good. I agree with the commit message's comments about tests needed. I
> would add:
> - native function frames get reasons too

We only create Debugger.Frame instances for JS functions, so we can't request the reason for a native function frame. I wish we could.

I've adapted a test from Panos' old patch that checks that we propagate reasons *through* native functions like Array.map, etc.

Comment 20

[Jim Blandy :jimb]

Attachment: Revised per comments r=jorendorff

Revised per comments. Try push:
https://tbpl.mozilla.org/?tree=Try&rev=efc7fba6904f

Comment 21

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #17)
> > (Whenever I read "getOlder" the Matthew Sweet song plays in my head.)
> 
> Argh. Fixed.

Nope, still happens. :(

It's OK, I don't expect you to fix that.

Comment 22

[Jim Blandy :jimb]

Attachment: revised for changes in bug 971673 r=jorendorff

Try: https://tbpl.mozilla.org/?tree=Try&rev=85ed9fec94cf

Comment 23

[Jim Blandy :jimb]

Comment on attachment 8454910 [details] [diff] [review]
Give SpiderMonkey an API that embeddings can use to safely construct values for Debugger clients.

https://hg.mozilla.org/integration/mozilla-inbound/rev/89953d3364de

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89953d3364de

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Ryan, should this be marked fixed since it's in mozilla-central now? Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Jim set [leave open] before his most-recent push, so I assume that was intentional :)