1432885 - We should always step-in/step-out to valid breakable positions instead of the next bytecode offset

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Logan Smyth [:loganfsmyth]]

Attachment: the debugger showing `undefined` instead of uninitialized

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

Note: This may be the same core issue as https://bugzilla.mozilla.org/show_bug.cgi?id=1432682 since they both involve offsets when entering blocks initially, but I'm filing separately until someone can actually look to make sure.

Run the following snippet in jsshell:

var js = `(function(){
  let obj = {
    method(param) {
      let foo;
    }
  };

  debugger;
  obj.method("param");
})()`;

function printLine(script, offset) {
  var loc = script.getOffsetLocation(offset);
  var line = js.split('\n')[loc.lineNumber - 1];
  return line.slice(0, loc.columnNumber) + '$' + line.slice(loc.columnNumber);
}

function printState(frame) {
  print('\n---- ' + frame.offset + ': ' +
        printLine(frame.script, frame.offset));
  var env = frame.environment;
  while (env) {
    print('==== ' + env.type);
    if (!env.parent) {
      print('... global stuff');
      break;
    }
    env.names().forEach(function (n) {
      var value = env.getVariable(n);
      print(n + ': ' + JSON.stringify(value));
    });
    env = env.parent;
  }
}

var g = newGlobal();
var dbg = new Debugger(g);

dbg.onDebuggerStatement = function (frame) {
  printState(frame);

  let i = 0;
  dbg.onEnterFrame = function (child) {
    dbg.onEnterFrame = undefined;

    child.onStep = function () {
      printState(child);
    };
    child.onPop = function () {
      child.onStep = undefined;
    };
  };
};

g.eval(js);


Actual results:

The important section of output is:

---- 0: $      let foo;
==== declarative
foo: undefined
==== declarative
arguments: {}
param: "param"
==== declarative
obj: {}
==== declarative
arguments: {}
==== declarative
==== object
... global stuff

---- 1: $      let foo;
==== declarative
foo: undefined
==== declarative
arguments: {}
param: "param"
==== declarative
obj: {}
==== declarative
arguments: {}
==== declarative
==== object
... global stuff

---- 5: $      let foo;
==== declarative
foo: {"uninitialized":true}
==== declarative
arguments: {}
param: "param"
==== declarative
obj: {}
==== declarative
arguments: {}
==== declarative
==== object
... global stuff


The breakpoint offset for the line is `0`, meaning that stepping into a function shows `undefined` instead of `(uninitialized)` in the debugger.


Expected results:

The bytecode offset for that line should be at least `5` since that is the first offset where the binding is marked uninitialized.

Comment 2

[Jason Orendorff [:jorendorff]]

This still happens, because--get this--the way we set up `let` variables is actually to initialize them to `undefined`, then overwrite that with a different bit-pattern that means "uninitialized". Then when we reach the declaration, we set them back to `undefined` (this part is actually in the spec, though, so I don't feel particularly silly about that).

line 4: method(param) {
    # on entry, lexical 0 (`foo`) is `undefined`
    # first set it to <uninitialized>
    uninitialized
    initlexical 0
    pop

line 5: let foo;
    # this line of code sets it back to `undefined`
    undefined
    initlexical 0
    pop

line 6: }
    # and exit the method
    debugleavelexicalenv
    retrval

So there really truly is a time, when we first enter this scope, when `foo` is undefined in our implementation. The best way to fix it is not to pause there.

Comment 4

[Jason Laster [:jlast]]

Logan, is this still an issue?

Comment 5

[Brian Hackett [Laid off!]]

I can reproduce similar behavior in the debugger. On an HTML file below:

<script>
foo();
function foo() {
let x;
let y = 3;
}
</script>

Breaking at the call to foo(), and then stepping in, marks both x and y as undefined. Stepping over to the 'let y = 3;' line marks x as undefined and y as marks x as undefined and y as uninitialized, as they should be.

Comment 6

[Logan Smyth [:loganfsmyth]]

I think the core thing for us here is really that we should never stop on an offset that isn't now flagged as a breakpoint-specificity step location.

We currently pause in onEnterFrame and what we should probably be doing is adding an onStep as soon as we enter a new frame, and then pausing at the first step location.

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #6)

I think the core thing for us here is really that we should never stop on an offset that isn't now flagged as a breakpoint-specificity step location.

We currently pause in onEnterFrame and what we should probably be doing is adding an onStep as soon as we enter a new frame, and then pausing at the first step location.

Cool, that's what I was wondering about. In the function above the first place we can set a breakpoint is the 'let y = 3' line, which seems like a good place to stop at.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Bug 1432885 - Step in to the first valid step target, r=loganfsmyth.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4478ea184906
Step in to the first valid step target, r=loganfsmyth.

Comment 11

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/4478ea184906