963879 - [jsdbg2] Reflect optimized out scopes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Optimized out slots may arise either from JIT optimization or frontend optimization. Unaliased variables do not show up in scopes for performance reasons, even though the Debugger might ask for it.

Currently Debugger doesn't reflect optimized out slots or scopes. It is able to synthesize missing scopes for live frames, but simply skips optimized away scopes for non-live frames.

For instance, consider:

>  var g = newGlobal();
>  var dbg = new Debugger;
>  dbg.addDebuggee(g);
>
>  g.eval("" + function f() {
>    var x = 42;
>    function g() { }
>    g();
>  });
>
>  dbg.onEnterFrame = function (f) {
>    if (f.callee && (f.callee.displayName === "g"))
>    {
>      print(f.callee.displayName + ".env.parent[x] = " +
>            uneval(f.environment.parent.getVariable("x")));
>    }
>  }
>
>  g.eval("f();");

This ends up printing "undefined", because |f.environment.parent| is actually a Debugger reflection of the global scope. (Subtle point: D.E.prototype.parent *cannot* commute with D.F.p.environment, even if the frames are live! If a function isn't heavyweight, we simply don't know if it's correct to synthesized the optimized out scope from a live frame of the same function.)

It would be nice to have Debugger.Environment be closer to what ES considers a "lexical environment", even if we can't reflect the actual values.

This bug proposes:
  - Reflect all scopes.
  - Add a .mutable property to D.E.p, which is true iff the reflected scope has ever been live and synthesized (which means it'll have been synthesized or has a snapshot) or if the engine has a dynamic counterpart for it (heavyweight fun or needs-clone block).
  - setVariable throws on D.E instances for whom .mutable is false.
  - getVariable returns |{ optimizedOut: true }| for optimized out variables.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

Note that the JIT parts won't be exercisable until bug 716647 lands.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8365447 [details] [diff] [review]
Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

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

Oops, meant to r? jorendorff since it touches public API.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Part 2: Consider optimized out scopes when getting debug scopes.

Hopefully straightforward. One thing I'm not entirely sure about is whether
it's a good idea to terminate the reflected optimized out call and cloned block
objects' scope chains immediately with the global.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Part 3: Add Debugger.Environment.prototype.mutable.

Comment 5

[Shu-yu Guo [:shu]]

Tests forthcoming.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8365449 [details] [diff] [review]
Part 2: Consider optimized out scopes when getting debug scopes.

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

This version is buggy.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Part 2: Consider optimized out scopes when getting debug scopes. v2

Fix bad recursion logic and off-by-1 in the SSI for GetDebugScopeForFunction.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Part 4: Tests.

Comment 9

[Luke Wagner [:luke]]

Sorry for the delay (DOM work week).  Great job identifying this bug.  Reading comment 0 I was shocked it didn't work this way already but then I remembered that ScopeIter (and all the debug scope stuff) predates the ability to iterate the inter-function static scope chain.  Instead of passing StaticScopeIter in parallel to ScopeIter, what if ScopeIter used StaticScopeIter internally?  That is, make ScopeIter *just work* (returning !hasScopeObject() for entire functions that aren't on the dynamic scope chain).  I also feel like we could use StaticScopeIter to drastically simplify ScopeIter::settle.  Atm, WithObjects are a sore point, since they are only present on the dynamic scope chain, but bug 966912 should make it better.  The only other corner case is direct eval where the static scope chain stops.

Comment 10

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #9)
> Sorry for the delay (DOM work week).  Great job identifying this bug. 
> Reading comment 0 I was shocked it didn't work this way already but then I
> remembered that ScopeIter (and all the debug scope stuff) predates the
> ability to iterate the inter-function static scope chain.  Instead of
> passing StaticScopeIter in parallel to ScopeIter, what if ScopeIter used
> StaticScopeIter internally?  That is, make ScopeIter *just work* (returning
> !hasScopeObject() for entire functions that aren't on the dynamic scope
> chain).  I also feel like we could use StaticScopeIter to drastically
> simplify ScopeIter::settle.  Atm, WithObjects are a sore point, since they
> are only present on the dynamic scope chain, but bug 966912 should make it
> better.  The only other corner case is direct eval where the static scope
> chain stops.

The refactoring and cleanup proposed sound solid, but I won't have time to work on it immediately. Perhaps during the devtools work week?

Jim, min waiting until then to land the whole shebang?

Comment 11

[Jim Blandy :jimb]

Sounds great to me. I'm excited to see this going in. (I especially appreciate the expanded comments in ScopeObject.cpp.)

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8365459 [details] [diff] [review]
Part 2: Consider optimized out scopes when getting debug scopes. v2

Great, thanks Shu!  Clearing review for now, then.

Comment 13

[Shu-yu Guo [:shu]]

Refactoring ScopeIter to use StaticScopeIter is harder than I originally thought, even with the StaticWithObject patch.

Luke, some questions for you:

1) The idea, AIUI, is to have ScopeIter iterate through all static scopes, per StaticScopeIter logic. This is thorny because optimized-out function scopes don't have a notion of a StaticCallObject, just the shape that the call object would have taken. This assumption is baked into a few places, like js::UnwindScope. We can also add a StaticCallObject <: NestedScopeObject, though. This is also a possible perf/memory hit though, but I doubt it'll affect anything. Feedback on having a StaticCallObject?

