1055175 - JSD2: Inconsistent handling of eval cache scripts

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Jason Orendorff [:jorendorff]]

fflorent discovered this, and pointed me to this patch

https://github.com/fflorent/firebug/commit/e1e2a404b1c72787ddaa56bde4f77bdcb9bde5cd#diff-81d6cb810a2c6de920852845d6cd8f6cR454

Here's a shell test case that fails:

var g = newGlobal();
var dbg = Debugger(g);
var hits = 0;
dbg.onNewScript = function (script) {
    script.setBreakpoint(0, {hit: function () { hits++; }});
};

g.eval(`
    function f() {
        for (var i = 0; i < 7; i++)
            eval("print(i);");
    }
    f();
`);

// We should set a breakpoint in the g.eval() script and hit it once.  Also,
// each time f() calls eval(), we should hit a breakpoint at the beginning of
// the eval script.
assertEq(hits, 8);  // FAILS

Comment 1

[Jason Orendorff [:jorendorff]]

To explain the buggy behavior a bit more:

The loop calls eval(), passing the same string, 7 times. After the first time, we find a matching script in the eval cache. onNewScript is *not* called when an eval cache hit is found.

So far so good, but what's bad is, the breakpoint doesn't hit, except the first time. Since it's the same script, it must be that the JS engine is clearing the breakpoint. I don't remember why.

We should either consistently act as though we're creating a new script each time (fire onNewScript again each time we have a cache hit --- this is probably the easiest thing for users to understand) OR consistently treat it as the same script and not mysteriously throw away breakpoints.

Comment 2

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> We should either consistently act as though we're creating a new script each
> time (fire onNewScript again each time we have a cache hit --- this is
> probably the easiest thing for users to understand) OR consistently treat it
> as the same script and not mysteriously throw away breakpoints.

Because Debugger clients can hold scripts live just by holding the Debugger.Script instances alive, I think we are required to reveal the existence of the eval cache: they can just tell that it's the same script. So we should call onNewScript when we actually compile it, and not when we find it in the eval cache; and we should never delete breakpoints while the script is still reachable.