Closed
Bug 969816
Opened 11 years ago
Closed 9 years ago
Stepping in when breaking in an inline event halts at function declaration
Categories
(DevTools :: Debugger, defect, P3)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 863089
People
(Reporter: fayolle-florent, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
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
Reporter | ||
Updated•11 years ago
|
Component: Untriaged → Developer Tools: Debugger
Reporter | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
Panos, do you know what could be the problem here?
Honza
Flags: needinfo?(past)
Comment 3•11 years ago
|
||
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.
Flags: needinfo?(past)
Reporter | ||
Comment 4•11 years ago
|
||
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
Flags: needinfo?(past)
Comment 5•11 years ago
|
||
(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.
Flags: needinfo?(past)
Comment 6•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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.
Updated•11 years ago
|
Priority: -- → P3
Updated•10 years ago
|
Blocks: dbg-breakpoint
Comment 11•10 years ago
|
||
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•10 years ago
|
||
(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.
Updated•10 years ago
|
Comment 13•9 years ago
|
||
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?
Flags: needinfo?(fayolle-florent)
Reporter | ||
Comment 14•9 years ago
|
||
I still reproduce the test case on Nightly 2015-07-21 (OS: Linux Debian).
Florent
Flags: needinfo?(fayolle-florent)
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
Comment 17•9 years ago
|
||
Comment 18•9 years ago
|
||
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)
Flags: needinfo?(fayolle-florent)
Reporter | ||
Comment 19•9 years ago
|
||
Sorry, I am not sure about what is your point, nor about what kind of information you need from me?
Florent
Flags: needinfo?(fayolle-florent)
Comment 20•9 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 21•9 years ago
|
||
I think this is fixed now.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•