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)

29 Branch
x86_64
Linux
defect

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
Component: Untriaged → Developer Tools: Debugger
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
Panos, do you know what could be the problem here? Honza
Flags: needinfo?(past)
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)
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)
(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)
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.
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.
(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.
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.
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.
Priority: -- → P3
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.
(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.
Blocks: dbg-dom
No longer blocks: dbg-breakpoint
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)
I still reproduce the test case on Nightly 2015-07-21 (OS: Linux Debian). Florent
Flags: needinfo?(fayolle-florent)
Attached image Firefox Breakpoint Detection (deleted) —
Attached image Chrome Breakpoint Detection (deleted) —
Attached image Firebug Breakpoint Detection (deleted) —
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)
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)
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
I think this is fixed now.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
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: