Last Comment Bug 665286 - Crash [@ JSContext::generatorFor] or "Assertion failure: fp->isGeneratorFrame(),"
: Crash [@ JSContext::generatorFor] or "Assertion failure: fp->isGeneratorFrame...
Status: VERIFIED FIXED
[ccbr] fixed-in-tracemonkey
: assertion, crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: jsfunfuzz 634472
  Show dependency treegraph
 
Reported: 2011-06-18 06:25 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-10 16:36 PST (History)
5 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack (4.52 KB, text/plain)
2011-06-18 06:25 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
draft fix, no tests yet (4.42 KB, patch)
2011-06-18 10:00 PDT, Dave Herman [:dherman]
no flags Details | Diff | Review
added tests (8.30 KB, patch)
2011-06-18 14:16 PDT, Dave Herman [:dherman]
cdleary: review+
Details | Diff | Review

Description Gary Kwong [:gkw] [:nth10sd] 2011-06-18 06:25:59 PDT
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)
Comment 1 Dave Herman [:dherman] 2011-06-18 09:01:25 PDT
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] 2011-06-18 09:18:20 PDT
> 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] 2011-06-18 09:44:07 PDT
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] 2011-06-18 10:00:42 PDT
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
Comment 5 Dave Herman [:dherman] 2011-06-18 14:16:30 PDT
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
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2011-06-23 13:41:20 PDT
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] 2011-06-23 14:10:06 PDT
> 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] 2011-06-23 15:22:27 PDT
http://hg.mozilla.org/tracemonkey/rev/2641200acd54
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-06-27 11:39:38 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2641200acd54
Comment 10 Christian Holler (:decoder) 2013-01-04 13:18:15 PST
A testcase for this bug was automatically identified at js/src/tests/js1_8/genexps/regress-665286.js.
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2013-01-10 16:36:55 PST
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.

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