Closed generator returns a completed object instead of throwing a error

Closed generator returns a completed object instead of throwing a error

From ES6 rev20 draft, closed generator returns completed object (the
value is `undefined` and done is `true`).
Since a error thrown in generator is propagated to the caller without
setting status of a thrown generator to "completed", once a generator is
suspended by a error, status becomes "executing" forever. This is filed
as v8:3096

LOG=N
BUG=v8:3097
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=18591

[yusukesuzuki, 2014-01-12 21:09:44]

PTAL

[yusukesuzuki, 2014-01-13 13:08:13]

Added wingo@ to the reviewers

[wingo, 2014-01-13 13:44:48]

Looks fine to me, just a few nits.  Leaving final review to Michael.

https://codereview.chromium.org/136003003/diff/8/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/136003003/diff/8/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2078: // to throw the value when the resumed
generator is already closed. ebx will
It's probably best to use a full sentence in the comment.  Code looks fine to me
though; nice catch. Let's cross fingers that the spec doesn't change yet again
:)

https://codereview.chromium.org/136003003/diff/8/test/mjsunit/harmony/generat...
File test/mjsunit/harmony/generators-iteration.js (right):

https://codereview.chromium.org/136003003/diff/8/test/mjsunit/harmony/generat...
test/mjsunit/harmony/generators-iteration.js:61: assertDoesNotThrow(function() {
iter.next(); });
Consider factoring this common double-check out to assertIteratorFinished, and
using that here and below.

https://codereview.chromium.org/136003003/diff/8/test/mjsunit/harmony/generat...
test/mjsunit/harmony/generators-iteration.js:89: */
The correct check is assertThrows(iter.next.bind(iter), TypeError), no?  (Per
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-generatorresume)

[yusukesuzuki, 2014-01-13 14:27:51]

Thank you for your review, wingo@, I've uploaded the revised patch.

https://codereview.chromium.org/136003003/diff/8/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/136003003/diff/8/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2078: // to throw the value when the resumed
generator is already closed. ebx will
On 2014/01/13 13:44:49, wingo wrote:
> It's probably best to use a full sentence in the comment.  Code looks fine to
me
> though; nice catch. Let's cross fingers that the spec doesn't change yet again
> :)

Thanks for your review. Done. Is it OK?

https://codereview.chromium.org/136003003/diff/8/test/mjsunit/harmony/generat...
File test/mjsunit/harmony/generators-iteration.js (right):

https://codereview.chromium.org/136003003/diff/8/test/mjsunit/harmony/generat...
test/mjsunit/harmony/generators-iteration.js:61: assertDoesNotThrow(function() {
iter.next(); });
On 2014/01/13 13:44:49, wingo wrote:
> Consider factoring this common double-check out to assertIteratorFinished, and
> using that here and below.

Introduced assertIteratorIsClosed and assertThrownIteratorIsClosed and used
them.
When v8:3096 is fixed, we can remove commented code in
assertThrownIteratorIsClosed.

https://codereview.chromium.org/136003003/diff/8/test/mjsunit/harmony/generat...
test/mjsunit/harmony/generators-iteration.js:89: */
On 2014/01/13 13:44:49, wingo wrote:
> The correct check is assertThrows(iter.next.bind(iter), TypeError), no?  (Per
> http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-generatorresume)

When iter.throw is executed and inserted error doesn't caught in the generator,
iter.throw propagates the error and iter's status becomes "completed"
(25.3.3.1-4-b and 25.3.3.1-4-d). So after that, `iter.next()` returns { value:
undefined, done: true } since iter's status is "completed". (25.3.3.2-5)

[Michael Starzinger, 2014-01-14 13:17:44]

Looking good. I have one comment.

https://codereview.chromium.org/136003003/diff/160002/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/136003003/diff/160002/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2171: __ CallRuntime(Runtime::kThrow, 1);
I am not sure if throwing directly from within here without first restoring all
stack handlers is generally correct. IMHO we should drop into the runtime at
this point similar to what we do for a general JSGeneratorObject::THROW. I think
this can be achieved by moving the "closed_state" block above the "slow_resume"
block and simply fall-through in the case of JSGeneratorObject::THROW. Please
correct me if I am missing something. I was thinking about a structure along the
lines of ...

// If we are sending a value and there is no operand stack, we can jump back
// in directly.
if (resume_mode == JSGeneratorObject::NEXT) {
  [...]
}

// Reach here when generator is closed.
__ bind(&closed_state);
if (resume_mode == JSGeneratorObject::NEXT) {
  [...]
}

// Otherwise, we push holes for the operand stack and call the runtime to fix
// up the stack and the handlers.
__ bind(&slow_resume);
[...]

[Michael Starzinger, 2014-01-14 13:28:33]

Hmm, of course my idea doesn't work because we haven't built a frame for the
generator to throw in yet. The code needs more reshuffling I guess.

[Michael Starzinger, 2014-01-14 13:47:47]

Oh, the spec actually wants the exception to be thrown from within the context
of Generator.prototype.throw according to 25.3.1.3-7, so I totally missed the
point. It surely helps to look at the latest draft of the spec. Sorry for the
noise.

LGTM in this case. I will land this for you.

[Michael Starzinger, 2014-01-14 15:19:43]

Committed patchset #2 manually as r18591 (presubmit successful).

[yusukesuzuki, 2014-01-15 01:32:54]

Thank you!