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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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" +
       "}());");

Comment 1

[Jim Blandy :jimb]

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

Comment 3

[Tom Tromey :tromey]

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.

Comment 4

[James Long (:jlongster)]

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.

Comment 5

[Tom Tromey :tromey]

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);

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 () { ... }());

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

That way the last byte code in the function would always be at the end.

Comment 8

[Tom Tromey :tromey]

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)".

Comment 9

[Tom Tromey :tromey]

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.

Comment 10

[Tom Tromey :tromey]

> 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.

Comment 11

[Tom Tromey :tromey]

Attachment: fix ending location of variable declaration

This fixes the only instance of the bug I could find.

Comment 12

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9694e7f390d3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f20b94bf8c

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9f20b94bf8c