2) ScopeIter currently only iterates a single frame's scopes. The refactor would change its semantics such that it goes beyond the frame's own scopes. Is the iterating-a-single-frame functionality actually needed anywhere? I couldn't find any such uses in the codebase.

Comment 14

[Luke Wagner [:luke]]

(In reply to Shu-yu Guo [:shu] from comment #13)
> 1) The idea, AIUI, is to have ScopeIter iterate through all static scopes,
> per StaticScopeIter logic. This is thorny because optimized-out function
> scopes don't have a notion of a StaticCallObject, just the shape that the
> call object would have taken.

In place of a StaticCallObject, we use the compiler-created JSFunction, from which one may then access fun->script->enclosingStaticScope and fun->script->bindings.  This is abstracted by StaticScopeIter::operator++.  Perhaps I'm missing your point, though?  (Ideally we'd regularize all this stuff; I'd like to get away from making all these static scopes be objects; seems like we could do them all like bindings.)

> 2) ScopeIter currently only iterates a single frame's scopes. The refactor
> would change its semantics such that it goes beyond the frame's own scopes.

Good point.  I think you're right: practically all uses of ScopeIter do not iterate until the ScopeIter is .done(), they iterate up until some other end condition (or don't iterate at all).  The one place that does seem to test .done() is GetDebugScope() which is what you're trying to fix anyhow :)

Comment 15

[Jim Blandy :jimb]

Comment on attachment 8365450 [details] [diff] [review]
Part 3: Add Debugger.Environment.prototype.mutable.

Cancelled per IRC discussion; patch is being revised.

Comment 16

[Jim Blandy :jimb]

Comment on attachment 8365461 [details] [diff] [review]
Part 4: Tests.

Cancelled per IRC discussion; patch is being revised.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365447 [details] [diff] [review]
Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

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

Landed in bug 716647.

Comment 18

[Shu-yu Guo [:shu]]

Attachment: Part 1: Overhaul ScopeIter and StaticScopeIter to share iteraiton logic and to go through direct evals.

Sorry, had trouble splitting it up. Let me know if it's too jumbled.

Comment 19

[Shu-yu Guo [:shu]]

Attachment: Part 1: Overhaul ScopeIter and StaticScopeIter to share iteration logic and to go through direct evals.

Rebased against the JS_ASSERT->MOZ_ASSERT change.

Comment 20

[Shu-yu Guo [:shu]]

Attachment: Part 2: Synthesize completely optimized out scopes.

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Part 3: Add Debugger.Environment.prototype.optimizedOut.

