JavaScript debugger points to wrong script for calls of global functions

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Wladimir Palant, Assigned: Wladimir Palant)

Tracking

({fixed1.9.0.9, fixed1.9.1})

Trunk
fixed1.9.0.9, fixed1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 attachments)

(Assignee)

Description

10 years ago
Noticed an issue while testing JavaScript Deobfuscator extension. I register a hook as jsdIDebuggerService.functionHook and jsdIDebuggerService.topLevelHook, then I run this code:

    var foo = 0;
    bar();

    function bar()
    {
      foo = 1;
    }

As a result I get two calls on the hook, both referring to this script (with different PC values):

    function bar() {
        foo = 1;
    }

    var foo = 0;
    bar();

    function bar() {
        foo = 1;
    }

Script hook however gets another script as well, that script should have been referenced in the second call:

    function bar() {
        foo = 1;
    }

Looking through the source code, I think the issue is jsd_NewThreadState() function in jsd_stak.c, in particular this code:

        /*
         * don't construct a JSDStackFrame for dummy frames (those without a
         * |this| object, or native frames, if JSD_INCLUDE_NATIVE_FRAMES
         * isn't set.
         */
        if (JS_GetFrameThis(cx, fp) &&

Confirmed that replacing "bar()" by "bar.apply(window)" makes the issue go away.
(Assignee)

Comment 1

10 years ago
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)

Updated

10 years ago
Summary: JavaScript debugger skips valid frames in thread state construction → JavaScript debugger points to wrong script for calls of global functions
(Assignee)

Updated

10 years ago
Assignee: general → rginda
Component: JavaScript Engine → JavaScript Debugger
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?
(Assignee)

Comment 3

9 years ago
I am testing it on 1.9.2a1pre build 20090210 right now, results were the same for 1.9.0.6 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...
(Assignee)

Comment 4

9 years ago
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()
(Assignee)

Comment 5

9 years ago
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).
(Assignee)

Comment 8

9 years ago
(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 2.0.0.14 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
Component: JavaScript Debugger → JavaScript Engine
Product: Other Applications → Core
QA Contact: venkman → general
(Assignee)

Comment 9

9 years ago
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)

Comment 10

9 years ago
Created attachment 361838 [details] [diff] [review]
Proposed patch

It seems that the JavaScript debugger expects the call hook to be called after cx->fp is set to the new frame. Otherwise it will pass an incorrect frame to the function hook that points to the function caller rather than the function itself. Easiest solution is moving the hook call slightly further down (this also fixes the current issue that the hook might be called but execution will continue to bad_inline_call and no call will happen). I am only unsure what CHECK_INTERRUPT_HANDLER() is doing here and whether moving it will have an effect on anything - at least Venkman still seems to step through the functions as before.
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+
http://hg.mozilla.org/tracemonkey/rev/bd9cf8703375
Whiteboard: fixed-in-tracemonkey
I think we want this for 1.9.1 to help out Firebug and the JavaScript deobfuscator.
Flags: wanted1.9.1?
Comment on attachment 361838 [details] [diff] [review]
Proposed patch

This is a safe patch that only affects debuggers.
Attachment #361838 - Flags: approval1.9.1?

Updated

9 years ago
Flags: wanted1.9.1? → wanted1.9.1+
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
(Assignee)

Comment 16

9 years ago
Created attachment 361949 [details] [diff] [review]
Same patch for 1.9 branch

I think it is too late for 1.9.0.7 but maybe this can be taken for 1.9.0.8. It is a pretty simple and safe patch, the code in question affects only debuggers. This will allow JavaScript Deobfuscator extension to rely on call hooks rather than use an execution hook "just in case" (with rather severe implications for the performance, not to mention that you cannot measure execution time of a function without reliable call hooks).
Attachment #361949 - Flags: approval1.9.0.8?

Comment 17

9 years ago
http://hg.mozilla.org/mozilla-central/rev/bd9cf8703375
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Comment on attachment 361949 [details] [diff] [review]
Same patch for 1.9 branch

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #361949 - Flags: approval1.9.0.8? → approval1.9.0.8+
Wladimir, mrbkap: Can we land this on 1.9.0 ?
(Assignee)

Comment 21

9 years ago
Yes, the patch got approval for 1.9.0.8 - but I cannot land it myself.
(Assignee)

Comment 22

9 years ago
Created attachment 364508 [details]
xpcshell testcase (not working)

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.
Keywords: fixed1.9.0.8
Wladimir, can you confirm this is fixed in the most recent 1.9.0 nightly?
(Assignee)

Comment 25

9 years ago
Sorry about the late response. I removed the hack from JavaScript Deobfuscator and tried it in Firefox 3.0.10 - I can still see all the functions that are being called, the issue is gone.

Updated

9 years ago
Attachment #361838 - Flags: approval1.9.1?
You need to log in before you can comment on or make changes to this bug.