Debugger pauses at last statement, even if dead code

RESOLVED FIXED in Firefox 57

Status

defect
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: account-mozilla-bugzilla, Assigned: tromey)

Tracking

54 Branch
Firefox 57

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Posted file example.html
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170524174609

Steps to reproduce:

1. Open attached example.html in Firefox 53.0 or Firefox Developer Edition 54.0 beta12 (both affected, new and old debugger!).
2. Open Developer Tools, go to Debugger.
3. Set breakpoints in line 7.
4. Reload.



Actual results:

- Debugger pauses and indicates that the breakpoint in line 7 is hit


Expected results:

- The breakpoint should not be hit!
- NOTE: The if condition is always false (1 !== 1) so the statement in line 7 is never executed. This can also be verified by looking at the console: There is no output, as expected.
- NOTE: adding another statement after the if, e.g. a console.log("blabla"); fixes the issue, then line 7 is no longer hit.
(Reporter)

Updated

2 years ago
Component: Untriaged → Developer Tools: Debugger
(Reporter)

Comment 1

2 years ago
Posted video screencapture.ogv
(Assignee)

Comment 2

2 years ago
It's another case where the bytecode emitter is not setting line notes properly.
Run this script through the js shell:

===
var g = newGlobal();

var dbg = Debugger(g);
dbg.onDebuggerStatement = function (frame) {
    print("Offsets " + frame.script.getAllOffsets());
    print("Line Offset " + frame.script.getLineOffsets(4));

    frame.onStep = function () {
	print("++ " + this.offset);
    };
}

let s = (`
      debugger;
      if (1 !== 1) {
        print("dead code!?"); // set breakpoint here
      }
`);
dis(s);
g.eval(s);
===


I get:

===
loc     op
-----   --
main:
00000:  debugger                        # 
00001:  one                             # 1
00002:  one                             # 1 1
00003:  strictne                        # (1 !== 1)
00004:  ifeq 29 (+25)                   # 
00009:  jumptarget                      # 
00010:  getgname "print"                # print
00015:  gimplicitthis "print"           # print THIS
00020:  string "dead code!?"            # print THIS "dead code!?"
00025:  call 1                          # print(...)
00028:  setrval                         # 

# from ifeq @ 00004
00029:  jumptarget                      # 
00030:  retrval                         # 

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     0 [   0] newline 
  1:    2     0 [   0] colspan 6
  3:    2     1 [   1] newline 
  4:    3     4 [   3] if      
  5:    3    10 [   6] newline 
  6:    4    10 [   0] colspan 8
  8:    4    30 [  20] xdelta  
  9:    4    30 [   0] colspan 21

Offsets ,,0,1,10,30
Line Offset 10,30
++ 1
++ 2
++ 3
++ 4
++ 29
++ 30
===

The problem is that the getLineOffsets call says that PC=10 is an entry point for line 4.
Also, I think getOffsets and getLineOffsets should probably not disagree.
(Assignee)

Comment 3

2 years ago
One additional interesting thing is that it doesn't fail if I wrap the code in a function.
(Assignee)

Comment 4

2 years ago
Here's a test that's more in the form that could be dropped into the tree.
(If you remove the dis() and the prints)

// Regression test for bug 1370648.

let g = newGlobal();

let dbg = Debugger(g);
let lines = [0, 0, 0, 0];
dbg.onDebuggerStatement = function (frame) {
  let dLine = frame.script.getOffsetLocation(frame.offset).lineNumber;
  lines[0] = 1;
  frame.onStep = function () {
    print("++ " + this.offset + " "
          + (frame.script.getOffsetLocation(this.offset).lineNumber - dLine));
    lines[frame.script.getOffsetLocation(this.offset).lineNumber - dLine] = 1;
  };
}

let s = (`
      debugger;                 // 0
      if (1 !== 1) {            // 1
        print("dead code!?");   // 2
      }                         // 3
`);
dis(s);
g.eval(s);
assertEq(lines.join(""), "1101");

