475334 - JavaScript debugger points to wrong script for calls of global functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wladimir Palant]

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.

Comment 1

[Wladimir Palant]

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).

Comment 2

[:Gijs (back August 25; he/him)]

It would be helpful to know what you tested this on. Fx 2.0? 3.0? 3.1b2? Something else? Did it work before?

Comment 3

[Wladimir Palant]

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...

Comment 4

[Wladimir Palant]

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()

Comment 5

[Wladimir Palant]

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.

Comment 6

[:Gijs (back August 25; he/him)]

(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.

Comment 7

[:Gijs (back August 25; he/him)]

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).

Comment 8

[Wladimir Palant]

(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...

Comment 9

[Wladimir Palant]

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.

Comment 10

[Wladimir Palant]

Attachment: 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.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 361838 [details] [diff] [review]
Proposed patch

Good catch!

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/bd9cf8703375

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

I think we want this for 1.9.1 to help out Firebug and the JavaScript deobfuscator.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 361838 [details] [diff] [review]
Proposed patch

This is a safe patch that only affects debuggers.

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Wladimir Palant]

Attachment: 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).

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bd9cf8703375

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 361949 [details] [diff] [review]
Same patch for 1.9 branch

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 19

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7675d1edc19e

Comment 20

[Bob Clary [:bc] (inactive)]

Wladimir, mrbkap: Can we land this on 1.9.0 ?

Comment 21

[Wladimir Palant]

Yes, the patch got approval for 1.9.0.8 - but I cannot land it myself.

Comment 22

[Wladimir Palant]

Attachment: 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.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.9.0 branch.

Comment 24

[Bob Clary [:bc] (inactive)]

Wladimir, can you confirm this is fixed in the most recent 1.9.0 nightly?

Comment 25

[Wladimir Palant]

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.