Closed Bug 979094 Opened 6 years ago Closed 5 years ago

[jsdbg2] Debugger.Script.prototype.lineCount is incorrect for immediately invoked function expressions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: fitzgen, Assigned: tromey)

References

Details

(Whiteboard: [js:p2])

Attachments

(1 file)

In the following js shell test case, lineCount is 1 not 4. I would definitely expect a D.Script's lineCount to be >= the sum of its child script line counts.

------------------------------------------------------

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

dbg.onNewScript = s => {
  assertEq(s.lineCount, 4);
};

g.eval("var x = (function () {\n" +
       "  var b = 9;\n" +
       "  return b;\n" +
       "}());");
At the moment, we derive the lineCount from the source notes --- it's just the last line number mentioned in the source notes. So if there are no bytecode operations attributed to the last line, then it looks to me like lineCount will be wrong.

https://hg.mozilla.org/integration/mozilla-inbound/file/e2db166682ac/js/src/vm/Debugger.cpp#l2965
https://hg.mozilla.org/integration/mozilla-inbound/file/e2db166682ac/js/src/jsscript.cpp#l2729
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [js:p2]
Duplicate of this bug: 1124230
I think the underlying bug is that the end position on a ParseNode is not reliably set.

The byte code emitter tries to emit a location at the end:

https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#7289

However in this case this value comes from the outermost node in the script,
which in this case is created here:

https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/Parser.cpp#5800

It's a bit hard to tell, but Parser::variables only updates the end location via
calls to handler.addList, like:

https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/Parser.cpp#3899

... and in particular this doesn't handle the right-hand-side of the assignment.

One fix for the immediate bug would be to change the code to update the
ending location when matching the ";".  Unfortunately, I think the full fix would
be to audit the parser to ensure that end locations are updated universally.
Perhaps updating Parser::statement to set the ending location from the most-recently-read
token (I dunno if that is readily available) or something like that would be
good enough.
Thanks for looking into it! Since we want to work with a script that is basically the entire source code of the Debugger.Source, I think we may not need this to be fixed for bug 1124258. We might just avoid constraining our query to within the start/end line, since it's the whole source anyway.

Not to say this shouldn't be fixed, but I don't we're blocked on it for a pretty big optimization that we want to do in the debugger.
One relatively simple fix might be to introduce a new class whose destructor
sets the end location on a node from the most recently used token.  Then every spot like:

    Node kid = unaryExpr();
    ...
    return kid;

could be updated with:

    EndRewriter r(kid);

Maybe too magical though.


I thought this test case might be handy for any future work here:

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

function test(scriptText, expectedLineCount) {
  let found = false;

  dbg.onNewScript = function(script, global) {
    assertEq(script.lineCount, expectedLineCount);
    found = true;
  }

  g.evaluate(scriptText);
  assertEq(found, true);
}

src = 'var a = (function(){\n' + // 0
      'var b = 9;\n' +           // 1
      'console.log("x", b);\n'+  // 2
      'return b;\n' +            // 3
      '})();\n';                 // 4
test(src, 5);
I think we could get a quick and dirty fix if we just move the source notes for whatever bytecodes get generated for function application from the start of the function expression to the end at the parens. Tromey, do you think this would work?

     +--- from here
     |                   +--- to here
     |                   |
     v                   v
     (function () { ... }());
Flags: needinfo?(ttromey)
That way the last byte code in the function would always be at the end.
Sorry Nick, I am not sure I understand.

The situation as I understand it is that the parser is losing
information.  The final location never makes it to the bytecode
emitter.

Here's the code:

pokyo. cat /tmp/t.js
var a =(function(){
    var b = 9;
    console.log("x", b);
    return b;
})();


Then disassembling it:

js> disfile("/tmp/t.js")
loc     op
-----   --
00000:  defvar "a"
main:
00005:  bindgname "a"
00010:  lambda (function (){
    var b = 9;
    console.log("x", b);
    return b;
})
00015:  undefined
00016:  call 0
00019:  setgname "a"
00024:  pop
00025:  retrval

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     0 [   0] colspan 4
  2:    1     5 [   5] colspan 4
  4:    1    16 [  11] xdelta  
  5:    1    16 [   0] colspan 4
  7:    1    25 [   9] xdelta  
  8:    1    25 [   0] colspan -3


This just disassembles the generated script.  You can see here that
the bytecode emitter only knows about line #1.  That's because the parser
has neglected to tell it about any later lines.


Re comment #5 - it would also be pretty simple to just wrap all the returns
in the parser, say from "return node" to "return UpdateEndLocation(node)".
Flags: needinfo?(ttromey)
A simple to see it is to use Reflect.parse:

Reflect.parse(`var a =(function(){
    var b = 9;
    console.log("x", b);
    return b;
})();`);


My output starts:

({loc:{start:{line:1, column:0}, end:{line:1, column:5}, source:null}, type:"Program", body:[{loc:{start:{line:1, column:0}, end:{line:1, column:5}, source:null}, type:"VariableDeclaration", kind:"var", declarations: ...

So the two outermost AST nodes here have the wrong end location.

Maybe the bug is smaller than I thought, since I'm having some difficulty finding
other bad cases.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
> Maybe the bug is smaller than I thought, since I'm having some difficulty
> finding other bad cases.

I spot-checked a number of the cases in Parser::statement and couldn't find
any other bad ones.
This fixes the only instance of the bug I could find.
Attachment #8584566 - Flags: review?(jimb)
Attachment #8584566 - Flags: review?(jimb) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b9f20b94bf8c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.