961325 - SpiderMonkey should let Firefox tell it why it called JavaScript code

Status: NEW

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jim Blandy :jimb]

At the moment, Firefox and other embeddings can call JS::Call or JS::Execute to run JavaScript code for content scripts, event handlers, XMLHttpRequest callbacks, and the like, but the embedding doesn't have any way to tell SpiderMonkey *why* it called that JS. The Debugger would like to be able to show this information: "Oh, this is an XHR callback." "Oh, this is a SetTimeout." The heap usage tools would also like to be able to associate objects with the kind of event/callback that they were allocated in the course of handling.

The problem has four parts:

- First, JSAPI needs to be extended to let embeddings tell SpiderMonkey why they're doing a particular JS invocation. That's this bug.

- Second, Firefox needs to be changed to actually use the new API, and provide the data.

- Third, Debugger needs to be extended to report the data.

- Fourth, the heap tools need to include the event types in its census categories.

I'm told the SPS "Reasons" API could be helpful here; we should look into that.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Jim, could you sketch out in more detail the work required in this bug? Am I correct to assume that it would include what you described in bug 961327 comment 2?

Also, by SPS "Reasons" API, do you by any chance mean the tools/profiler/ProfilerMarkers.h API?

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Also, I can't find JS::Execute, are you maybe referring to JS::Evaluate?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP

This is what I understood from the comments above and those in the related bugs. Jim, can you let me know how far off the mark I have been?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Make sure to follow Gecko code style in Gecko code.

Comment 5

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #2)
> Also, I can't find JS::Execute, are you maybe referring to JS::Evaluate?

I guess I meant JS_ExecuteScript and its variants:
https://hg.mozilla.org/mozilla-central/file/7d24d7dfbb53/js/src/jsapi.h#l3807

Comment 6

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #1)
> Jim, could you sketch out in more detail the work required in this bug? Am I
> correct to assume that it would include what you described in bug 961327
> comment 2?

Yes, exactly.

> Also, by SPS "Reasons" API, do you by any chance mean the
> tools/profiler/ProfilerMarkers.h API?

I think so... but let's leave that aside for now.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 8371511 [details] [diff] [review]
WIP

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

Yes, that looks lovely!

Of course, event handlers will be one of the most commonly encountered use cases, so we definitely want to get those done as soon as we can.

Even though it's what I suggested myself... it seems like there's no good reason to have *two* objects involved with every one of these registrations. Wouldn't it be sufficient to simply have the JSExecutionReason base class constructor take a cx and do the registration itself? Then we could delete AutoJSExecutionReason altogether.

We'd probably want to adopt a convention of giving subclasses names starting with "Auto" to make it more apparent why we were declaring variables that are never used.

We need to ensure that, when there are several JSExecutionReasons stacked up at the same time, that we can recover each one at the appropriate place on the stack. I think js::Activation is the right type to use for that:

https://hg.mozilla.org/mozilla-central/file/7d24d7dfbb53/js/src/vm/Stack.h#l1127

We create an instance of some subclass of Activation each time we enter the interpreter, run JIT code, or run AsmJS code, to monitor the JS stack frames that belong to that execution method. (Each execution method has its own representation for stack frames; subclassing Activation lets us use the easiest representation in each case.)

Since each invocation of JS from Firefox is going to get a fresh Activation (subclass) instance, it seems that Activation is the right place to record the JSExecutionReason that caused it. So:

- Activation should get a new JSExecutionReason * member;

- Activation's constructor should initialize its JSExecutionReason from the JSContext's reason, and clear the latter, so that child activations don't pick the reason up; and

- Activation's destructor should restore the JSContext's reason, so that any subsequent Activations (its "siblings"?) created in the same JSExecutionReason's scope will pick up the same reason.

Then it will be pretty natural to add a Debugger.Frame accessor that returns its referent frame's activation's reason.

::: js/src/jsapi.h
@@ +3944,5 @@
> +struct AutoJSExecutionReason
> +{
> +    AutoJSExecutionReason(JSContext *cx, JSExecutionReason &reason);
> +    ~AutoJSExecutionReason();
> +};

Let's actually put these in the JS namespace, and drop the "JS" from the names.

