Closed
Bug 911065
Opened 11 years ago
Closed 10 years ago
Fix source notes for for/of loops
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: ivan.dejanovic, Assigned: fitzgen)
Details
Attachments
(2 files, 1 obsolete file)
1.83 MB,
image/png
|
Details | |
3.55 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release) Build ID: 20130829030201 Steps to reproduce: I used a var within a for loop. Actual results: true == false Expected results: true != false
Updated•11 years ago
|
Assignee: nobody → general
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Comment 1•11 years ago
|
||
I can reproduce this. It seems to be a problem with the debugger or the debugger UI.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•11 years ago
|
Flags: needinfo?(jimb)
Comment 3•11 years ago
|
||
var g = newGlobal(); var dbg = new Debugger; var gw = dbg.addDebuggee(g); g.eval("var line0 = Error().lineNumber;\n" + "function f() {\n" + " for (var x of [0]) {\n" + " if (true == false)\n" + " return false;\n" + // line0 + 4 " }\n" + " return true;\n" + "}\n"); var script = gw.getOwnPropertyDescriptor("f").value.script; var hits = 0; var handler = {hit: function () { hits++; }}; var offs = script.getLineOffsets(g.line0 + 4); for (var i = 0; i < offs.length; i++) script.setBreakpoint(offs[i], handler); assertEq(g.f(), true); // passes assertEq(hits, 0); // fails
Comment 4•11 years ago
|
||
Note that the function does return true. The behavior of the debuggee is correct. It's just that the debugger is breaking when that line of code isn't actually executing. The bug is that the source-location information is wrong for some of the code implementing the for-loop. Jim, can you take this?
Updated•11 years ago
|
Assignee: general → jimb
Comment 6•11 years ago
|
||
I once saw insane control flow in a generator. It would be great if this were the same bug.
Updated•10 years ago
|
Summary: Javascript engine, let collateral damage: true == false → [jsdbg2] Javascript engine, let collateral damage: true == false
Comment 8•10 years ago
|
||
EmitForOf calls EmitLoopEntry with a null third argument. jsop_loopentry and subsequent instructions should, I think, always be attributed to the position of the loop header (in this case, line0+2). I think EmitLoopEntry needs to call UpdateSourceCoordNotes unconditionally; callers that pass a null third argument need alternative other way to tell EmitLoopEntry the position of the loop header.
Assignee | ||
Comment 9•10 years ago
|
||
Wasn't sure where to get relevant offsets to make EmitLoopEntry always emit source notes, but passing in the for/of head's expr being iterated over (if available) fixes the test case. Also made the iteration protocol's calls reported to the expr parse node, because that seems more correct to me. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b2e433feb231
Attachment #8520834 -
Flags: review?(jorendorff)
Comment 10•10 years ago
|
||
Comment on attachment 8520834 [details] [diff] [review] for-of-source-notes.patch Review of attachment 8520834 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks for taking this. With this patch applied, it'd be interesting to open the devtools debugger and run some code like this: for (let x of reallyLongExpression()) y(x); Set breakpoints on all three lines and see if the behavior is what you expect. I think it'll be a little weird, but perhaps acceptable.
Attachment #8520834 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Carrying over r+ New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0fd34ee75399
Attachment #8520834 -
Attachment is obsolete: true
Attachment #8522509 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Summary: [jsdbg2] Javascript engine, let collateral damage: true == false → Fix source notes for for/of loops
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/261e7681ed02
Flags: in-testsuite+
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/261e7681ed02
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•