1577639 - [jsdbg2] Debugger::replaceFrameGuts drops breakpoints from eval scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Jim Blandy :jimb]

The debugger will miss breakpoints set in eval scripts, if they happen to be OSR'd.

When a frame transitions from one representation to another (say, from an InterpreterFrame to a BaselineFrame for OSR), Debugger::replaceFrameGuts uses Debugger::removeFromFrameMapsAndClearBreakpointsIn to remove the old AbstractFramePtr's entry from the js::Debugger::frames map, but rFFMACBPI (hmm, name is still too long) assumes that the frame is exiting, not being OSR'd, and thus removes all breakpoints from the frame's script if it is an eval script, in an attempt to conceal the existence of the eval cache. This is incorrect when the frame is simply being OSR'd, and will continue to execute.

The following test shows the failure:

// Debugger::replaceFrameGuts doesn't drop breakpoints set in eval scripts.

var g = newGlobal({newCompartment: true});
var dbg = Debugger(g);

let debuggerHits = 0;
let breakpointHits = 0;

dbg.onDebuggerStatement = function (frame) {
  debuggerHits++;

  let loc = frame.script.getPossibleBreakpoints({ line: 3 })[0];
  frame.script.setBreakpoint(loc.offset, {
    hit(frame) {
      breakpointHits++;
    }
  });

  dbg.onDebuggerStatement = function(frame) {
    debuggerHits++;
  };
};

g.eval(`
  for (var i = 0; i < 100; i++) {
    /* bp set here */ debugger;
  }
`);

assertEq(debuggerHits, 100);
assertEq(breakpointHits, 99);

Comment 2

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1577639 - Do not eagerly clear breakpoints for evalled scripts. r=arai!

The goal of this code has been the conceal the fact that internally
SpiderMonkey may cache and re-execute JSScripts for evalled code, which could
be counterintuitive for the average user.

Unfortunately, this behavior is not necessarily worthwhile because while it may
address this specific case, the existence of the cache is also exposed by
the fact that onNewScript is only called for the first eval call.
In the case where users expect to be able to add breakpoints in onNewScript,
the current code ends up leaving us with the inverse of the problem because
if we delete the breakpoints and never call onNewScript, nothing will add
the breakpoints back for future eval attempts.

I think realistically we should drop this code and always have the expectation
that a script may persist and run multiple times. This is already what the
DevTools debugger assumes because Gecko itself also may cache and re-execute
JSScripts.

Comment 3

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1577639 - Do not attempt to clean up frames for non-OOM conditions. r=arai!

The core issue is that we run removeFromFrameMapsAndClearBreakpointsIn(cx, from)
even for non-OOM paths in this function, which is undesirable because that will
remove the breakpoints for evaled scripts. In the non-OOM case there are no
Debugger.Frame objects left in the frames map for the "frame" list, so there
is no reason to actually call it in that case.

This patch merges the two OOM handlers together for simplicity, though it
is not technically necessary, it seems like the very minor increase in code
run if there is an OOM is worth it to keep things centralized.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3310725a4fcd
Do not attempt to clean up frames for non-OOM conditions. r=arai

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/3310725a4fcd