969816 - Stepping in when breaking in an inline event halts at function declaration

Status: RESOLVED DUPLICATE of bug 863089

(DevTools :: Debugger, defect, P3)

Description

[Florent Fayolle]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140127194636

Steps to reproduce:

1. Go to that page: https://getfirebug.com/tests/head/script/stepping/StepIntoInIFrame/test.html
2. Open the Firefox devtools
3. Evaluate this expression using the Console (changes the onclick event so we break in it) :
   $("iframe").contentDocument.querySelector("#testButton").setAttribute("onclick", "debugger; funcTest(1,2,3);")
4. Click on the "Click Me" button
   |=> The debugger breaks at line 1 of test-frame.html (see bug #911721)
5. Step in 


Actual results:

The debugger halts at line 2 of test.js, which contains the function declaration. I rather expect it to halt at line 4 (the first instruction of the function).

Florent

Comment 1

[Florent Fayolle]

Even though the bug might not look important at a first glance, I wonder why the Debugger has this bug.
If you put a breakpoint at line 2 of test.js, the breakpoint is corrected and moves to line 4.

So it might be a deeper problem with the JSD2 internals or the Debugger Server...

Florent

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

Panos, do you know what could be the problem here?

Honza

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

I don't see what the problem is. The modified event handler calls funcTest() after the debugger statement, which is translated to JavaScript bytecode. So, since execution is paused on the bytecode of the debugger statement, stepping would cause execution to pause in the next bytecode, which would be the function invocation bytecode, not the console.log() call inside that function.

Comment 4

[Florent Fayolle]

Indeed. 
So if I am not wrong, after hitting the debugger statement, the expected scenario would be:
1. Hit the debugger statement 
   |=> Stop at line 1 of test-frame.html
2. Step in
   |=> Stop at line 1 of test-frame.html, on the funcTest() call site
3. Step in
   |=> Stop at line 4 of test.js, on the first intruction of the funcTest function

Is that correct?

And what I get is:
1. Hit the debugger statement 
   |=> Stop at line 1 of test-frame.html
2. Step in
   |=> Stop at line 2 of test.js (the function definition)
3. Step in
   |=> Stop at line 4 of test.js (the first instruction of the function)

Note that at 2., |a|, |b| and |c| are defined, but |arguments| isn't.

Florent

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to fayolle-florent from comment #4)
> Is that correct?

I think so, yes. I'm not quite sure what is the exact bytecode generated for those scripts, so I'm CCing Jim, who knows better.

But aside from pure debugger backend correctness, let's think what kind of behavior we ultimately want here. Stepping inside a function call indicates on the user's part an intent to look inside that function. What would be the value of pausing at the call site in that case instead of the function definition, where both call parameters and function code can be inspected?

From personal experience, identifying the function is easier when looking at the definition, if there are other, similarly-named functions (e.g. makeXHRForUser, makeXHRForAnonUser, makeXHRForAdmin, makeXHRForOwner, etc. - hopefully you get the idea). So even if the behavior seems erratic for someone familiar with how stepping is implemented, I would argue that the user actually gets more value out of it.

Comment 6

[Jim Blandy :jimb]

The first time the debugger pauses, pointing at line 1 of test-frame.html, it is paused at the 'debugger;' statement. So, at least one bug is that the line number for that is wrong.

I would have expected this to be fallout from bug 332176, but the original bug report cites a January beta build, and 332176 only landed on Feb 4th.

Then, the step-into operation is also a bit surprising. The source location as appearing in the protocol packet is line 2, column 4 --- which is in the middle of the "function" keyword. The line is... plausible? but the column seems bogus. However, I don't think it's simply a case of source and numbers being out of sync, since the "Begin" string is logged to the console at the point you'd expect --- when you step over line 4. So Debugger's own source information is attributing something to that surprising position.

In any case, there's at least one genuine bug here, and one bit of confusing behavior.

Comment 7

[Jim Blandy :jimb]

For what it's worth, yes, there are some bytecodes in the function that appear before the first statement --- so there exist bytecode offsets that would naturally correspond to the 'function foo(...) {' line of the source.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #6)
> The first time the debugger pauses, pointing at line 1 of test-frame.html,
> it is paused at the 'debugger;' statement. So, at least one bug is that the
> line number for that is wrong.

Was bug 332176 supposed to fix line/location information for DOM event handlers, too? I assumed that what would make this work as expected would be fixing bug 967769 so that the debugger knows it's a dynamically-added event handler (and can therefore ignore the URL) and using Debugger.Source.prototype.text all over the place to get the source.

Comment 9

[Jim Blandy :jimb]

I think I shouldn't have fingered bug 332176. I believe Firefox compiles event handlers using JS::CompileFunction, which calls frontend::CompileFunctionBody, and thus should not be affected by bug 332176's patches. Bug 332176 should only have affected code passed to the actual 'eval' or 'Function' constructors.

It seems to me that, in the test case, that event handler's code should absolutely be attributed to line 8 of test-frame.html. The fact that it's being compiled as a function body under the hood shouldn't affect that at all.

Comment 10

[Jim Blandy :jimb]

That is, note that if we skip step 3 of comment 0's instructions, hit the 'debugger;' statement at test.js:6, and then look at the stack, the source position is *still* given as test-frame.html:1. Here there's no dynamic evaluation involved, but the code positions are still bogus.

Comment 11

[Tom Tromey :tromey]

I can still reproduce this with today's fx-team, but it works when
the patches for bug 1003554 and bug 1013219 are applied.
I didn't dig any deeper but I imagine it's another case where
some compiler-generated "artificial" instruction appears to take on
a line number, and so is a candidate for stopping.

Comment 12

[Eddy Bruel [:ejpbruel]]

(In reply to fayolle-florent from comment #1)
> Even though the bug might not look important at a first glance, I wonder why
> the Debugger has this bug.
> If you put a breakpoint at line 2 of test.js, the breakpoint is corrected
> and moves to line 4.
> 
> So it might be a deeper problem with the JSD2 internals or the Debugger
> Server...
> 
> Florent

We use a different mechanism for setting breakpoints when breaking on DOM events. In particular, we try to set the breakpoint on the first line of the script that has an entry point. Also see comment 11 by Tom.

Comment 13

[Asher]

This seems to be working on OSX (tested on nightly 7/19/2015), I also tested it on a CentOS linux build with (nightly 7/3/2015).

Can you double check?

Comment 14

[Florent Fayolle]

I still reproduce the test case on Nightly 2015-07-21 (OS: Linux Debian).

Florent

Comment 15

[Asher]

Attachment: Firefox Breakpoint Detection

Comment 16

[Asher]

Attachment: Chrome Breakpoint Detection

Comment 17

[Asher]

Attachment: Firebug Breakpoint Detection

Comment 18

[Asher]

I tested this out in Chrome (43), Firefox (42), Firefug (2) and you get similar results.  

=== Bug ===
1.) Chrome lets you set a breakpoint on line 1 even though there isn't any code there, Firefox and Firebug don't allow this (see screenshot).
2.) Chrome / Firefox / Firebug all allow you to set a breakpoint at the end of the test.js file even though there is a "}" there and nothing else.
3.) KEY: Breakpoints should be allowed at the function() call definition and for sure not on empty lines like Chrome or on a "}".

=== Problem ===
- Breakpoint detection is slightly broken or needs updating.

(see attached screenshot comparison)

Comment 19

[Florent Fayolle]

Sorry, I am not sure about what is your point, nor about what kind of information you need from me?

Florent

Comment 20

[Tom Tromey :tromey]

I looked into this again.

This is the prologue problem identified in bug 863089 comment #13.

The issue is that Debugger.Script.getOffsetLine does not skip the function
prologue, so instructions in the prologue have the line number of the start
of the function -- before the first line of code.

The patch in that bug changes this code to skip the prologue first, so any
prologue instruction will be attributed to the first line of code.

Comment 21

[Tom Tromey :tromey]

I think this is fixed now.