Last Comment Bug 687683 - Debugger ignores Debugger.prototype.onEnterFrame hook's resumption value
: Debugger ignores Debugger.prototype.onEnterFrame hook's resumption value
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jim Blandy :jimb
:
Mentors:
Depends on: 700070 701274
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-19 15:15 PDT by Jim Blandy :jimb
Modified: 2012-02-01 13:57 PST (History)
0 users
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests for onEnterFrame hooks' resumption values. (2.74 KB, patch)
2011-09-19 15:15 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Remove unnecessary 'js::' namespace prefixes from Debugger code. (5.08 KB, patch)
2011-11-05 17:46 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Separate ScriptDebugPrologue add ScriptDebugEpilogue from and ScriptPrologue and ScriptEpilogue. (5.46 KB, patch)
2011-11-05 17:48 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Test that onEnterFrame hooks' resumption values are respected. (9.80 KB, patch)
2011-11-05 17:48 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Some random comment fixes I came across. (1.69 KB, patch)
2011-11-05 17:50 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Assert that REJOIN_CHECK_ARGUMENTS and REJOIN_FUNCTION_PROLOGUE are never generated in debug mode. (1.59 KB, patch)
2011-11-05 17:51 PDT, Jim Blandy :jimb
bhackett1024: review+
Details | Diff | Splinter Review
Collect resumption values from onEnterFrame handlers, and respect them. (12.14 KB, patch)
2011-11-05 17:51 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Jim Blandy :jimb 2011-09-19 15:15:19 PDT
Created attachment 561040 [details] [diff] [review]
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 2011-11-05 17:46:02 PDT
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 2011-11-05 17:46:55 PDT
Created attachment 572243 [details] [diff] [review]
Remove unnecessary 'js::' namespace prefixes from Debugger code.
Comment 3 Jim Blandy :jimb 2011-11-05 17:48:03 PDT
Created attachment 572244 [details] [diff] [review]
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 2011-11-05 17:48:42 PDT
Created attachment 572245 [details] [diff] [review]
Test that onEnterFrame hooks' resumption values are respected.
Comment 5 Jim Blandy :jimb 2011-11-05 17:50:06 PDT
Created attachment 572247 [details] [diff] [review]
Some random comment fixes I came across.
Comment 6 Jim Blandy :jimb 2011-11-05 17:51:02 PDT
Created attachment 572248 [details] [diff] [review]
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 2011-11-05 17:51:54 PDT
Created attachment 572249 [details] [diff] [review]
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 2011-11-05 17:52:42 PDT
Unfortunately, Jason's on vacation until November 14th.
Comment 9 Jason Orendorff [:jorendorff] 2011-11-22 15:16:54 PST
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] 2011-11-23 09:11:18 PST
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] 2011-11-23 11:20:51 PST
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 2011-12-05 13:39:29 PST
(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 14 Jim Blandy :jimb 2011-12-06 12:09:57 PST
That changeset pushes all patches in this bug.
Comment 15 Jim Blandy :jimb 2011-12-06 12:14:07 PST
Here's the try server push for these patches:
https://tbpl.mozilla.org/?tree=Try&rev=c2689bb6b4a8

Note You need to log in before you can comment on or make changes to this bug.