Last Comment Bug 683738 - Function can contain both a return statement with an argument and a yield expression
: Function can contain both a return statement with an argument and a yield exp...
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
Depends on:
Blocks: jsfunfuzz 634472
  Show dependency treegraph
 
Reported: 2011-08-31 14:50 PDT by Jesse Ruderman
Modified: 2011-09-08 10:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
recheck TCF_RETURN_EXPR after lazily setting TCF_FUN_IS_GENERATOR (7.76 KB, patch)
2011-09-01 00:59 PDT, Dave Herman [:dherman]
cdleary: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-08-31 14:50:41 PDT
js> (function() { if (x) {return this;} else {(yield 3);}})
(function () {if (x) {return this;} else {yield 3;}})

js> (function () {if (x) {return this;} else {yield 3;}})
typein:3: TypeError: anonymous generator function returns a value:
typein:3: (function () {if (x) {return this;} else {yield 3;}})
typein:3: ..........................................^

I think the bug is that the first function is *not* rejected. Before rev 81c343a150a4, it was rejected.

jsfunfuzz found this because yield *statements* are rejected, and the expression happened to turn into a statement in this case.

The first bad revision is:
changeset:   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-08-31 23:28:24 PDT
This is definitely bad, and definitely my fault, and I'm going to try to fix it tonight.

But it might not be *too* bad. I believe generator functions are always run in the interpreter, and I suspect the interpreter just pushes the return value onto the stack harmlessly before throwing StopIteration. Nevertheless, this is definitely supposed to be statically rejected.

I'll report back tonight after I dig into this.

Dave
Comment 2 Dave Herman [:dherman] 2011-09-01 00:59:22 PDT
Created attachment 557435 [details] [diff] [review]
recheck TCF_RETURN_EXPR after lazily setting TCF_FUN_IS_GENERATOR

Pretty simple mistake: since we lazily set TCF_FUN_IS_GENERATOR, the eager check whenever it encounters a `yield' token is insufficient; we need a check at the point where we actually set TCF_FUN_IS_GENERATOR to see if TCF_RETURN_EXPR is set.

I also went through to see where else jsparse.cpp ever checks TCF_FUN_IS_GENERATOR, in case there might be other subtle bugs caused by the lazy checking, but all of them happen in the context of Parser::analyzeFunctions(), which gets called after the containing function body has been completely parsed (so all lazy `yield'-checking is completely finished).

The patch also includes a new regression test file.

Dave
Comment 3 Dave Herman [:dherman] 2011-09-01 01:02:32 PDT
PS All this stuff should be simpler with the new function* syntax in ES6: the * token makes it clear up front that you're in a generator, so you don't have to infer your context lazily.

Unfortunately, we probably won't be able to remove this at least in the short term, since existing Firefox-specific code uses generators with the current syntax.

Dave
Comment 4 Dave Herman [:dherman] 2011-09-02 14:27:54 PDT
Update: I'm out for the next two weeks. Chris has graciously agreed to shepherd this bug while I'm out. If it is deemed unimportant I can take it over again when I get back, but otherwise Chris can take over the patch and land it as he sees fit.

Thanks,
Dave
Comment 5 :Ehsan Akhgari (away Aug 1-5) 2011-09-08 10:29:12 PDT
http://hg.mozilla.org/mozilla-central/rev/01751bc07cca

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