Open Bug 971673 Opened 10 years ago Updated 4 months ago

[jsdbg2] Debugger hooks need to instantiate an AutoEntryScript

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
ASSIGNED

People

(Reporter: simon.lindholm10, Assigned: jimb)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [leave open])

Attachments

(7 files, 7 obsolete files)

Replace ScriptSettingsStack's array with a C++-stack-allocated linked list.
12.82 KB, patch
bholley
: review+
jimb
: checkin+
Details | Diff | Splinter Review
Add a SpiderMonkey API for tracking script settings objects that coordinates with Debugger.
26.42 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
Don't require a JSContext to manipulate the JS::dbg::ScriptSettings stack.
4.69 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
Ensure mozilla::dom::ScriptSettingsStackEntry is only used as a base class: make its constructor protected.
922 bytes, patch
bholley
: review+
jimb
: checkin+
Details | Diff | Splinter Review
Assert that AutoJSAPI is always given a JSContext.
812 bytes, patch
bobowen
: review+
jimb
: checkin+
Details | Diff | Splinter Review
Use JS::dbg::ScriptSettings for the browser's script settings stack.
13.87 KB, patch
bholley
: review-
Details | Diff | Splinter Review
Change all places that instantiate AutoEntryScript to call its Init function.
25.11 KB, patch
bholley
: review-
Details | Diff | Splinter Review 
Consider the following being run from a chrome Scratchpad:

Components.utils.import("resource://gre/modules/jsdebugger.jsm");
window.addDebuggerToGlobal(window);

w = content;
dbg = new Debugger;
dw = dbg.makeGlobalObjectReference(w);
setTimeout(function() {
  var r = dw.evalInGlobal(' location = "x"; ');
  alert(r.return || r.throw.unsafeDereference());
});

It fails with an error "Error: Access to 'chrome://browser/content/x' from script denied". I don't know Gecko enough to be able to confirm, but it looks like the chrome entry global (set by setTimeout, but apparently not when not using setTimeout, for whatever reason) is reused across the call to evalInGlobal. This is a bug: dw.evalInGlobal ought to set w as the entry global.

As far as I can tell the same happens with Debugger.Frame.eval - there I presume the entry global ought to be the same as that of the frame, but the frame's global probably also works.

