1066331 - Multi-line template string breaks debugger line number

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tomasz Kołodziejski [:tomasz]]

I was writing some xpcshell test. Let's simplify it like this:

```js
function run_test() {
  let q = `

    some
    sql
    stuff

  `;

  equal(1, 2);
}
```

Result:
/Users/mozilla/gecko-dev/obj-ff-dbg/_tests/xpcshell/toolkit/components/contentprefs/tests/unit_cps2/test_string.js:run_test:4

Expected:
Line 10.

It looks like debugger collapses that multi-line string into one line (try removing all the line breaks in the string).

Comment 1

[Jeff Walden [:Waldo]]

Looks like TokenStream::getStringOrTemplateToken doesn't updateLineInfoForEOL() or updateFlagsForEOL() at all anywhere in it.  Probably it should call both in the TokenBuf::isRawEOLChar(c) block in that method, tho my understanding of the quirks of line info checking are spotty enough I'm not 100% sure that's the exact, complete fix.

Comment 2

[guptha]

Attachment: debugv0.diff

This patch is correct as tested in the shell using dis():
http://pastebin.mozilla.org/6444228

How would we test something like this from within jstests?

Comment 3

[ziyunfei]

(In reply to guptha from comment #2)
> Created attachment 8488284 [details] [diff] [review]
> debugv0.diff
> 
> This patch is correct as tested in the shell using dis():
> http://pastebin.mozilla.org/6444228
> 
> How would we test something like this from within jstests?

How about this:


try {
    `
    a
    b
    c
    `;
    throw Error("error");
} catch (e) {
    assertEq(e.stack.indexOf(":7:11") > -1, true);
}

if (typeof reportCompare === "function")
    reportCompare(true, true);

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8488284 [details] [diff] [review]
debugv0.diff

Review of attachment 8488284 [details] [diff] [review]:
-----------------------------------------------------------------

Error objects also have a lineNumber property.

Comment 5

[guptha]

Attachment: Patch to ensure proper line numbers in debugger v1

Added test.

Comment 6

[guptha]

https://tbpl.mozilla.org/?tree=Try&rev=da20ae5ac388

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8489175 [details] [diff] [review]
Patch to ensure proper line numbers in debugger v1

Review of attachment 8489175 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. Please also test that in

    function tagThatThrows(...args) { throw new Error(); }

    tagThatThrows`    <-- error.stack should cite this line number
       multi-line
       template
       string`

the correct line number appears in the error.stack property, and the same for

    ` multi-line
      template
      with
      ${substitutionThatThrows()}`  // <-- error.stack should cite this line number

Comment 8

[Jason Orendorff [:jorendorff]]

It would also be nice to check that the "loc" properties within Reflect.parse(script) are correct when the script contains a multiline template string. They are certainly wrong without this patch; I think the patch probably fixes them.

js> Reflect.parse("`\n\n   ${2}\n\n\n`")
({
  loc:{start:{line:1, column:0}, end:{line:1, column:14}, source:null},
  type:"Program",
  body:[
    {
      loc:{start:{line:1, column:0}, end:{line:1, column:14}, source:null},
      type:"ExpressionStatement",
      expression:{
        loc: {start:{line:1, column:0}, end:{line:1, column:14}, source:null},
        type: "TemplateLiteral",
        elements: [
          {loc:{start:{line:1, column:0}, end:{line:1, column:8}, source:null},
           type:"Literal", value:"\n\n   "},
          {loc:{start:{line:1, column:8}, end:{line:1, column:9}, source:null},
           type:"Literal", value:2},
          {loc:{start:{line:1, column:9}, end:{line:1, column:14}, source:null},
           type:"Literal", value:"\n\n\n"}
        ]
      }
    }
  ]
})

Comment 9

[guptha]

>     function tagThatThrows(...args) { throw new Error(); }
> 
>     tagThatThrows`    <-- error.stack should cite this line number
>        multi-line
>        template
>        string`
> 

Is the line number for the above example correct? For example,

js> try { function tagThatThrows(...args) { throw new Error(); }
tagThatThrows();
}
catch(e) { print(e.lineNumber);}
1

I presume tagged templates behave similarly.

Comment 10

[Jason Orendorff [:jorendorff]]

error.stack contains a stack trace, so it should get both lines:

    function tagThatThrows(...args) { throw new Error(); }

    try {
        tagThatThrows`    <-- error.stack should cite this line number
           multi-line
           template
           string`
    } catch (exc) {
        print(exc.stack);
    }

And it looks like this works correctly even without your patch:

    [e-mail]:1:41
    @pos.js:4:5

The other case doesn't, but I think your patch probably fixes it.

Comment 11

[guptha]

Attachment: Patch to ensure proper line numbers in debugger v2

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1317149f4e31

Does this also need uplift to 34?

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Jason Orendorff [:jorendorff]]

This isn't the sort of bug fix we usually uplift to prior releases.