687683 - Debugger ignores Debugger.prototype.onEnterFrame hook's resumption value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Attachment: Tests for onEnterFrame hooks' resumption values.

When SpiderMonkey calls a debugger's onEnterFrame hook, it ignores the resumption value that hook returns.

Comment 1

[Jim Blandy :jimb]

Okay, I have something that works for me; submitted to the try server:
https://tbpl.mozilla.org/?tree=Try&rev=ca380da97a65

Comment 2

[Jim Blandy :jimb]

Attachment: Remove unnecessary 'js::' namespace prefixes from Debugger code.

Comment 3

[Jim Blandy :jimb]

Attachment: Separate ScriptDebugPrologue add ScriptDebugEpilogue from and ScriptPrologue and ScriptEpilogue.

At the moment, ScriptDebugPrologue is called (conditionally) from within
ScriptPrologue. For onEnterFrame handlers to be able to return a resumption
value, we need ScriptDebugPrologue to return a JSTrapStatus value, but it
is (non-debug) ScriptPrologue's callers that would need to handle those
values.

It seems strange to have ScriptPrologue return a JSTrapStatus. So this
patch brings ScriptDebugPrologue out of ScriptPrologue (and
ScriptPrologueOrGeneratorResume), and has ScriptPrologue's callers call
ScriptDebugPrologue explicitly.

For symmetry, we do the same with ScriptEpilogue,
ScriptEpilogueOrGeneratorYield, and ScriptDebugEpilogue.

Actually adding and processing the JSTrapStatus values comes in a later
patch. This is just meant to be a behavior-preserving rearrangement.

Comment 4

[Jim Blandy :jimb]

Attachment: Test that onEnterFrame hooks' resumption values are respected.

Comment 5

[Jim Blandy :jimb]

Attachment: Some random comment fixes I came across.

Comment 6

[Jim Blandy :jimb]

Attachment: Assert that REJOIN_CHECK_ARGUMENTS and REJOIN_FUNCTION_PROLOGUE are never generated in debug mode.

Explanation in the comments.

Fixing bug 699196 properly entails replacing the assertion this patch adds
with real code to call ScriptDebugPrologue. If this patch lands, hopefully
the person fixing 669196 will notice that and write tests that reach the
assertion/new code, but just to make sure we should probably add a note to
that bug about it.

Comment 7

[Jim Blandy :jimb]

Attachment: Collect resumption values from onEnterFrame handlers, and respect them.

This patch makes SpiderMonkey respect resumption values returned by
Debugger onEnterFrame handler functions, as documented.

In Debugger, we change fireEnterFrame to collect a resumption value from
the hook, and change onEnterFrame and slowPathOnEnterFrame to propagate
them out. These now need an 'rval' argument, so that they can return forced
return values and exceptions.

ScriptDebugPrologue now accepts a JSTrapStatus from Debugger::onEnterFrame,
takes care of placing the return value or exception where it belongs, and
returns a JSTrapStatus.

Calls to ScriptDebugPrologue now handle the JSTrapStatus:
- at the head of js::Interpret;
- in the JSOP_NEW/JSOP_CALL/JSOP_FUNCALL/JSOP_FUNAPPLY case;
- in stubs::ScriptDebugPrologue, which JM epilogues call; and
- in the REJOIN_THIS_PROTOTYPE case in js_InternalInterpret (you must add a
  JS_GC call to ic::GetPropNoCache for Debugger-onEnterFrame-resumption-05.js to
  hit this reliably).

We also rearrange the js_InternalThrow JSTrapStatus-handling switch statement to
have an explicit default case that raises a JS_NOT_REACHED assertion, instead of
just omitting JSTRAP_CONTINUE.

Comment 8

[Jim Blandy :jimb]

Unfortunately, Jason's on vacation until November 14th.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 572243 [details] [diff] [review]
Remove unnecessary 'js::' namespace prefixes from Debugger code.

Fine with me if it passes all the compilers we care about. I admit I'm a little fuzzy on the rules around "friend class foo;".

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 572245 [details] [diff] [review]
Test that onEnterFrame hooks' resumption values are respected.

Review of attachment 572245 [details] [diff] [review]:
-----------------------------------------------------------------

Why are the tests separate from the patch changing the behavior? It seems bad to commit tests either before or after the changeset that makes them pass.

::: js/src/jit-test/tests/debug/Debugger-onEnterFrame-resumption-01.js
@@ +24,5 @@
> +// Call g.f as a constructor.
> +var r = new g.f;
> +assertEq(typeof r, "object");
> +assertEq(savedFrame.live, false);
> +assertEq(g.set, false);

Consider setting ‘savedFrame = undefined;’ before this. Same thing in all the other tests where savedFrame is checked in multiple "runs".

@@ +34,5 @@
> +        savedFrame = frame;
> +        return {return: "pass"};
> +    }
> +    else
> +        return void 0;

Follow the house rules on else-after-return, and ‘return undefined;’ instead of ‘return void 0;’.

