Bug 673125 (onStep)

Implement Debugger.Frame.prototype.onStep

RESOLVED FIXED in mozilla9



JavaScript Engine
6 years ago
6 years ago


(Reporter: jorendorff, Assigned: jimb)


(Blocks: 1 bug)

Other Branch
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [inbound])


(4 attachments, 2 obsolete attachments)



6 years ago
Created attachment 547403 [details] [diff] [review]

This was midstream when we decided to land in m-c.

Here's jimb's initial cut.


6 years ago
Attachment #547403 - Attachment is patch: true

Comment 1

6 years ago
Created attachment 547927 [details] [diff] [review]

Here are some tests. This patch is meant to be qfolded with the onStep patch.

A few of these fail. I haven't tried to diagnose the failures, much:

* onStep doesn't work when the tracejit kicks in
  (Frame-onStep-07.js fails with -j or -m -a -j)

* The interpreter doesn't check for interrupts properly
  (Frame-onStep-05.js fails with no jitflags)
  Others fail too, but 05 is the simplest test for this.

* Recursion seems to break the "stepping is per-frame, not per-script" fiction
  (Frame-onStep-04.js always fails)

* Stepping apparently doesn't cooperate with breakpoints in methodjit?
  (Frame-onStep-08.js fails with -m -a)

I renamed the original test to Frame-onStep-lines-01.js.


6 years ago
Alias: onStep

Comment 2

6 years ago
With the new findReferences shell function, I can write a test that reliably fails because a live onStep handler does not keep the debugger alive. Yay!

Comment 3

6 years ago
Well, not so fast. Of course, JS_TraceRuntime doesn't run the iterative marking loop, so findReferences doesn't work on weak maps or Debugger objects.

Comment 4

6 years ago
The patch series I'm about to attach for this depends somewhat on bug 679136 (already r+) --- "somewhat", because:

a) 679136 removes a perf problem (an extra test in every END_CASE in js::Interpret), and 

b) the patch series here could possibly introduce another one to balance it out (taking the address of jumpTable/switchMask, which would otherwise live in a register).

I argue that a) pays for b). :)
Depends on: 679136

Comment 5

6 years ago
Created attachment 553361 [details] [diff] [review]
Patch 1: Maintain a list of active js::Interrupt frames, their FrameRegs, and their interruptors.

The comment atop InterpreterFrames explains why this is needed, although it
is only used by later patches in the series.
Attachment #547403 - Attachment is obsolete: true
Attachment #547927 - Attachment is obsolete: true
Attachment #553361 - Flags: review?(jorendorff)

Comment 6

6 years ago
Created attachment 553362 [details] [diff] [review]
Patch 2: Have each JSScript maintain both a count and a flag indicating whether it should be in single-step mode.

The rationale is explained in the comments in jsscript.h.
Attachment #553362 - Flags: review?(jorendorff)

Comment 7

6 years ago
Created attachment 553363 [details] [diff] [review]
Patch 3: Provide stack-allocated vectors of JSObject pointers.
Attachment #553363 - Flags: review?(luke)

Comment 8

6 years ago
Created attachment 553364 [details] [diff] [review]
Patch 4: Implement the Debugger.Frame.prototype.onStep accessor.

This includes all the tests Jason attached above, and a few new ones.
Attachment #553364 - Flags: review?(jorendorff)


6 years ago
Attachment #553363 - Attachment description: Provide stack-allocated vectors of JSObject pointers. → Patch 3: Provide stack-allocated vectors of JSObject pointers.

Comment 9

6 years ago
That's the full series. No regressions in js/src/tests or js/src/jit-test.

I've used SunSpider and cachegrind to assess the performance impact of Patch 1, and I've also looked at the machine code generated. If I understand what cachegrind was telling me, random changes in the arrangement of memory (and thus the property cache) have effects that dwarf that of Patch 1.

I'm leaving on vacation for a week; Jason Orendorff has kindly offered to shepherd this patch series in my absence.


6 years ago
Attachment #553363 - Flags: review?(luke) → review+

Comment 10

