1479391 - Forcing return from an onPop handler for a generator's yield crashes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 1e5fa52a612e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --baseline-eager min.js):

var g = newGlobal();
var dbg = new Debugger;
var gw = dbg.addDebuggee(g);
dbg.onDebuggerStatement = function handleDebugger(frame) {
    frame.onPop = function() {
        return {
            return: "fit"
        };
    };
};
g.eval("function* g() { for (var i = 0; i < 10; i++) { debugger; yield i; } }");
g.eval("var it = g();");
var rv = gw.executeInGlobal("it.next();");
assertEq(g.it.next().value, undefined);


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00000275d596d546 in ?? ()
#0  0x00000275d596d546 in ?? ()
#1  0xfff9800000000000 in ?? ()
#2  0xfffe7ffff4d005d0 in ?? ()
#3  0xfff9800000000000 in ?? ()
#4  0xfffe7ffff4d005d0 in ?? ()
#5  0x0000000000000000 in ?? ()
rax	0x7ffff4d005d0	140737300661712
rbx	0x7ffff4ca6a30	140737300294192
rcx	0x4000000000000	1125899906842624
rdx	0x1	1
rsi	0x0	0
rdi	0x7ffff5f95208	140737320145416
rbp	0x7fffffffca68	140737488341608
rsp	0x7fffffffca18	140737488341528
r8	0x0	0
r9	0x1044	4164
r10	0xfff9800000000000	-1829587348619264
r11	0x7ffff5f7c0a8	140737320042664
r12	0x8	8
r13	0x7fffffffd2b0	140737488343728
r14	0x7ffff4d005d0	140737300661712
r15	0x7fffffffd120	140737488343328
rip	0x275d596d546	2705117861190
=> 0x275d596d546:	mov    0x30(%rcx),%rdx
   0x275d596d54a:	mov    0x40(%rdx),%rdx

Comment 1

[David Durst [:ddurst]]

We've passed this by in last week's triage. How important is this?

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to David Durst [:ddurst] (REO for 63) from comment #1)
> We've passed this by in last week's triage. How important is this?

Ussually "debugger" test cases have a lower priority for us.
However, knowing that Jason work in a similar area recently, this might be related.

Note: Crash-stat results are unlikely to be only the result of similar test cases.

Comment 3

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/09d4547a9714
user:        Jason Orendorff
date:        Fri Jul 06 18:09:05 2018 -0500
summary:     Bug 1471954 - Change behavior of `{return:}` resumption values in generators. r=jimb

This iteration took 232.974 seconds to run.

Comment 4

[Christian Holler (:decoder)]

This is causing an increasing amount of trouble due to missing stacks.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Fix crash after forced return from a generator

Comment 6

[Jim Blandy :jimb]

Comment on attachment 9003290 [details] [diff] [review]
Fix crash after forced return from a generator

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

::: js/src/vm/Debugger.cpp
@@ +1004,5 @@
> +
> +        // Keep restoreGenSuspended from resurrecting the generator if
> +        // processParsedHandlerResult already closed it.
> +        if (genObj && genObj->isClosed())
> +            genObj = nullptr;

This feels really delicate.

How about an RAII class that saves the generator's YIELD_AND_AWAIT_INDEX_SLOT, sets it to running, and then restores it? That could be cuddled directly around the `handler->onPop` calls, so that the call to `processParsedHandlerResult` fell outside its scope. Then, the RAII thing would be carefully saving and restoring the effects of processParsedHandlerResult, not overwriting it.

It could be quiescent when passed a null pointer, or take a Maybe<GeneratorObject&> for super-explicitness, so genObj could continue to serve as the "is this even a generator" indicator.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Fix crash after forced return from a generator

This patch switches from GeneratorObject::finalSuspend to setClosed in order to
dodge an assertion in finalSuspend that the Generator state machine is
transitioning along an expected edge. The way the Debugger manipulates
Generator state is decidedly unexpected, from the perspective of the normal
rules, and we've decided to accept that.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 9003625 [details] [diff] [review]
Fix crash after forced return from a generator

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

::: js/src/vm/Debugger.cpp
@@ +908,5 @@
> +    int32_t yieldAwaitIndex;
> +    Rooted<GeneratorObject*> genObj;
> +
> +  public:
> +    AutoSetGeneratorRunning(JSContext* cx, AbstractFramePtr frame)

Yes, this is what I had in mind - thanks!

I was thinking the constructor would take a nullable GeneratorObject*, not a stack frame. Then all the hair about isFunctionFrame, isGenerator or isAsync, GetGeneratorObjectForFrame, isBeforeInitialYield, !isClosed, isSuspended, could stay where it is, and just decide whether genObj was nullptr or not. Then, the only thing that would occur around each handler->onPop call would be the saving, setting, and then restoring of the slot.

That seems simpler and cleaner to me. But the patch is fine as it is, so just take it as a suggestion.

Comment 9

[David Durst [:ddurst]]

Is this good to land?

Comment 10

[Jason Orendorff [:jorendorff]]

looking.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Fix crash after forced return from a generator

This patch switches from GeneratorObject::finalSuspend to setClosed in order to
dodge an assertion in finalSuspend that the Generator state machine is
transitioning along an expected edge. The way the Debugger manipulates
Generator state is decidedly unexpected, from the perspective of the normal
rules, and we've decided to accept that.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 9005023 [details] [diff] [review]
Fix crash after forced return from a generator

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

Carrying over jimb's review.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55517d66b222
Fix crash after forced return from a generator. r=jimb

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/55517d66b222