609687 - crash jsd single stepping [@ jsd_FunctionCallHook]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[John J. Barton]

1) Install Firebug from:
http://www.softwareishard.com/temp/firebug-1.7X.0a4.608763.xpi
2) Load page: https://getfirebug.com/tests/content/script/singleStepping/index.html

3) Enable the Script panel, reload the page
4) Create breakpoint on line 14
5) Press the button on the page
crash

http://crash-stats.mozilla.com/report/index/bp-8e9476ab-d01a-4c4c-9a2c-8418d2101104
http://crash-stats.mozilla.com/report/index/bp-431e9f45-79be-43fa-9465-003832101104

Comment 1

[John J. Barton]

            Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101104 Firefox/4.0b8pre

Comment 2

[John J. Barton]

Hmm. I also have Chromebug and FBTest running

Comment 3

[John J. Barton]

With just FBTrace and Firebug I don't crash

Comment 4

[Steve Fink [:sfink] [:s:]]

I am seeing this too. I am only using Firebug1.7, no Chromebug or FBTest.

What I see happening, going backwards:

 - jsd_FunctionCallHook is passed a NULL 'closure' arg.

 - the interpreter's ScriptEpilogue invokes jsd_FunctionCallHook with this guard:

    if (hook && fp->hasHookData() && !fp->isExecuteFrame())
        hook(cx, fp, JS_FALSE, &ok, fp->hookData());

 - hasHookData() is testing the frame's JSFRAME_HAS_HOOK_DATA flag, not the hook data itself (which is why that guard passes it)

 - earlier, setHookData is called to set a NULL hookData_

 - the NULL comes from the interpreter's ScriptProlog:

    JSInterpreterHook hook = cx->debugHooks->callHook;
    if (JS_UNLIKELY(hook != NULL) && !fp->isExecuteFrame())
        fp->setHookData(hook(cx, fp, JS_TRUE, 0, cx->debugHooks->callHookData));

 - all of this is triggered when I set a breakpoint on a for loop and then single-step.

The intent here is that the entry hook gets to decide whether the exit hook is called. Which suggests to me that the unconditional call to fp->setHookData is wrong. I'll chase it back through hg history, but hopefully someone knowledgeable will chip in.

Comment 5

[timeless]

Someone broke the js api design.

http://mxr.mozilla.org/mozilla1.8/source/js/src/jsinterp.c#1367

1365     /* call the hook if present */
1366     if (hook && (native || script))
1367         hookData = hook(cx, &frame, JS_TRUE, 0, cx->runtime->callHookData);

1369     /* Call the function, either a native method or an interpreted script. ....
1413 out:
1414     if (hookData) {
1415         hook = cx->runtime->callHook;
1416         if (hook)
1417             hook(cx, &frame, JS_FALSE, &ok, hookData);
1418     }

The design is such that the return from hook() is either a closure or null. If it's null, then the hook on the other side is *not* called.

Comment 6

[timeless]

Attachment: restore jsapi behavior antebellum

Comment 7

[Steve Fink [:sfink] [:s:]]

An alternative would be to make setHookData clear the FRAME_HAS_HOOK_DATA flag when NULL is passed in, and leave everything else alone.

I'm not sure why the flag exists, if it's purely redundant with !!hookData_. I guess it's just mimicking several other fields, and for some of those, NULL must be a valid value?

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 490038 [details] [diff] [review]
restore jsapi behavior antebellum

Luke, would you be willing to review and follow up on the frame flag issue Steve pointed out? Thanks,

/be

Comment 9

[Luke Wagner [:luke]]

(In reply to comment #7)
The flag allows us to leave hookData_ uninitialized when pushing a stack frame, as part of the general "make calls fast" effort.

Comment 10

[timeless]

Attachment: for commit

luke/et al: please land according to whatever process is being used

I'm at a conference for the week...

Comment 11

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/22273789b140

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/22273789b140