6 years ago
Comment on attachment 553361 [details] [diff] [review]
Patch 1: Maintain a list of active js::Interrupt frames, their FrameRegs, and their interruptors.

GenericInterruptEnabler seems like a phony abstraction to me, but it makes the code shorter, so ok.
Attachment #553361 - Flags: review?(jorendorff) → review+

Comment 11

6 years ago
Comment on attachment 553362 [details] [diff] [review]
Patch 2: Have each JSScript maintain both a count and a flag indicating whether it should be in single-step mode.

>-/* Turn on single step mode. Requires debug mode. */
>-extern JS_FRIEND_API(JSBool)
>-js_SetSingleStepMode(JSContext *cx, JSScript *script, JSBool singleStep);
>+/* Increment or decrement |script|'s single step count. Requires debug mode. */
>+extern JSBool
>+js_ChangeSingleStepModeCount(JSContext *cx, JSScript *script, int delta);

This can just be dropped from jsdbgapi.h/cpp, and then part 4 can call JSScript::changeStepModeCount directly.

The assertion in js_ChangeSingleStepModeCount can also be moved to the JSScript method.
Attachment #553362 - Flags: review?(jorendorff) → review+


6 years ago
Blocks: 680562


6 years ago
Blocks: 680549

Comment 12

6 years ago
I didn't quite finish this review, and more to the point, onStep doesn't seem to work with the methodjit on my machine. I'll look at it over the weekend.

Comment 13

6 years ago
Back to patch 2 for a moment:

A lot of the JSScript inlines didn't really need to be inline, so I moved them to JSScript.cpp.

recompileForStepMode doesn't need the bool argument; I removed it.

I changed JSScript::tryNewStepMode to walk the InterpreterFrames. That simplifies two things in part 4. First, I think CHECK_INTERRUPT_HANDLER doesn't have to check script->stepModeEnabled(). The interpreter only has to check that when the value of |script| changes (inline calls and returns, and unwinding). Second, DebuggerFrame_setOnStep is much shorter.