Running this shows that it thinks that PC=29 comes from (relative) line 2.
But, it clearly doesn't.  This is a case of the "location inheritance" problem
that I thought we'd fixed once and for all a while back; but worse, because
at the time the "if" is emitted (that's what emits the jumptarget instruction),
the location of the subsequent instruction isn't known.
(Assignee)

Comment 5

2 years ago
One idea that would probably work would be to have BytecodeEmitter::updateLineNumberNotes back up
over any previous jumptarget instructions before setting the line number.
But, this seems very gross.
(Assignee)

Comment 6

2 years ago
(In reply to Tom Tromey :tromey from comment #5)
> One idea that would probably work would be to have
> BytecodeEmitter::updateLineNumberNotes back up
> over any previous jumptarget instructions before setting the line number.

In SF, :jorendorff said that this approach sounded ok.
(Assignee)

Comment 7

2 years ago
I discovered today that BytecodeRangeWithPosition has logic to try to skip the jumptarget opcodes.
So, maybe the solution is to see why this is not performing properly.
(Assignee)

Comment 8

2 years ago
I have a patch to change the parser and BCE to use the location of the EOF
when emitting the retrval.  This improves the situation somewhat, but you
can still see the bug if the script doesn't end with a newline.
(I'll attach a version of the test page showing this behavior).

I'm not sure what to do about this.
(Assignee)

Updated

2 years ago
Assignee: nobody → ttromey
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 10

2 years ago
In the end I think there's nothing to do about the no-newline case.
It's another thing that can be fixed by column stepping and column breakpoints.
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8888029 [details]
Bug 1370648 - use final token as end location of statement list;

https://reviewboard.mozilla.org/r/158900/#review164808

This all looks good, but I don't really understand why the change fixes the bug. Why would changing the line number attributed to the `retrval` affect what happens when we set a breakpoint inside the `if`?
Attachment #8888029 - Flags: review?(jimb) → review+
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8888029 [details]
Bug 1370648 - use final token as end location of statement list;

https://reviewboard.mozilla.org/r/158900/#review164808

Before the patch, the retrval was getting the location of the last token that was read before EOF -- column 29 on the "print" line, aka the ";".  This makes it seem that PC=30 is an entry point for that line.  See the "Line Offsets" output in the example in comment #2 (my conclusions in that comment are confused, though).

The patch just uses the location of the actual EOF for the retrval.  This works when there's a newline; not ideal, but an improvement, and anyway the best I could think of.

Comment 14

2 years ago
Wouldn't the last token read before EOF be the }?
(Assignee)

Comment 15

2 years ago
(In reply to Jim Blandy :jimb from comment #14)
> Wouldn't the last token read before EOF be the }?

Yeah.  I will re-debug it tomorrow and figure out what is going on.
(Assignee)

Comment 16

2 years ago
(In reply to Tom Tromey :tromey from comment #15)
> (In reply to Jim Blandy :jimb from comment #14)
> > Wouldn't the last token read before EOF be the }?
> 
> Yeah.  I will re-debug it tomorrow and figure out what is going on.

If you look at comment #2, the final colspan there is emitted here:

https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/js/src/frontend/BytecodeEmitter.cpp#11263

In particular this is emitting a PNK_STATEMENTLIST.
The statement list gets this particular end location due to:

https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/js/src/frontend/ParseNode.h#766

From this I think the BytecodeEmitter.cpp hunk in the patch is probably not needed.

Maybe another approach to the patch would be to change statement list parsing to choose a different
end point.  I don't know offhand what other effects that might have.
Comment hidden (mozreview-request)

Comment 18

2 years ago
(In reply to Tom Tromey :tromey from comment #16)
> Maybe another approach to the patch would be to change statement list
> parsing to choose a different
> end point.  I don't know offhand what other effects that might have.

If I'm reading that front-end code correctly, I take this to mean that when I have:

   {
   A;
   B;
   C;
   }

This builds a statement list node by making a series of calls to `append`, each of which updates the ending location of the list itself to cover that of the node being appended. So the last update to the statement list's ending position occurs when we append C;. Is that correct?

That suggests that we *don't* update the source position when we read the closing curly brace. Is that true?? We totally should, shouldn't we?

Everything I saw in the patch seemed like an improvement regardless of the bug, but I would like to really understand why the patch fixes the bug before landing this, and I don't right now.
(Assignee)

Comment 19

2 years ago
(In reply to Jim Blandy :jimb from comment #18)

>  So the last update to the statement list's
> ending position occurs when we append C;. Is that correct?

Yes.

> That suggests that we *don't* update the source position when we read the
> closing curly brace. Is that true?? We totally should, shouldn't we?

I don't know.  I can try it of course.

> Everything I saw in the patch seemed like an improvement regardless of the
> bug, but I would like to really understand why the patch fixes the bug
> before landing this, and I don't right now.

It sets the end position of the statement list -- not by changing statement list
parsing, but by using the EOF location when parsing a script.
(Assignee)

Comment 20

2 years ago
This patch seems to work ok.
Let me know if you'd prefer this.

diff --git a/js/src/frontend/FullParseHandler.h b/js/src/frontend/FullParseHandler.h
index b592a0d..91a72bf 100644
--- a/js/src/frontend/FullParseHandler.h
+++ b/js/src/frontend/FullParseHandler.h
@@ -455,6 +455,11 @@ class FullParseHandler
         }
     }
 
+    void setListEndPosition(ParseNode* list, const TokenPos& pos) {
+        MOZ_ASSERT(list->isKind(PNK_STATEMENTLIST));
+        list->pn_pos.end = pos.end;
+    }
+
     void addCaseStatementToList(ParseNode* list, ParseNode* casepn) {
         MOZ_ASSERT(list->isKind(PNK_STATEMENTLIST));
         MOZ_ASSERT(casepn->isKind(PNK_CASE));
diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
index 97e38de..9357a03 100644
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -4190,8 +4190,14 @@ Parser<ParseHandler, CharT>::statementList(YieldHandling yieldHandling)
                 isUnexpectedEOF_ = true;
             return null();
         }
-        if (tt == TOK_EOF || tt == TOK_RC)
+        if (tt == TOK_EOF || tt == TOK_RC) {
+            TokenPos pos;
+            if (!tokenStream.peekTokenPos(&pos, TokenStream::Operand)) {
+                return null();
+            }
+            handler.setListEndPosition(pn, pos);
             break;
+        }
         if (afterReturn) {
             if (!tokenStream.peekOffset(&statementBegin, TokenStream::Operand))
                 return null();
diff --git a/js/src/frontend/SyntaxParseHandler.h b/js/src/frontend/SyntaxParseHandler.h
index db82655..460a58f 100644
--- a/js/src/frontend/SyntaxParseHandler.h
+++ b/js/src/frontend/SyntaxParseHandler.h
@@ -303,6 +303,7 @@ class SyntaxParseHandler
 
     Node newStatementList(const TokenPos& pos) { return NodeGeneric; }
     void addStatementToList(Node list, Node stmt) {}
+    void setListEndPosition(Node list, const TokenPos& pos) {}
     void addCaseStatementToList(Node list, Node stmt) {}
     MOZ_MUST_USE bool prependInitialYield(Node stmtList, Node gen) { return true; }
     Node newEmptyStatement(const TokenPos& pos) { return NodeEmptyStatement; }

Comment 21

2 years ago
I think I do prefer that. It makes sense that the source positions of a node representing a bracketed statement should enclose the brackets.

It'll be interesting to see what, if anything, depends on it being wrong...
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 years ago
This patch is failing here:

https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/devtools/client/debugger/new/test/mochitest/browser_dbg-breaking.js#14

The breakpoint line is set at line 18, but I think with these changes it should be 19.
But, when I change it to 19, it doesn't set, because of this code:

https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/devtools/server/actors/source.js#802

... which causes the actor to think that the script stops at line 18.
(Assignee)

Comment 24

2 years ago
Changing the script in the test case to read:

       inline_script = function() { var x = 5; }

... makes it work.

In the passing case, _setBreakpointAtGeneratedLocation finds an offset at line 18.
In the failing case (with the line number changed to 19), there don't seem to be any
offsets for line 19.  So, something strange is going on.
(Assignee)

Comment 25

2 years ago
getLineOffets(19) returns [] in this case, but getAllOffsets returns
[null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,null,[0]]
(Assignee)

Comment 26

2 years ago
Once again I wish I could call dis() to get a string to log.
(Assignee)

Comment 27

2 years ago
Now I think comment #25 is mistaken.
Instead what I'm seeing is that doc-scripts.html shows different available line numbers at different times.
The first time it is seen, it has an entry point at line 18.
Later it shows an entry point at line 19.

I can't really explain this.
(Assignee)

Comment 28

2 years ago
With more logging I can see that the Debugger object can see a script at line 18 with line length 1.
This appears to be the function.
Sometimes it also sees a script at line 14, line length 6.  This is the <script> -- the one we are looking for.
However, it's not always there.  How can this happen?  I thought the presence of this function was intended
to avoid GCing the script...
(Assignee)

Comment 29

2 years ago
Ok, I think I see what is happening.

We might or might not see the script representing the <script>.
If we don't, there is still a line 18 -- but because the function is never called, the breakpoint doesn't fire.

So, I think there's nothing really wrong here, and I can apply the change from comment #24 and move on.
Comment hidden (mozreview-request)

Comment 31

2 years ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/34a05b4aae38
use final token as end location of statement list; r=jimb

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34a05b4aae38
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.