(Naturally, they'll need doc comments before they land, but this is a WIP.)

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v2

Mafe the suggested changes, but I still haven't finished reading about Activation, which is my next task.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 8372497 [details] [diff] [review]
WIP v2

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

This is looking really nice now, don't you think? I especially like the ExecutionReason declaration in jsapi.h --- there's nothing that could be removed from that!

Some further comments:

::: content/base/src/nsObjectLoadingContent.cpp
@@ +3173,5 @@
> +    JS::Rooted<JSString*> type(aCx, JS_NewStringCopyZ(aCx, "pluginCall"));
> +    JS::Rooted<JS::Value> typev(aCx, STRING_TO_JSVAL(type));
> +    if (!JS_SetProperty(aCx, obj, "type", typev)) {
> +      return nullptr;
> +    }

I wonder --- if all ExecutionReason subclasses are going to be allocating objects with 'type' properties like this, would it be worth having a static member function in ExecutionReason itself that takes a type as a char * and returns a fresh, plain object with the type property set? That might help the derived classes concentrate on what's relevant.

::: js/src/jsapi.cpp
@@ +5073,5 @@
>  
> +JS::ExecutionReason::ExecutionReason(JSContext *cx)
> +{
> +    JS_ASSERT(cx);
> +    cx->reason = this;

Ah, you actually need to save the old reason pointer, and then restore it in the destructor.

That way, at any given time, the youngest ExecutionReason is always the one the JSContext points to.

::: js/src/jsapi.h
@@ +3933,5 @@
>  {
>      return Call(cx, thisv, OBJECT_TO_JSVAL(funObj), argc, argv, rval);
>  }
>  
> +struct ExecutionReason

If you mark this with the MOZ_STACK_CLASS attribute, then we have analyses that will prevent people from misusing it. This will be especially valuable if you want to store JS::Handles in subclasses... those are only safe when their lifetimes can be assured to be a subset of the lifetime of the thing they're pointing to --- which is the case, if it's allocated on the stack.

::: js/xpconnect/src/Sandbox.cpp
@@ +823,5 @@
>      if (thisVal == ObjectValue(*sandboxGlobal)) {
>          thisVal = ObjectValue(*js::GetProxyTargetObject(sandboxProxy));
>      }
>  
> +    AutoCallableProxyReason reason(cx, proxy);

I don't know what a "callable proxy" is, but if it's a JS proxy object (and this inherits from js::Wrapper, so I think it is), then it's generally going to have a JS caller. So I don't think this particular JS::Call site is one we should mark up --- it doesn't produce calls apparently from nowhere, like event handlers and the like.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v3

Thank you for the feedback Jim, it has been very helpful!

I have incorporated all your suggestions so far and will move on to finding all the JS_ExecuteScript entry points next.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #9)
> I don't know what a "callable proxy" is, but if it's a JS proxy object (and
> this inherits from js::Wrapper, so I think it is), then it's generally going
> to have a JS caller. So I don't think this particular JS::Call site is one
> we should mark up --- it doesn't produce calls apparently from nowhere, like
> event handlers and the like.

I talked to Gabor and he confirmed this, so I took it out.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 8373445 [details] [diff] [review]
WIP v3

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

Yes, the Activation stuff looks as I'd expect.

You have lots of OOM checks missing.

For testing, it would be really nice if we could split this into two patches:

- First, a patch affecting js/src only, with a "callWithReason" shell primitive for pushing new reasons, and with the Debugger accessor. This is the patch that would include a bunch of shell-only unit tests for the Debugger accessor. (See WithSourceHook in js/src/shell/js.cpp for an example of a shell function that temporarily establishes some condition and then calls a function passed to it: https://hg.mozilla.org/mozilla-central/file/d8d8fa98ee7d/js/src/shell/js.cpp#l4397 )

- Second, a patch that has all the stuff outside js/src that makes the browser actually supply the information.

We generally try to have all aspects of Debugger testable within js/src alone; it makes debugging much easier.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +3151,5 @@
>  }
>  
> +class AutoPluginCallReason : public JS::ExecutionReason
> +{
> +  JS::Handle<JSObject*> mPlugin;

If I'm understanding correctly, it would be natural to include this as a property on the reason object. (But I don't really know what this code is doing at all...)

If we do that, keep in mind that 'explain' is called with a different current compartment (probably the Debugger's) from the one that mPlugin was constructed in. So before storing mPlugin as a property, |explain| will need to store it in some temporary Rooted<JSObject *>, and then call cx->compartment()->wrap(cx, &temp) on that, to get it properly wrapped for cx->compartment() --- the compartment in which we're building the reason object.

::: dom/workers/WorkerPrivate.cpp
@@ +5430,5 @@
>        options.setPrincipals(principal)
>          .setFileAndLine(info->mFilename.get(), info->mLineNumber);
>  
> +      if (!expression.IsEmpty()) {
> +        AutoWorkerTimeoutReason reason(aCx, info->mFilename.get(), info->mLineNumber);

Since this declaration is scoped to the { block } that it's in, it will be out of scope and destructed when we call JS::Evaluate below, and have no effect.

If you need to conditionally construct something, look at mozilla::Maybe:
https://hg.mozilla.org/mozilla-central/file/d8d8fa98ee7d/mfbt/Maybe.h#l21
You want to declare an instance of Maybe<AutoWorkerTimeoutReason> that is in the scope surrounding JS::Evaluate; and then *conditionally* call its construct(...) method. The destructor will only fire if the contained instance was actually constructed.

::: js/src/jsapi.cpp
@@ +5088,5 @@
> +
> +/* static */ Handle<JSObject*>
> +JS::ExecutionReason::createWithType(JSContext *cx, const char *type)
> +{
> +    Rooted<JSObject*> obj(cx, JS_NewObject(cx, nullptr, NullPtr(), NullPtr()));

It seems like we use NewObjectWithClassProto within SpiderMonkey.

@@ +5090,5 @@
> +JS::ExecutionReason::createWithType(JSContext *cx, const char *type)
> +{
> +    Rooted<JSObject*> obj(cx, JS_NewObject(cx, nullptr, NullPtr(), NullPtr()));
> +    if (!obj)
> +      return NullPtr();

This and the one below can be nullptr, if the return type changes to 'JSObject *' (see below).

@@ +5093,5 @@
> +    if (!obj)
> +      return NullPtr();
> +
> +    Rooted<JSString*> jstype(cx, JS_NewStringCopyZ(cx, type));
> +    Rooted<Value> typev(cx, STRING_TO_JSVAL(jstype));

In SpiderMonkey you should use StringValue, not STRING_TO_JSVAL.

@@ +5096,5 @@
> +    Rooted<JSString*> jstype(cx, JS_NewStringCopyZ(cx, type));
> +    Rooted<Value> typev(cx, STRING_TO_JSVAL(jstype));
> +    if (!JS_SetProperty(cx, obj, "type", typev)) {
> +      return NullPtr();
> +    }

We don't brace single-line-body, single-line-condition ifs in SpiderMonkey.

::: js/src/jsapi.h
@@ +3947,5 @@
> +    ExecutionReason *previousReason;
> +
> +    ExecutionReason(JSContext *cx);
> +    ~ExecutionReason();
> +    static Handle<JSObject*> createWithType(JSContext *cx, const char* type);

This should return JSObject *, not Handle<JSObject *>. A Handle is just a pointer to a rooted location --- and in this case, you'd be returning a pointer to the RootedObject that was local to the function that just returned. So it's returning a pointer off the end of the stack.

Then, the callers should all be using Rooted<JSObject *>.

::: js/src/jscntxt.h
@@ +534,5 @@
>      inline JSScript *currentScript(jsbytecode **pc = nullptr,
>                                     MaybeAllowCrossCompartment = DONT_ALLOW_CROSS_COMPARTMENT) const;
>  
> +    /*
> +     * The most recent reason for JavaScript code execution in this context.

"The most recent live reason..."

Comment 13

[Jim Blandy :jimb]

Attachment: Draft of WithExecutionReason shell test function

Here's a draft of what a WithExecutionReason shell function might look like. Untested; just wanted to get the idea across.

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #12)
> You have lots of OOM checks missing.

Yeah.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Thanks for the feedback, keep it coming!
I'm going to split the patch as you suggested and I assume I should attach the Gecko part to bug 961327? Or would you rather keep both in this bug?

(In reply to Jim Blandy :jimb from comment #12)
> ::: content/base/src/nsObjectLoadingContent.cpp
> @@ +3151,5 @@
> >  }
> >  
> > +class AutoPluginCallReason : public JS::ExecutionReason
> > +{
> > +  JS::Handle<JSObject*> mPlugin;
> 
> If I'm understanding correctly, it would be natural to include this as a
> property on the reason object. (But I don't really know what this code is
> doing at all...)
> 
> If we do that, keep in mind that 'explain' is called with a different
> current compartment (probably the Debugger's) from the one that mPlugin was
> constructed in. So before storing mPlugin as a property, |explain| will need
> to store it in some temporary Rooted<JSObject *>, and then call
> cx->compartment()->wrap(cx, &temp) on that, to get it properly wrapped for
> cx->compartment() --- the compartment in which we're building the reason
> object.

My plan was to see if there are any useful properties on the plugin for identification purposes, like "Java" or "Flash" and possibly location information, which would be strings. The "object" tag has "data" and "type" properties that I believe would be useful and a few others as well. I don't think it would be useful to hold a reference to the plugin object itself.

> @@ +5090,5 @@
> > +JS::ExecutionReason::createWithType(JSContext *cx, const char *type)
> > +{
> > +    Rooted<JSObject*> obj(cx, JS_NewObject(cx, nullptr, NullPtr(), NullPtr()));
> > +    if (!obj)
> > +      return NullPtr();
> 
> This and the one below can be nullptr, if the return type changes to
> 'JSObject *' (see below).
> 
> ::: js/src/jsapi.h
> @@ +3947,5 @@
> > +    ExecutionReason *previousReason;
> > +
> > +    ExecutionReason(JSContext *cx);
> > +    ~ExecutionReason();
> > +    static Handle<JSObject*> createWithType(JSContext *cx, const char* type);
> 
> This should return JSObject *, not Handle<JSObject *>. A Handle is just a
> pointer to a rooted location --- and in this case, you'd be returning a
> pointer to the RootedObject that was local to the function that just
> returned. So it's returning a pointer off the end of the stack.
> 
> Then, the callers should all be using Rooted<JSObject *>.

I can't seem to make this work. The compiler complains about "no viable conversion from 'JSObject *' to 'JS::Rooted<JSObject *>'" when I do:

JS::Rooted<JSObject*> reason = createWithType(aCx, "workerScriptLoader");

I'm probably doing something dumb, but I just can't see it.

Comment 16

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

RootedObject reason(cx, createWithType(aCx, "workerScriptLoader"));

Comment 17

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #15)
> Thanks for the feedback, keep it coming!
> I'm going to split the patch as you suggested and I assume I should attach
> the Gecko part to bug 961327? Or would you rather keep both in this bug?

Yes, that's the bug for that work. Then this bug can become a lovely shell-only project.

Comment 18

[Jim Blandy :jimb]

Yeah, what Nick said. You need a cx to construct a Rooted, so initialization doesn't look quite as you'd expect.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v4

OK, this is the last version before the split, which I'll do tomorrow, when I'll also look into writing some tests.

Comment 20

[Jim Blandy :jimb]

Comment on attachment 8374992 [details] [diff] [review]
WIP v4

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

It seems like the return value from createWithType is not being checked by its callers; that's a fallible function, so if it returns an error, that needs to be propagated out. (If you know all this and are going to take care of it later, let me know and I'll stop checking for it.)

::: js/src/jsapi.cpp
@@ +5079,5 @@
> +}
> +
> +JS::ExecutionReason::~ExecutionReason()
> +{
> +    cx_->reason = previousReason;

Here, we should assert that cx_->reason == this.

::: js/src/jsapi.h
@@ +3967,5 @@
> + */
> +struct MOZ_STACK_CLASS ExecutionReason
> +{
> +    JSContext *cx_;
> +    ExecutionReason *previousReason;

You probably want to make this a class, and give the data members 'protected' or 'private' visibility. createWithType should be 'protected', since it's only for use by subclass methods.

::: js/src/jscntxt.h
@@ +540,5 @@
> +     * loaders, sandboxes etc. and all these reasons, when they happen
> +     * consecutively, will form a stack. This member variable will always point
> +     * at the top of that stack.
> +     */
> +    JS::ExecutionReason *reason;

The constructor in jscntxt.cpp should initialize this to nullptr; and the destructor should perhaps assert that it is nullptr (we can't destroy a JSContext while it has running frames, I'm pretty sure).

::: js/src/shell/js.cpp
@@ +4012,5 @@
>      return true;
>  }
>  
> +class AutoShellReason : public JS::ExecutionReason {
> +    HandleValue data;

It seems like this should be a RootedValue, and then we should say ... data(cx, data) ... in the constructor's initializer list.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v5

SpiderMonkey part, sans tests.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #20)
> (If you know all this and are going to take care
> of it later, let me know and I'll stop checking for it.)

I was going to take care of it, but please keep checking for it :-)
I'm not to be trusted with heavy machinery.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v6

Added Debugger.Frame.prototype.reason and a test, but it doesn't look like I get the right reason back. I'm confused by how I'm supposed to get the activation from a frame.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v7

Still trying to figure out why it doesn't work, but passing a JSContext to explain() seemed redundant as we already hold a reference to it, so I took it out.

Comment 25

[Jim Blandy :jimb]

Comment on attachment 8375670 [details] [diff] [review]
WIP v7

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

This is looking good. It's nice to see it separated out into shell-only stuff; that makes it *so* much easier to test.

::: js/src/jit-test/tests/debug/Frame-04.js
@@ +1,1 @@
> +// Execution reason for a debugger frame.

This file should be called Frame-reason-01.js, probably (or Frame-executionReason-01.js; see my comments in Debugger.cpp).

@@ +3,5 @@
> +var g = newGlobal();
> +var dbg = Debugger(g);
> +var hits = 0;
> +dbg.onDebuggerStatement = function (frame) {
> +    assertEq(!frame.reason, false);

How about: assertEq(typeof frame.reason, 'object');

::: js/src/jsapi.cpp
@@ +5079,5 @@
>      return &args.rval().toObject();
>  }
>  
> +JS::ExecutionReason::ExecutionReason(JSContext *cx)
> +: previousReason(cx->reason), cx_(cx)

indentation

@@ +5094,5 @@
> +
> +/* static */ JSObject *
> +JS::ExecutionReason::createWithType(JSContext *cx, const char *type)
> +{
> +    Rooted<JSObject*> obj(cx, NewObjectWithClassProto(cx, nullptr, nullptr,

The style within the JS engine is to use RootedObject, RootedValue, HandleValue, and so on.

::: js/src/jscntxt.h
@@ +538,5 @@
> +     * The most recent live reason for JavaScript code execution in this
> +     * context. Script execution can be initiated by event handlers, script
> +     * loaders, sandboxes etc. and all these reasons, when they happen
> +     * consecutively, will form a stack. This member variable will always point
> +     * at the top of that stack.

This comment should note that creating an Activation steals this pointer, leaving it null while the Activation is live.

::: js/src/shell/js.cpp
@@ +4016,5 @@
> +    RootedValue data;
> +
> +  public:
> +    AutoShellReason(JSContext *cx, HandleValue data)
> +        : JS::ExecutionReason(cx), data(cx, data) { }

indentation

::: js/src/vm/Debugger.cpp
@@ +4182,5 @@
> +{
> +    THIS_FRAME_ITER(cx, argc, vp, "get reason", args, thisobj, _, iter);
> +    Debugger *dbg = Debugger::fromChildJSObject(thisobj);
> +
> +    for (++iter; !iter.done(); ++iter) {

Why is there a loop needed here? The iter created by THIS_FRAME_ITER is always referring to the Debugger.Frame instance's referent already. That means that we should just be able to grab iter.activation() right after the THIS_FRAME_ITER call, without messing with iter at all.

(A lot of the stack-related "iterators" in SpiderMonkey seem to be used this way in practice --- people really only care about the top frame, so the iterator is never ++'ed at all. For example, see the way js::CurrentScriptFileLineOrigin in jsscript.h uses NonBuiltinScriptFrameIter.)

@@ +4186,5 @@
> +    for (++iter; !iter.done(); ++iter) {
> +        if (iter.isIon())
> +            continue;
> +        if (dbg->observesFrame(iter.abstractFramePtr())) {
> +            if (!iter.activation()->cx()->reason) {

Remember that when we construct an Activation, it steals the reason pointer from its JSContext for the Activation's lifetime. So activation->cx->reason will always be null --- simply because the activation *exists*. You want, simply, activation->reason. (Perhaps activation needs a 'reason' accessor, since reason is private?)

The body of this 'if' ought to be all there is in the function, unless I'm missing something.

@@ +4663,5 @@
>      JS_PSG("generator", DebuggerFrame_getGenerator, 0),
>      JS_PSG("live", DebuggerFrame_getLive, 0),
>      JS_PSG("offset", DebuggerFrame_getOffset, 0),
>      JS_PSG("older", DebuggerFrame_getOlder, 0),
> +    JS_PSG("reason", DebuggerFrame_getReason, 0),

I think "reason" is too vague a name for this property. "executionReason" would be better.

::: js/src/vm/Stack.h
@@ +1146,5 @@
>  
> +    /*
> +     * The reason for JavaScript code execution in this activation.
> +     */
> +    JS::ExecutionReason *reason_;

Since reason_ is private, we probably want a public reason() accessor.

Comment 26

[Jim Blandy :jimb]

Comment on attachment 8375670 [details] [diff] [review]
WIP v7

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

::: js/src/jsapi.h
@@ +3978,5 @@
> +
> +  public:
> +    ExecutionReason(JSContext *cx);
> +    ~ExecutionReason();
> +    virtual JSObject *explain() = 0;

Are you sure that explain doesn't need a JSContext? The browser switches contexts at times that I don't really understand --- and it's critical that 'explain' construct its value in the debugger's compartment, not in the debuggee compartment.

Let's add back the JSContext argument to explain, and MOZ_ASSERT that it's equal to cx_ --- I'll bet that assertion will fail when we write mochitests to check browser-provided reasons.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v8

OK, I didn't know that about JSContext. Change reverted. And thanks for the simple way to get the activation from the frame iter, I was hoping for something like that. And of course, it goes without saying that activation->cx->reason is just me being retarted.

I have updated the patch, but I still don't get an executionReason != null in the test. Adding some logging I see this sequence of events:

new Activation: stealing null
new Activation: stealing null
~Activation: giving back null
new Activation: stealing null
~Activation.:giving back null
new Activation: stealing null
~Activation: giving back null
~Activation: giving back null
new Activation: stealing null
~Activation: giving back null
new Activation: stealing null
~Activation: giving back null
new Activation: stealing null
new ExecutionReason: cx->reason = new reason
new Activation stealing not null
new Activation: stealing null
new Activation: stealing null
~Activation: giving back null
~Activation: giving back null
~Activation: giving back not null
~ExecutionReason: cx->reason = restoring null
~Activation: giving back null
TypeError: frame.executionReason is null

Still investigating...

Comment 28

[Jim Blandy :jimb]

That all looks correct --- by the time you've called frame.executionReason, we've left the scope of the ExecutionReason, haven't we?

Comment 29

[Jim Blandy :jimb]

Note that, since every global gets a separate compartment (the famed "compartment-per-global" principle), the 'g.eval' call is a cross-compartment call. Cross-compartment calls, like many other operations (getter and setter invocations; iterator re-activations) create a fresh Activation. This means that the innermost Activation may not be the one we want.

Perhaps we do need DebuggerFrame_getExecutionReason to create an ActivationIterator, and then scan activations until we find one with a non-null reason.

Comment 30

[Jim Blandy :jimb]

... That's not quite right, as there are going to be many sorts of Activations on the chain, I would assume, including those of non-debuggee code, which it seems strange to expose.

We need to talk a little bit with Jason or Shu about how to find the innermost Activation with a reason that is really part of 'this' frame's continuation.

Comment 31

[Jim Blandy :jimb]

... But certainly, just checking the top Activation is flat-out wrong, as new Activations are introduced by all kinds of extraneous constructs. For reasons, the effect we want is that of a dynamic parameter.

Comment 32

[Jim Blandy :jimb]

What we want is to walk Activations just as the ScriptFrameIter constructed by THIS_FRAME_ITER would, with the constraints in DebuggerFrame_getOlder.

Comment 33

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v9

Fixed the activation iteration as discussed in the work week and now the test passes! \o/

I remember you had suggested some more tests that I could write, but I can't remember what those tests were. Could you refresh my memory?

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v10

I thought of a few more tests that should be useful.

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v11

Rebased.

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v12

Switched from JS_SetProperty to JS_DefineProperty per bz's request in bug 961327.

Comment 37

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v13

Moved the execution reason from JSContext to JSRuntime, per bz's and bholley's requests in bug 961327. As I understand it we are going to get rid of JSContext in the future.

Comment 38

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #33)
> I remember you had suggested some more tests that I could write, but I can't
> remember what those tests were. Could you refresh my memory?

I thought I'd made some suggestions too, but I can't find them. I'm going to try to be more careful in the future about writing things in the appropriate places...

Anyway, we should test that the following expectations are met:

- When multiple execution reasons are active, as in Frame-executionReason-02.js, the older Debugger.Frames should see the older execution reasons.

- When multiple execution reasons are active, returning from the newer frames past the newer execution reason makes the older execution reason visible.

- When we have several frames within the scope of a given execution reason, all of them should report the same execution reason.

- If we establish an execution reason for a call to code in global A, which then calls a function in another global B, asking for the execution reason of the stack frame in global B returns the original execution reason. In other words, crossing globals doesn't erase execution reasons.

- Invoking getters, setters, and proxy handlers preserves execution reasons.

- Calls to JS function proxies *other than CCWs* preserves execution reasons.

- Implicit toString and valueOf calls preserve execution reasons.

- Generator bodies can see execution reasons. Test both old-style and new-style generators.

- Calls through C++, like "foo".replace(/o/, function () { return "x"; }), preserve execution reasons. I think all the Array functions that take callbacks, like A.p.map and .reduce, are now self-hosted; but it would be good to test those, too.

- In a multiple-Debugger setup, one Debugger mutating the execution reason object doesn't affect what other Debuggers see, nor the reason object returned by older frames within the same execution reason's scope.

- I know that we're trying to cuddle the JS::ExecutionReason subclass instances up as close to the JSAPI invocation calls as possible, but if there are any cases where we have multiple JSAPI invocation calls within the "scope" of a single ExecutionReason instance, we should make sure that the reason is indeed visible to both the calls. I see the Frame-executionReason-03.js test, but I'm specifically concerned about the maneuver where, when we pop an Activation, it places the reason back in the runtime. That's the kind of delicate hand-off that we should verify is actually taking place.

- Mochitests showing that dispatchEvent either does preserve, or does not preserve (what do we want here? "establishes a new execution reason", right?) execution reasons.

- The 'saveFrameChain' argument to 'evaluate' should hide the ambient execution reason.

- I *believe* JS::HideScriptedCaller should have no effect on execution reasons. And we have no shell-visible primitives for testing that. So I think we can leave that untested.

Comment 39

[Jim Blandy :jimb]

Oh, obviously:

- eval (direct and indirect), 'evaluate', Function.prototype.call and .apply all preserve execution reasons

Comment 40

[Jim Blandy :jimb]

Comment on attachment 8398483 [details] [diff] [review]
v13

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

This looks great; my only concern is the wrapping behavior in 'explain', and the tests.

Clearing review for now; when you have those ready, re-request review and I'll give it a quick look-over.

::: js/src/jsapi.h
@@ +3801,5 @@
>  
> +/*
> + * The base class for objects that can identify the reason for the execution of
> + * a script. Every concrete subclass must provide an |explain| function that
> + * returns a JSObject with a |type| property indicating the reason of the

"the reason for the"

@@ +3810,5 @@
> +{
> +    ExecutionReason *previousReason;
> +
> +  protected:
> +    JSRuntime *rt_;

In SpiderMonkey we don't usually use the 'X_' convention for member variable names unless we have a getter named 'X'. So this should just be 'rt'. Note that the constructor can still take an 'rt' argument, if it refers to its own member as this->rt. In the initializer list, rt(rt) is actually unambiguous, as the former must refer to a member, and the latter is evaluated in the scope containing the constructor's arguments.

@@ +3816,5 @@
> +
> +  public:
> +    ExecutionReason(JSRuntime *rt);
> +    ~ExecutionReason();
> +    virtual JSObject *explain(JSContext *cx) = 0;

This needs a comment clarifying the allocation behavior, perhaps:

"Return an execution reason object freshly allocated in the current compartment, explaining why the JS invoked in this ExecutionReason's dynamic scope was run."

::: js/src/shell/js.cpp
@@ +4360,5 @@
> +    RootedValue data;
> +
> +  public:
> +    AutoShellReason(JSRuntime *rt, HandleValue data)
> +      : JS::ExecutionReason(rt), data(rt->contextList.getLast(), data) { }

Data can just take 'rt' directly here, it seems. No need for '->contextList.getLast()'.

@@ +4363,5 @@
> +    AutoShellReason(JSRuntime *rt, HandleValue data)
> +      : JS::ExecutionReason(rt), data(rt->contextList.getLast(), data) { }
> +
> +    JSObject *explain(JSContext *cx) MOZ_OVERRIDE {
> +        MOZ_ASSERT(cx == rt_->contextList.getLast());

This assertion is contrary to our requirements, I think.

@@ +4368,5 @@
> +        RootedObject reason(cx, createWithType(cx, "JS shell"));
> +        if (!reason)
> +            return nullptr;
> +        if (!JS_DefineProperty(cx, reason, "data", data, nullptr, nullptr,
> +                               JSPROP_ENUMERATE))

This is going to freak out if cx's compartment is different from data's. You'll need to move data to a temporary variable, do cx->compartment->wrap() on that (see the many other uses of this pattern elsewhere in the code), and then store the wrapped data as reason's 'data' property.

But! Some of the tests I suggested should catch this! So write those tests first, and see if you get this crash. If not, something surprising is going on.

@@ +4392,5 @@
> +    }
> +
> +    AutoShellReason reason(cx->runtime(), args[0]);
> +    RootedObject fun(cx, &args[1].toObject());
> +    return Call(cx, UndefinedHandleValue, fun, JS::HandleValueArray::empty(), args.rval());

If I'm reading correctly,
1) Call takes the function as a JS::HandleFunction, and
2) args[1] is a MutableValueHandle.

So you should be able to simply pass args[1] directly as the 'fun' argument to Call.

::: js/src/vm/Debugger.cpp
@@ +4225,5 @@
> +    THIS_FRAME_ITER(cx, argc, vp, "get reason", args, thisobj, _, iter);
> +    if (iter.done()) {
> +        args.rval().setNull();
> +        return true;
> +    }

Can this actually happen??? What does it mean for a Debugger.Frame's own iter to be immediately 'done'??? I thought a Debugger.Frame always referred to a real frame. Have we reached Peak Question Marks??????

@@ +4229,5 @@
> +    }
> +
> +    Activation *activation = iter.activation();
> +    while (activation && !activation->reason())
> +        activation = activation->prev();

This iteration should stop early if hasSavedFrameChain() is set, and return null.

I think my suggested 'saveFrameChain' test will notice that this is walking into activations with non-zero savedFrameChain_ counts; again, let's leave the bug in and see if the test catches it.

Comment 41

[Jim Blandy :jimb]

Comment on attachment 8398483 [details] [diff] [review]
v13

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

::: js/src/vm/Debugger.cpp
@@ +4222,5 @@
> +static bool
> +DebuggerFrame_getExecutionReason(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    THIS_FRAME_ITER(cx, argc, vp, "get reason", args, thisobj, _, iter);
> +    if (iter.done()) {

I've filed bug 996265 to assert !iter.done() in THIS_FRAME_ITER, so this check can go away entirely.

Comment 42

[Jim Blandy :jimb]

Looking through the discussion in bug 979730, it seems like JS_SavedFrameChain is on its way out, in favor of checks based on principal subsumption. Hum.

Comment 43

[Jim Blandy :jimb]

[argh, I wrote this all up, and then apparently closed the tab or otherwise dropped it, and I can't find it...]

We should be sure to document this in js/src/doc/Debugger, since that's in-tree now.

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 44

[Jim Blandy :jimb]

The documentation in js/src/doc/Debugger should cover the changes to Debugger, but a new page in that directory should accumulate descriptions of execution reason properties we've implemented to date.

Comment 45

[Jim Blandy :jimb]

Would it be worthwhile to have a function that returns the next-youngest Debugger.Frame with a different execution reason? Then one could walk reasons, not just stack frames.

Comment 46

[Bobby Holley (:bholley)]

See also bug 979730 comment 13.

Comment 47

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v14

I believe I've addresses all of your comments, except for the mochitest, which I'm going to add in the other patch in bug 961327. I couldn't get any test to crash or fail even without the wrapping in 'explain', but I added the wrapping anyway. Frame-executionReason-11.js is specifically for that, but it passes either way. I guess keeping it is better than leaving it out?

I added the doc changes for the new property in the Debugger.Frame doc. It doesn't look to me like its size warrants an additional file, but let me know if you'd prefer that instead.

Regarding your thoughts about how execution reason providers and consumers will evolve, I wonder how many consumers do you imagine coming up in the future, besides our own tools? As the providers are part of Gecko and therefore under our own control, it seems that we could be aggressive about introducing new 'type' values if we feel it's safe. To me it looks like the number of tools that would use such information will be quite limited, so I'm not that worried about getting bad fallback behavior from unknown 'type' values. In addition to that, I can't think of a UI that would present execution reasons in a way that a fallback value would look really bad. It seems to me that if we are careful not to overload a 'type' value with incompatible meanings, an older consumer will just group those reasons and only miss showing the fine details until it gets updated. I totally agree about duck typing being the way to use this API though, which probably makes this argument rather academic.

For adding a function to walk reasons, not just stack frames, I don't see the need, yet. The UIs that I have in mind for both the debugger and the memory tool won't be showing a string of reasons independently of the string of frames, so it seems to me that adding it would be premature at this point. If you have a particular visualization in mind, let me know.

Lastly, I read the comments in bug 979730, but I know nothing yet about AutoEntryScript or how it's used, so I'm wondering if we could move forward for the time being with this approach and refactor afterwards? Would that be acceptable or do you think that an iterative approach will end up being significantly more work?

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v13-v14 Interdidff

Here is an interdiff to help your review.

Comment 49

[Bobby Holley (:bholley)]

(In reply to Panos Astithas [:past] from comment #47)
> Lastly, I read the comments in bug 979730, but I know nothing yet about
> AutoEntryScript or how it's used, so I'm wondering if we could move forward
> for the time being with this approach and refactor afterwards? Would that be
> acceptable or do you think that an iterative approach will end up being
> significantly more work?

I think that the js/src parts of this patch are going to have to be undone, at which I think we might as well do it right the first time.

Basically, Spidermonkey should take a callback (set in XPCJSRuntime.cpp, along with the jillion other callbacks you'll find there) that lets it just ask the embedding for the current execution reason. That code can then query the execution reason of the topmost entry point (see ScriptSettings.cpp). When callers create an AutoEntryScript, we can just force them to provide some kind of execution reason to the constructor.

Currently, we don't yet instantiate an AutoEntryScript for all the places where we call into the JS engine (bug 951991 tracks this work). But 90% of the work there is a long tail that devtools is unlikely to care about, so we can pretty easily ensure that we have an AutoEntryScript in all of the places that you'll need it. 

Make sense? Feel free to ping me on IRC if you want to talk about it more.

Comment 50

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Alternative approach

Bobby is this something close to what you have in mind? It's not finished and in fact it doesn't even compile, as I don't know how to reach XPCJSRuntime from SpiderMonkey. But if I understand correctly, the basic idea is that the execution reasons are stored in the script settings, not in the runtime, and activations don't steal them. Instead XPCJSRuntime can retrieve the youngest one, but then how does the debugger API use that?

Also, is this approach compatible with Jim's idea of keeping a JS shell-only patch for this bug and doing the Gecko work in bug 961327, or do I need to use xpcshell for testing?

Is my understanding correct that I can't use the WebIDL MozExecutionReason from that bug in the debugger API and I still need to keep the JSAPI one as well?

Comment 51

[Andrew McCreight [:mccr8]]

> as I don't know how to reach XPCJSRuntime from SpiderMonkey

The way to do this has a few parts.

1. Define a type for a callback in jsapi.h
2. Add a field to JSRuntime to store a callback of that type.  Initialize the field to null in the ctor for JSRuntime.
3. In the JS engine, use the new callback.  You probably want to do a null check, because odds are it will end up getting called from situations (like, maybe in some kind of stand alone shell build) where there is no browser, and thus nothing gets registered.
4. Create a function in jsapi.h or jsfriendapi.h to register the callback, by storing it on a JSRuntime.
5. Implement the callback in the browser.
6. Register the callback.  Where you do this depends on whether you want it only on the main thread, or in both workers and the main thread.  If you only want it on the main thread, register it in the constructor for XPCJSRuntime.  If you want it on both, register it in the constructor for CycleCollectedJSRuntime.

If you'd like an example of this, check out JS_SetDestroyCompartmentCallback.  See where that is called, the field it sets, and how that all works.

Comment 52

[Bobby Holley (:bholley)]

Comment on attachment 8415379 [details] [diff] [review]
Alternative approach

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

Looks good overall!

::: dom/base/ScriptSettings.h
@@ +57,5 @@
>  //   Components.utils.
>  nsIPrincipal* GetWebIDLCallerPrincipal();
>  
> +// TODO
> +JS::ExecutionReason* GetExecutionReason();

I'd call this "GetScriptEntryReason" or somesuch.

@@ +172,5 @@
>    AutoEntryScript(nsIGlobalObject* aGlobalObject,
>                    bool aIsMainThread = NS_IsMainThread(),
>                    // Note: aCx is mandatory off-main-thread.
> +                  JSContext* aCx = nullptr,
> +                  JS::ExecutionReason* aReason = nullptr);

Is there any reason for this to be optional? I think it should be a mandatory second argument, and we should make it an nsString or something else that's super convenient to generate inline. What's the use case of all the fancy RAII explain() machinery?

@@ +179,5 @@
>    void SetWebIDLCallerPrincipal(nsIPrincipal *aPrincipal) {
>      mWebIDLCallerPrincipal = aPrincipal;
>    }
>  
> +  JS::ExecutionReason* GetExecutionReason() {

And given that I think it should be non-optional, this should just be "ExecutionReason()".

::: js/src/vm/Debugger.cpp
@@ +4383,5 @@
> +DebuggerFrame_getExecutionReason(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    THIS_FRAME_ITER(cx, argc, vp, "get reason", args, thisobj, _, iter);
> +
> +    // XXX: how do I get a ref to XPCJSRuntime or nsXPConnect?

See mccr8's comments about how to set this up with a callback.

::: js/xpconnect/src/xpcprivate.h
@@ +561,5 @@
>  
>      PRTime GetWatchdogTimestamp(WatchdogTimestampCategory aCategory);
>      void OnAfterProcessNextEvent() { mSlowScriptCheckpoint = mozilla::TimeStamp(); }
>  
> +    ExecutionReason* GetExecutionReason();

I don't think you need this.

Comment 53

[Bobby Holley (:bholley)]

(In reply to Panos Astithas [:past] from comment #50)

> Also, is this approach compatible with Jim's idea of keeping a JS shell-only
> patch for this bug and doing the Gecko work in bug 961327, or do I need to
> use xpcshell for testing?

Given that this approach involve storing the execution reason in the embedding, you'll either need to test it with the browser, or write a custom reason provider callback in js.cpp.
 
> Is my understanding correct that I can't use the WebIDL MozExecutionReason
> from that bug in the debugger API and I still need to keep the JSAPI one as
> well?

I don't understand this question.

Comment 54

[Panos Astithas (he/him) [:past] (please ni?)]

Thanks for the feedback! Some clarifications until I come back with an updated patch:

(In reply to Bobby Holley (:bholley) from comment #52)
> > +                  JS::ExecutionReason* aReason = nullptr);
> 
> Is there any reason for this to be optional? I think it should be a
> mandatory second argument, and we should make it an nsString or something
> else that's super convenient to generate inline. What's the use case of all
> the fancy RAII explain() machinery?

The idea is that the tools will need to know more than one thing, e.g. the type of the reason (script loader, event handler, evalInSandbox, etc.), possibly a subtype as well (click handler, keyup handler, etc.), the script location that caused this execution (URL, line, maybe even column), and maybe more that I haven't though of yet. Does that make sense?

As for whether it should be optional, I thought that we would want to not force every singe call site right away to provide an execution reason, but grepping through the code there don't seem to be that many uses of AutoEntryScript, so maybe I should make it mandatory.

(In reply to Bobby Holley (:bholley) from comment #53)
> (In reply to Panos Astithas [:past] from comment #50)
> > Is my understanding correct that I can't use the WebIDL MozExecutionReason
> > from that bug in the debugger API and I still need to keep the JSAPI one as
> > well?
> 
> I don't understand this question.

What I mean is that ideally I'd like to represent execution reasons via a single class, not both MozExecutionReason and JS::ExecutionReason. As far as I understand it though, SpiderMonkey cannot use WebIDL APIs and bz rightfully suggested that using WebIDL would simplify things in Gecko. Now that it seems that I can't (easily) use the js shell for tests, the only place in SpiderMonkey code that would absolutely need to deal with execution reasons is Debugger.cpp. It would be nice if there was a way to use MozExecutionReason in that, somehow.

Comment 55

[Bobby Holley (:bholley)]

(In reply to Panos Astithas [:past] from comment #54)
> The idea is that the tools will need to know more than one thing, e.g. the
> type of the reason (script loader, event handler, evalInSandbox, etc.),
> possibly a subtype as well (click handler, keyup handler, etc.), the script
> location that caused this execution (URL, line, maybe even column), and
> maybe more that I haven't though of yet. Does that make sense?

Yes. But I want it to be simple to add an execution reason that's just a string, so that in the common case it isn't a PITA to specify an execution reason.

> As for whether it should be optional, I thought that we would want to not
> force every singe call site right away to provide an execution reason, but
> grepping through the code there don't seem to be that many uses of
> AutoEntryScript, so maybe I should make it mandatory.

Yes. Anyone instantiating an AutoEntryScript should have a crystal-clear understanding of why the expect to execute script. And again, the ergonomics should allow for doing this without creating an entire class to convey the execution reason.

> (In reply to Bobby Holley (:bholley) from comment #53)
> > (In reply to Panos Astithas [:past] from comment #50)
> > > Is my understanding correct that I can't use the WebIDL MozExecutionReason
> > > from that bug in the debugger API and I still need to keep the JSAPI one as
> > > well?
> > 
> > I don't understand this question.
> 
> What I mean is that ideally I'd like to represent execution reasons via a
> single class, not both MozExecutionReason and JS::ExecutionReason. As far as
> I understand it though, SpiderMonkey cannot use WebIDL APIs and bz
> rightfully suggested that using WebIDL would simplify things in Gecko. Now
> that it seems that I can't (easily) use the js shell for tests, the only
> place in SpiderMonkey code that would absolutely need to deal with execution
> reasons is Debugger.cpp. It would be nice if there was a way to use
> MozExecutionReason in that, somehow.

It seems like the best design is to for JS engine to just have a callback that says "explain my current execution reason" (returning a JSObject). So we should probably eliminate JS::ExecutionReason entirely.

Once that's done, we can represent the reason objects as a MozExecutionReason, and just provide the result of reason->Wrap(cx) (which is a JSObject*) to the JS engine. Make sense?

Comment 56

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Alternative approach v2

This time I got something that almost works! I've included the parts from the patch in bug 961327 that are still valid and the mochitests almost pass, with the only failures being the missing URL and line numbers.

I tried to keep the parameter to AutoEntryScript a string and have the callback infer the URL and line number from that, but I couldn't see how. Hopefully I will be pointed out something obvious that I've missed, but in case there is no way to do it, I can only see 2 options:

(a) pass a MozExecutionReason as a parameter to AutoEntryScript
(b) add 3 new arguments to AutoEntryScript: type, url & line

An advantage of (a) is that it's open-ended and we can easily provide more information in some cases (e.g. event type) in a clean way. To be clear, I don't think the API will be useful for us if it only provides the execution reason as a simple string.

I'm not sure if the types I've added to AutoEntryScript instantiations are sane, so I'd like to get some feedback on that, too.

The fact that I just had to scan the code base for AutoEntryScript made things much easier, but at the same time I worry that we may be sacrificing some flexibility, by being too far from the useful information. For example I don't see how to get more information about the kind of callback that is being executed from inside CallbackObject.cpp, while I had managed to get that information in the previous patch. I wonder if it would be viable to create a MozExecutionReason early on and somehow pass it on until we get to teh point where an AutoScriptEntry is created.

I also have an almost-finished support for the JS shell helper that I've been using for tests, but it seems to me that I should just throw it away, as the code paths are different from the production code and the tests that I tried to preserve aren't testing useful things in the new design. I haven't modified the tests from the previous versions of the patch(es) or added/removed any.

One potentially ugly workaround that I had to use was to change the visibility of ScriptSettingsStackEntry in AutoScriptEntry, due to a compiler error that I couldn't understand:

In file included from /home/past/src/fx-team/obj-ff-dbg/dom/base/Unified_cpp_dom_base0.cpp:171:
/home/past/src/fx-team/dom/base/ScriptSettings.cpp:206:26: error: cannot cast protected base class 'mozilla::dom::ScriptSettingsStackEntry' to 'mozilla::dom::AutoEntryScript'
  AutoEntryScript* aes = static_cast<AutoEntryScript*>(entry);
                         ^
../../dist/include/mozilla/dom/ScriptSettings.h:173:25: note: declared protected here
                        protected ScriptSettingsStackEntry {
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The exact same code doesn't cause any errors in GetWebIDLCallerPrincipal() (the immediately adjacent function in ScriptSettings.cpp).

Comment 57

[Bobby Holley (:bholley)]

Comment on attachment 8416604 [details] [diff] [review]
Alternative approach v2

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

(In reply to Panos Astithas [:past] from comment #56)
> Created attachment 8416604 [details] [diff] [review]
> Alternative approach v2
> 
> This time I got something that almost works! I've included the parts from
> the patch in bug 961327 that are still valid and the mochitests almost pass,
> with the only failures being the missing URL and line numbers.

Great!

> I tried to keep the parameter to AutoEntryScript a string and have the
> callback infer the URL and line number from that, but I couldn't see how.

This is information that we should get from the JS engine - when someone instantiates an AutoEntryScript, they generally don't have easy access to the file and line of whatever script or function will eventually start running.

However, you're right that, at the moment, there's no good way of getting this information from the engine. So we should add one! I'll file a bug for that now.

> An advantage of (a) is that it's open-ended and we can easily provide more
> information in some cases (e.g. event type) in a clean way. To be clear, I
> don't think the API will be useful for us if it only provides the execution
> reason as a simple string.

I'm fine to have the option of additional information, but I want it to be convenient to make an AutoEntryScript in the common case. So Something like:

AutoEntryScript aes(global, nsPrintfCString("The %dth callback in this kingdom!", foopy));

is easy to do. If someone wants to add information, they can do:

aes.SetEventType(A_VERY_IMPORTANT_KIND_OF_EVENT);
 
> The fact that I just had to scan the code base for AutoEntryScript made
> things much easier, but at the same time I worry that we may be sacrificing
> some flexibility, by being too far from the useful information. For example

> I don't see how to get more information about the kind of callback that is
> being executed from inside CallbackObject.cpp, while I had managed to get
> that information in the previous patch.

What kind of information are you missing?

> The exact same code doesn't cause any errors in GetWebIDLCallerPrincipal()
> (the immediately adjacent function in ScriptSettings.cpp).

You need to add it as a 'friend' function. See:

friend nsIPrincipal* GetWebIDLCallerPrincipal();

::: content/base/src/nsScriptLoader.cpp
@@ +1103,5 @@
>    }
>  
>    // New script entry point required, due to the "Create a script" sub-step of
>    // http://www.whatwg.org/specs/web-apps/current-work/#execute-the-script-block
> +  AutoEntryScript entryScript(globalObject, "scriptLoader", true, context->GetNativeContext());

Don't we want these strings to me more informative? Here, I'd say something like: "Script loaded from HTML <script> element (or similar)".

::: dom/base/ScriptSettings.cpp
@@ +209,5 @@
> +  if (!cx) {
> +    return nullptr;
> +  }
> +
> +  MozExecutionReason reason;

You need to heap-allocate this. nsRefPtr<MozExecutionReason> reason = new MozExecutionReason(). Otherwise, it will be destroyed when this function returns.

And actually, we should probably cache this as a member on the AutoEntryScript, so that we return the same thing each time.

::: dom/base/ScriptSettings.h
@@ +57,5 @@
>  //   Components.utils.
>  nsIPrincipal* GetWebIDLCallerPrincipal();
>  
> +// This function is used to identify the reason for the current script
> +// execution. It returns a JSObject with a |type| property indicating the

Mention that this JSObject is a MozExecutionReason DOM Object.

@@ +174,3 @@
>  public:
>    AutoEntryScript(nsIGlobalObject* aGlobalObject,
> +                  const char* aReason,

This should be an nsCString so that people can use NS_LITERAL_CSTRING and nsPrintfCString.

::: dom/base/nsGlobalWindow.cpp
@@ +11927,5 @@
>      handler->GetLocation(&filename, &lineNo);
>  
>      // New script entry point required, due to the "Create a script" sub-step of
>      // http://www.whatwg.org/specs/web-apps/current-work/#timer-initialization-steps
> +    AutoEntryScript entryScript(this, "timeoutHandler", true, aScx->GetNativeContext());

"Timeout handler from window.setTimeout (or similar)"

::: dom/bindings/CallbackObject.cpp
@@ +120,5 @@
>      if (!globalObject->GetGlobalJSObject()) {
>        return;
>      }
>  
> +    mAutoEntryScript.construct(globalObject, "callback", mIsMainThread, cx);

"WebIDL Callback Object".

::: dom/src/jsurl/nsJSProtocolHandler.cpp
@@ +241,5 @@
>          (aExecutionPolicy == nsIScriptChannel::EXECUTE_IN_SANDBOX);
>  
>      // New script entry point required, due to the "Create a script" step of
>      // http://www.whatwg.org/specs/web-apps/current-work/#javascript-protocol
> +    AutoEntryScript entryScript(innerGlobal, "jsProtocol", true,

"Script embedded in a javascript:// URI".

Comment 58

[Boris Zbarsky [:bzbarsky]]

> Otherwise, it will be destroyed when this function returns.

Why is that a problem?  It's a dictionary, not a "DOM object".  Once we've called ToObject() on it, we no longer care about it, since ToObject() captures all the data.

> we should probably cache this as a member on the AutoEntryScript

You mean cache the JSObject* the ToObject() call created?  We could do that, as long as AutoEntryScript has a cx to root with...  But why?  What the patch does seems to be lower-overhead in the common case when no one is calling GetScriptEntryReason(), and when people do call it I don't expect them to call it lots of times on the same AutoEntryScript, right?

Comment 59

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Bobby Holley (:bholley) from comment #57)
> > +  AutoEntryScript entryScript(globalObject, "scriptLoader", true, context->GetNativeContext());
> 
> Don't we want these strings to me more informative? Here, I'd say something
> like: "Script loaded from HTML <script> element (or similar)".

I expect these strings to be used as type identifiers to group allocations per reason in the memory profiler tool we're working on. In the debugger we will probably display a short label or an icon in the stack list. For both cases we would need simple type names that we could filter on. There will certainly be a need to present what these execution reasons mean in layman's terms, but those strings will need to be localized too, so we'll do our usual mapping in the frontend code.

Comment 60

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Bobby Holley (:bholley) from comment #57)
> > I don't see how to get more information about the kind of callback that is
> > being executed from inside CallbackObject.cpp, while I had managed to get
> > that information in the previous patch.
> 
> What kind of information are you missing?

I was specifically looking for the event type in this case.

Comment 61

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #58)
> Why is that a problem?  It's a dictionary, not a "DOM object".  Once we've
> called ToObject() on it, we no longer care about it, since ToObject()
> captures all the data.

Oh! I missed that.
 
> > we should probably cache this as a member on the AutoEntryScript
> 
> You mean cache the JSObject* the ToObject() call created?

No, I meant that DOM object. But if it's a dictionary, this is all moot.

Comment 62

[Bobby Holley (:bholley)]

(In reply to Panos Astithas [:past] from comment #59)
> I expect these strings to be used as type identifiers to group allocations
> per reason in the memory profiler tool we're working on. In the debugger we
> will probably display a short label or an icon in the stack list. For both
> cases we would need simple type names that we could filter on. There will
> certainly be a need to present what these execution reasons mean in layman's
> terms, but those strings will need to be localized too, so we'll do our
> usual mapping in the frontend code.

Ok.


(In reply to Panos Astithas [:past] from comment #60)
> (In reply to Bobby Holley (:bholley) from comment #57)
> > > I don't see how to get more information about the kind of callback that is
> > > being executed from inside CallbackObject.cpp, while I had managed to get
> > > that information in the previous patch.
> > 
> > What kind of information are you missing?
> 
> I was specifically looking for the event type in this case.

In that case, my suggestion above should cover this.

Comment 63

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Alternative approach v3

Here is an updated version which follows the suggestion for a setter function to add more information to AutoEntryScript after contruction. I've added two such methods, SetExecutionReasonSubtype and SetExecutionReasonLocation, and those pass the tests for <script> tags in HTML and XUL documents successfully.  I also had to add a GetEntryScript() funtcion to get the current AutoEntryScript from places where we are not creating one.

The problem is that they don't help in the event handler test, because by the time the AutoEntryScript gets created we are past the point where this information is readily available (I used the same code I have in the alternative patch).  It seems to me that I would need to somehow pass this information on from  EventListenerManager::HandleEventSubType down to CallbackObject::CallSetup, but I don't know how easy that would be.

Even if AutoEntryScript could get the line and URL automatically from bug 1006291, that would still leave me with the problem of retrieving the event name in CallbackObject::CallSetup. Any ideas for how to do that?

Comment 64

[Bobby Holley (:bholley)]

Comment on attachment 8418723 [details] [diff] [review]
Alternative approach v3

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

Most of this depends on the discussion in bug 1006291, so I'm going to hold off on looking at this again until we get that sorted out.

(In reply to Panos Astithas [:past] from comment #63)
> The problem is that they don't help in the event handler test, because by
> the time the AutoEntryScript gets created we are past the point where this
> information is readily available (I used the same code I have in the
> alternative patch).  It seems to me that I would need to somehow pass this
> information on from  EventListenerManager::HandleEventSubType down to
> CallbackObject::CallSetup, but I don't know how easy that would be.
> 
> Even if AutoEntryScript could get the line and URL automatically from bug
> 1006291, that would still leave me with the problem of retrieving the event
> name in CallbackObject::CallSetup. Any ideas for how to do that?

That's a good question. Boris, is this a problem we could solve with Codegen?

Comment 65

[Boris Zbarsky [:bzbarsky]]

So for a start, see bug 961327 comment 5 and maybe bug 961327 comment 15.

Past that, I'm not sure I entirely agree with comment 49.  The thing I liked about Panos' original approach was that execution reasons were very lightweight unless someone asked for them, since you could try to defer all the work (except taking strong refs to the objects you needed to compute the execution reason, of course) until that point.  I would like us to preserve that property if possible.  I suspect that doing so would automatically solve the event issue too.

Comment 66

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #65)
> So for a start, see bug 961327 comment 5 and maybe bug 961327 comment 15.

Ahah! Great.

> Past that, I'm not sure I entirely agree with comment 49.  The thing I liked
> about Panos' original approach was that execution reasons were very
> lightweight unless someone asked for them, since you could try to defer all
> the work (except taking strong refs to the objects you needed to compute the
> execution reason, of course) until that point.  I would like us to preserve
> that property if possible.

I agree that we should keep the overhead low. Conceptually, the only thing I'm asking in comment 49 is that whatever checkpoint we perform happens When we instantiate the AutoEntryScript, rather than the call to JS::ExecuteScript, because the former is much less whack-a-mole.

In general, it seems like doing things with AutoEntryScript are likely to be less overhead in general, because we can leverage the pre-existing information in the ScriptSettings stack.

> I suspect that doing so would automatically solve the event issue too.

I don't follow this part. Are you saying we can compute the event type from the callable?

Comment 67

[Boris Zbarsky [:bzbarsky]]

No, I'm saying that the current control flow for calling an event listener is as follows:

1)  We're somewhere in the listener manager, and we have a dom::EventListener.
2)  We call into the EventListener.
3)  The call instantiates a CallSetup.
4)  The CallSetup messes with the script entry stack.
5)  We actually call into JS.

The entity that knows the "why" of why we're calling into JS in the detail we want is at step 1.

So ideally we would create some data structure at that point that encapsulates that knowledge and make sure that after step 5 when the JS debugger needs that info it can access that data structure.

The original proposal here just maintained a stack of such data structures on the runtime, right?

I guess we could maintain them in the script settings stack... we'd need to add an optional argument (e.g. a pointer, defaulting to null) to the method being called in step 2, and punch that down the callstack through to step 4.  We'd also need to decide what to do in the case when the argument is not passed. For example, step 3 could, if not given any other information, just create something that would report we're calling "EventListener.handleEvent" or some such.

What I don't want us doing is forcing step 1 to do any significant computation to pass the info it has down to the later steps, where it will likely get ignored.

Does that make sense?

Comment 68

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #67)
 
> So ideally we would create some data structure at that point that
> encapsulates that knowledge and make sure that after step 5 when the JS
> debugger needs that info it can access that data structure.
> 
> The original proposal here just maintained a stack of such data structures
> on the runtime, right?

Yes. But those data structures had to be created by the embedding, and maintained by the VM. It doesn't seem any cheaper than what I'm imagining we'd be doing here.

> I guess we could maintain them in the script settings stack... we'd need to
> add an optional argument (e.g. a pointer, defaulting to null) to the method
> being called in step 2

Do we? AFAICT, this information lives in the the nsIDOMEvent, and is retrievable from the mozilla::dom::Event, which inherits from the above, and is the thing that is passed to HandleEvent.

> What I don't want us doing is forcing step 1 to do any significant
> computation to pass the info it has down to the later steps, where it will
> likely get ignored.

Agreed. And _because_ it's so likely to get ignored, I don't want us to try to manually hunt down all the places where we might need to add one of these things, and I want to leverage the existing data structure that represents the concept of calling into JS.

Comment 69

[Boris Zbarsky [:bzbarsky]]

> AFAICT, this information lives in the the nsIDOMEvent

In this case, that happens to be the case.  But teaching the callee about that seems pretty painful to me, and not terribly flexible...

Comment 70

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #69)
> > AFAICT, this information lives in the the nsIDOMEvent
> 
> In this case, that happens to be the case.  But teaching the callee about
> that seems pretty painful to me, and not terribly flexible...

I don't fully see why it would be any more painful or less flexible than adding these arguments down this particular call path. But I probably just don't understand this code very well.

Comment 71

[Boris Zbarsky [:bzbarsky]]

So the callee is codegenned right?

So if we teach the callee how to extract the info from something passed to it, that involves codegenning a bunch of code depending on the exact callee.  And changing anything about it means changing the codegen.

Whereas if we just pass the callee some abstract class with a virtual method that the caller then implements, then the caller can implement that however they want.

Comment 72

[Bobby Holley (:bholley)]

Ah, I see.

Ok, so it sounds like we want to do the following:
* Fix bug 1006291, so that we have a way to get the file/line of the entry point from the JS engine.
* Create a MOZ_STACK_CLASS ExecutionReason, which has a virtual JSObject* explain(). By default, ExecutionReason takes a string argument, and its explain() method produces a MozExecutionReason with the message, and with file/line retrieved from the JS engine.
* Make AutoEntryScript take a mandatory ExecutionReason& argument. It's all stack-allocated, so it can just save a reference.  
* EventListenerManager::HandleEventSubType has a subclass of ExecutionReason that also takes an event type, and munges explain() appropriately.
* Callback object invocation takes an optional ExecutionReason*. If it's null, we instantiate a Maybe<ExecutionReason> with a helpful string, and pass that instead to the AutoEntryScript.
* HandleEventSubType instantiates this subclass and passes it to the callback.

Sound good to everyone?

Comment 73

[Boris Zbarsky [:bzbarsky]]

That's pretty much what I was thinking, yes.

Comment 74

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v18

Here is my WIP patch, rebased on top of the one in bug 1006291. I tried to accommodate all the suggestions so far and most tests pass, but I have a crash in the event handler test when it ends up in explain() that I'm trying to figure out.

Comment 75

[Bobby Holley (:bholley)]

Cool! Feel free to flag me for review/feedback on this patch once it's ready.

Comment 76

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v19

Fixed the crash and removed any traces of code from the previous approaches. There are two issues that remain now:

1. figure out how to pass the EventHandlerReason down to CallSetup
2. fix the entry point discovery from bug 1006291 to skip the debugger's entry point

Boris or Bobby, could you help me understand how to do #1? I can see a couple of places in codegen where CallSetup is being constructed, but I don't understand how I get there from HandleEventSubType.

Also, for the non-WebIDL cases I see that a different code path is taken. Do I need to tweak every HandleEvent method in the tree? Do I need to wait for AutoEntryScript to be more broadly used first?

Comment 77

[Boris Zbarsky [:bzbarsky]]

> 1. figure out how to pass the EventHandlerReason down to CallSetup

Need to change callback codegen to have an optional ExecutionReason* argument, right?

If you search for aExceptionHandling in Codegen.py that should hunt down the places where we need to add the new thing.  Let me know if you want me to just do this part; I'm happy to do that.

> Also, for the non-WebIDL cases I see that a different code path is taken.

For the non-WebIDL cases, the call is being made into C++, not into JS.

Comment 78

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Boris Zbarsky [:bz] from comment #77)
> Let me know if you want me to just do this part; I'm happy to do that.

If it's not too much trouble that would be superb!

Comment 79

[Boris Zbarsky [:bzbarsky]]

Attachment: codegen bits

Here you go.  The default description for WebIDL callbacks is the "pretty
name", which means that for a callback interface the getter and setter of an
attribute will both have a description of "InterfaceName.attributeName".  We
can improve that if needed.

A question that arose as I was doing this.  Why does ExecutionReason store an
nsCString?  Can we not just store a const char*?  Mostly I'm worried about the
copy costs.  Or do we not need to copy literals into nsCString nowadays?

Comment 80

[Boris Zbarsky [:bzbarsky]]

Er, I guess with that patch CallSetup::mExecutionReason is unused.  We should get rid of it.

Comment 81

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v20

Thank you very much for the codegen bits! I've incorporated them into the larger patch, but I still have a couple of problems.

The good news is that I have a working patch in bug 1006291, so all the script location checks now pass. I've also added a few more tests for setTimeout and promises to make sure everything works as expected.

The two test failures I'm getting are:
- the eventType in a click event (from the EventHandlerReason subclass) which doesn't seem to propagate through codegen
- the execution reason for setTimeout doesn't seem to be the one I set in nsGlobalWindow::RunTimeoutHandler, but the one from codegen that returns a type of 'Function'

Comment 82

[Boris Zbarsky [:bzbarsky]]

> - the eventType in a click event (from the EventHandlerReason subclass) which doesn't
> seem to propagate through codegen

You're not passing your ExecutionReason* to HandleEvent, afaict.

> - the execution reason for setTimeout doesn't seem to be the one I set

You're only setting an execution reason on the branch that deals with setTimeout calls whose first argument is a string, no?  You probably want to hoist that ExecutionReason out of the if/else and pass a pointer to it to the Call() in the other branch.

Comment 83

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v21

OK, I did both of those things, but it turns out that the test was using an event handler and not an event listener. I've modified the test and now it's all green, but is there another central location in the code for setting the execution reason of event handlers where we have access to the attribute or property name (e.g. "onclick")? Or perhaps it is available in codegen somewhere?

Another thing I'm curious about is how to test the execution reason for the Promise::ResolveInternal path. I've added callbacks for the promise constructor, promise.then and promise.catch in the promises test, but none of them gets the ResolveInternal execution reason.

Finally, I forgot to mention previously that I used nsCString at Bobby's suggestion in comment 57. I confess complete ignorance on the performance tradeoffs between nsCString and const char*.

Comment 84

[Boris Zbarsky [:bzbarsky]]

> central location in the code for setting the execution reason of event handlers 

Yes.  JSEventHandler::HandleEvent.  It has mEventName, which will be something like "onclick" (so include the "on" prefix).

> how to test the execution reason for the Promise::ResolveInternal path

  var p = new Promise(function(resolve, reject) {
     resolve({ then: function(a, b) { return a(5); } });
  });

Note that you need to merge to tip; that code just got moved around yesterday.  And the reason there should perhaps be something more like "promise.ResolvePromiseViaThenableTask" or something?

One other note.  Do we not need a execution reason for the get we perform there?  Or does the JS engine sort of make one up for us?

> I used nsCString at Bobby's suggestion in comment 57.

Ah.  I personally think if someone wants to do PrintfCString they should be implementing a new subclass instead, so the default thing can be as close to 0-overhead as possible.  I guess I get to fight that out with Bobby.

Comment 85

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #84)
> Ah.  I personally think if someone wants to do PrintfCString they should be
> implementing a new subclass instead, so the default thing can be as close to
> 0-overhead as possible.  I guess I get to fight that out with Bobby.

const char* is fine with me too.

Comment 86

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v22

Converted ExecutionReason.mType to const char* and fixed all the execution-reason-related problems in this version. I also added tests for thenables and event handlers. A number of new AutoScriptEntry references appeared in the tree, so I added execution reasons for those, too. For stuff I don't understand, I'm using the Class.Method naming style, but I'd like to provide a more descriptive name.

Fixing bug 1006291 is the main obstacle right now.

(In reply to Boris Zbarsky [:bz] from comment #84)
> One other note.  Do we not need a execution reason for the get we perform
> there?  Or does the JS engine sort of make one up for us?

I didn't understand this one. Which get are you referring to?

Comment 87

[Boris Zbarsky [:bzbarsky]]

> Which get are you referring to?

1121     if (!JS_GetProperty(aCx, valueObj, "then", &then)) {

That one.  That can run script, obviously (though commonly will not).

Fwiw, in terms of getting this reviewed and landed it might be good to break it up into several patches (introduce the type, add to codegen bits, add to AutoEntryScript, add the bits that pass specialized reasons to some webidl callbacks), but I'm not sure how feasible that is at this point.  :(

Comment 88

[Boris Zbarsky [:bzbarsky]]

Oh, and also, if AutoEntryScript requires an ExecutionReason, should we be passing in a const reference to it instead of a pointer?  And should explain() be a const method?

I think we should differentiate between "event handler" and "event listener", also.  For one thing, right now you use EventHandlerReason for both, but for a click event one will report a type of "onclick" while the other reports "click", as the code is currently written.  Better to have an EventListenerReason that holds a DOM event pointer and an EventHandlerReason that holds an nsIAtom pointer and then they can compute whatever they want in their explain() methods.

Is the mType of ExecutionReason going to be directly exposed to webdevs?  Or just internally to the debugger?  That is, how much should we worry about making those types non-misleading?

Comment 89

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v23

(In reply to Boris Zbarsky [:bz] from comment #87)
> That can run script, obviously (though commonly will not).

Ah, good point! I had to read the spec again, I hadn't realized a getter property 'then' would qualify as a thenable.

I don't know how to add an execution reason for that case though. We don't seem to be creating an AutoEntryScript at that point and with the current solution execution reasons are tied to AutoEntryScript. I've added a test for this particular case, but I've left it verifying that we get the preceding execution reason.

> Fwiw, in terms of getting this reviewed and landed it might be good to break
> it up into several patches (introduce the type, add to codegen bits, add to
> AutoEntryScript, add the bits that pass specialized reasons to some webidl
> callbacks), but I'm not sure how feasible that is at this point.  :(

OK, I'll try to do that as soon as the dependent bug is done first, otherwise rebasing patches gets unwieldy.

(In reply to Boris Zbarsky [:bz] from comment #88)
> Oh, and also, if AutoEntryScript requires an ExecutionReason, should we be
> passing in a const reference to it instead of a pointer?  And should
> explain() be a const method?

I made explain() a const method, but I couldn't convince the compiler to let me pass a reference instead of a pointer in AutoEntryScript.

> I think we should differentiate between "event handler" and "event
> listener", also.  For one thing, right now you use EventHandlerReason for
> both, but for a click event one will report a type of "onclick" while the
> other reports "click", as the code is currently written.  Better to have an
> EventListenerReason that holds a DOM event pointer and an EventHandlerReason
> that holds an nsIAtom pointer and then they can compute whatever they want
> in their explain() methods.
> 
> Is the mType of ExecutionReason going to be directly exposed to webdevs?  Or
> just internally to the debugger?  That is, how much should we worry about
> making those types non-misleading?

I don't think we will ever expose those strings to the UI directly (at the very least for l10n reasons). My plan is to use them for associating frames and objects allocated in them to execution reasons. I think event handlers and listeners in particular need to be grouped together in the UI, as web devs most of the time treat them the same.

I'd like to be able to tell them apart in case the need arises, and my idea was to use reason.eventType.startsWith("on"), but using different mType strings as you suggest is arguably cleaner, so I switched to that.

Comment 90

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: pointer-to-reference attempt

Here is my attempt to use an ExecutionReason referece instead of a pointer in AutoEntryScript. The compiler produces the following error that I don't know how to fix:

In file included from /home/past/src/fx-team/obj-ff-opt/dom/bindings/Unified_cpp_dom_bindings0.cpp:2:
In file included from /home/past/src/fx-team/dom/bindings/BindingUtils.cpp:7:
In file included from /home/past/src/fx-team/dom/bindings/BindingUtils.h:11:
In file included from ../../dist/include/jswrapper.h:12:
In file included from ../../dist/include/jsproxy.h:10:
../../dist/include/mozilla/Maybe.h:75:30: error: no matching constructor for initialization of 'mozilla::dom::AutoEntryScript'
      ::new (storage.addr()) T(t1, t2, t3, t4);
                             ^ ~~~~~~~~~~~~~~
/home/past/src/fx-team/dom/bindings/CallbackObject.cpp:125:22: note: in instantiation of function template specialization 'mozilla::Maybe<mozilla::dom::AutoEntryScript>::construct<nsIGlobalObject *, mozilla::dom::ExecutionReason, bool, JSContext *>' requested here
    mAutoEntryScript.construct(globalObject, aExecutionReason,
                     ^
../../dist/include/mozilla/dom/ScriptSettings.h:255:3: note: candidate constructor not viable: 2nd argument ('const mozilla::dom::ExecutionReason') would lose const qualifier
  AutoEntryScript(nsIGlobalObject* aGlobalObject,
  ^
../../dist/include/mozilla/dom/ScriptSettings.h:252:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 4 were provided
class AutoEntryScript : public AutoJSAPI,
      ^
1 error generated.

Comment 91

[Bobby Holley (:bholley)]

(In reply to Panos Astithas [:past] from comment #89)
> Ah, good point! I had to read the spec again, I hadn't realized a getter
> property 'then' would qualify as a thenable.
> 
> I don't know how to add an execution reason for that case though. We don't
> seem to be creating an AutoEntryScript at that point and with the current
> solution execution reasons are tied to AutoEntryScript. I've added a test
> for this particular case, but I've left it verifying that we get the
> preceding execution reason.

If we don't expect to be running getters, we should just use an AutoJSAPI. In the future, the lack of an AutoEntryScript will cause us to make getters throw instead of executing, which is probably what we want in corner cases like this one.

Comment 92

[Boris Zbarsky [:bzbarsky]]

> In the future, the lack of an AutoEntryScript will cause us to make getters throw instead
> of executing

Per spec, we should be running the getter here.

> but I couldn't convince the compiler to let me pass a reference instead of a pointer in
> AutoEntryScript

Since explain() is const, you can (and should) pass a const reference to AutoEntryScript.

Comment 93

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v24

Turns out I was very close to fixing the const reference error, but it was too late on a Friday night for me to see it.

Comment 94

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v25

Fixed some tests and rebased on top of the updated patch in bug 1006291. All tests pass again.

Comment 96

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v26

This is the latest update with all the old JS shell tests ported over to mochitests. Results are 250/pass, 2/fail and the ones that fail are caused by a limitation in the current approach, which can only return the topmost execution reason via the registered callback. test_executionReason-older.html contains the relevant tests.

I've discussed this with Jim and Bobby and bug 971673 also has some prior discussion around this topic. Jim is working on an updated design document that should help move us forward.

I've also changed the spec around executionReason.url and executionReason.line, to reflect the fact that the currently retrieved values point to the JS code that is executed as a result of this executionReason. Previously I was describing these properties as pointing to the code that "registered" or "initiated" the execution. In other words I used to refer to the location of a "setTimeout(foo, bar)" call, while in fact the code returns the location of the "foo()" declaration. Ideally we would want to return the latter, but until we get there there is some value in returning the former.

Comment 97

[Panos Astithas (he/him) [:past] (please ni?)]

This is currently blocked on a solution for retrieving older execution reasons, as described in comment 96. That solution will likely come from bug 971673.

Comment 98

[Jim Blandy :jimb]

Attachment: proposed scriptEntryReason docs

Here are some draft docs for the accessor. I've changed the names a bit to align them better with the HTML5 spec, and I've hinted at how we would add information about who registered the event listener / installed the promise value consumer / etc.

I'm working on adapting past's v14 patch to the JS::dyn::Param API I proposed in bug 971673, but I think that will get us where we need to go.

Comment 99

[Jim Blandy :jimb]

Attachment: Give PersistentRooted a copy constructor that can take a 'const' original.

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

Comment 100

[Terrence Cole [:terrence]]

Comment on attachment 8443003 [details] [diff] [review]
Give PersistentRooted a copy constructor that can take a 'const' original.

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

Yup, seems like a fine solution.

Comment 101

[Jim Blandy :jimb]

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

Work in progress, posted for those interested. This is the interface 'explain' implementations would use to construct explanation values. Extensive comments in the header file.

Comment 102

[Jim Blandy :jimb]

Attachment: Add a SpiderMonkey API for tracking dynamically scoped parameters that coordinates with Debugger.

Another work-in-progress patch. This partially integrates Panos' v14 patch, and partially re-implements it. No testing facilities are included yet; that'll be tomorrow.

Bobby, you may be interested in the Debugger.cpp changes here. This makes DebuggerGenericEval call back into the embedding (if it has registered the appropriate hook) to do whatever kind of stack prep it needs to, and passes an Evaluation object that the embedding is supposed to invoke to actually do the work. See the comments in DebugAPI.h for details.

Comment 103

[Jim Blandy :jimb]

Resetting accidental steal.

Comment 104

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4540307da2ef

Comment 105

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4540307da2ef

Comment 106

[Jim Blandy :jimb]

Comment on attachment 8443137 [details] [diff] [review]
Add a SpiderMonkey API for tracking dynamically scoped parameters that coordinates with Debugger.

Marking obsolete; this work is proceeding in bug 971673.

Comment 107

[Jim Blandy :jimb]

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

The next patch shows this API in action.

Comment 108

[Jim Blandy :jimb]

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

Not requesting review yet: we need to reach consensus on the DynamicParam stuff, on which this is based. That's discussed in Bug 971673.

Comment 109

[Jim Blandy :jimb]

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

This patch belongs on bug 961326. Moving.

Comment 110

[Jim Blandy :jimb]

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

This patch also belongs on bug 961326.

Comment 111

[Paul Rouget [:paul]]

Jim, this bug blocks bug 1050500, which is very important for the new performance tool we plan to ship this cycle. Do you think this bug will be fixed in time? Otherwise, we'll figure out a workaround.

Comment 112

[Victor Porof [:vporof][:vp]]

Whoops

Comment 113

[Victor Porof [:vporof][:vp]]

Ok, so this is blocking bug 961326, which is blocking bug 1050500, so comment 111 still stands.

Comment 114

[Jim Blandy :jimb]

I've filed bug 1152577 for a greatly simplified version of this patch that will serve the performance tool's needs; that has a patch that is under review. Bug 1050500 no longer depends on this bug.

Comment 115

[Jim Blandy :jimb]

Unassigning myself. This is still a reasonable feature, and we could use the information, but it's not essential to any of our current projects.

Comment 116

[Nicolas B. Pierron [:nbp]]

Sounds like this is a feature which might be easier to address using the profiler API, to figure out the caller's of the callback without adding extra instrumentation to SpiderMonkey. In which case, I am not even sure if this should be a SpiderMonkey feature.