Comment 22

[Shu-yu Guo [:shu]]

Attachment: Part 4: Tests.

Comment 23

[Luke Wagner [:luke]]

Comment on attachment 8499264 [details] [diff] [review]
Part 1: Overhaul ScopeIter and StaticScopeIter to share iteration logic and to go through direct evals.

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

Man, this is such a fantastic simplification; great work!

It looks like you also fixed direct eval case too so static block chains are always "complete"; is that right?  Probably not a priority but, to wit, with this info, we could probably use this to fully optimize free variables inside direct eval :)

I got the feeling that a lot of the special cases for indirect eval could be removed if you also created static scope objects for indirect eval.  (You could reuse the same object class, just add a separate 'bool indirect' field.)

Mostly nits and a few smaller requests below.  I'd appreciate giving the revised patch one more pass, though.

::: js/src/builtin/Eval.cpp
@@ +355,4 @@
>          if (!compiled)
>              return false;
>  
> +        if (evalType == DIRECT_EVAL && compiled->strict() && staticScope)

Is the last conjunct necessary?  It would seem to be implied by DIRECT_EVAL.

::: js/src/jsscript.h
@@ +1582,5 @@
> +    JSObject *innermostStaticScope(jsbytecode *pc);
> +
> +    // Returns either the innermost static scope at pc, if there is one, or
> +    // the enclosing static scope.
> +    JSObject *nearestStaticScope(jsbytecode *pc);

'nearest' sounds pretty fuzzy; I think this function is still returning the inntermostStaticScope, just with different handling of the "not in this script" case.  How about:
  s/innermostStaticScope/innermostStaticScopeInScript/
  s/nearestStaticScope/innermostStaticScope/
?

::: js/src/vm/ScopeObject.cpp
@@ +1172,5 @@
> +
> +    if (hasScopeObject()) {
> +        scope_ = &scope_->as<ScopeObject>().enclosingScope();
> +        if (scope_->is<DeclEnvObject>())
> +            scope_ = &scope_->as<DeclEnvObject>().enclosingScope();

I guess we've gotten along so far without named lambda scopes in ScopeIter; I see, we special-case this in GetDebugScopeForScope.  But maybe it'd be cleaner to just exposed these via ScopeIter?)

@@ +1177,3 @@
>      }
> +
> +    // Strict indirect evals create still create an extra CallObject, which is

"create still create"

@@ +1218,5 @@
> +JSObject *
> +ScopeIter::staticScope() const
> +{
> +    if (ssi_.done())
> +        return nullptr;

Since this method can return null, can you name it maybeStaticScope?

@@ +1935,5 @@
>               *
>               * Thus, we must explicitly remove the entries from both liveScopes
>               * and missingScopes here.
>               */
> +            // TODOshu see if can remove: liveScopes.remove(&(*debugScope)->scope());

I expect 'yes', but need to think about this.

@@ -2474,5 @@
> -        ScopeIter si(*maybeLiveScope, cx);
> -        return GetDebugScope(cx, si);
> -    }
> -    ScopeIter si(scope->enclosingScope(), cx);
> -    return GetDebugScopeForScope(cx, scope, si);

Now that this corner case is dealt with more cleanly, could you change GetDebugScopeForScope to just take a single ScopeIter arg (un-incremented).  Maybe you're you're about to refactor all this in a later patch in which case n/m.

::: js/src/vm/ScopeObject.h
@@ +448,5 @@
>      }
>  };
>  
> +// Static direct eval scope template objects on the static scope. Created at the
> +// time of compiling the eval script, and set as its static enclosing script.

In "as its static enclosing script." do you mean 'scope' instead of 'script'?

@@ +681,5 @@
>  
>  /*****************************************************************************/
>  
> +//
> +// A scope iterator describes the active scopes starting from a dynamic scope,

Could you remove the leading/trailing "//\n"?