(undefined is a non-writable, non-configurable property. It's OK to use it.)

::: js/src/jit-test/tests/debug/Debugger-onEnterFrame-resumption-03.js
@@ +1,1 @@
> +// If debugger.onEnterFrame returns {throw:val}, an exception is thrown in the debuggee.

Same comment as 02. It should say "If debugger.onEnterFrame returns null, ..."

::: js/src/jit-test/tests/debug/Debugger-onEnterFrame-resumption-04.js
@@ +6,5 @@
> +var savedFrame;
> +dbg.onEnterFrame = function (frame) {
> +    hits++;
> +    savedFrame = frame;
> +    return void 0;

"undefined" rather than "void 0" please.

::: js/src/jit-test/tests/debug/Debugger-onEnterFrame-resumption-05.js
@@ +5,5 @@
> +// points for JIT code (arityCheckEntry, argsCheckEntry, fastEntry); debug
> +// mode uses only invokeEntry. This means most of the bytecode tails in
> +// js_InternalInterpret that might call ScriptPrologue or ScriptEpilogue are
> +// unreachable in debug mode: they're only called from the out-of-line entry
> +// points.

Does this mean that there are paths in js_InternalInterpret where we do not call DebugScriptPrologue/Epilogue because we know that we can't get there in debug mode? Do those cases JS_ASSERT that we're not in debug mode?

@@ +8,5 @@
> +// unreachable in debug mode: they're only called from the out-of-line entry
> +// points.
> +//
> +// The exception is REJOIN_THIS_PROTOTYPE, which can be reached reliably if you
> +// add a JS_GC call to stubs::GetPropNoCache. JIT code calls that stub to

Does this mean the test is ineffective or unreliable unless you hack a JS_GC call into the source? If so, should this test call gczeal()?

@@ +31,5 @@
> +    return undefined;
> +};
> +hits = 0;
> +debuggee.hits = 0;
> +assertEq(typeof debuggee.eval("function f(){ hits++; }; f.prototype = {}; new f;"), "object");

The semicolon after function f is extraneous. (Also two other times in this file.)

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 572249 [details] [diff] [review]
Collect resumption values from onEnterFrame handlers, and respect them.

Review of attachment 572249 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

Comment 12

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> Why are the tests separate from the patch changing the behavior? It seems
> bad to commit tests either before or after the changeset that makes them
> pass.

I think I had them separate so I could ensure that some changes were detected by the tests. But you're certainly right, they should be combined. Done.

> ::: js/src/jit-test/tests/debug/Debugger-onEnterFrame-resumption-01.js
> @@ +24,5 @@
> > +// Call g.f as a constructor.
> > +var r = new g.f;
> > +assertEq(typeof r, "object");
> > +assertEq(savedFrame.live, false);
> > +assertEq(g.set, false);
> 
> Consider setting ‘savedFrame = undefined;’ before this. Same thing in all
> the other tests where savedFrame is checked in multiple "runs".

Done.

> @@ +34,5 @@
> > +        savedFrame = frame;
> > +        return {return: "pass"};
> > +    }
> > +    else
> > +        return void 0;
> 
> Follow the house rules on else-after-return,

Done.

> and ‘return undefined;’ instead
> of ‘return void 0;’.
> 
> (undefined is a non-writable, non-configurable property. It's OK to use it.)

Done here and elsewhere.

> ::: js/src/jit-test/tests/debug/Debugger-onEnterFrame-resumption-03.js
> @@ +1,1 @@
> > +// If debugger.onEnterFrame returns {throw:val}, an exception is thrown in the debuggee.
> 
> Same comment as 02. It should say "If debugger.onEnterFrame returns null,
> ..."

Fixed, thanks.

> ::: js/src/jit-test/tests/debug/Debugger-onEnterFrame-resumption-05.js
> @@ +5,5 @@
> > +// points for JIT code (arityCheckEntry, argsCheckEntry, fastEntry); debug
> > +// mode uses only invokeEntry. This means most of the bytecode tails in
> > +// js_InternalInterpret that might call ScriptPrologue or ScriptEpilogue are
> > +// unreachable in debug mode: they're only called from the out-of-line entry
> > +// points.
> 
> Does this mean that there are paths in js_InternalInterpret where we do not
> call DebugScriptPrologue/Epilogue because we know that we can't get there in
> debug mode? Do those cases JS_ASSERT that we're not in debug mode?

The original code did call DebugScript{Prologue,Epilogue}, but it was impossible to exercise that case. In bug 687683, I replace it with an assertion.

> @@ +8,5 @@
> > +// unreachable in debug mode: they're only called from the out-of-line entry
> > +// points.
> > +//
> > +// The exception is REJOIN_THIS_PROTOTYPE, which can be reached reliably if you
> > +// add a JS_GC call to stubs::GetPropNoCache. JIT code calls that stub to
> 
> Does this mean the test is ineffective or unreliable unless you hack a JS_GC
> call into the source? If so, should this test call gczeal()?

Yes, this test is ineffective without the hacked-in JS_GC call. I'm not happy about that, but even gczeal won't help, because the call may not do an allocation at all. What we need to exercise this case (and a lot of the other cases in js_InternalInterpret) is a way for tests to reliably elicit recompilations at given points.

> @@ +31,5 @@
> > +    return undefined;
> > +};
> > +hits = 0;
> > +debuggee.hits = 0;
> > +assertEq(typeof debuggee.eval("function f(){ hits++; }; f.prototype = {}; new f;"), "object");
> 
> The semicolon after function f is extraneous. (Also two other times in this
> file.)

Fixed.

Comment 13

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bffaef968b08

Comment 14

[Jim Blandy :jimb]

That changeset pushes all patches in this bug.

Comment 15

[Jim Blandy :jimb]

Here's the try server push for these patches:
https://tbpl.mozilla.org/?tree=Try&rev=c2689bb6b4a8

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6a2bb05dd97e
https://hg.mozilla.org/mozilla-central/rev/9aca452cf451
https://hg.mozilla.org/mozilla-central/rev/2e7abb74ed3e
https://hg.mozilla.org/mozilla-central/rev/cca7e56a13d9
https://hg.mozilla.org/mozilla-central/rev/bffaef968b08