Closed Bug 475334 Opened 12 years ago Closed 12 years ago
Script debugger points to wrong script for calls of global functions
Looks like my guess was wrong, removing that code didn't change anything - and the stack frames I get in the function hook seem to be different after all (different PC values).
Assignee: general → rginda
Product: Core → Other Applications
QA Contact: general → venkman
It would be helpful to know what you tested this on. Fx 2.0? 3.0? 3.1b2? Something else? Did it work before?
I am testing it on 1.9.2a1pre build 20090210 right now, results were the same for 126.96.36.199 however. I see now that I wasn't quite right. For the call to bar() the function hook gets called twice. First call with TYPE_FUNCTION_CALL shows the wrong script. The second call with TYPE_FUNCTION_RETURN shows the bar() function correctly however. Will investigate further...
What I see are the following four calls of the hooks: type = TYPE_TOPLEVEL_START frame.script.tag = 7 frame.pc = 69151272 frame.script.functionSource = function bar() .. var foo .. bar() .. function bar() ------------------------------ type = TYPE_FUNCTION_CALL frame.script.tag = 7 frame.pc = 14 frame.script.functionSource = function bar() .. var foo .. bar() .. function bar() ------------------------------ type = TYPE_FUNCTION_RETURN frame.script.tag = 8 frame.pc = 8 frame.script.functionSource = function bar() ------------------------------ type = TYPE_TOPLEVEL_END frame.script.tag = 7 frame.pc = 69151272 frame.script.functionSource = function bar() .. var foo .. bar() .. function bar()
It seems that I found the culprit. In jsinterp.cpp cx->debugHooks->callHook is being called before the frame is fully initialized (inline call case). Moving that hook call slightly further down seems to fix the issue.
(In reply to comment #5) > It seems that I found the culprit. In jsinterp.cpp cx->debugHooks->callHook is > being called before the frame is fully initialized (inline call case). Moving > that hook call slightly further down seems to fix the issue. OK. Sounds like a JS engine issue. Are you able to read the HG blame pages? Because I can't figure out how to get from there to a bug number easily, and am therefore not sure where exactly this went wrong (nor how far forward this call could be moved sanely). Anyway, this bug should move to JS core, and presumably it's a regression in one way or other, assuming your analysis is correct.
Also, as a sidenote: it would be really cool if we could start doing JSD unit tests with the kind of simplified testcase you did in comment #0. It's not usually this simple (though obviously I am not sure exactly how much effort it took you to get the test case as simple as it is now) - but it would help if these kinds of regressions caused by JS changes would be caught (assuming it did, at some point, work correctly).
(In reply to comment #6) > OK. Sounds like a JS engine issue. Yes, seems to be a JS engine issue. However, I could just verify the same issue in Firefox 188.8.131.52 so even if it is a regression - it isn't a recent one. As to testcases: an xpcshell testcase for this issue should be easy enough...
Assignee: rginda → general
Product: Other Applications → Core
QA Contact: venkman → general
xpcshell testcase runs fine - but the debugger doesn't "see" its scripts of course. This will have to wait until I figure that one out.
Assignee: general → trev.moz
Status: NEW → ASSIGNED
Attachment #361838 - Flags: review?(mrbkap)
Comment on attachment 361838 [details] [diff] [review] Proposed patch Good catch!
Attachment #361838 - Flags: review?(mrbkap) → review+
Comment on attachment 361838 [details] [diff] [review] Proposed patch This is a safe patch that only affects debuggers.
Way to go, Trev -- I should have seen this (and may have at one point based on a nagging memory, but I obviously forgot to file a bug on it). My apologies for the long-standing inconsistency. /be
Attachment #361949 - Flags: approval184.108.40.206?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 361949 [details] [diff] [review] Same patch for 1.9 branch Approved for 220.127.116.11, a=dveditz for release-drivers
Attachment #361949 - Flags: approval18.104.22.168? → approval22.214.171.124+
Wladimir, mrbkap: Can we land this on 1.9.0 ?
Yes, the patch got approval for 126.96.36.199 - but I cannot land it myself.
Btw, I'm attaching my (dysfunctional) testcase so that it isn't lost. Function hook isn't being called however, I'm unsure why that is.
Fixed on the 1.9.0 branch.
Wladimir, can you confirm this is fixed in the most recent 1.9.0 nightly?
You need to log in before you can comment on or make changes to this bug.