Closed Bug 1370648 Opened 7 years ago Closed 7 years ago

Debugger pauses at last statement, even if dead code

Categories

(DevTools :: Debugger, defect)

Product:
DevTools
Component:
Debugger
Version:
54 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

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

Details

Attachments

(4 files)

example.html
228 bytes, text/html
Details
screencapture.ogv
389.22 KB, video/ogg
Details
example.html but without trailing newlines
204 bytes, text/html
Details
Bug 1370648 - use final token as end location of statement list;
59 bytes, text/x-review-board-request
jimb
: review+
Details
Attached 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.
Component: Untriaged → Developer Tools: Debugger
Attached video screencapture.ogv — 

    
    

    
  
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.

    
    

    
  
One additional interesting thing is that it doesn't fail if I wrap the code in a function.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
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: nobody → ttromey
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
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 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+

    
    

    
  
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.

    
    

    
  
Wouldn't the last token read before EOF be the }?

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
      
  

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
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; }

    
    

    
  
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...

    
    

    
      
  

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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]]

    
    

    
  
Once again I wish I could call dis() to get a string to log.

    
    

    
  
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.

    
    

    
  
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...

    
    

    
  
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.

    
    

    
      
  

    
    

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/34a05b4aae38
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: