Last Comment Bug 690135 - create scope objects eagerly or not at all (and don't break the debugger)
: create scope objects eagerly or not at all (and don't break the debugger)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 713311 746423
Blocks: 659577
  Show dependency treegraph
 
Reported: 2011-09-28 15:45 PDT by Luke Wagner [:luke]
Modified: 2012-05-19 08:53 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 (98.50 KB, patch)
2011-12-16 17:46 PST, Luke Wagner [:luke]
no flags Details | Diff | Review
fix debugger (WIP) (59.17 KB, patch)
2012-04-24 10:28 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
create scopes eagerly or not at all (thereby breaking the debugger) (71.09 KB, patch)
2012-04-24 18:54 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
fix the debugger (61.06 KB, patch)
2012-04-24 18:57 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
rm unused jsdbgapi stuff (7.47 KB, patch)
2012-04-24 18:58 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Review
create scopes eagerly or not at all (thereby breaking the debugger) (70.43 KB, patch)
2012-04-25 15:57 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
fix the debugger (68.57 KB, patch)
2012-04-25 15:58 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
create scopes eagerly or not at all (thereby breaking the debugger) v.2 (73.83 KB, patch)
2012-05-04 11:45 PDT, Luke Wagner [:luke]
jimb: review+
Details | Diff | Review
fix the debugger v.2 (73.27 KB, patch)
2012-05-04 19:50 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
fix the debugger v.3 (74.45 KB, patch)
2012-05-07 21:19 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
hoist DeclEnvObject creation logic, make callee enumerable (2.00 KB, patch)
2012-05-08 14:59 PDT, Luke Wagner [:luke]
jimb: review+
Details | Diff | Review
fix the debugger v.4 (75.09 KB, patch)
2012-05-08 15:00 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
don't use ScopeObject::maybeStackFrame (12.93 KB, patch)
2012-05-08 15:03 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
fix the debugger v.5 (74.97 KB, patch)
2012-05-09 00:02 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
combined for Panos (applies on cset 7082192622e6) (150.04 KB, patch)
2012-05-09 09:55 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
fix the debugger v.6 (77.78 KB, patch)
2012-05-10 00:28 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
fix the debugger v.7 (82.06 KB, patch)
2012-05-11 14:57 PDT, Luke Wagner [:luke]
jimb: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2011-09-28 15:45:40 PDT
JS_GetFrameScopeChain causes unexpected call object creation which blocks bug 659577.  There are two tricky uses: evalInFrame and jsd.  In the case of jsd, this scope chain is exposed to general JS via frame.scope (and is used all over by Firebug) which is really bad since we generally assume Call objects don't escape into the wild.

The proposed solution is to have JS_GetFrameScopeChain build a parallel scope chain, one object for each frame, where each object either defers to the frame's call object, if the call object already exists, and otherwise manipulates the stack frame directly.
Comment 1 Luke Wagner [:luke] 2011-09-29 12:43:39 PDT
Change of plans.  I'm going to wait for the js::dbg2 to remove the use of scopes-as-objects.  The more immediate concern is that fp->scopeChain() should always have a statically-known number of activation objects.  To fix that, we can just create plain old Call/Block objects as usual (in JS_GetFrameScopeChain), but not stick them on fp->scopeChain().  Rather, we can rely on Debugger::onLeave{Block,Frame} to call Put{Call,Block}.  This will be decidedly simpler.
Comment 2 Luke Wagner [:luke] 2011-12-16 17:46:23 PST
Created attachment 582450 [details] [diff] [review]
WIP 1

Here is a WIP for comment by Jason/Jim.  With the patch, there is no js::GetScopeChain and fp->scopeChain() is the real scope.  Instead, the debugger uses js::GetDebugEnv which creates a faux scope chain of proxies that wrap the real scope chain objects (filling in new objects where missing).

The patch passes all jit-tests (nice work on jit-tests/tests/debug).  There is a known bug involving nested closures that I have yet to fix (store unwrapped objects in the Environment map and fixup as necessary).

Ignore the lack of comments and gross obj->getFixedSlot(...)/(bool)obj->getPrivate() stuff all over.  I'm going to write another patch (preceding this) that extrudes a vm/BlockObject.h and vm/WithObject.h.
Comment 3 Luke Wagner [:luke] 2011-12-22 09:47:56 PST
Comment on attachment 582450 [details] [diff] [review]
WIP 1

Nevermind, I'm going to back up and do things a bit differently.
Comment 4 Jim Blandy :jimb 2012-01-27 00:08:54 PST
Thanks, bugzilla.
Comment 5 Jim Blandy :jimb 2012-01-27 00:09:39 PST
I guess that was a display issue, and not a real problem in the bug. Sorry for the noise.
Comment 6 Luke Wagner [:luke] 2012-04-24 10:28:55 PDT
Created attachment 617930 [details] [diff] [review]
fix debugger (WIP)

So after a bunch of intermediary patches, I came back to this and the problem is, unsurprisingly, much easier to solve and I am much happier with the solution.  This patch even passes all jit/ref tests, including the jsdbg2 ones.
Comment 7 Luke Wagner [:luke] 2012-04-24 18:54:08 PDT
Created attachment 618133 [details] [diff] [review]
create scopes eagerly or not at all (thereby breaking the debugger)
Comment 8 Luke Wagner [:luke] 2012-04-24 18:57:42 PDT
Created attachment 618135 [details] [diff] [review]
fix the debugger

I even tested and Firebug seems to basically work (after the two tiny hacks added to jsdbgapi and jsd to let DebugScopeObjects act like scope objects used to).

With these two patches, scope chains finally have a fixed depth.
Comment 9 Luke Wagner [:luke] 2012-04-24 18:58:35 PDT
Created attachment 618136 [details] [diff] [review]
rm unused jsdbgapi stuff
Comment 10 Luke Wagner [:luke] 2012-04-25 15:57:49 PDT
Created attachment 618469 [details] [diff] [review]
create scopes eagerly or not at all (thereby breaking the debugger)

Found a few more bugs

(Jason looks busy and this is solidly about the debugger so I'll hand the review over to jimb.)
Comment 11 Luke Wagner [:luke] 2012-04-25 15:58:14 PDT
Created attachment 618470 [details] [diff] [review]
fix the debugger
Comment 12 Jason Orendorff [:jorendorff] 2012-04-26 13:08:20 PDT
Comment on attachment 618136 [details] [diff] [review]
rm unused jsdbgapi stuff

>+GetPropertyDesc(JSContext *cx, JSObject *obj_, Shape *sprop, JSPropertyDesc *pd)
> {
>     assertSameCompartment(cx, obj_);
>-    Shape *shape = (Shape *) sprop;
>+    Shape *shape = sprop;

It looks like after this sprop is unused and shape is only used to the left of ->, so just rename the argument to "shape"?
Comment 13 Luke Wagner [:luke] 2012-04-30 12:40:15 PDT
Comment on attachment 618469 [details] [diff] [review]
create scopes eagerly or not at all (thereby breaking the debugger)

Canceling review since changes are necessary to remove dependency on bug 746423.
Comment 15 Ed Morley [:emorley] 2012-05-04 11:40:39 PDT
https://hg.mozilla.org/mozilla-central/rev/ff1d8e3ebdf9
Comment 16 Luke Wagner [:luke] 2012-05-04 11:45:13 PDT
Created attachment 621119 [details] [diff] [review]
create scopes eagerly or not at all (thereby breaking the debugger) v.2

Now with more DeclEnvObject!  Also, ScopeDesc was turned into a more more rational/usable ScopeIter.  It is only lightly used in this patch (to re-implement (beautifully) UnwindScope), but it will be used more heavily in the "fix the debugger" patch (to be posted soon) and other patches to be posted.
Comment 17 Jim Blandy :jimb 2012-05-04 17:16:08 PDT
Comment on attachment 621119 [details] [diff] [review]
create scopes eagerly or not at all (thereby breaking the debugger) v.2

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

By changing the GetScopeChain calls to scopeChain calls, we're no longer verifying that the blockChain and scopeChain are aligned when we're figuring out something's parent. In other words, we're now putting new scope chain elements atop parents without checking that the parent is the one we'd expect from our static knowledge of the problem. Would it make sense to provide a variant of scopeChain that asserts that scopeChain and blockChain are aligned, for use in cases where we're producing a parent link? (When creating 'with' blocks, we don't have any information about what sort of thing to expect as a parent, but just checking that blockChain and scopeChain are aligned would be better than nothing.)

::: js/src/jsinterp.cpp
@@ +762,5 @@
>              return JS_FALSE;
>          sp[-1].setObject(*obj);
>      }
>  
> +    JSObject *withobj = WithObject::create(cx, obj, fp->scopeChain(),

Here, for example, it would be reassuring to assert that the scope chain is the one atop which this WithObject belongs. Since we have no real record of the full nesting of 'with' and 'let' blocks, asserting that the blockChain and the scopeChain are aligned is perhaps as close as we can get.

@@ -971,5 @@
> -           obj.getPrivate() == js_FloatingFrameIfGenerator(cx, cx->fp()) &&
> -           obj.asNestedScope().stackDepth() >= stackDepth;
> -}
> -
> -/* Unwind block and scope chains to match the given depth. */

This comment is still correct, right?

@@ -3285,5 @@
>      RootedVarFunction &fun = rootFunction0;
>      fun = script->getFunction(GET_UINT32_INDEX(regs.pc));
> -    JSObject *obj = fun;
> -
> -    /* do-while(0) so we can break instead of using a goto. */

Hah, there are no more 'break's in this 'do-while' statement!

::: js/src/vm/ScopeObject.cpp
@@ +1133,5 @@
> +{
> +    /*
> +     * Given an iterator state (cur_, block_), figure out which (potentially
> +     * optimized) scope the iterator should report. Thus, the result is a pair
> +     * (type_, hasScopeObject_) where hasScopeObject_. Beware: while ScopeIter

Luke says this sentence was meant to continue: "... indicates whether the iterator's current scope object has been optimized away."

@@ +1171,5 @@
> +        hasScopeObject_ = true;
> +    } else if (block_) {
> +        type_ = Block;
> +        hasScopeObject_ = block_->needsClone();
> +        JS_ASSERT_IF(hasScopeObject_, cur_->asClonedBlock().staticBlock() == *block_);

This was surprising until I found the == operator on JSObject & values.

::: js/src/vm/Stack.cpp
@@ +275,5 @@
> +
> +    if (blockChain_->needsClone()) {
> +        ClonedBlockObject &clone = scopeChain_->asClonedBlock();
> +        JS_ASSERT(clone.staticBlock() == *blockChain_);
> +        clone.put(cx);

I take it this 'put' would go away if we fix bug 659577?

@@ +285,5 @@
> +
> +void
> +StackFrame::popWith(JSContext *cx)
> +{
> +    setScopeChain(scopeChain()->asWith().enclosingScope());

Why do we use scopeChain() here, but scopeChain_ in popBlock's cloned case?

::: js/src/vm/Stack.h
@@ +916,5 @@
>  
> +    bool pushBlock(JSContext *cx, StaticBlockObject &block);
> +    void popBlock(JSContext *cx);
> +
> +    /* Exits (via execution or exception) a with block. */

It'd be nice to have a symmetrical comment over pushBlock and popBlock, mentioning that they maintain both blockChain and scopeChain.
Comment 18 Luke Wagner [:luke] 2012-05-04 17:41:42 PDT
(In reply to Jim Blandy :jimb from comment #17)
> By changing the GetScopeChain calls to scopeChain calls, we're no longer
> verifying that the blockChain and scopeChain are aligned when we're figuring
> out something's parent.

In general, blockChain and scopeChain are not necessarily aligned.  About the only thing you can say is that, if you iterate down block chain, if block->needsClone(), you can find it on a parallel iteration of fp->scopeChain (skipping with blocks).  Is that what you mean?  Note: this already gets asserted by normal execution of popBlock and AssertValid*ScopeAtExit and any iteration of ScopeIter.

> This was surprising until I found the == operator on JSObject & values.

Waldo added this a while ago.  It was a bit weird at the beginning (b/c objects aren't "values" in the traditional C++ sense), then I started using it b/c it's shorter and I saw it in enough places to get used to it.  Given that equality of objects can only mean identity, I don't see it surprising anyone (well, in the bad "surprise you have a bug" way).

> I take it this 'put' would go away if we fix bug 659577?

Yeah it will!

> Why do we use scopeChain() here, but scopeChain_ in popBlock's cloned case?

No reason.  I'll use scopeChain()
Comment 19 Luke Wagner [:luke] 2012-05-04 19:50:26 PDT
Created attachment 621235 [details] [diff] [review]
fix the debugger v.2

Now with more DeclEnvObject!

jimb: the heart of the patch (and perhaps the first thing to review) is js::GetDebugScopeFor(Frame|Function).  This is a fun recursive algorithm that walks over the entire scope chain and proxies everything.  A good starting point is the big "Debug scope objects" comment in ScopeObject.h.
Comment 20 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-04 21:06:26 PDT
(In reply to Luke Wagner [:luke] from comment #18)
> Waldo added this a while ago.

Suggested, I think, not added:

http://hg.mozilla.org/mozilla-central/annotate/0a48e6561534/js/src/jsobj.h#l1003
Comment 21 Jim Blandy :jimb 2012-05-07 13:13:06 PDT
(In reply to Luke Wagner [:luke] from comment #18)
> In general, blockChain and scopeChain are not necessarily aligned.  About
> the only thing you can say is that, if you iterate down block chain, if
> block->needsClone(), you can find it on a parallel iteration of
> fp->scopeChain (skipping with blocks).

I see; so even the block chains that escape are not complete. So the correct assertion would be, "scopeChain contains all blocks on blockChain that have needsClone". It seems pretty clear that pushBlock satisfies that condition by construction, and by the assertion it makes on entry. So I'm happy.
Comment 22 Luke Wagner [:luke] 2012-05-07 21:19:54 PDT
Created attachment 621873 [details] [diff] [review]
fix the debugger v.3

This patch is the same as v.2 except that DebugScopeCompartment (per-compartment) is renamed and hoisted to DebugScopes (per-runtime).  The important thing is that this avoids the problem where a WeakMap's compartment gets destroyed while the WeakMap is still in gcWeakMapList.   (I had a different approach to this that I was about to ask billm to review when I realized I could just do this.)
Comment 23 Luke Wagner [:luke] 2012-05-08 11:15:43 PDT
(Green on try)
Comment 24 Jim Blandy :jimb 2012-05-08 14:30:45 PDT
Comment on attachment 621235 [details] [diff] [review]
fix the debugger v.2

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

Luke says there's a new rev of this patch going up tout de suite, but here are some comments on the old one.

::: js/src/jscntxt.h
@@ +1318,5 @@
>  
> +#ifdef DEBUG
> +    /*
> +     * Controls whether a quadratic-complexity assertion is performed during
> +     * stack iteration, defaults to true.

Existing comment, I know, but that should be a semicolon, not a comma.

@@ +1324,5 @@
> +    bool stackIterAssertionEnabled;
> +
> +    /*
> +     * When set, it is ok to accessed non-aliased fields of ScopeObjects
> +     * because the accesses is coming from the DebugScopeProxy.

"when greater than zero"; "because the accesses are"

::: js/src/vm/ScopeObject.cpp
@@ +1650,5 @@
> +        return &obj;
> +    }
> +
> +    ScopeObject &scope = obj.asScope();
> +    if (!scope.isWith() && scope.maybeStackFrame())

Is this !scope.isWith() check there simply because we know that, if there's a 'with' environment, that the rest of the scope chain is fully reified, and we needn't bother creating a ScopeIter to walk it?

@@ +1656,5 @@
> +    return GetDebugScopeForScope(cx, scope, ScopeIter());
> +}
> +
> +static JSObject *
> +GetDebugScope(JSContext *cx, JSObject &obj, ScopeIter si)

As we talked about on IRC, the 'obj' parameter should go away, since 'si' has all the data we need.

@@ +1663,5 @@
> +
> +    if (si.done())
> +        return GetDebugScope(cx, obj);
> +
> +    switch (si.type())

On IRC, we agreed that this switch statement could be removed if ScopeIter had a method to return the current ScopeObject (usable only when hasScopeObject returns true).

@@ +1716,5 @@
> +js::GetDebugScopeForFunction(JSContext *cx, JSFunction *fun)
> +{
> +    assertSameCompartment(cx, fun);
> +    if (!EnsureDebugScopeCompartment(cx))
> +        return false;

should be "return NULL;", right?

@@ +1725,5 @@
> +js::GetDebugScopeForFrame(JSContext *cx, StackFrame *fp)
> +{
> +    assertSameCompartment(cx, fp);
> +    if (!EnsureDebugScopeCompartment(cx))
> +        return false;

Same here.

::: js/src/vm/ScopeObject.h
@@ +434,5 @@
> +    bool isForDeclarative() const;
> +};
> +
> +inline bool
> +IsDebugScope(const JSObject *obj)

Why do we need both this and JSObject::isDebugScope? Can't we put the inline definition of JSObject::isDebugScope here?
Comment 25 Luke Wagner [:luke] 2012-05-08 14:39:21 PDT
(In reply to Jim Blandy :jimb from comment #24)
> Is this !scope.isWith() check there simply because we know that, if there's
> a 'with' environment, that the rest of the scope chain is fully reified, and
> we needn't bother creating a ScopeIter to walk it?

Exactamundo!  This terrible hackiness will go away in a down patch (that is alllmost green) which removes all dependence on ScopeObject::maybeStackFrame (b/c that field is going away with bug 659577).

The rest sounds great.  New patch in a bit.
Comment 26 Luke Wagner [:luke] 2012-05-08 14:59:24 PDT
Created attachment 622158 [details] [diff] [review]
hoist DeclEnvObject creation logic, make callee enumerable

Almost forgot this tiny patch.  Note: this makes the function name property enumerable (just like all call/block properties) which is necessary for proxies to work (since JSITER_HIDDEN is broken).
Comment 27 Luke Wagner [:luke] 2012-05-08 15:00:30 PDT
Created attachment 622159 [details] [diff] [review]
fix the debugger v.4

This addresses previous comments (greatly simplifying things).  It also fixes some try bustage where GetDebugScope gets called outside debugMode.  See CanUseDebugScopeMaps and comment.
Comment 28 Luke Wagner [:luke] 2012-05-08 15:03:30 PDT
Created attachment 622161 [details] [diff] [review]
don't use ScopeObject::maybeStackFrame

This is a new patch, on top of "fix the debugger", to avoid using ScopeObject::maybeStackFrame since these are going away with bug 659577.
Comment 29 Jim Blandy :jimb 2012-05-08 15:07:49 PDT
One (pre-existing) issue here is that we don't have complete static information about the scopes surrounding functions --- only the nesting of functions themselves.

That is, if we have:

var g;
let (x = 1) {
  g = function g() { return 42; }
}
debugger;

and the onDebuggerStatement handler gets its hands on g, then there's no way for it to discover that g's scope chain ought to include a binding for x. Statically, the block chains only mention other blocks within the same immediately surrounding function; and the function tree only has functions. Dynamically, we won't create any real scope chain element for x's block.

It would be cool to have some static information, available from the script, that provided all this data. Then we could construct a proper ScopeIter for g's enclosing scope.
Comment 30 Luke Wagner [:luke] 2012-05-08 15:17:36 PDT
Agreed.  That will be a good quality-of-debugger followup once everything else in place.
Comment 31 Luke Wagner [:luke] 2012-05-09 00:02:34 PDT
Created attachment 622301 [details] [diff] [review]
fix the debugger v.5

The previous patches were mis-handling DeclEnvObject.  The root of the problem is that ScopeIter was treating DeclEnvObject as separate from CallObject but the two are usually handled together (created, popped, etc).  This patch removes NamedFunExprScope from ScopeIter and handles DeclEnvObject with CallObject in the debug scope path.
Comment 32 Panos Astithas [:past] 2012-05-09 01:31:10 PDT
Are some of these patches obsolete? I'm trying to fix bug 752770 and I'd like to apply its dependencies.
Comment 33 Luke Wagner [:luke] 2012-05-09 09:55:45 PDT
Created attachment 622402 [details] [diff] [review]
combined for Panos (applies on cset 7082192622e6)

Here is an all-in-one patch that applies to 7082192622e6.  Thanks!
Comment 34 Jim Blandy :jimb 2012-05-09 12:25:43 PDT
I'm curious, is there some subtle reason CallObject::createForFunction uses 'fp->fun()' in some places and 'fp->callee()' in others? Would they ever be different in a non-eval function frame?
Comment 35 Jim Blandy :jimb 2012-05-09 12:40:36 PDT
Comment on attachment 622158 [details] [diff] [review]
hoist DeclEnvObject creation logic, make callee enumerable

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

Looks good.

The only thing I'd say is that, given that lambdaName in CallObject::createForFunction is only used for the 'if' condition now, I wonder if we could just call js_IsNamedLambda in the condition instead.

It's kind of funny that DeclEnvObject has such a general-sounding name but is only used in one very specific case.
Comment 36 Jim Blandy :jimb 2012-05-09 12:44:07 PDT
Comment on attachment 621119 [details] [diff] [review]
create scopes eagerly or not at all (thereby breaking the debugger) v.2

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

::: js/src/vm/ScopeObject.h
@@ +281,5 @@
>      bool isAliased(unsigned i);
>  
> +    /*
> +     * A static block object is cloned (when entering the block) iff some
> +     * variable of the block isAlised.

This should be "isAliased".
Comment 37 Luke Wagner [:luke] 2012-05-09 12:46:24 PDT
(In reply to Jim Blandy :jimb from comment #34)
Brace yourself: they can be different.  Only callee is guaranteed to be the right function instance; fp->fun() can be a different function instance (with the same script).  This goes back to when JSFunction* was not an object so fp->callee().getFunctionPrivate() == fp->fun().  To wit: the only place that exercises this divergence is the mjit, where 'fun' is baked into the prologue.  (In the future we should remove fp->fun but, for the moment, it is rather difficult.)
Comment 38 Luke Wagner [:luke] 2012-05-09 16:00:58 PDT
Comment on attachment 622161 [details] [diff] [review]
don't use ScopeObject::maybeStackFrame

This patch is actually the first of several so I'll use a separate bug.
Comment 39 Luke Wagner [:luke] 2012-05-10 00:28:46 PDT
Created attachment 622645 [details] [diff] [review]
fix the debugger v.6

I'm sorry for the churn, but later-on patches keep suggesting minor changes to choices made in this patch.  I've reach to the top of the patch stack (bug 659577) so things should be more stable.

Green on try
Comment 40 Jim Blandy :jimb 2012-05-10 11:16:52 PDT
Churn is no problem; they keep getting more legible. :)
Comment 41 Jim Blandy :jimb 2012-05-11 10:49:25 PDT
Rats, it seems like my pending comments on v.5 got dropped, perhaps because it was marked obsolete?
Comment 42 Jim Blandy :jimb 2012-05-11 11:32:48 PDT
Comment on attachment 622645 [details] [diff] [review]
fix the debugger v.6

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

Some initial thoughts. (I'm going to publish more frequently, since Splinter ate my comments on v.5.)

::: js/src/vm/ScopeObject.cpp
@@ +1104,5 @@
> +ScopeIter::ScopeIter(StackFrame *fp, ScopeObject &scope)
> +  : fp_(fp),
> +    cur_(&scope)
> +{
> +    /*

I'd like this comment to start with something like:

"Find the appropriate static block for this iterator, given |scope|.

Since not all blocks enclosing a function are cloned, the correct static block may not have a clone on the chain starting at |scope|."

@@ +1106,5 @@
> +    cur_(&scope)
> +{
> +    /*
> +     * We know that 'scope' is a (non-optimized) scope on fp's scope chain. We
> +     * do not, however, know whether fp->maybeScopeChain() is. In cases like:

I read the second sentence as "We do not, however, know whether fp->maybeScopeChain() is a (non-optimized) scope on fp's scope chain." Um, if fp->maybeScopeChain() is non-NULL, it sure as heck better be on fp's scope chain, because it *is* fp's scope chain. Anyway, I'm confused.

@@ +1114,5 @@
> +     *     let (y = 1) g();
> +     *   }
> +     *
> +     * g will have x's block in its enclosing scope but not y's. However, at
> +     * the debugger statement, both y's and x's blocks will on fp->blockChain.

"will be on"

@@ +1116,5 @@
> +     *
> +     * g will have x's block in its enclosing scope but not y's. However, at
> +     * the debugger statement, both y's and x's blocks will on fp->blockChain.
> +     * Fortunately, we can compare scope object stack depths to determine the
> +     * joint point.

"join point"?

@@ +1124,5 @@
> +        while (block_) {
> +            if (block_->stackDepth() <= cur_->asNestedScope().stackDepth())
> +                break;
> +            block_ = block_->enclosingBlock();
> +        }

Can we assert here that, if cur_ is a block, we should have found it?

::: js/src/vm/ScopeObject.h
@@ +389,5 @@
>      ScopeObject &scope() const { JS_ASSERT(hasScopeObject()); return cur_->asScope(); }
>  
>      StaticBlockObject &staticBlock() const { JS_ASSERT(type() == Block); return *block_; }
> +
> +    /* For use as hash policy */

Yay, "duck typing" in C++!
Comment 43 Jim Blandy :jimb 2012-05-11 11:40:02 PDT
This patch needs Debugger tests for at least the following:

var g;
function f(x) {
  let (y = x) {
    if (x)
      g = function () { eval('debugger;'); }
    else
      g();
  }
}
g(true);
g(false); // and check that env has y, and it's bound to true

Another:

var g;
let (y = 1) {
  g = function () { debugger; }
  let (z = 2) {
    g(); // and check that the function's env includes y, but not z.
  }
}

Another:

var g;
for each (var x = [true, false]) {
  let (y = x) { // should not be considered aliased, right?
    if (x)
      g = function () { debugger; }
    else
      g(); // and check that y is unavailable or true
  }
}

In that one, if the debugger statement causes blocks to be marked aliased, then change it to g = function () { h(); } where h does a debugger;.
Comment 44 Luke Wagner [:luke] 2012-05-11 14:02:01 PDT
(In reply to Jim Blandy :jimb from comment #42)
> I read the second sentence as "We do not, however, know whether
> fp->maybeScopeChain() is a (non-optimized) scope on fp's scope chain." Um,
> if fp->maybeScopeChain() is non-NULL, it sure as heck better be on fp's
> scope chain, because it *is* fp's scope chain. Anyway, I'm confused.

Oops, that is confusing.  I meant "We do not, however, know whether fp->maybeScopeChain() encloses 'scope'."

> > +     * Fortunately, we can compare scope object stack depths to determine the
> > +     * joint point.
> 
> "join point"?

I'll change this to "... to determine the block (if any) that encloses 'scope'."

> Can we assert here that, if cur_ is a block, we should have found it?

Yep, will do.

(In reply to Jim Blandy :jimb from comment #43)
The last doesn't quite work because, if 'y' isn't aliased, it won't show up on g's scope chain (the first enclosing aliased scope will).  However, I could test the interesting case by adding a new enclosing scope that *is* aliased but keep y unaliased.
Comment 45 Jim Blandy :jimb 2012-05-11 14:26:33 PDT
Comment on attachment 622645 [details] [diff] [review]
fix the debugger v.6

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

Some further thoughts.

::: js/src/vm/ScopeObject.cpp
@@ +1552,5 @@
> +    if (!enclosingDebug)
> +        return NULL;
> +
> +    JSObject &maybeDecl = scope.enclosingScope();
> +    if (maybeDecl.isDeclEnv()) {

It's strange to special-case this here and below. Suggestion below.

@@ +1581,5 @@
> +    if (!enclosingDebug)
> +        return NULL;
> +
> +    /*
> +     * Create the missing the missing scope object. This takes care of storing

"the missing the missing"

@@ +1582,5 @@
> +        return NULL;
> +
> +    /*
> +     * Create the missing the missing scope object. This takes care of storing
> +     * bindings after the StackFrame has been popped. To preserve scopeChain

This is for querying the Environment object after the frame for which it was created is gone? Could we stay "storing variable values" instead of "storing bindings"? Because, at least as I've learned the terminology, even a static block stores 'bindings'; but only a cloned block stores values.

@@ +1595,5 @@
> +            return NULL;
> +
> +        JSObject &maybeDecl = callobj->enclosingScope();
> +        if (maybeDecl.isDeclEnv()) {
> +            JS_ASSERT(CallObjectLambdaName(callobj->getCalleeFunction()));

It seems like, rather than special-casing the DeclEnv here and in GetDebugScopeForScope, it would be simpler just to add a NamedFunExprScope case here. That would remove the assumption (made here, if I'm reading correctly) that, if the reified scope chain lacks a DeclEnv, then the debugger-view scope chain doesn't need one either.

@@ +1652,5 @@
> +#endif
> +        return &obj;
> +    }
> +
> +    ScopeObject &scope = obj.asScope();

Suggested comment: "If |scope| is a 'with' block, then the chain is fully reified from that point outwards, and there's no point in bothering with a ScopeIter. If |scope| has an associated stack frame, we can get more detailed scope chain information from that."
Comment 46 Luke Wagner [:luke] 2012-05-11 14:43:40 PDT
(In reply to Jim Blandy :jimb from comment #45)
> Suggested comment: "If |scope| is a 'with' block, then the chain is fully
> reified from that point outwards, and there's no point in bothering with a
> ScopeIter. If |scope| has an associated stack frame, we can get more
> detailed scope chain information from that."

I'll can add the comment, but the reason I didn't put it in initially is that the isWith special case is about to get ripped out anyway with the patches (in bug 659577) to remove use of ScopeObject::maybeStackFrame altogether.
Comment 47 Luke Wagner [:luke] 2012-05-11 14:57:28 PDT
Created attachment 623317 [details] [diff] [review]
fix the debugger v.7

With comments address.

The DeclEnvObject weirdness we decided to leave in since it follows how CallObject:;createForFunction creates DeclEnvObject.  A good followup simplification will be to change createForFunction not to do this (have the prologue call DeclEnvObject::create manually) and then add back the NamedFunExprScope case to ScopeIter.
Comment 48 Jim Blandy :jimb 2012-05-11 15:44:08 PDT
Well, heck. I was really hoping to get this done before end of business today. I'll try to finish it off tonight.
Comment 49 Jim Blandy :jimb 2012-05-15 17:14:31 PDT
Comment on attachment 623317 [details] [diff] [review]
fix the debugger v.7

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

::: js/jsd/test/test_jsval_retval.js
@@ +37,5 @@
>  
>      f();
> +
> +    jsd.breakpointHook = null;
> +    jsd = null;

What is this for?

::: js/src/frontend/Parser.cpp
@@ +4119,5 @@
>          pn = new_<DebuggerStatement>(tokenStream.currentToken().pos);
>          if (!pn)
>              return NULL;
>          tc->flags |= TCF_FUN_HEAVYWEIGHT;
> +        tc->noteBindingsAccessedDynamically();

This ensures that all bindings visible from a 'debugger' statement will behave as the source would suggest, right?

::: js/src/jit-test/tests/debug/Environment-getVariable-12.js
@@ +1,2 @@
> +
> +// The value returned by getVariable can be a Debugger.Object.

Isn't this comment a copy-paste vestige?

::: js/src/jit-test/tests/jaeger/bug563000/eif-call-newvar.js
@@ +5,5 @@
> +  var caught = false;
> +  try {
> +    evalInFrame(1, "var y = 'success'");
> +  } catch (e) {
> +    assertEq(String.prototype.indexOf.call(e, "TypeError"), 0);

Could we have a comment here explaining why that evalInFrame is now expected to throw?

::: js/src/jsapi.cpp
@@ +859,5 @@
>  
>      if (!scriptFilenameTable.init())
>          return false;
>  
> +    debugScopes = this->new_<DebugScopes>(this);

If we're going to allocate this unconditionally, couldn't we just make the DebugScopes a direct member of JSRuntime, instead of having a pointer to it?

::: js/src/jscompartment.cpp
@@ +628,3 @@
>          updateForDebugMode(cx->runtime->defaultFreeOp());
> +        if (!enabledAfter)
> +            cx->runtime->debugScopes->onCompartmentLeaveDebugMode(this);

JSCompartment::removeDebuggee can also call updateForDebugMode upon leaving debug mode; should we call onCompartmentLeaveDebugMode from there as well?

::: js/src/jsdbgapi.cpp
@@ +577,5 @@
> +    while (o) {
> +        if (o->isCall() || (o->isDebugScope() && o->asDebugScope().scope().isCall()))
> +            return o;
> +        o = o->enclosingScope();
> +    }

If this function is applied to the frame for a lightweight call, what guarantees that this loop will never walk out too far and return a Call object for some enclosing call is that GetDebugScopeForFrame always synthesizes a Call(-like) object. Right? 

It might be nice to assert that the returned Call(-like)'s frame is fp.

@@ +831,5 @@
>  JS_GetPropertyDescArray(JSContext *cx, JSObject *obj, JSPropertyDescArray *pda)
>  {
>      assertSameCompartment(cx, obj);
>  
> +    if (obj->isDebugScope()) {

It's a pity we need to replicate essentially the entire loop. But I don't think it's worth carefully architecting JSD support. Hopefully it'll be gone in a year. :)

@@ +846,5 @@
> +                return false;
> +            pd[i].id = IdToValue(props[i]);
> +
> +            if (!js_AddRoot(cx, &pd[i].value, NULL))
> +                return false;

The code below (at bad:) suggests that one needs to release all these roots when an error occurs.

::: js/src/jsinterpinlines.h
@@ +431,5 @@
>          return false;
>  
>      /* Steps 8c, 8d. */
>      if (!prop || (obj2 != varobj && varobj->isGlobal())) {
> +        if (!varobj->defineProperty(cx, dn, UndefinedValue(), JS_PropertyStub,

This change is to make it work when varobj is a DebugScopeObject; when varobj isn't, the assertion above ensures that the call to JSObject::defineProperty will end up at DefineNativeProperty anyway. Correct?

::: js/src/jsobjinlines.h
@@ +742,5 @@
>  {
>      return setFlag(cx, js::BaseShape::DELEGATE, GENERATE_SHAPE);
>  }
>  
> +inline bool JSObject::isVarObj()

This can't take a const this any more, because of asDebugScope?

::: js/src/jsscope.h
@@ +886,5 @@
>  
> +    inline Shape *search(JSContext *cx, jsid id) {
> +        Shape **_;
> +        return search(cx, this, id, &_);
> +    }

I can't find where this gets used.

::: js/src/jsscript.cpp
@@ +1345,5 @@
>          script->isGenerator = true;
> +#ifdef JS_METHODJIT
> +    if (cx->compartment->debugMode())
> +        script->debugMode = true;
> +#endif

Wow, did this only become necessary with this patch? Why? (Or was it an extant bug?)

::: js/src/vm/Debugger.cpp
@@ +2594,5 @@
>          if (!SN_IS_TERMINATOR(sn))
>              snpc += SN_DELTA(sn);
>          updateLine();
> +        while (frontPC() != script->main())
> +            popFront();

This looks like part of separate bug; does it have tests?

::: js/src/vm/ScopeObject.cpp
@@ +1107,5 @@
> +{
> +    /*
> +     * Find the appropriate static block for this iterator, given 'scope'. We
> +     * know that 'scope' is a (non-optimized) scope on fp's scope chain. We do
> +     * not, however, know whether fp->maybeScopeChain() encloses 'scope. E.g.:

'scope'

@@ +1187,5 @@
>       * scope chain. As a final twist: even if cur_ points into an enclosing
>       * frame's scope chain, the current frame may still have uncloned blocks.
> +     *
> +     * Note: DebugScopeObject falls nicely into this plan since they are only
> +     * ever introduced as the *enclosing* scope of a frame, they should never

s/frame,/frame;/, I think. Or s/into this plan/into this plan:/.
Comment 50 Luke Wagner [:luke] 2012-05-15 17:58:32 PDT
(In reply to Jim Blandy :jimb from comment #49)
Thanks a lot!

> What is this for?

It prevents CC leak involving jsd that showed up on try server.

> > +        tc->noteBindingsAccessedDynamically();
> 
> This ensures that all bindings visible from a 'debugger' statement will
> behave as the source would suggest, right?

Correct: as if there was an eval() at any statement.

> > +    debugScopes = this->new_<DebugScopes>(this);
> 
> If we're going to allocate this unconditionally, couldn't we just make the
> DebugScopes a direct member of JSRuntime, instead of having a pointer to it?

Oh I tried; file-based inclusion + needing complete types for member variables FTL.

> JSCompartment::removeDebuggee can also call updateForDebugMode upon leaving
> debug mode; should we call onCompartmentLeaveDebugMode from there as well?

Good find; done.

> what guarantees that this loop will never walk out too far and return a Call
> object for some enclosing call is that GetDebugScopeForFrame always
> synthesizes a Call(-like) object. Right? 

That's right.

> This change is to make it work when varobj is a DebugScopeObject; when
> varobj isn't, the assertion above ensures that the call to
> JSObject::defineProperty will end up at DefineNativeProperty anyway. Correct?

Correct

> > +inline bool JSObject::isVarObj()
> 
> This can't take a const this any more, because of asDebugScope?

That's right.

> Wow, did this only become necessary with this patch? Why? (Or was it an
> extant bug?)

I do think it is an extant bug but I don't think it mattered until this patch.

> ::: js/src/vm/Debugger.cpp
> @@ +2594,5 @@
> >          if (!SN_IS_TERMINATOR(sn))
> >              snpc += SN_DELTA(sn);
> >          updateLine();
> > +        while (frontPC() != script->main())
> > +            popFront();
> 
> This looks like part of separate bug; does it have tests?

It does and it is exposed by this bug: basically, because of script->debugMode being correctly set (as you noticed above), more scripts get script->argumentsHasLocalBindings which generates more 'arguments;setlocal;pop' prologues which caused failures in debugger jit tests of DebuggerScript_getAllOffsets et al.
Comment 51 Jim Blandy :jimb 2012-05-15 20:53:00 PDT
Are there tests that actually check for the new scopes that become visible because of this patch? We really should have some, if not.
Comment 52 Luke Wagner [:luke] 2012-05-18 12:19:26 PDT
(In reply to Jim Blandy :jimb from comment #51)
Not yet, but we should.  Before we pin down more what is and isn't cloned, it'd be nice to fix bug 753145 (and the as-yet unfiled bug of souping up DebugScopeProxy to use this information).

https://hg.mozilla.org/integration/mozilla-inbound/rev/f45eec2bd4c7
Comment 53 Ryan VanderMeulen [:RyanVM] 2012-05-18 18:41:16 PDT
https://hg.mozilla.org/mozilla-central/rev/f45eec2bd4c7

Note You need to log in before you can comment on or make changes to this bug.