Closed
Bug 1066331
Opened 10 years ago
Closed 10 years ago
Multi-line template string breaks debugger line number
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: tomasz, Assigned: gupta.rajagopal)
Details
Attachments
(1 file, 2 obsolete files)
4.47 KB,
patch
|
gupta.rajagopal
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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.
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?
Attachment #8488284 -
Flags: feedback?(jorendorff)
(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•10 years ago
|
||
Comment on attachment 8488284 [details] [diff] [review] debugv0.diff Review of attachment 8488284 [details] [diff] [review]: ----------------------------------------------------------------- Error objects also have a lineNumber property.
Attachment #8488284 -
Flags: feedback?(jorendorff) → feedback+
Added test.
Attachment #8488284 -
Attachment is obsolete: true
Attachment #8489175 -
Flags: review?(jorendorff)
Comment 7•10 years ago
|
||
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
Attachment #8489175 -
Flags: review?(jorendorff) → review+
Comment 8•10 years ago
|
||
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"} ] } } ] })
> 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.
Flags: needinfo?(jorendorff)
Comment 10•10 years ago
|
||
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: tagThatThrows@pos.js:1:41 @pos.js:4:5 The other case doesn't, but I think your patch probably fixes it.
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 11•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1317149f4e31 Does this also need uplift to 34?
Attachment #8489175 -
Attachment is obsolete: true
Attachment #8502241 -
Flags: review+
Flags: needinfo?(jorendorff)
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a24b22bc3fe5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a24b22bc3fe5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 14•10 years ago
|
||
This isn't the sort of bug fix we usually uplift to prior releases.
Flags: needinfo?(jorendorff)
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•