@@ +684,5 @@
> +//
> +// A scope iterator describes the active scopes starting from a dynamic scope,
> +// static scope pair. This pair may be derived from the current point of
> +// execution in a frame. If derived in such a fashion, the ScopeIter tracks
> +// whether the current scope is within the extent of this initial frame.

Could you expand on this a bit more (probably in a separate comment paragraph)?  E.g., my initial question upon reading this was, if I have a scope chain A -> B -> C -> D and both A and C's frames are on the stack, will withinFrame be true for both A and C or only A?  I think the answer is: only for A (and only if constructed with a frame).  How about renaming withinFrame/frame to withinOriginalFrame/originalFrame?

@@ +691,5 @@
> +//
> +// By design, ScopeIter exposes *all* scopes, in the same iteration order as
> +// StaticScopeIter, even those that have been optimized away (i.e., no
> +// ScopeObject was created when entering the scope and thus there is no
> +// ScopeObject on fp->scopeChain representing the scope).

This second paragraph is the expected/correct behavior, so I'd leave it out :)

@@ +697,3 @@
>  class ScopeIter
>  {
> +    StaticScopeIter<NoGC> ssi_;

How do you know that ScopeIter can be used in a NoGC context?  It seems like a GC can occur in the scope (hence the RootedObject).

@@ +777,5 @@
>          k = newKey;
>      }
>  };
>  
> +// The value in LievScopeMap, mapped from by live scope objects.

Liev