This came up in Firebug, where running "location = 'x';" from the command line in 27+ fails with said error. See https://code.google.com/p/fbug/issues/detail?id=7177.
(In reply to Simon Lindholm from comment #0)
> Consider the following being run from a chrome Scratchpad:
> 
> Components.utils.import("resource://gre/modules/jsdebugger.jsm");
> window.addDebuggerToGlobal(window);
> 
> w = content;
> dbg = new Debugger;
> dw = dbg.makeGlobalObjectReference(w);
> setTimeout(function() {
>   var r = dw.evalInGlobal(' location = "x"; ');

Where does |x| come from?

> It fails with an error "Error: Access to 'chrome://browser/content/x' from
> script denied". I don't know Gecko enough to be able to confirm, but

> it
> looks like the chrome entry global (set by setTimeout, but apparently not
> when not using setTimeout, for whatever reason)

I don't understand what you mean here.

> is reused across the call to
> evalInGlobal. This is a bug: dw.evalInGlobal ought to set w as the entry
> global.

It's quite likely that evalInGlobal doesn't push an entry global, and it probably should. But the entry global doesn't really determine security checks, so I doubt that's what the problem is here. If you can tell me what |x| is, we can go from there.
"x" is a relative URI.  The issue is that the base URI that got used to resolve it is "chrome://browser/content/" and then a CheckLoadURI check fails (as it should).  So the problem is not the security check but the incorrect base URI, which is all to do with the entry global.
Oh, I see. Yes, in that case all the JS debugger eval stuff probably needs to be hooked up to entry globals. How to do that is an interesting question - the standard JS solution is to use callbacks, but that doesn't play well with our RAII AutoEntryGlobal...
I'm not able to find anything in the tree named AutoEntryGlobal. I haven't heard of entry globals, so I don't know what to do here.

If worse comes to worst, we could have a type:

struct GenericGuard {
  virtual ~GenericGuard() = 0;
};

AutoEntryGlobal could inherit from that, which wouldn't affect its use in normal cases. The JS engine could call a hook that returns a GenericGuard * and stores it in a ScopedDeletePtr<GenericGuard>.

It's a funny way to use RAII, but it seems not too wrong.
Sorry, AutoEntryScript.

I think it actually makes more sense overall to just expose a hook for the JS engine to invoke nsJSUtils::EvaluateString in the scope of a given global, which can do all of the necessary browser-related munging.

Does that sound workable Jim?
Jim and I were just discussing this on IRC and he made an interesting point:

  When I have a Debugger.Frame instance F, then F.eval(C) should be sure to reinstate F's
  entry global when running C.

That is, the code C should look as if it ran as part of F.

Bobby, any ideas how we can try to do that, given that entry globals are not tied to frames?  :(
Also, if I have a Debugger.Object G referring to a global, then it seems clear that G.evalInGlobal(C) should push a fresh entry global, as the evaluation of C isn't necessarily related to any particular extant dynamic context. But is G always the right entry global to push? Or does it need to be a parameter, say G.evalInGlobal(C, { withEntryGlobal: G2 })?
(In reply to Bobby Holley (:bholley) from comment #5)
> Sorry, AutoEntryScript.
> 
> I think it actually makes more sense overall to just expose a hook for the
> JS engine to invoke nsJSUtils::EvaluateString in the scope of a given
> global, which can do all of the necessary browser-related munging.
> 
> Does that sound workable Jim?

I don't think so.

So, here's the Debugger's eval primitive:
https://hg.mozilla.org/mozilla-central/file/879038dcacb7/js/src/vm/Debugger.cpp#l4472

There's logic in there to handle both evaluate-in-frame and evaluate-in-global. This proposed hook would only be used in the second case, so ignore the evaluate-in-frame gunk. 

There's also logic to handle the F.evalWithBindings(C, { x: 42 }) stuff, and the similar G.evalInGlobalWithBindings.

There's also support for the options: F.eval(C, { url: "glerk.html", lineNumber: 42 })

So, I think there's quite a bit of stuff to be taken care of there. As I see it now, I would rather have a hook:

  ScopedDeletePtr<GenericGuard> = cx->runtime()->embeddingEvalGuard(cx, global, options)

or something.
... having the embedding's guard try to parse the options sounds like a bad idea.

I think Debugger just needs to know about entry globals.
It's not like we don't know about other aspects of the browser already: WindowProxy, scripts' owning elements, etc.
(In reply to Boris Zbarsky [:bz] from comment #6)
>   When I have a Debugger.Frame instance F, then F.eval(C) should be sure to
> reinstate F's
>   entry global when running C.
> 
> That is, the code C should look as if it ran as part of F.
> 
> Bobby, any ideas how we can try to do that, given that entry globals are not
> tied to frames?  :(

I need to understand more about the execution model of executing code in non-top frames. Do we just restore the in-scope variables and push another frame, or do we do something more complex?

(In reply to Jim Blandy :jimb from comment #7)
> Also, if I have a Debugger.Object G referring to a global, then it seems
> clear that G.evalInGlobal(C) should push a fresh entry global, as the
> evaluation of C isn't necessarily related to any particular extant dynamic
> context. But is G always the right entry global to push? Or does it need to
> be a parameter, say G.evalInGlobal(C, { withEntryGlobal: G2 })?

I would be very surprised if callers ever had a reason to push a different entry global. Can you think of any? In the worst case, they could always just do G2.evalInGlobal('G.eval(C)'); right?

Still thinking about the rest.
I think some kind of munging to heap-allocation the stack guard and pass it back to the engine is probably the way to go. It's annoying, because we'll have to remove some MOZ_STACK_CLASS annotations, but it's the best thing I can think of.

The JS engine is always going to have some concept of entry scripts - currently it's the notion of a "saved frame chain". But there's a lot of DOM goop that goes along with them that really doesn't belong in the JS engine.
(In reply to Bobby Holley (:bholley) from comment #11)
> I need to understand more about the execution model of executing code in
> non-top frames. Do we just restore the in-scope variables and push another
> frame, or do we do something more complex?

For F.eval(C), we:
- compile C as appropriate for being nested within F's script;
- take F's scope chain and 'this' value, and create a new stack frame with F as its caller; and
- run C in that stack frame.

This is a little misleading, as the eval's continuation is not F --- it's the frame for the call to F.eval. But the intention is to run C as if it were a direct eval in F's code, so it's consistent in that sense.

What's relevant here is that entry globals are essentially dynamically scoped parameters: the one in force depends on the function's caller. And C should definitely run as if it were in F's dynamic scope, not in the dynamic scope of any JS frames younger than F.
What does the stack look like if you throw an exception inside C? Do you see the intermediate frames?

The problem with entry globals is that they deposit savedFrameChain markers on the execution stack. So if the stack-top entry is A and the entry underneath is B, repushing B still doesn't give the same semantics as executing in the original context of B.

Now, exactly what we will use these savedFrameChain markers for going forward is an interesting question. Currently they terminate exception stack-walking, but I'm interested in changing things to just do a per-frame principal check instead. It's possible that they may disappear altogether.
(In reply to Bobby Holley (:bholley) from comment #14)
> What does the stack look like if you throw an exception inside C? Do you see
> the intermediate frames?

Why, let me direct you to the unit test which asks exactly that question!

https://hg.mozilla.org/mozilla-central/file/a2939bac372b/js/src/jit-test/tests/debug/Frame-eval-stack.js#l1

In short, no, in F.eval(C) errors don't see the live debuggee frames that are younger than F. They do see the younger live frames pushed to run C itself, naturally.

> The problem with entry globals is that they deposit savedFrameChain markers
> on the execution stack. So if the stack-top entry is A and the entry
> underneath is B, repushing B still doesn't give the same semantics as
> executing in the original context of B.
> 
> Now, exactly what we will use these savedFrameChain markers for going
> forward is an interesting question. Currently they terminate exception
> stack-walking, but I'm interested in changing things to just do a per-frame
> principal check instead. It's possible that they may disappear altogether.

That's a lot like what Fitzgen's patch in bug 972045 does.

It seems like, since F.eval(C) appears to set aside the frames younger than F --- from C's dynamic point of view --- we need to do the same for the savedFrameChain markers: set aside those pushed within F's dynamic scope, and then restore them when Debugger.Frame.prototype.eval returns.

If the savedFrameChain markers are a linked list, could we do for them what we do with the stack frames --- save the true head pointer, reset it to the one in force for F, run C, and then restore the true head pointer?

Note that F.eval(c) is allowed to hit breakpoints, which calls a breakpoint handler function; and that handler might itself call F'.eval(C) on some other frame F', so there's actually an arbitrary tree of frames in existence at any given time. However, regardless of how hairy this tree gets, an older frame is never popped before a younger frame, even on separate branches.
Ok. How tightly is this integrated with the canonical VM stack representation, and how much is it an after-market job on the part of the debugger? The saved frame chain markers are actually stored in the stack frames themselves IIUC, so if the underlying representation is in fact a tree, we should get this behavior for free.

In contrast, if the debugger does a bunch of magic to try to map a tree abstraction onto an underlying linear stack data model, this is likely to get quite complicated.
Each Debugger.Frame whose referent is still alive has an AbstractFramePtr, which is a type the engine itself uses to refer to stack frames. In some cases we actually store a pointer to a ScriptFrameIter::Data, but again, that's the engine's own representation.

So I think we're pretty close to the canonical VM stack representation. The "tree" is just an issue of carefully chosing your parent pointers.
I think in practice all the stack frames within an Activation (the abstract base class that we subclass for the interpreter and JITs) represent their parents implicitly: a frame's parent is the one that precedes it in memory. However, between one Activation its parent Activation we have explicit 'prev_' pointers, so we can create interesting shapes at the Activation lever; the actual stack frames just fall within the outline the Activations establish.
s/lever/level/
(In reply to Jim Blandy :jimb from comment #18)
> However, between one Activation its parent Activation we have explicit
> 'prev_' pointers, so we can create interesting shapes at the Activation
> lever; the actual stack frames just fall within the outline the Activations
> establish.

Ok. Is that what we do for the debugger? Things like saved frame chain counters and scripted caller overrides live on the Activation. So if we want them to work right with the debugger, we need to make those interesting shapes.

Though I suppose then the question is what to do about the data structures on the DOM side. Those are purely stack-y at this point, and would probably be confused if the JS engine hopped between leaf nodes in a tree.
Actually, I was wrong about the behavior of Activations. That'll teach me to assume.

Activations' prev links are always strictly stack-like: all the live Activations on a given thread, regardless of JSContext or JSCompartment, are chained together via their prev links.

Rather, the tree structure is created by links in js::StackFrames themselves: if a StackFrame's DEBUGGER flag is set, then its evalInFramePrev_ member points at the 'logical' next frame. Iteration is supposed to treat evalInFramePrev_ as that frame's previous frame.
(In reply to Bobby Holley (:bholley) from comment #14)
> The problem with entry globals is that they deposit savedFrameChain markers
> on the execution stack. So if the stack-top entry is A and the entry
> underneath is B, repushing B still doesn't give the same semantics as
> executing in the original context of B.

Doesn't that mean that eval-ing in the dynamic scope of some frame F should behave as if savedFrameChain markers for frames younger than F were not present, for the duration of the eval? That means that temporarily reinstating F's context should effectively undo some markers. I don't quite understand what those markers do, but it sounds hairy.

> Now, exactly what we will use these savedFrameChain markers for going
> forward is an interesting question. Currently they terminate exception
> stack-walking, but I'm interested in changing things to just do a per-frame
> principal check instead. It's possible that they may disappear altogether.

That sounds like it would help.

I see that AutoEntryScript switches contexts and compartments. It seems like Debugger.Frame.prototype.eval already takes care of reverting those effects. So if the savedFrameChain markers were gone, then the only thing that would still matter was finding the right global object. Then, pushing a new ScriptSettingsStackEntry would have all the effect we need, right?
This stuff is currently in flux, so I think we should wait until it settles a bit. Blocking on bug 979730 for now.
Depends on: 979730
I think this really depends more on bug 989528, which is fixed. I think we can start moving on this now.

It's worth noting that the AutoEntryScript is also useful for getting the right JSContext, which we don't do very well right now (see bug 1014553 comment 5).
Depends on: 989528
No longer depends on: 979730
There are basically two ways to do this that I can think of:

(a) Using a callback to create/destroy these objects on the heap, at the expense of losing our MOZ_STACK_CLASS annotations.
(b) Set up a macro-aided mechanism by which SpiderMonkey can call into the embedding and say (call me back with an AutoEntryScript on the stack). This basically means having a |JS_FRIEND_API FooHookImpl| for every FooHook, but that might not be so bad.

Thoughts?
Summary: [jsdbg2] Debugger.Object.evalInGlobal needs to set an entry global → [jsdbg2] Debugger hooks need to instantiate an AutoEntryScript
Talking to Panos, it sounds like this is also an issue for inspecting various stack frames (and determining their execution reasons). We really need to sit down and sort this out.
Maybe MarkEntryPoint should take a void* handle to an AutoEntryScript, and store that on the activation. Then, whenever a debugger wanted to restore the state at a given entry point, it could invoke an embedding callback that says "push this entry point again and call me back".
I was talking to jimb about this last Thursday and he had a new design in mind that would fix this. He was also working on an updated spec for execution reasons, so he might post it in bug 961325.
Sounds like we should sync up about this. Do you guys have time for a vidyo call today?
Flags: needinfo?(past)
Flags: needinfo?(jimb)
Attached patch store-aes.patch (obsolete) — — Splinter Review 
(In reply to Bobby Holley (:bholley) from comment #27)
> Maybe MarkEntryPoint should take a void* handle to an AutoEntryScript, and
> store that on the activation. Then, whenever a debugger wanted to restore
> the state at a given entry point, it could invoke an embedding callback that
> says "push this entry point again and call me back".

This was my attempt to do that for the limited scope of execution reasons (no debugger pushing entry points, just looking for the right one), which Jim has explained I interpreted too literally. I'm just leaving it here for reference, since I couldn't make it work and Jim is already working on a better approach.
Flags: needinfo?(past)
Flags: needinfo?(jimb)
Here's a sketch (uncompiled!) of the sort of API I think could handle both the browser's and Debugger's requirements here. JS::dyn::Entry is missing an 'explain' method, which I'll add tomorrow, but this should be at least enough to get a general idea of how things could go.
Attachment #8437349 - Flags: feedback?(bobbyholley)
Attachment #8437349 - Flags: feedback?(past)
Here's a possible pre-requisite patch for the prior patch, that removes the array-based stack from ScriptSettings.cpp altogether. All allocation is now done in C++ stack frames. I think it's a simplification of the current structure, overall, and loses no features.

If we adopt this patch, then the implementation of the previous patch I posted becomes simpler, as ScriptSettings.cpp can delegate its stack management to JS::dyn::Entry, by making ScriptSettingsStackEntry inherit from JS::dyn::Entry. That is, ScriptSettingsStackEntry would be the 'MyEntry' type used in the comments in the prior patch's js/public/DebugAPI.h.
Attachment #8437353 - Flags: feedback?(bobbyholley)
Comment on attachment 8437353 [details] [diff] [review]
Replace ScriptSettingsStack's array with a C++-stack-allocated linked list.

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

I must say I find this easier to follow.

::: dom/base/ScriptSettings.cpp
@@ +59,1 @@
>      }

Unless I'm missing something, you meant to add:
entry = entry->mOlder;
(In reply to Panos Astithas [:past] from comment #33)
> I must say I find this easier to follow.

I'm glad to hear that! It's a net loss of code:

 ScriptSettings.cpp |  137 ++++++++++++++++++++++++-----------------------------
 ScriptSettings.h   |   46 ++++++-----------
 2 files changed, 80 insertions(+), 103 deletions(-)

> ::: dom/base/ScriptSettings.cpp
> @@ +59,1 @@
> >      }
> 
> Unless I'm missing something, you meant to add:
> entry = entry->mOlder;

D'oh! 81 insertions.
Comment on attachment 8437349 [details] [diff] [review]
Add a SpiderMonkey API for tracking dynamically scoped parameters that coordinates with Debugger.

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

It's not yet clear to me how the Debugger will interact with MyEntry::Stack and associate JS frames with JS::dyn::Entry objects, so I'd better wait for your next update with explain().
Attachment #8437349 - Flags: feedback?(past)
Comment on attachment 8437353 [details] [diff] [review]
Replace ScriptSettingsStack's array with a C++-stack-allocated linked list.

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

This is awesome - thanks Jim!

r=bholley with my and panos' comments addressed.

::: dom/base/ScriptSettings.cpp
@@ +84,1 @@
>    sScriptSettingsTLS.set(nullptr);

Can we just MOZ_ASSERT that it's already null?
Attachment #8437353 - Flags: feedback?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #36)
> This is awesome - thanks Jim!

Ah, glad you like it!

> >    sScriptSettingsTLS.set(nullptr);
> 
> Can we just MOZ_ASSERT that it's already null?

I guess it stands to reason that destroying the script settings stack while there are JS frames on the stack is almost certainly incorrect. Changed.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=ca6feab1a0dc
Try push looks good.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc124fd973a
Whiteboard: [leave open]
Attachment #8437353 - Flags: checkin+
Comment on attachment 8437349 [details] [diff] [review]
Add a SpiderMonkey API for tracking dynamically scoped parameters that coordinates with Debugger.

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

This seems great at a first pass. I'd like to see an update version with the comment issues fixed, that compiles, and that integrates with the gecko Script Settings Stack to have a more concrete picture. But this seems pretty clearly on the right track. :-)

::: js/public/DebugAPI.h
@@ +28,5 @@
> +// parameter. To run a callback function, the spec's "jump to a code entry
> +// point" algorithm tells us to push a new entry on the script settings stack;
> +// execute the callback's code; and then pop the script setting stack. Then, the
> +// "incumbent settings object" is whatever script settings object is presently
> +// on the top of the stack.

In general, the "entry settings object" would be a clearer example here, because the incumbent settings object is partially computed from the JS stack, and only computed from the script settings stack in the weird corner case of callback objects.

Reading on below, I think this is just the result of an incorrect conflation of entry global and incumbent global. You're basically talking about entry globals. The incumbent global is the global of the most-recently-executed scripted function (i.e. JS::GetScriptedCallerGlobal) _except_ in the case where we do a stack-scoped JS::HideScriptedCaller() and use an AutoIncumbentScript).

@@ +49,5 @@
> +//   [event dispatch, pushing entry global G1]
> +//
> +// and Debugger is asked to evaluate code in the scope of the frame running f,
> +// then Debugger must coordinate with the browser to ensure that the incumbent
> +// settings object seen by the evaluated code has the global G1, not G2,

This should read "the entry settings object seen by...".

@@ +50,5 @@
> +//
> +// and Debugger is asked to evaluate code in the scope of the frame running f,
> +// then Debugger must coordinate with the browser to ensure that the incumbent
> +// settings object seen by the evaluated code has the global G1, not G2,
> +// because that was the incumbent global in force when f's frame was pushed.

"entry global".

@@ +99,5 @@
> +//   selecting the correct JSContext is one of the roles rewindAndEvaluate is
> +//   meant to play.
> +
> +namespace js {
> +namespace dyn {

The meaning of the "dyn" namespace is not immediately clear to me.

@@ +114,5 @@
> +// JS::dyn::Entry<T>::Stack::rewindAndEvaluate.
> +class Evaluation {
> +    // Carry out the evaluation requested by Debugger's customer, whatever it
> +    // is. The context's current compartment must be the same as that of the
> +    // context passed to rewindAndEvaluate.

Hm, do we really want this invariant? It seems like that generally makes things more complicated for the embedding in the case that it switches JSContexts. Can't invoke() just enter the right compartment?

@@ +137,5 @@
> +
> +    Derived *older() const { return static_cast<Derived *>(older_); }
> +
> +  public:
> +    class Stack : private StackBase {

It seems to me that it would be clearer for Stack to be the outer class and Entry to be the inner class.

@@ +143,5 @@
> +        Derived *top() const { return top_; }
> +
> +      protected:
> +        // Restore the state of this dynamically scoped parameter to what it was
> +        // when |rewindTo| was the incumbent entry (setting aside any newer

incumbent has a more specific meaning than the way you're using it here. I'm going to stop pointing these out from here out.

@@ +174,5 @@
> +} // namespace dyn
> +} // namespace JS
> +
> +
> +// Types and functions for direct use within SpiderMonkey only. These base

?
Attachment #8437349 - Flags: feedback?(bobbyholley) → feedback+
(In reply to Bobby Holley (:bholley) from comment #39)
> In general, the "entry settings object" would be a clearer example here,
> because the incumbent settings object is partially computed from the JS
> stack, and only computed from the script settings stack in the weird corner
> case of callback objects.
> 
> Reading on below, I think this is just the result of an incorrect conflation
> of entry global and incumbent global.

Yes, I was conflating the two. I think I've fixed up all the comments appropriately.


> @@ +99,5 @@
> > +//   selecting the correct JSContext is one of the roles rewindAndEvaluate is
> > +//   meant to play.
> > +
> > +namespace js {
> > +namespace dyn {
> 
> The meaning of the "dyn" namespace is not immediately clear to me.

I've renamed stuff; this is all under js::dbg and JS::dbg now. I'll post the revised patch.


> @@ +114,5 @@
> > +// JS::dyn::Entry<T>::Stack::rewindAndEvaluate.
> > +class Evaluation {
> > +    // Carry out the evaluation requested by Debugger's customer, whatever it
> > +    // is. The context's current compartment must be the same as that of the
> > +    // context passed to rewindAndEvaluate.
> 
> Hm, do we really want this invariant? It seems like that generally makes
> things more complicated for the embedding in the case that it switches
> JSContexts. Can't invoke() just enter the right compartment?

It could. I'll give this a try.


> @@ +137,5 @@
> > +
> > +    Derived *older() const { return static_cast<Derived *>(older_); }
> > +
> > +  public:
> > +    class Stack : private StackBase {
> 
> It seems to me that it would be clearer for Stack to be the outer class and
> Entry to be the inner class.

I'll give that a try, too. It seems reasonable. I was struggling a bit to get things to come together right, but perhaps that struggle was unrelated to this.


> @@ +143,5 @@
> > +        Derived *top() const { return top_; }
> > +
> > +      protected:
> > +        // Restore the state of this dynamically scoped parameter to what it was
> > +        // when |rewindTo| was the incumbent entry (setting aside any newer
> 
> incumbent has a more specific meaning than the way you're using it here. I'm
> going to stop pointing these out from here out.

Yep; the comments don't use the term any more.
(In reply to Bobby Holley (:bholley) from comment #39)
> It seems to me that it would be clearer for Stack to be the outer class and
> Entry to be the inner class.

Okay, I've gotten this working. It's a little more circuitous, but it's not bad.
(In reply to Jim Blandy :jimb from comment #42)
> Okay, I've gotten this working. It's a little more circuitous, but it's not
> bad.

Now that I'm actually instantiating the template, it turns out this can't work.

My goal here is to have a template that gives you a strictly typed API. Here's what I was trying, where the stack entry type is a member class of the stack type:

//     class MyStack : JS::dbg::DynamicStack<MyStack> {
//        class Entry : EntryBase {
//          ... embedding-specific dynamically scoped parameter state ...
//        };
//
//        // Define pure virtual member function; explained below.
//        bool rewindAndEvaluate(...);
//     };

And DynamicStack is something like this:

    template<typename Derived>
    class DynamicStack : private js::dbg::DynamicStackBase {
        ...
        // Return the top entry on this stack, or nullptr if this stack is
        // empty.
        typename Derived::Entry *top() const {
            return static_cast<typename Derived::Entry *>(top_);
        }
        ...
    };

The idea here is that the DynamicStack template can provide stack operations like 'top' that actually know the real stack entry type, referring to it as typename Derived::Entry.

However, C++ doesn't permit this. While we are instantiating DynamicStack<MyStack>, MyStack is still an incomplete struct type, and we can't refer to member types of incomplete struct types, like Derived::Entry.

Here's a reduced program that exhibits the same problem:

$ cat -n crtp.cc
     1	struct EntryBase {  };
     2	struct StackBase { EntryBase *top_; };
     3	
     4	template<typename Derived>
     5	struct Stack : StackBase {
     6	  typename Derived::Entry *top();
     7	};
     8	
     9	struct MyStack : Stack<MyStack> {
    10	  struct Entry : EntryBase {
    11	    int x;
    12	  };
    13	};
    14	
    15	MyStack myStack;
    16	
    17	
    18	int main() { return 0; }
$ g++ -c crtp.cc
crtp.cc: In instantiation of ‘struct Stack<MyStack>’:
crtp.cc:9:18:   required from here
crtp.cc:6:28: error: invalid use of incomplete type ‘struct MyStack’
   typename Derived::Entry *top();
                            ^
crtp.cc:9:8: error: forward declaration of ‘struct MyStack’
 struct MyStack : Stack<MyStack> {
        ^
$
Okay, this actually compiles and has some tests. No browser integration, and the context compartment invariant is still there (but is still expected to be removable). But the comments in DebugAPI.h are intended to be acceptable, as should be the Debugger's use of rewindAndEvaluate.
Attachment #8437349 - Attachment is obsolete: true
Here are running tests. The patch to js/src/shell/js.cpp shows a working example of JS::dbg::DynamicStackEntry in action.
Attachment #8435866 - Attachment is obsolete: true
Attachment #8446985 - Flags: feedback?(jorendorff)
Attachment #8446985 - Flags: feedback?(past)
When reviewing the additions to DebugAPI.h:

The public types in JS::dbg are really just more strictly typed veneers
over the base types declared in js::dbg. To understand the actual behavior
being implemented, it may help to start by looking at how
js::dbg::DynamicStackBase and js::dbg::DynamicStackEntryBase are used
within SpiderMonkey itself, and then see how JS::dbg::DynamicStackEntry fronts
that behavior.
Comment on attachment 8446985 [details] [diff] [review]
Add a SpiderMonkey API for tracking dynamically scoped parameters that coordinates with Debugger.

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

::: js/public/DebugAPI.h
@@ +254,5 @@
>  
>  
> +
> +// Coordinating dynamically scoped parameters with Debugger
> +// --------------------------------------------------------

This can be streamlined. I feel like what we're doing here is conceptually simple; we should be able to make this comment complete with <=25 lines of text.

One suggestion: Maybe we're not doing the embedding any favors by implementing the stack-as-linked-list data structure for them. Surely they have their own stack already? A simpler design would be like this:

    struct Callback {
        virtual void invoke() = 0;
    };

    struct EntrySettingsStack {
        virtual void *getTopEntry() const = 0;
        virtual void revisit(void *entry, Callback &callback) = 0;
    };

    void SetEntrySettingsStack(JSRuntime *rt, EntrySettingsStack *dsc);

The embedding will have to static_cast a void * value in just one place, which I think is acceptable.

(I renamed all the classes to see what it would look like, and I think "EntrySettingsStack" is abstract enough to use here and more meaningful than "DynamicStack".)

Kill the Callback interface, too, if our embedding is comfortable with simple push() and pop() methods. That would save us a bunch of code in vm/Debugger.cpp.

@@ +314,5 @@
> +//   scoped data.
> +//
> +//   For example:
> +//
> +//     class MyParam : JS::dbg::DynamicStackEntry<MyParam> {

(all other comments aside) inherit publicly?

@@ +323,5 @@
> +//   override MyParam::Stack's rewindAndEvaluate method, described below.
> +//
> +//   For example:
> +//
> +//     class MyStack : MyParam::Stack {

and here

@@ +357,5 @@
> +//   You must provide your own MyStack::rewindAndEvaluate override that
> +//   temporarily rewinds to the given state, invokes the Evaluation, and then
> +//   restores the original state. Your override may work by temporarily pushing
> +//   new entries on the stack; by temporarily peeling old entries off the stack;
> +//   or by taking any other course that has the needed visible effects.

"Temporarily peeling old entries off the stack" seems like it would lend itself to a particular kind of bug where the rewindAndEvaluate target entry is not on the stack because we re-entered the debugger.

For example, say we first create this stack of DynamicStackEntries:
   [a, b, c, d, e]   # e is the top entry
then we enter the debugger and call rewindAndEvaluate(c, evalInFrame), which adjusts the stack like so:
   [a, b, c]
and while we are evaluating code in that frame, we hit a debugger statement again, and then the user decides to run some code in the frame corresponding to e. We'll issue rewindAndEvalute(e, evalInFrame2). The Stack object must be prepared to deal with the fact that e isn't on the stack.

I suspect pushing copies of entries is the only reasonable implementation strategy. If so, that would affect at least the name of this method.
Attachment #8446985 - Flags: feedback?(jorendorff)
Comment on attachment 8446985 [details] [diff] [review]
Add a SpiderMonkey API for tracking dynamically scoped parameters that coordinates with Debugger.

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

This looks good to me and I agree that EntrySettingsStack makes sense from a naming standpoint, once you've encountered the term from the HTML5 spec.
Attachment #8446985 - Flags: feedback?(past) → feedback+
(In reply to Panos Astithas [:past] from comment #48)

> This looks good to me and I agree that EntrySettingsStack makes sense from a
> naming standpoint, once you've encountered the term from the HTML5 spec.

"Entry Settings Stack" is not a term from the HTML5 spec - the term should be "Script Settings Stack".
(In reply to Jason Orendorff [:jorendorff] from comment #47)
> This can be streamlined. I feel like what we're doing here is conceptually
> simple; we should be able to make this comment complete with <=25 lines of
> text.

If that's so, then by all means we should do it.


> One suggestion: Maybe we're not doing the embedding any favors by
> implementing the stack-as-linked-list data structure for them. Surely they
> have their own stack already?

They do, but letting us build it is key. Remember that the whole goal here is to build an association between JS stack frames and parameter stack values, so that Debugger.Frame.prototype.eval can find the right thing to rewind to. To implement that association, either their stack points to ours, or ours points to theirs. However, the JSAPI exposes no data type representing a live JS stack frame. Letting SpiderMonkey maintain the stack is actually *less* code for the embedding --- we replace their own linked list code with our base class --- and allows our stack to point to their stack, which requires SM to expose no stack frame type, in addition to being the data that Debugger actually wanted anyway.


> Kill the Callback interface, too, if our embedding is comfortable with
> simple push() and pop() methods. That would save us a bunch of code in
> vm/Debugger.cpp.

Having Debugger run a bunch of push and pop operations is going to be ferociously more complicated. My latest version of this patch includes tests which elucidate what's really going on: While we have temporarily rewound to an earlier point on the stack, we can run new code which can itself push new entries, making the "stack" actually a tree, if you follow the parent links. All the nodes have nicely nested LIFO lifetimes, but the parent links form a tree. And, thanks to Debugger, you can always pick any node in that tree for new evaluation. Unless I'm missing something, you do *not* want to be constructing sequences of pushes and pops that walk around in that tree.

But take a look at the revised patch and see if you agree. The code for dynamically scoped parameters ran correctly the first time.


> The embedding will have to static_cast a void * value in just one place,
> which I think is acceptable.

I feel strongly that we should be providing strictly typed interfaces where possible. This API is carefully constructed so that the embedding has to choose one (base) type and stick with it, and all the member functions provide properly types values.


> 
> @@ +314,5 @@
> > +//   scoped data.
> > +//
> > +//   For example:
> > +//
> > +//     class MyParam : JS::dbg::DynamicStackEntry<MyParam> {
> 
> (all other comments aside) inherit publicly?

Okay.


> @@ +357,5 @@
> > +//   You must provide your own MyStack::rewindAndEvaluate override that
> > +//   temporarily rewinds to the given state, invokes the Evaluation, and then
> > +//   restores the original state. Your override may work by temporarily pushing
> > +//   new entries on the stack; by temporarily peeling old entries off the stack;
> > +//   or by taking any other course that has the needed visible effects.
> 
> "Temporarily peeling old entries off the stack" seems like it would lend
> itself to a particular kind of bug where the rewindAndEvaluate target entry
> is not on the stack because we re-entered the debugger.

Mwahaha. This is your "push and pop" worldview biting you. See the test cases in the revised patch.
Just requesting feedback?, not review?, since we're not agreed on the general approach.

There are two things to look at here that I think are germane to our considerations:

- First, the Frame-eval-dynamic-02.js test. This shows, with psychedelic ASCII art and James Thurber references, the behavior we're trying to implement. Note that it covers the "revisiting popped frame" question you raised.

- Second, the implementation, both in Debugger.cpp and in our toy "embedding", the JS shell. The shell code is quite straightforward. If you set aside the "JS::dbg::Evaluation" thunk (which is something that bholley and I arrived at after a lot of discussion), I think this is actually quite simple: find the right stack entry, and ask the embedding to restore it and call us back.

I'm happy renaming the types; I don't particularly like the names they have now. But there's already a mozilla::dom::ScriptSettingsStack; do we want a JS::dbg::ScriptSettingsStack too?
Attachment #8446985 - Attachment is obsolete: true
Attachment #8446986 - Attachment is obsolete: true
Attachment #8451322 - Flags: feedback?(jorendorff)
Comment on attachment 8451322 [details] [diff] [review]
Add a SpiderMonkey API for tracking dynamically scoped parameters that coordinates with Debugger.

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

jimb and I had a good, wide-ranging discussion on IRC. There's plenty of middle ground here.

The shell/js.cpp code here always pushes and pops (it does not really rewind), which is pretty much what I figured. The test case does address the cases I was worried about. I made an argument for void * (on simplicity grounds) with jimb rather successfully countered (virtual methods are just as simple and also type-safe).

jimb's tree view of dynamic environments is right; but the execution stack (we also agree) is very much a stack, and Frame#older should reflect this.

jimb's going to tweak the design some more. I think I might take a few hours to straighten out the Frame#older chain.
Attachment #8451322 - Flags: feedback?(jorendorff)
Blocks: 1027553
This revision abandons the baroque templates and gives up on trying to hand you an API that works in terms of your own types. And it's vastly simpler, so I'm pretty sure this was a win.
Assignee: nobody → jimb
Attachment #8451322 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8454865 - Flags: review?(jorendorff)
Comment on attachment 8454865 [details] [diff] [review]
Add a SpiderMonkey API for tracking dynamically scoped parameters that coordinates with Debugger.

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

Sorry for this *terrible* review, but I figure I should cut my losses.

r=me with or without any of the changes below except for the one error message that's clearly wrong.

::: js/public/DebugAPI.h
@@ +257,5 @@
> +// --------------------------------------------------------
> +//
> +// It's common for a SpiderMonkey embedding (a web browser, say) to associate
> +// its own metadata with JavaScript execution by maintaining some sort of stack
> +// that parallels the JavaScript call stack: we push something on the parallel

Comment nit: I don't think it helps being all embedding-agnostic here. Please lead with the HTML script settings object. (not "entry settings object")

@@ +293,5 @@
> +  public:
> +    // Carry out the evaluation requested by Debugger's customer, whatever it
> +    // is. The context's current compartment must be the same as that of the
> +    // context passed to rewindAndEvaluate.
> +    virtual bool invoke(JSContext *cx) = 0;

It might help mnemonically to give these interfaces names that match their methods:

    class Evaluation {
        bool evaluate()
    }

    class DynamicRewinder {
        bool dynamicRewind()
    }

Just a thought.

I think it would be even better to make them identical except for the initial capital letter, so you'd have EvalInFrame::evalInFrame and AdjustSettingsAndEval::adjustSettingsAndEval or something like that.

@@ +296,5 @@
> +    // context passed to rewindAndEvaluate.
> +    virtual bool invoke(JSContext *cx) = 0;
> +};
> +
> +class DynamicParameter MOZ_STACK_CLASS {

"DynamicParameter" is no good. I'd prefer any of: ScriptSettings, DynamicallyScopedSettings, Settings, DynamicScope, DynamicContext, ScriptSettingsSnapshot.

(HTML specifies "There is always a 1:1 mapping of global objects to script settings objects." To me, that means that "script settings object" denotes a collection of dynamically-scoped variables, as opposed to a snapshot of their values, which is what this class is for. That's one reason to avoid "ScriptSettingsObject".)

@@ +311,5 @@
> +    DynamicParameter * const older;
> +
> +  public:
> +    // Push this entry on the top of our context's dynamic parameter stack.
> +    bool Init();

Seems odd that this is presented as fallible whereas AutoDynamicRewind's constructor (which does exactly the same thing) isn't.

This could be implemented using a Maybe<AutoDynamicRewind> and perhaps should be. Or perhaps this class should be mostly empty:

    class Settings {    // <---- note, no MOZ_STACK_CLASS here
      public: virtual ~Settings() {}
    };

and then AutoDynamicRewind could be renamed to AutoPushSettings, and embeddings could construct Settings objects whenever they want, but use AutoPushSettings exclusively to make a given Settings the current Top().

@@ +322,5 @@
> +class DynamicRewinder {
> +  public:
> +    // Rewind the state of this dynamically scoped parameter to what it was when
> +    // |param| was the top entry, setting aside any newer entries; invoke
> +    // |evaluation|; restore the stack to its original state; and return

I think this is the part of the patch that confused me. It's using the word "stack"; but it's really a tree; yet there is a static DynamicParameter::Top() method... I had to talk with you for a longish time before I realized that in your mind there is *both* a stack and a tree.

I think this will be 400% better for readers if it's shorter, focuses on the stack-nature, and does not assume that the reader knows "dynamically scoped". It will be unfamiliar jargon to most.

Still not a fan of "rewind". I'd write it like this:

// An embedding that has dynamically-scoped settings should register an EvalWithSettings
// implementation with the JSRuntime.
//
// evalWithSettings() is called only from Debugger.Frame#eval and company. All other code
// simply inherits the dynamic settings of the caller. (That's what "dynamically-scoped"
// means.) But Frame#eval code needs to run in the context of an older frame, so the
// Debugger uses evalWithSettings() to ask the embedding to restore the settings of that
// frame temporarily, just for one evaluation.
class EvalWithSettings {
    // The embedding implements this method. It must:
    // -   temporarily reinstate the given Settings
    //     (using an AutoPushSettings object is a necessary part of this);
    // -   call evaluation.invoke(cx) to run the Frame#eval code;
    // -   revert to the prior Settings; and finally
    // -   return the result of the invoke call.
    virtual bool evalWithSettings(JSContext *cx, Settings *settings, Evaluation &evaluation) = 0;
};

@@ +350,5 @@
> +
> +// Utility for temporarily rewinding the dynamic parameter stack.
> +class AutoDynamicRewind MOZ_STACK_CLASS {
> +    js::PerThreadData * const perThreadData;
> +    DynamicParameter * const saved;

Repeating myself again, it seems perverse to name something AutoDynamicRewind that does nothing but push a new entry onto a stack, then pop it later.

::: js/src/jit-test/tests/debug/Frame-eval-dynamic-02.js
@@ +1,3 @@
> +// Code evaluated with D.F.p.eval can itself extend the dynamically scoped
> +// parameter stack. You are lost in a twisty maze of Debugger-created stack
> +// frames.

Just so you know, I did read this entire thing. I do get it.

@@ +32,5 @@
> +
> +  // frames: eval #1 --> Xingu --> Zorn --> Golux
> +  //                                 |
> +  //                                 `---> (*here*)
> +  assertEq(goluxFrame.older.eval('dynamicParameter()').return, 'Xingu');

OK, trying one more time to explain why I don't like the tree. You don't have to change it, but in case you're curious:

Diagrams should mean something, else we're using bad notation. If we're drawing a tree, that's a digraph, and the edges should represent something that the target audience cares about. Two examples:

1. In a Scheme diagram showing dynamic-wind dynamic extents, the edges are very important---they represent dynamic-wind calls, and more to the point, whenever you traverse one, something happens. The interaction with call/cc reinforces the explanatory power of that tree diagram.

2. In discussing Lisp dynamic variables, I'd say the edges are derivation edges. This matters because you typically only set one or two dynamic variables at a time, and the rest are unchanged -- a node inherits all other dynamic variable values from its predecessor, and that inheritance relation is important to understand. An arrow nicely expresses the idea.

But here do the arrows mean anything? If the arrow from Zorn to Golux means "at one point we replaced Zorn with Golux", there'd be an arrow from Golux back to Zorn. So what do those arrows mean?

In this diagram you have an arrow pointing to "(*here*)" which isn't even a node. Sloppy!

If it's implemented as a stack, behaves in every way like a stack, can be correctly described, understood, and used referring only to a stack --- if every operation on it in fact is either a push or a pop and actually creates or destroys an object on the C++ stack, then...?

::: js/src/shell/js.cpp
@@ +4596,5 @@
> +        return false;
> +    }
> +
> +    if (!args[0].isObject() || !JS_ObjectIsCallable(cx, &args[0].toObject())) {
> +        ReportUsageError(cx, callee, "Second argument must be a function.");

I ... would've liked it better with the arguments reversed. But that doesn't matter --- please fix the error message to say "first argument". :)

@@ +4988,5 @@
> +    JS_FN_HELP("dynamicParameter", DynamicParameter, 0, 0,
> +"dynamicParameter()",
> +"  Return the current value of the shell's dynamic parameter, or |undefined|\n"
> +"  if the stack is empty. The dynamic parameter stack is registered with the\n"
> +"  runtime the first time this function or 'withDynamicParameter' is called.\n"),

I don't think that last sentence is accurate... "is registered with" must refer to either the call to JS::dbg::SetDynamicRewinder or the first call to JS::dbg::DynamicParameter::Init, which happen in main() and WithDynamicParameter() respectively, not in DynamicParameter().

::: js/src/vm/Stack-inl.h
@@ +922,5 @@
> +    MOZ_ASSERT(cx_->perThreadData->dynamicParameterStack == dynamicParameter);
> +}
> +#else
> +inline void Activation::recheckDynamicStack() { }
> +#endif

You don't need the #ifdef, right? MOZ_ASSERT does that.
Attachment #8454865 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #54)
> Sorry for this *terrible* review, but I figure I should cut my losses.

Ugh. What I mean by this is: I'd better just go ahead and unblock you rather than try to revise the review. Sorry again though.
(In reply to Jason Orendorff [:jorendorff] from comment #54)
> > +// It's common for a SpiderMonkey embedding (a web browser, say) to associate
> > +// its own metadata with JavaScript execution by maintaining some sort of stack
> > +// that parallels the JavaScript call stack: we push something on the parallel
> 
> Comment nit: I don't think it helps being all embedding-agnostic here.
> Please lead with the HTML script settings object. (not "entry settings
> object")

Done.


> It might help mnemonically to give these interfaces names that match their
> methods:

Done.

> "DynamicParameter" is no good. I'd prefer any of: ScriptSettings,
> DynamicallyScopedSettings, Settings, DynamicScope, DynamicContext,
> ScriptSettingsSnapshot.

Changed to DynamicScope.

> This could be implemented using a Maybe<AutoDynamicRewind> and perhaps
> should be. Or perhaps this class should be mostly empty:
> 
>     class Settings {    // <---- note, no MOZ_STACK_CLASS here
>       public: virtual ~Settings() {}
>     };
> 
> and then AutoDynamicRewind could be renamed to AutoPushSettings, and
> embeddings could construct Settings objects whenever they want, but use
> AutoPushSettings exclusively to make a given Settings the current Top().

This could be okay, but needs a tweak; I'll explain below.



> I think this will be 400% better for readers if it's shorter, focuses on the
> stack-nature, and does not assume that the reader knows "dynamically
> scoped". It will be unfamiliar jargon to most.

I've changed this section to read:

    // In normal evaluation, we push the script settings stack when we enter
    // JavaScript, and pop it when we leave; the top of the stack is always the
    // proper incumbent script settings object.
    //
    // However, Debugger.Frame.prototype.eval allows us to select some older stack
    // frame and evaluate code in its dynamic context. In this case, we must
    // temporarily revert the script settings stack to the state it was in when
    // that older stack frame was running; do the evaluation in that context; and
    // then restore the script settings stack.
    //
    // If the embedding calls SetDynamicRewinder to provide Debugger with a
    // DynamicRewinder instance, Debugger will use that instance to revert and
    // restore the script settings stack when carrying out evaluations in older
    // frames.
    class DynamicRewinder {
      public:
        // Revert the script settings stack to what it was when |param| was the top
        // entry, setting aside any newer entries; evaluate |evaluation|; restore
        // the stack to its original state; and return whatever |evaluation|
        // returned.
        //
        // Note that |param| may be null, in which case this invokes |evaluation| in
        // a state equivalent to that before any entries were pushed on the stack.
        //
        // Although we say "revert" and "restore", this function really need only
        // produce a state *equivalent* to that when |param| was in force. Whether
        // it accomplishes that by actually temporarily removing stack entries, or
        // by pushing a new top stack entry (or even by doing nothing at all because
        // |param| is already on top!), is up to the implementation.
        //
        // Your override may pass a JSContext other than |cx| to the |evaluation|
        // thunk. (In fact, selecting the correct JSContext for the evaluation is
        // one of the roles dynamicRewind is meant to play.) However, if the
        // evaluation throws, you are responsible for propagating that exception out
        // to |cx| appropriately.
        virtual bool dynamicRewind(JSContext *cx,
                                   DynamicScope *parameter,
                                   Evaluation &evaluation) = 0;
    };

> If it's implemented as a stack, behaves in every way like a stack, can be
> correctly described, understood, and used referring only to a stack --- if
> every operation on it in fact is either a push or a pop and actually creates
> or destroys an object on the C++ stack, then...?

... then it should be a stack.

However, HTML5's definition of the "entry settings object" requires that we actually look further back on the stack than the top:

http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#entry-settings-object

In other words, it's not just the top entry on the stack that matters; its "older" link needs to be as it would have been when D.F.p.eval's 'this' frame was current, too. The tree shown in the test reflects the correct "older" links between DynamicScope (originally DynamicParameter) entries.

(It is also the case that I was misled by D.F.p.older's behavior; I was taking it as meaningful, when it fact, as we have agreed, it probably isn't. The diagram edges show the D.F.p.older relationships as they exist now, but that's not significant.)

The separation between Settings and AutoPushSettings could work, but Settings needs to have the 'older' link, and AutoPushSettings needs to save the old top-of-stack independently of that. It seems weirder to do that than to have a class that temporarily resets the top of the stack.


> I ... would've liked it better with the arguments reversed. But that doesn't
> matter --- please fix the error message to say "first argument". :)

We add more value-ish arguments in the Script Entry Reason patch.

> I don't think that last sentence is accurate... "is registered with" must
> refer to either the call to JS::dbg::SetDynamicRewinder or the first call to
> JS::dbg::DynamicParameter::Init, which happen in main() and
> WithDynamicParameter() respectively, not in DynamicParameter().

Thanks for catching that; it's a remnant of the prior iteration of the patch.


> You don't need the #ifdef, right? MOZ_ASSERT does that.

Duh. Fixed.
(In reply to Jim Blandy :jimb from comment #56)
> ... then it should be a stack.
> 
> However, HTML5's definition of the "entry settings object" requires that we
> actually look further back on the stack than the top:
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.
> html#entry-settings-object
> 
> In other words, it's not just the top entry on the stack that matters; its
> "older" link needs to be as it would have been when D.F.p.eval's 'this'
> frame was current, too. The tree shown in the test reflects the correct
> "older" links between DynamicScope (originally DynamicParameter) entries.

It's the topmost "candidate entry settings object" (i.e. AutoEntryScript) that matters. But given that the debugger API probably isn't going to do anything with non-candidate entry settings objects, I think you can actually more or less assume that the top entry is the only one that matters.

I think the current stack model is actually overly-complicated, and I'm going to suggest to Hixie that we separate them into two stacks at some point.

Please flag me for sr on the final patch once you've got the details sorted out with jorendorff.
The reason you're feeling so apologetic is that you wrote "r+" when you meant "feedback-".
(In reply to Bobby Holley (:bholley) from comment #57)
> It's the topmost "candidate entry settings object" (i.e. AutoEntryScript)
> that matters. But given that the debugger API probably isn't going to do
> anything with non-candidate entry settings objects, I think you can actually
> more or less assume that the top entry is the only one that matters.
> 
> I think the current stack model is actually overly-complicated, and I'm
> going to suggest to Hixie that we separate them into two stacks at some
> point.

Well, if the relationships between entries don't matter, then we really can simplify things drastically. I'll go around again.

> Please flag me for sr on the final patch once you've got the details sorted
> out with jorendorff.

Will do.
Okay, this patch is further simplified, and implements some of the suggestions in the review. Try push:

https://tbpl.mozilla.org/?tree=Try&rev=e57e92163e5f
Attachment #8454865 - Attachment is obsolete: true
Attachment #8462932 - Flags: review?(jorendorff)
Comment on attachment 8462932 [details] [diff] [review]
Add a SpiderMonkey API for tracking script settings objects that coordinates with Debugger.

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

I do like this better. r=me, and I'm sorry for the delay.

::: js/src/jit-test/tests/debug/Frame-eval-scriptSettings-01.js
@@ +22,5 @@
> +  assertEq(scriptSettings(), 'g');
> +  assertEq(frame.older.eval('scriptSettings()').return, 'f');
> +  assertEq(scriptSettings(), 'g');
> +  assertEq(frame.eval('scriptSettings()').return, 'g');
> +  assertEq(scriptSettings(), 'g');

These tests are much more concise than the Golux ones. I think they're about as good, though. +1.

::: js/src/vm/Debugger.cpp
@@ +4768,5 @@
> +class DebuggerEvaluation : public ScriptSettingsRewinder::Evaluation {
> +    JSContext *outerCx;
> +    Handle<Env*> env;
> +    HandleValue thisv;
> +    AbstractFramePtr frame;

...Wow. C++ lambdas just can't come soon enough, can they.

(This is no reflection on the code, it's a reflection on C++)

::: js/src/vm/Runtime.cpp
@@ +806,5 @@
> +
> +namespace JS {
> +namespace dbg {
> +void
> +SetScriptSettingsRewinder(JSRuntime *runtime, ScriptSettingsRewinder *rewinder)

This one I would declare without the enclosing namespace { blocks:

   void
   JS::dbg::SetScriptSettingsRewinder( .....
   ......

because it's a compile error (not a link error) if the signature doesn't match the one in the header, but your call.

::: js/src/vm/Stack-inl.h
@@ +903,5 @@
> +{
> +    // Changing the script settings object should always accompany entering and
> +    // exiting Activations. That is, there is a one-to-many relationship between
> +    // script settings objects and Activations. So the current SSetO had better
> +    // be the same as it was when this activation was constructed.

Comment micro-nit: This doesn't need to duplicate so much of the comment in the header.
Attachment #8462932 - Flags: review?(jorendorff) → review+
Comment on attachment 8462932 [details] [diff] [review]
Add a SpiderMonkey API for tracking script settings objects that coordinates with Debugger.

Flagging bholley for sr as promised. I'll have a patch to make the browser use this API in a few hours.
Attachment #8462932 - Flags: superreview?(bobbyholley)
Comment on attachment 8462932 [details] [diff] [review]
Add a SpiderMonkey API for tracking script settings objects that coordinates with Debugger.

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

I'd like to see the code that hooks this up to the embedding before sr+ing this.

::: js/public/DebugAPI.h
@@ +294,5 @@
> +    static ScriptSettings *Top(JSContext *cx);
> +};
> +
> +// A class the embedding can use to establish which ScriptSettings object to use
> +// for given invocations of JavaScript code.

Similarly, the intention is for the embedding to extend this, right?

@@ +307,5 @@
> +  public:
> +    // Establish |settings| as the ScriptSettings object to apply to all
> +    // JavaScript frames entered on |cx|'s thread, when this is the thread's
> +    // youngest live AutoScriptSettings instance.
> +    AutoScriptSettings(JSContext *cx, ScriptSettings *settings);

Hm, how are we going to deal with the fact that AutoEntryScript is about to become fallible?
(In reply to Bobby Holley (:bholley) from comment #63)
> Comment on attachment 8462932 [details] [diff] [review]
> Add a SpiderMonkey API for tracking script settings objects that coordinates
> with Debugger.
> 
> Review of attachment 8462932 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see the code that hooks this up to the embedding before sr+ing
> this.
> 
> ::: js/public/DebugAPI.h
> @@ +294,5 @@
> > +    static ScriptSettings *Top(JSContext *cx);
> > +};
> > +
> > +// A class the embedding can use to establish which ScriptSettings object to use
> > +// for given invocations of JavaScript code.
> 
> Similarly, the intention is for the embedding to extend this, right?

No, this is just supposed to be a push/pop manager. It's only ScriptSettings that's meant to be extended. I think the sample patch will make this clearer.

> Hm, how are we going to deal with the fact that AutoEntryScript is about to
> become fallible?

Here, too, I think the sample patch will be helpful.
In debugging the sample patch, I came across the following in mozilla::dom::CallbackObject::CallSetup::CallSetup:

https://hg.mozilla.org/mozilla-central/file/0c7eb00d3ef6/dom/bindings/CallbackObject.cpp#l129

    mAutoEntryScript.construct(globalObject, mIsMainThread, cx);
    mAutoEntryScript.ref().SetWebIDLCallerPrincipal(webIDLCallerPrincipal);
    nsIGlobalObject* incumbent = aCallback->IncumbentGlobalOrNull();
    if (incumbent) {
      // The callback object traces its incumbent JS global, so in general it
      // should be alive here. However, it's possible that we could run afoul
      // of the same IPC global weirdness described above, wherein the
      // nsIGlobalObject has severed its reference to the JS global. Let's just
      // be safe here, so that nobody has to waste a day debugging gaia-ui tests.
      if (!incumbent->GetGlobalJSObject()) {
        return;
      }
      mAutoIncumbentScript.construct(incumbent);
    }

Note that we construct an AutoEntryScript, which inherits from AutoJSAPI, but never call its Init method.

My understanding was that we don't want to push an AutoJSAPI, or anything derived from that, until its Init has succeeded. I think I implemented that behavior, but now this construction asserts because the AutoIncumbentScript is not a candidate entry point, and hence requires JSAPI use to be enabled.

Since mAutoEntryScript.ref() has never had its 'Init' called, the context pusher and the auto compartment have never been constructed. Is that how this is supposed to behave?
Flags: needinfo?(bobbyholley)
Oh, I see this now:

https://hg.mozilla.org/mozilla-central/file/0c7eb00d3ef6/dom/base/ScriptSettings.h#l177

protected:
  // Protected constructor, allowing subclasses to specify a particular cx to
  // be used. This constructor initialises the AutoJSAPI, so Init must NOT be
  // called on subclasses that use this.
  // If aGlobalObject, its associated JS global or aCx are null this will cause
  // an assertion, as will setting aIsMainThread incorrectly.
  AutoJSAPI(nsIGlobalObject* aGlobalObject, bool aIsMainThread, JSContext* aCx);

So indeed, the CallSetup constructor is correct; I think I see the change I need to make. Sorry for the noise.
Flags: needinfo?(bobbyholley)
This is an amendment to the original JS::dbg::ScriptSettings patch.

Since one of the main things the browser needs to use the JS::dbg::ScriptSettings to manage is exactly choosing the right JSContext, it's dumb to require a JSContext to work with SpiderMonkey's ScriptSetting stack.

This patch changes the API to use SpiderMonkey's existing TLS.
Attachment #8472103 - Flags: review?(jorendorff)
Per our conversation, this should be appropriate.
Attachment #8472104 - Flags: review?(bobbyholley)
I'm running out of time to test it tonight, but this is generally what it's going to look like. Will test tomorrow.

It turns out that we never need to make pushing conditional on a successful Init: only AutoEntryScript is both an AutoJSAPI and a ScriptSettingsStackEntry, and that doesn't need an Init call. Hence, ScriptSettingsStackEntry can continue to have its constructor push, and everything stays simple.

If we add a no-push-until-successful-Init class later, we'll know how to do it.
Attachment #8472107 - Flags: feedback?(bobbyholley)
Attachment #8472104 - Flags: review?(bobbyholley) → review+
Comment on attachment 8472107 [details] [diff] [review]
Use JS::dbg::ScriptSettings for the browser's script settings stack.

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

(In reply to Jim Blandy :jimb from comment #69)
> Created attachment 8472107 [details] [diff] [review]
> Use JS::dbg::ScriptSettings for the browser's script settings stack.
> 
> I'm running out of time to test it tonight, but this is generally what it's
> going to look like. Will test tomorrow.
> 
> It turns out that we never need to make pushing conditional on a successful
> Init: only AutoEntryScript is both an AutoJSAPI and a
> ScriptSettingsStackEntry, and that doesn't need an Init call. Hence,
> ScriptSettingsStackEntry can continue to have its constructor push, and
> everything stays simple.
> 
> If we add a no-push-until-successful-Init class later, we'll know how to do
> it.

But we're switching AutoEntryScript to using an Init() in bug 1027553? Didn't you say yesterday that you were going to fix that bug as a prerequisite for this patch?
Attachment #8472107 - Flags: feedback?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #70)
> But we're switching AutoEntryScript to using an Init() in bug 1027553?
> Didn't you say yesterday that you were going to fix that bug as a
> prerequisite for this patch?

You're right --- I'd swapped out the context of bug 1027553.
Comment on attachment 8472104 [details] [diff] [review]
Ensure mozilla::dom::ScriptSettingsStackEntry is only used as a base class: make its constructor protected.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1674868544c
Attachment #8472104 - Flags: checkin+
Comment on attachment 8462932 [details] [diff] [review]
Add a SpiderMonkey API for tracking script settings objects that coordinates with Debugger.

Cancelling this for now, given that we're still waiting on the patches to hook this up to the embedding.
Attachment #8462932 - Flags: superreview?(bobbyholley)
Attachment #8472103 - Flags: review?(jorendorff) → review+
Not a prerequisite; just something that I was wondering about that seems to be true.
Attachment #8480263 - Flags: review?(bobowencode)
Now revised to give AutoEntryScript a fallible Init. Changes to all AutoEntryScript instantiations in the subsequent patch for ease of review, but they should be squished before landing.
Attachment #8472107 - Attachment is obsolete: true
Attachment #8480268 - Flags: review?(bobowencode)
These are the changes to the places that instantiate AutoEntryScript. This script is mostly mechanical, so it's broken out into its own thing for ease of review.
Attachment #8480269 - Flags: review?(bobowencode)
Try push for the whole stack:
https://tbpl.mozilla.org/?tree=Try&rev=08cad9d7f106
Comment on attachment 8480263 [details] [diff] [review]
Assert that AutoJSAPI is always given a JSContext.

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

Yeah, something has gone badly wrong if we don't have a JSContext by this point.
Attachment #8480263 - Flags: review?(bobowencode) → review+
Attachment #8480263 - Flags: checkin+
Comment on attachment 8480268 [details] [diff] [review]
Use JS::dbg::ScriptSettings for the browser's script settings stack.

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

Sorry Jim, but I'm afraid I'm going to have to pass on these.

While I felt confident reviewing the simple assertion, I don't fully understand how Bobby wants to implement all of this, particularly some of the invariants he wants to enforce once this is all being used properly.

For what they're worth, I've added some thoughts, which may or may not be useful, that came to mind as I was reading through this.

::: dom/base/ScriptSettings.cpp
@@ +63,5 @@
>  {
>    MOZ_ASSERT(mGlobalObject);
>    MOZ_ASSERT(mGlobalObject->GetGlobalJSObject(),
>               "Must have an actual JS global for the duration on the stack");
>    MOZ_ASSERT(JS_IsGlobalObject(mGlobalObject->GetGlobalJSObject()),

I rather hoped that we would be able to leave these checks until the Init.
That way we would be able to remove some of the boiler plate checking that exists in a lot of the code before the AutoEntryScripts.
As I remember it, this was the main motivation for adding the fallible Inits to AutoJSAPI.

The first two are done by the AutoJSAPI and I imagine we could add the third as an assertion as well.
Although we might have to check we are always passing inner windows to AutoJSAPI.

::: dom/base/ScriptSettings.h
@@ +135,5 @@
>    return *GetIncumbentGlobal()->GetGlobalJSObject();
>  }
>  
>  class ScriptSettingsStack;
> +class ScriptSettingsStackEntry: public JS::dbg::ScriptSettings {

Just an observation, but it seems slightly odd to me that something that is part of standard script execution by the embedding, inherits from something in JS::dbg.

@@ +152,5 @@
> +  // We don't just do this automatically in the constructor, because some of our
> +  // derived classes have fallible Init methods, and we don't want to push an
> +  // instance of such a class until its Init has succeeded. Subclasses with
> +  // infallible initialization can call this from their constructor.
> +  void Push();

What if we did do this in the constructor and we didn't pass the nsIGlobalObject until the Init.
Then I guess if the Init was missed (or AutoJSAPI.Init() was called) this would act like an AutoNoJSAPI (at least as far as the ScriptSettingsStack is concerned) and eventually cause assertions.

This would be on top of the assertion you would get if you tried to access .cx().

Actually maybe NoJSAPI() could be changed to work off a bool that is set by sub-classes or just implemented in sub-classes.
Then we could assert against the use of anything on the stack that returned false, but didn't have an mGlobalObject.
Attachment #8480268 - Flags: review?(bobowencode)
Attachment #8480269 - Flags: review?(bobowencode)
(In reply to Bob Owen (:bobowen) from comment #82)
> Sorry Jim, but I'm afraid I'm going to have to pass on these.

Well, thanks for taking a look! Bobby's almost back, anyway.


> I rather hoped that we would be able to leave these checks until the Init.
> That way we would be able to remove some of the boiler plate checking that
> exists in a lot of the code before the AutoEntryScripts.
> As I remember it, this was the main motivation for adding the fallible Inits
> to AutoJSAPI.

Can you show me some of this boilerplate? I looked around and didn't see anything I recognized as such.

It does seem like it could be moved to a fallible Init; but doesn't a failure to meet those conditions indicate an internal inconsistency in Firefox, more than an error a chrome JS or web developer would want to see reported?


> ::: dom/base/ScriptSettings.h
> @@ +135,5 @@
> >    return *GetIncumbentGlobal()->GetGlobalJSObject();
> >  }
> >  
> >  class ScriptSettingsStack;
> > +class ScriptSettingsStackEntry: public JS::dbg::ScriptSettings {
> 
> Just an observation, but it seems slightly odd to me that something that is
> part of standard script execution by the embedding, inherits from something
> in JS::dbg.

That inheritance is what allows SpiderMonkey to record which ScriptSettingsStackEntry goes with which JS stack frames. The only use of that type is to be inherited from here. You can review attachment 8462932 [details] [diff] [review] to see how this all fits together; I've got a general explanation in the comments in js/public/DebugAPI.h.
Comment on attachment 8480268 [details] [diff] [review]
Use JS::dbg::ScriptSettings for the browser's script settings stack.

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

::: dom/base/ScriptSettings.cpp
@@ +354,5 @@
>  AutoIncumbentScript::AutoIncumbentScript(nsIGlobalObject* aGlobalObject)
>    : ScriptSettingsStackEntry(aGlobalObject, /* aCandidate = */ false)
>    , mCallerOverride(nsContentUtils::GetCurrentJSContextForThread())
>  {
> +  Push();

Hmm. Looking this over, it occurs to me that the patch changes the order in which the push of the AutoIncumbentScript-as-ScriptSettingsStackEntry and the mCallerOverride construction occur. But since pushing a ScriptSettingsStackEntry can't affect the return value of GetCurrentJSContextForThread, it can't matter, so I guess this is okay.
(In reply to Jim Blandy :jimb from comment #83)

> > I rather hoped that we would be able to leave these checks until the Init.
> > That way we would be able to remove some of the boiler plate checking that
> > exists in a lot of the code before the AutoEntryScripts.
> > As I remember it, this was the main motivation for adding the fallible Inits
> > to AutoJSAPI.
> 
> Can you show me some of this boilerplate? I looked around and didn't see
> anything I recognized as such.

Perhaps boilerplate is the wrong word, here are some examples, it might mean moving the construction of the AutoEntryScript in some cases:

http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#1088
http://dxr.mozilla.org/mozilla-central/source/dom/bindings/CallbackObject.cpp#124
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsJSNPRuntime.cpp#613 (and 741, 775)
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#1481
http://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLProtoImplField.cpp#404

> It does seem like it could be moved to a fallible Init; but doesn't a
> failure to meet those conditions indicate an internal inconsistency in
> Firefox, more than an error a chrome JS or web developer would want to see
> reported?

Quite possibly, but generally when I was replacing the old context pushing with AutoEntryScript (and AutoJSAPI), if the original code had null checking (usually for an nsIScriptGlobalObject), I kept similar null checking to maintain behaviour.
Where the existing code assumed that the objects existed I did the same.

> > Just an observation, but it seems slightly odd to me that something that is
> > part of standard script execution by the embedding, inherits from something
> > in JS::dbg.
> 
> That inheritance is what allows SpiderMonkey to record which
> ScriptSettingsStackEntry goes with which JS stack frames. The only use of
> that type is to be inherited from here. You can review attachment 8462932 [details] [diff] [review]
> [details] [diff] [review] to see how this all fits together; I've got a
> general explanation in the comments in js/public/DebugAPI.h.

Yeah, I at least partially understand what this is trying to achieve.
So, if the ScriptSettingsObject is something associated with JS Stack Frames and these are only used by the debugger I guess that makes sense of the inheritance.
Attachment #8480268 - Flags: review?(bobbyholley)
Attachment #8480269 - Flags: review?(bobbyholley)
In the future, if you want to partially land a patch stack, please split it out into separate bugs. Having a bug landing straddling two releases makes things really confusing for release management. ;-)
Comment on attachment 8480268 [details] [diff] [review]
Use JS::dbg::ScriptSettings for the browser's script settings stack.

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

I haven't fully groked the whole patch thing, but the argumentless-constructor thing and NS_ENSURE thing (in the other patch) are going to make things look fairly different, so I'd like to take a look at an updated revision before going further. :-)

::: dom/base/ScriptSettings.cpp
@@ +28,4 @@
>  class ScriptSettingsStack {
>  public:
>    static ScriptSettingsStackEntry* Top() {
> +    return static_cast<ScriptSettingsStackEntry *>(JS::dbg::ScriptSettings::Top());

In the patch for this API that I'm looking at, Top() takes a cx. Am I looking at the wrong one?

@@ +63,5 @@
>  {
>    MOZ_ASSERT(mGlobalObject);
>    MOZ_ASSERT(mGlobalObject->GetGlobalJSObject(),
>               "Must have an actual JS global for the duration on the stack");
>    MOZ_ASSERT(JS_IsGlobalObject(mGlobalObject->GetGlobalJSObject()),

As Bob notes, these should live in Init() or Push().

::: dom/base/ScriptSettings.h
@@ +276,5 @@
>   */
>  class AutoEntryScript : public AutoJSAPI,
>                          protected ScriptSettingsStackEntry {
>  public:
> +  explicit AutoEntryScript(nsIGlobalObject* aGlobalObject);

I think this should be an empty constructor and the global should be passed to Init. Is there any reason we can't do that?
Attachment #8480268 - Flags: review?(bobbyholley) → review-
Comment on attachment 8480269 [details] [diff] [review]
Change all places that instantiate AutoEntryScript to call its Init function.

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

Would you mind actually just merging this patch into the other one?  They'll need to be merged before landing, and since neither one is correct without the other I think it'd be simpler to look at all together.

Technically, we're not supposed to add new instances of NS_ENSURE_*. The enforcement on that has been pretty lax, and we've more or less kept using them in a lot of places because they're so useful. However, there's been a long taboo against putting "real work" inside those macros. This is a priori necessary for things that return an nsresult, because otherwise we have no way to propagate the correct failure code. That doesn't apply to bool-valued functions, but the pattern still stands, and I think we shouldn't rock the boat here.

So the two acceptable styles would be:

AutoEntryScript aes;
bool ok = aes.Init(global);
NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);

or

AutoEntryScript aes;
if (NS_WARN_IF(!aes.Init(global))) {
  return NS_ERROR_FAILURE;
}

The latter is the way that Bob did all of the AutoJSAPI instances, and it has the advantage of not crossing bsmedberg's decree. So let's go with that one.
Attachment #8480269 - Flags: review?(bobbyholley) → review-
Depends on: 1164664
No longer blocks: 1173883
Priority: -- → P3
Component: DOM → DOM: Core & HTML
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: