Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 674171
(Assignee)

Updated

6 years ago
No longer blocks: 674171
(Assignee)

Comment 1

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

Comment 2

6 years ago
Created attachment 572243 [details] [diff] [review]
Remove unnecessary 'js::' namespace prefixes from Debugger code.
Assignee: general → jimb
Attachment #572243 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
Depends on: 700070
(Assignee)

Comment 3

6 years ago
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.
Attachment #572244 - Flags: review?(jorendorff)
(Assignee)

Comment 4

6 years ago
Created attachment 572245 [details] [diff] [review]
Test that onEnterFrame hooks' resumption values are respected.
Attachment #572245 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
Attachment #561040 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 572247 [details] [diff] [review]
Some random comment fixes I came across.
Attachment #572247 - Flags: review?(jorendorff)
(Assignee)

Comment 6

6 years ago
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.
Attachment #572248 - Flags: review?(bhackett1024)
(Assignee)

Comment 7

6 years ago
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.
Attachment #572249 - Flags: review?(jorendorff)
(Assignee)

Comment 8

6 years ago
Unfortunately, Jason's on vacation until November 14th.
Attachment #572248 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

6 years ago
Depends on: 701274
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;".
Attachment #572243 - Flags: review?(jorendorff) → review+
Attachment #572244 - Flags: review?(jorendorff) → review+
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.)
Attachment #572245 - Flags: review?(jorendorff) → review+
Attachment #572247 - Flags: review?(jorendorff) → review+
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.
Attachment #572249 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bffaef968b08
Status: NEW → ASSIGNED
Whiteboard: [inbound]
(Assignee)

Comment 14

6 years ago
That changeset pushes all patches in this bug.
(Assignee)

Comment 15

6 years ago
Here's the try server push for these patches:
https://tbpl.mozilla.org/?tree=Try&rev=c2689bb6b4a8
Flags: in-testsuite+
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla11
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.