Closed Bug 911065 Opened 6 years ago Closed 5 years ago

Fix source notes for for/of loops

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ivan.dejanovic, Assigned: fitzgen)

Details

Attachments

(2 files, 1 obsolete file)

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
Assignee: nobody → general
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
I can reproduce this. It seems to be a problem with the debugger or the debugger UI.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Not a security bug.
Group: core-security
Flags: needinfo?(jimb)
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
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?
Lurvely.
Flags: needinfo?(jimb)
Assignee: general → jimb
I once saw insane control flow in a generator. It would be great if this were the same bug.
Summary: Javascript engine, let collateral damage: true == false → [jsdbg2] Javascript engine, let collateral damage: true == false
Stealing.
Assignee: jimb → nfitzgerald
Status: NEW → ASSIGNED
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.
Attached patch for-of-source-notes.patch (obsolete) — Splinter Review
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 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+
Summary: [jsdbg2] Javascript engine, let collateral damage: true == false → Fix source notes for for/of loops
https://hg.mozilla.org/mozilla-central/rev/261e7681ed02
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.