@@ +1010,5 @@
>  
> +inline bool
> +ScopeIter::onIndirectStrictEvalScope() const
> +{
> +    return ssi_.done() && scope_->is<CallObject>() && scope_->as<CallObject>().isForEval();

It seems like scope_->as<CallObject>().isForEval() would have to be true (otherwise we'd not be ssi_.done()).  If you agree, can you move the last conjunct to an assert?

@@ +1016,5 @@
> +
> +inline bool
> +ScopeIter::done() const
> +{
> +    return !onIndirectStrictEvalScope() && ssi_.done();

I'd switch the order since the first conjunct makes more sense as a refinement of the first.

Comment 24

[Shu-yu Guo [:shu]]

Attachment: Part 1: Overhaul ScopeIter and StaticScopeIter to share iteration logic and to go through evals.

v2.

Applied comments. Also made StaticScopeIter go through all eval scopes,
including indirect evals. Includes some driveby fixes in the frontend since now
global eval scripts have an enclosing static scope.

Comment 25

[Shu-yu Guo [:shu]]

Attachment: Part 2: Synthesize completely optimized out scopes.

Rebased against new part 1.

Comment 26

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #23)
> Comment on attachment 8499264 [details] [diff] [review]
> Part 1: Overhaul ScopeIter and StaticScopeIter to share iteration logic and
> to go through direct evals.
> 
> Review of attachment 8499264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +1935,5 @@
> >               *
> >               * Thus, we must explicitly remove the entries from both liveScopes
> >               * and missingScopes here.
> >               */
> > +            // TODOshu see if can remove: liveScopes.remove(&(*debugScope)->scope());
> 
> I expect 'yes', but need to think about this.

I found out the answer to this. It is "no". Things were crashing, so this line is still needed to maintain liveScopes invariants somehow; I'll just trust in the comment above it.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 8500796 [details] [diff] [review]
Part 1: Overhaul ScopeIter and StaticScopeIter to share iteration logic and to go through evals.

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

::: js/src/jsapi.cpp
@@ +3922,5 @@
> +
> +    // If a function was compiled to be lexically nested inside some other
> +    // script, we cannot clone it without breaking the compiler's assumptions.
> +    JSObject *scope = fun->nonLazyScript()->enclosingStaticScope();
> +    if (scope && (!scope->is<StaticEvalObject>() || scope->as<StaticEvalObject>().isDirect()))

What about strict indirect eval (which has its own dynamic scope); I would assume that is like the function case.  Either way, I'd add a comment explaining why the exception for indirect (non-strict) eval.

Btw, are there test cases that fail without this exception?  If not, I'd leave it out.

::: js/src/jsscript.cpp
@@ +3141,4 @@
>      RootedObject scope(cx, script->enclosingStaticScope());
> +    if (script->compartment() != cx->compartment() && scope) {
> +        MOZ_ASSERT(!scope->as<StaticEvalObject>().isDirect());
> +        scope = StaticEvalObject::create(cx, NullPtr());

Don't forget to update this bit according to the previous question.

::: js/src/vm/Debugger.cpp
@@ +4870,5 @@
> +    if (frame)
> +        enclosingScope = frame.script()->innermostStaticScope(pc);
> +    Rooted<StaticEvalObject *> staticEvalScope(cx, StaticEvalObject::create(cx, enclosingScope));
> +    if (!staticEvalScope)
> +        return nullptr;

IIUC, you can remove this hunk now.

::: js/src/vm/Interpreter.cpp
@@ +887,4 @@
>          return;
>  
> +    RootedObject staticScope(cx, si.initialFrame().script()->innermostStaticScopeInScript(pc));
> +    for (; si.maybeStaticScope() != staticScope; ++si)

I'm a bit concerned by this loop, particularly the case where innermostStaticScopeInScript(pc) is null while innermostStaticScope(pc) is non-null, since it seems like, in that case, we'd iloop.  It seems like you could hit this with a direct eval?  I assume tests cover this, but I don't know why it works.

Anyhow, that got me thinking that we really don't need innermostStaticScopeInScript; that innermostStaticScope is fine here and the two other uses.  Can we just remove innermostStaticScopeInScript?

::: js/src/vm/ScopeObject.cpp
@@ +1123,4 @@
>  {
> +    // For named lambdas, DeclEnvObject scopes are always attached to their
> +    // CallObjects. Skip it here, as it is not entirely sensical to query
> +    // hasScopeObject(), since it is not a freestanding scope.

I'm not sure this comment is correct.  It *would* make sense if we represented named-lambda scopes as blocks (and when we do, they'll magically start showing up).  The only reason we're allowed to hide them is b/c all consumers of ScopeIter special-case named lambdas and handle them specially.

@@ +1135,5 @@
> +    // would have already left the frame.
> +    //
> +    // Also check for trying to iterate a heavyweight function frame before
> +    // the prologue has created the CallObject, in which case we have the
> +    // skip.

Can you pull this comment and the associated code up above?  It seems logically separate.

@@ +1943,5 @@
>               * Thus, we must explicitly remove the entries from both liveScopes
>               * and missingScopes here.
>               */
>              liveScopes.remove(&(*debugScope)->scope());
>              e.removeFront();

Oops, when I said "yes" in my initial review, I was answering "yes" to the question in my head "is this needed?", not to the "can I remove this?" question in your comment.  Sorry.

::: js/src/vm/ScopeObject.h
@@ +683,5 @@
> +// frame is tracked. That is, suppose there exists a dynamic scope chain of
> +// A -> B -> C, and both A and C have frames on the stack. If A's frame is
> +// provided as the initial frame, the ScopeIter will not consider C to be
> +// withinInitialFrame. For tracking of all live scopes, DebugScopes keep its
> +// own map of live scopes.

With the new description in the first paragraph, I'm not sure the second paragraph is necessary; the first makes it pretty clear.

Comment 28

[Shu-yu Guo [:shu]]

Attachment: Part 1: Overhaul ScopeIter and StaticScopeIter to share iteration logic and to go through evals. (r=luke)

Rebased for Jim. Carrying r=luke.

Comment 29

[Shu-yu Guo [:shu]]

Attachment: Part 2: Synthesize completely optimized out scopes.

Rebased.

Comment 30

[Shu-yu Guo [:shu]]

Jim, parts 3 and 4 do not need rebasing.

Comment 31

[Shu-yu Guo [:shu]]

Review ping.

Comment 32

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #31)
> Review ping.

Thanks. I'll be back from PTO on Jan 2, and it's my oldest review, so it's a priority for me.

Comment 33

[Jim Blandy :jimb]

Clearing needinfo, since it's in my review queue anyway. I'm not forgetting about it.

Comment 34

[Jim Blandy :jimb]

Comment on attachment 8499266 [details] [diff] [review]
Part 3: Add Debugger.Environment.prototype.optimizedOut.

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

Thanks for documenting!

Comment 35

[Jim Blandy :jimb]

Comment on attachment 8499267 [details] [diff] [review]
Part 4: Tests.

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

Could we have tests that D.E.p.inspectable, .type, .parent, and .callee behave reasonably on optimized-out environments?

Can an eval frame ever have an optimized-out environment?

Does D.E.p.find behave properly on optimized-out environments? It seems like that's the only thing that applies JSObject::lookupGeneric to the Envs.

::: js/src/jit-test/tests/debug/Environment-getVariable-14.js
@@ +10,5 @@
> +  g();
> +});
> +
> +dbg.onEnterFrame = function (f) {
> +  if (f.callee && (f.callee.displayName === "g"))

Aesthetics: we could use .name here, instead of .displayName.

@@ +14,5 @@
> +  if (f.callee && (f.callee.displayName === "g"))
> +    assertEq(f.environment.parent.getVariable("x").optimizedOut, true);
> +}
> +
> +g.eval("f();");

Just for aesthetics: this could be g.f(), here and in the other tests.

::: js/src/jit-test/tests/debug/Environment-names-03.js
@@ +12,5 @@
> +
> +dbg.onEnterFrame = function (f) {
> +  if (f.callee && (f.callee.displayName === "g")) {
> +    var names = f.environment.parent.names();
> +    assertEq(names.indexOf("x") !== -1, true);

It should have "g", too, and a length of exactly 2, shouldn't it? Certainly we'd want to know if it didn't.

Comment 36

[Jim Blandy :jimb]

Comment on attachment 8499267 [details] [diff] [review]
Part 4: Tests.

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

Blocks can be .optimizedOut too, right? We should have some tests for that.

Comment 37

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #35)
> Comment on attachment 8499267 [details] [diff] [review]
> Part 4: Tests.
> 
> Review of attachment 8499267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could we have tests that D.E.p.inspectable, .type, .parent, and .callee
> behave reasonably on optimized-out environments?
> 
> Can an eval frame ever have an optimized-out environment?

I don't see why not. Accessing bindings on the scope would result in an error being thrown.

> 
> Does D.E.p.find behave properly on optimized-out environments? It seems like
> that's the only thing that applies JSObject::lookupGeneric to the Envs.
> 

It should, yeah. That goes through eventually to DebugScopeProxy::has, which will pull out the "hollow" scopes and check their properties. The hollow scopes have all the bindings, except that all the bindings have the value JS_OPTIMIZED_OUT.

Comment 38

[Jim Blandy :jimb]

Comment on attachment 8524949 [details] [diff] [review]
Part 2: Synthesize completely optimized out scopes.

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

::: js/src/js.msg
@@ +381,5 @@
>  MSG_DEF(JSMSG_NOT_TRACKING_ALLOCATIONS, 1, JSEXN_ERR, "Cannot call {0} without setting trackingAllocationSites to true")
>  MSG_DEF(JSMSG_OBJECT_METADATA_CALLBACK_ALREADY_SET, 0, JSEXN_ERR, "Cannot track object allocation, because other tools are already doing so")
>  MSG_DEF(JSMSG_QUERY_INNERMOST_WITHOUT_LINE_URL, 0, JSEXN_TYPEERR, "findScripts query object with 'innermost' property must have 'line' and either 'displayURL', 'url', or 'source'")
>  MSG_DEF(JSMSG_QUERY_LINE_WITHOUT_URL, 0, JSEXN_TYPEERR, "findScripts query object has 'line' property, but no 'displayURL', 'url', or 'source' property")
> +MSG_DEF(JSMSG_DEBUG_CANT_SET_OPT_ENV, 1, JSEXN_REFERENCEERR, "can't set `{0}' in an optimized out environment")

"optimized-out", I think.

::: js/src/vm/ScopeObject.cpp
@@ +285,5 @@
> +    MOZ_ASSERT(!callee->isHeavyweight());
> +
> +    // This is not a real scope -- even less real than missing scopes
> +    // synthesized from live frames. As such, we terminate their scope chain
> +    // at the global immediately.

This comment is not very helpful: all the interesting information is hidden behind the phrase "not a real scope". Let's change this comment to:

This scope's parent link is never used: the DebugScopeObject that refers to this scope carries its own parent link, which is what Debugger uses to construct the tree of Debugger.Environment objects. So just parent this scope directly to the global.

@@ +739,5 @@
> +    MOZ_ASSERT(!block->needsClone());
> +
> +    // This is not a real scope -- even less real than missing scopes
> +    // synthesized from live frames. As such, we terminate their scope chain
> +    // at the global immediately.

Same comment here.

@@ +2168,5 @@
>      MOZ_ASSERT(!si.hasScopeObject());
>      MOZ_ASSERT(cx->compartment() == debugScope.compartment());
>      MOZ_ASSERT_IF(si.withinInitialFrame() && si.initialFrame().isFunctionFrame(),
>                    !si.initialFrame().callee()->isGenerator());
> +    MOZ_ASSERT_IF(si.type() == ScopeIter::Call, !si.fun().isGenerator());

I think this could use the

 // Generators should always reify their scopes.

comment as well.

::: js/src/vm/ScopeObject.h
@@ +771,5 @@
> +//
> +// This necessarily means that completely optimized out scopes have
> +// indistinguishable identity. Note that even if the frame corresponding to
> +// the static scope is live on the stack, it is unsound to synthesize a scope
> +// from that live frame. In other words, the scope chain is the provenance of

Do you really mean "provenance" here? Or "province"? Something feels weird.

provenance:
1. Place of origin; derivation.

province:
6. the field or extent of a person's activities or office

More importantly, I think we should add something to the effect of:

The scopes we synthesize for static scope objects are read-only, and we never use their parent links, so they don't need to be distinct.

Comment 39

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #37)
> (In reply to Jim Blandy :jimb from comment #35)
> > Comment on attachment 8499267 [details] [diff] [review]
> > Part 4: Tests.
> > 
> > Review of attachment 8499267 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Could we have tests that D.E.p.inspectable, .type, .parent, and .callee
> > behave reasonably on optimized-out environments?
> > 
> > Can an eval frame ever have an optimized-out environment?
> 
> I don't see why not. Accessing bindings on the scope would result in an
> error being thrown.
> 

I take that back. D.F.p.eval can't have an optimized-out env. D.E.p.eval, if we implement that, obviously will be able to have an optimized-out environment.

Comment 40

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b14f46d65f73
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7b619dc152
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bbbcf8b4d66d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4acf60209a94

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a9355863e299 for apparently breaking jit1 tests:
https://treeherder.mozilla.org/logviewer.html#?job_id=5473978&repo=mozilla-inbound
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Ubuntu%20VM%2012.04%20x64%20mozilla-inbound%20opt%20test%20jittest-1

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like this might also be responsible for this static analysis bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=5470359&repo=mozilla-inbound

Comment 43

[Shu-yu Guo [:shu]]

Thanks, hopefully I fixed both things: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6bbd26897ba

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb00dedf441c
https://hg.mozilla.org/mozilla-central/rev/f7498b5f244b
https://hg.mozilla.org/mozilla-central/rev/22b2612b533c
https://hg.mozilla.org/mozilla-central/rev/0ab0ac065bc3