Open Bug 714857 Opened 12 years ago Updated 2 years ago

Debugger.Frame.prototype.eval can miss inner bindings

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect

Tracking

()

People

(Reporter: jimb, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In code like this:

var x = 'outer';
function f() { var x = 'inner'; return function g() { h(); } }
function h() { debugger; }

Evaluating f()(), stopping at the debugger statement, and evaluating 'x' in h's caller's frame can return 'outer': f's local binding for x is not visible to Debugger.Frame.prototype.eval.

This is probably due to SpiderMonkey using the null closure representation for g's closure, and Debugger.Environment not catching that.
I don't think SpiderMonkey should try to keep the inner 'x' around in this case; almost all the time, the user won't need to see it, and will appreciate the space-saving optimization. Instead, I think the call to Debugger.Frame.prototype.eval should throw an error indicating that the variable's value isn't available due to optimizations.
Agreed.

From IRC, and for the record: bug 690135 is going to fix JS_GetFrameScopeChain to return a scope chain composed of wrappers (which wrap pre-existing or lazily-reified ScopeObjects).  This bug could be fixed by extending that approach with a new Proxy which represents an environment that could not be lazily reified.  This Proxy could then use the script's Bindings to throw errors on lookups that hit.

A challenge is that inner scripts do not root outer scripts and probably shouldn't so we can't work our way from fp->fun()->script() outward.  A direct solution that hopefully wouldn't waste too much memory would be to instead have JSScript.bindings root the bindings of enclosing scripts.

An alternative is to use the js::types::TypeScriptNesting that TI attaches to JSScripts.  The problem is that, IIRC, the tree of TypeScriptNestings is incomplete and gets thrown away at various times.  I'm not totally sure what the invariants are, though.  If we went with the first strategy, perhaps it could be generalized so that TI could reuse it instead of doing the same thing?  What do you think Brian?
Bug 692984 comment 3 mentions this bug. In short: when this gets fixed, please also add a test that looks for the bug using frame.environment.
Assignee: general → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.