The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: gkw, Unassigned)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Linux
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ccbr] fixed-in-tracemonkey, crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 540234 [details]
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)
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
> 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
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
Created attachment 540253 [details] [diff] [review]
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
Attachment #540253 - Flags: feedback?(cdleary)
Created attachment 540270 [details] [diff] [review]
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
Attachment #540253 - Attachment is obsolete: true
Attachment #540253 - Flags: feedback?(cdleary)
Attachment #540270 - Flags: review?(cdleary)
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.
Attachment #540270 - Flags: review?(cdleary) → review+
> 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
http://hg.mozilla.org/tracemonkey/rev/2641200acd54
Whiteboard: [ccbr] → [ccbr] reflect-parse fixed-in-tracemonkey

Updated

6 years ago
Whiteboard: [ccbr] reflect-parse fixed-in-tracemonkey → [ccbr] fixed-in-tracemonkey
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2641200acd54
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/tests/js1_8/genexps/regress-665286.js.
Flags: in-testsuite+
(Reporter)

Comment 11

4 years ago
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.