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

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