665286 - Crash [@ JSContext::generatorFor] or "Assertion failure: fp->isGeneratorFrame(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(function() {
    function f() {
        x
    }
    f(yield #2=[])
})()

crashes js opt shell on TM changeset e59b1d2a2f79 without CLI arguments at JSContext::generatorFor and asserts js debug shell at Assertion failure: fp->isGeneratorFrame(),

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   70753:81c343a150a4
user:        Dave Herman
date:        Thu Jun 16 08:20:18 2011 -0700
summary:     disallow yield and arguments in generator expressions (bug 634472, r=cdleary)

Comment 1

[Dave Herman [:dherman]]

The sharp-literal is a red herring, and in fact so is the nested function. Simpler test:

    (function(){print(yield)})()

I'm looking into it. This may have to do with the fact that the patch for bug 634472 eliminates a check in the emitter, IIRC.

Dave

Comment 2

[Dave Herman [:dherman]]

> This may have to do with the fact that the patch for
> bug 634472 eliminates a check in the emitter, IIRC.

Actually, it doesn't seem like that should be relevant; the check was replaced by a JS_ASSERT that isn't being triggered. This is a weirder bug than I thought. How confident are we in the autoBisect?

Dave

Comment 3

[Dave Herman [:dherman]]

Ah, got it -- the patch for bug 634472 delays setting TCF_FUN_IS_GENERATOR until it knows for sure that occurrences of |yield| were not actually in nested genexps, but in the case of |yield| in an arglist it doesn't actually ever perform the delayed check.

Patch forthcoming.

Dave

Comment 4

[Dave Herman [:dherman]]

Attachment: draft fix, no tests yet

I believe this fixes the bug but it needs tests before it's ready for review. I also noticed that I was calling guard.endBody() suspiciously late in Parser::argumentList, so I need to add some tests that demonstrate that that was wrong, too.

I'll write the tests later today.

Dave

Comment 5

[Dave Herman [:dherman]]

Attachment: added tests

I added tests.

I believe the fact that guard.endBody() appeared late in Parser::argumentList wasn't really a bug. I think it just loses some opportunities to be eager about setting TCF_FUN_IS_GENERATOR. So I didn't create a test case for moving .endBody() because I can't come up with a case that breaks.

(In fact, looking at the logic again, I think the whole parenDepth thing is unnecessary complexity. I'll file a separate bug for cdleary's feedback on that.)

Dave

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 540270 [details] [diff] [review]
added tests

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

::: js/src/jsparse.cpp
@@ +7315,5 @@
>  #endif /* JS_HAS_GENERATOR_EXPRS */
>  #endif /* JS_HAS_GENERATORS */
>  
> +bool
> +Parser::registerGenerator()

For consistency with other constructs like this, can we call it noteGenerator (or maybeNoteGenerator, since it's conditional on having a positive |yieldCount| in |tc|) and provide a description of the scenarios in which it should be called?

Correct me if I'm wrong, but proper usage of this method going forward actually seems a bit tricky... you use it when you're deferring the decision of whether this current |tc| is a generator because you saw a yield, *BUT* that yield could just be causing you to make a generator expression. We're being lazy about declaring the current |tc| a generator function until we know a little more.

To ramble on a bit, there are traps here WRT syntax constructs we don't permit in generator expressions, like reporting a syntax error when we see |return|. It works out okay for argumentList and parenExpr because you can't have a return statement for the current |tc| with any of the productions we call from there.

Comment 7

[Dave Herman [:dherman]]

> Correct me if I'm wrong, but proper usage of this method going forward
> actually seems a bit tricky... you use it when you're deferring the decision
> of whether this current |tc| is a generator because you saw a yield, *BUT*
> that yield could just be causing you to make a generator expression. We're
> being lazy about declaring the current |tc| a generator function until we
> know a little more.

Basically, yes, except one detail: it's not that the yield causes you to interpret it as a genexp. It's that the yield may be (illegally) contained inside a genexp, which you'll only learn late after discovering a |for| token.

> To ramble on a bit, there are traps here WRT syntax constructs we don't
> permit in generator expressions, like reporting a syntax error when we see
> |return|. It works out okay for argumentList and parenExpr because you can't
> have a return statement for the current |tc| with any of the productions we
> call from there.

Good point. If Brendan's proposed block-lambdas got accepted in Harmony, or if we added expression forms that can nest statements, |return| would become a hazard as well.

Dave

Comment 8

[Dave Herman [:dherman]]

http://hg.mozilla.org/tracemonkey/rev/2641200acd54

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2641200acd54

Comment 10

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8/genexps/regress-665286.js.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.