Also, there was a bug:
>        uint32 prior = stepMode;
>        /* The compiler checks stepMode, so we need to actually update it now. */
>        stepMode = newValue;
>        if (!stepMode != !newValue && !recompileForStepMode(cx, newValue)) {

The condition on the last line can't be true. It should be
         if (!prior != !newValue && ...

Comment 14

6 years ago
Here's my review of patch 4; however, I obviously missed something because a few tests still crash for me when the methodjit is enabled. :-|

In js.msg:
> MSG_DEF(JSMSG_DEBUG_BAD_LINE,         283, 0, JSEXN_TYPEERR, "invalid line number")
> MSG_DEF(JSMSG_DEBUG_NOT_DEBUGGING,    284, 0, JSEXN_ERR, "can't set breakpoint: script global is not a debuggee")
>-MSG_DEF(JSMSG_DEBUG_COMPARTMENT_MISMATCH, 285, 2, JSEXN_TYPEERR, "{0}: descriptor .{1} property is an object in a different compartment than the target object")
>-MSG_DEF(JSMSG_NOT_CALLABLE_OR_UNDEFINED, 286, 0, JSEXN_TYPEERR, "value is not a function or undefined")
>+MSG_DEF(JSMSG_DEBUG_NOT_SCRIPT_FRAME, 285, 0, JSEXN_ERR, "stack frame is not running JavaScript code")
>+MSG_DEF(JSMSG_DEBUG_COMPARTMENT_MISMATCH, 286, 2, JSEXN_TYPEERR, "{0}: descriptor .{1} property is an object in a different compartment than the target object")
>+MSG_DEF(JSMSG_NOT_CALLABLE_OR_UNDEFINED, 287, 0, JSEXN_TYPEERR, "value is not a function or undefined")

Putting the new entry last reduced the size of the diff a bit.

In jsinterp.cpp:
> #define CHECK_INTERRUPT_HANDLER()                                           \
>     JS_BEGIN_MACRO                                                          \
>-        if (cx->debugHooks->interruptHook)                                  \
>+        if (cx->debugHooks->interruptHook || script->stepModeEnabled())     \
>             ENABLE_INTERRUPTS();                                            \

I made changes so that CHECK_INTERRUPT_HANDLER does not have to check
for script->stepModeEnabled(), per comment 13 about

But we do have to check script->stepModeEnabled() each time |script|
changes, so I implemented that.

Now that we're taking the address of jumpTable/switchMask, maybe we
should use the new mechanism everywhere and remove
CHECK_INTERRUPT_HANDLER. Filed bug 680562 for that.

In methodjit/StubCalls.cpp, stubs::Trap:
>+        if (trapTypes & JSTRAP_TRAP)
>+            result = Debugger::onTrap(f.cx, &rval);
>+        /* If we hit a breakpoint here, don't also report a single-step. */
>+        else if (trapTypes & JSTRAP_SINGLESTEP)
>+            result = Debugger::onSingleStep(f.cx, &rval);

This differed from the interpreter behavior. The interpreter reported both.

For now I changed the methodjit to behave like the interpreter, because
that makes Frame-onStep-08.js pass, but we should consider the
alternatives. Filed bug 680660 for that.

In Debugger.cpp:
>+#ifdef DEBUG
>+    /*
>+     * Validate the single-step count on this frame's script, to ensure that
>+     * we're not receiving traps we didn't ask for. Even when frames is
>+     * non-empty (and thus we know this trap was requested), do the check
>+     * anyway, to make sure the count has the correct non-zero value.
>+     *
>+     * The converse --- ensuring that we do receive traps when we should --- can
>+     * be done with unit tests.
>+     */
>+    if (GlobalObject::DebuggerVector *debuggers = global->getDebuggers()) {
>+        JSScript *trappingScript = fp->script();
>+        uint32 stepperCount = 0;
>+        for (Debugger **p = debuggers->begin(); p != debuggers->end(); p++) {
>+            Debugger *dbg = *p;
>+            for (FrameMap::Range r = dbg->frames.all(); !r.empty(); r.popFront()) {
>+                StackFrame *frame = r.front().key;
>+                JSObject *debuggerFrame = r.front().value;
>+                if (frame->isScriptFrame() &&
>+                    frame->script() == trappingScript &&
>+                    !debuggerFrame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined())
>+                    stepperCount++;
>+            }
>+        }
>+        JS_ASSERT(stepperCount == trappingScript->stepModeCount());
>+    }

For now, I think we still have the situation where a single
(non-compile-n-go) script can be run against multiple different
globals. This code only searches for debuggers debugging `global`, which
comes from the stack frame's scope chain.

I weakened the assertion to account for this. It now only expects an
exact match when the script is compileAndGo.

>-     * Mark Debugger.Frame objects that are reachable from JS if we look them up
>-     * again (because the corresponding StackFrame is still on the stack).
>+     * Mark Debugger.Frame objects. These are all reachable from JS, because the
>+     * corresponding StackFrames are still on the stack.
>+     *
>+     * (Once we support generator frames properly, we will need
>+     * weakly-referenced Debugger.Frame objects as well, for suspended generator
>+     * frames.)

Much better. Thanks.

>+static JSBool
>+DebuggerFrame_getOnStep(JSContext *cx, uintN argc, Value *vp)
>+    THIS_FRAME(cx, argc, vp, "get onStep", args, thisobj, fp);

This throws if the frame is no longer on the stack. Same thing in
DebuggerFrame_setOnStep. In combination with slowPathOnLeaveFrame not
clearing the onStep handler, this means that once the frame exits, it
has a reference to the onStep handler that the program can't get or
clear. It's not exactly a leak, since debuggers ordinarily won't keep
Frames alive after they leave the stack. Just not ideal. I filed
follow-up bug 680549 to fix this.

In DebuggerFrame_setOnStep:
>+    if (!IsValidHook(vp[2])) {
>+        js_ReportIsNotFunction(cx, &vp[2], 0);
>+        return false;
>+    }

This should probably be JSMSG_NOT_CALLABLE_OR_UNDEFINED. I changed it.

I also changed code that used vp to use args instead.
For example, vp[2] --> args[0].

The rest is tests.

>+++ b/js/src/jit-test/lib/findReferences.js

I removed this since it's unused so far.

In Frame-onStep-04.js:
>+dbg.onDebuggerStatement = function (frame) {
>+    // This is called with 8 nested frames on the stack.
>+ [...]

"nested" here is kind of embarrassing, so I changed the comment to say
    // This is called with 8 call frames on the stack, 7 down to 0.
and then changed g.f(6) to g.f(7) to make that true.

In Frame-onStep-06.js:
>+g.log = '';
>+g.eval("'' + x;\n" +
>+       "log += 'y';\n");
>+assertEq(g.log, 'dxsy');
>+g.log = '';
>+g.eval("0 + x;\n" +
>+       "log += 'y';\n");
>+assertEq(g.log, 'dxsy');
>+g.log = '';
>+g.eval("0 - x;\n" +
>+       "log += 'y';\n");
>+assertEq(g.log, 'dxsy');
>+g.log = '';
>+g.eval("0 * x;\n" +
>+       "log += 'y';\n");
>+assertEq(g.log, 'dxsy');

Factoring out some common code made this test a lot clearer.

In Frame-onStep-10.js:
>+stepped = false;
>+g.eval("try { f(); }                            \n" +
>+       "catch (x) { assertEq(x, 'mud'); };      \n");
>+assertEq(stepped, true);

This would pass if the catch-block didn't execute. I changed it to check.
Of course we pass the tougher test just fine.

In Frame-onStep-lines-01.js:
>+var d = new Debugger(g);

I renamed it to dbg, just for consistency's sake.

Also changed indentation to 4 spaces and changed tabs to spaces everywhere.

In Frame-onStep-resumption-04.js:
>\ No newline at end of file

I added one.

Comment 15

6 years ago
Here is a test case that crashes for me with -m -a. I'm not sure how single-step mode is causing this, because the onStep hook is never even called, but we crash in ic::SetGlobalName trying to assign to |caught|. We're looking up the atom in the wrong script, because the StackFrame for f() hasn't been removed from the stack; f.regs.fp() returns that frame.

var g = newGlobal('new-compartment');
var dbg = Debugger(g);
dbg.onEnterFrame = function (frame) {
    if (frame.type === 'eval')
	frame.onStep = function () {};

g.eval("function f() { throw 0; }");
g.e = 1;
g.eval("try { f(); }\n" +
       "catch (x) { e = 0; }\n");

Comment 16

6 years ago
Looks like the patch is innocent. I was able to reproduce the assertion in comment 15 without the patch, in fact without jsdbg2, using jsdbgapi.h in a jsapi-test. Filed bug 680684.
Depends on: 680684


6 years ago
Attachment #553364 - Flags: review?(jorendorff) → review+

Comment 17

6 years ago
Whiteboard: [inbound]

Comment 18

6 years ago
Backed out. There's a compiler warning and a few test failures; in particular one of the tests had a useless dis() call.
Whiteboard: [inbound]

Comment 19

6 years ago
I'm getting a crash in Frame-onStep-06.js that didn't appear on the buildbots. Valgrind shows it reliably (for me on Mac). It looks like js::mjit::Recompiler::recompile() is failing to patch something, because we return to code that uses an IC that we freed.

// Reduced from Frame-onStep-06.js.
var g = newGlobal('new-compartment');
g.eval("var y, x = {toString: function () { debugger; return '1'; }};");
g.eval("'' + x;");
g.eval("var a = {get f() { debugger; y = 0; return function() { y = 0; }; }}, b = {set x(v) { debugger; return y = 0; }}; ");
var dbg = Debugger(g);
dbg.onDebuggerStatement = function (frame) {
    frame.older.onStep = function () {
g.eval("b.x = 1;");

Invalid read of size 8
   at 0x1002F60DE: js::mjit::ic::CallProp(js::VMFram…) (PolyIC.cpp:1902)
   by 0x68AC500: ???
   by 0x100298327: js::mjit::EnterMethodJIT(JSContext*, j…) (MethodJIT.cpp:687)
   by 0x100298436: CheckStackAndEnterMethodJIT(JSContext*…) (MethodJIT.cpp:717)
   by 0x100298549: js::mjit::JaegerShot(JSContext*) (MethodJIT.cpp:734)
   by 0x1000E8BCA: js::RunScript(JSContext*, JSScript*, js…) (jsinterp.cpp:611)
   by 0x1000E8E2E: js::ExecuteKernel(JSContext*, JSScript*…) (jsinterp.cpp:911)
   by 0x10010DCC6: EvalKernel(JSContext*, js::CallArgs const…) (jsobj.cpp:1278)
   by 0x10010DEE3: eval(JSContext*, unsigned int, js::Value*) (jsobj.cpp:1320)
   by 0x10030C6C5: js::CallJSNative(JSContext*, int (*…) (jscntxtinlines.h:281)
   by 0x1000E93B2: js::InvokeKernel(JSContext*, js::CallAr…) (jsinterp.cpp:657)
   by 0x1000662F7: js::Invoke(JSContext*, js::InvokeArgsGuar…) (jsinterp.h:163)
 Address 0x100ab2c50 is 272 bytes inside a block of size 364 free'd
   at 0xDCDC: free (vg_replace_malloc.c:320)
   by 0x1000FCD92: js_free (jsutil.h:262)
   by 0x100113346: js::Foreground::free_(void*) (jsutil.h:504)
   by 0x1001A8670: JSRuntime::free_(void*) (jscntxt.h:753)
   by 0x1001A8694: JSContext::free_(void*) (jscntxt.h:1277)
   by 0x100298065: js::mjit::ReleaseScriptCode(JSContext*…) (MethodJIT.cpp:934)
   by 0x10030D984: js::mjit::Recompiler::recompile() (Retcon.cpp:182)
   by 0x10018DCF2: JSScript::recompileForStepMode(JSConte…) (jsscript.cpp:1841)
   by 0x10018DD4F: JSScript::tryNewStepMode(JSContext*, u…) (jsscript.cpp:1855)
   by 0x10018DE69: JSScript::changeStepModeCount(JSContex…) (jsscript.cpp:1886)
   by 0x1001EFA0B: DebuggerFrame_setOnStep(JSContext*, un…) (Debugger.cpp:2812)
   by 0x10030C6C5: js::CallJSNative(JSContext*, int (*…) (jscntxtinlines.h:281)

I checked that recompile is in fact freeing this memory. Still looking.

Comment 20

6 years ago
This might be related to bug 632343. Cc-ing luke and adrake.


6 years ago
Blocks: 681672
The issue with discarding a script's jitcode (and IC structures) while an ic:: stack frame is manipulating that IC should be fixed after landing TI.  With TI the ic:: functions need to monitor for recompilations triggered by VM operations and then back out, finishing the property access / etc. that triggered the ic:: call but not touching the IC itself anymore.

Comment 22

6 years ago
(In reply to Brian Hackett from comment #21)

Yep. Filed bug 681672 which might or might not be the bug for that.

Comment 23

6 years ago
Explanation of the bug described in comment 19:

On the last line of the script,
the stack looks like this (most recent call on top):

   <jitcode for 'function (frame) { frame.older.onStep = function () {...}; }'>
   <jitcode for 'get f() { debugger; y = 0; return function() { y = 0; }; }'>
   <jitcode for 'a.f()'>

The recompiler patches the return to a.f(), but it doesn't patch the PICInfo *pic pointer passed to ic::CallProp(). We eventually return to CallProp, which asks for pic->atom and crashes (or gets NULL or something).

I'm going to land onStep anyway, with this particular test commented out. We can't keep delaying onStep until the underlying implementation is perfect--single-step mode is already exposed to chrome; it's just not well-tested. Without onStep, tests for this stuff have to be either jsapi-tests or chrome mochitests. With onStep, they're ordinary jit-tests or js-tests; langfuzz can operate on them.

Once TI lands (this weekend), we'll fix this particular issue in bug 681672.

Comment 24

6 years ago
Got a green Try Server run, landing again.



6 years ago
Whiteboard: [inbound]
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.