[parser] Fix bug in for-of desugaring.

[parser] Fix bug in for-of desugaring.

When reading the value property of an iterator result fails, we must not close the iterator.
This was not discovered earlier because the tests had a subtle bug.

This CL fixes both the desugaring and the tests.

BUG=

Committed: https://crrev.com/d3aefe8cc3e8bae9fd4e3fd308aa4a35d3829efa
Cr-Commit-Position: refs/heads/master@{#37571}

[neis, 2016-07-05 12:11:22]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 12:11:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 12:34:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-05 12:34:12]

Dry run: This issue passed the CQ dry run.

[neis, 2016-07-05 13:06:35]

Description was changed from

==========
Fix bug in for-of desugaring.

BUG=
==========

to

==========
[parser] Fix bug in for-of desugaring.

When reading the value property of an iterator result fails, we must not close
the iterator.  This was not discovered earlier because the tests had a subtle
bug.

This CL fixes both the desugaring and the tests.

BUG=
==========

[neis, 2016-07-05 13:06:35]

neis@chromium.org changed reviewers:
+ littledan@chromium.org

[neis, 2016-07-05 13:06:47]

Description was changed from

==========
[parser] Fix bug in for-of desugaring.

When reading the value property of an iterator result fails, we must not close
the iterator.  This was not discovered earlier because the tests had a subtle
bug.

This CL fixes both the desugaring and the tests.

BUG=
==========

to

==========
[parser] Fix bug in for-of desugaring.

When reading the value property of an iterator result fails, we must not close
the iterator.
This was not discovered earlier because the tests had a subtle bug.

This CL fixes both the desugaring and the tests.

BUG=
==========

[neis, 2016-07-05 13:08:18]

ptal

[neis, 2016-07-05 13:08:25]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 13:08:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 13:32:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-05 13:32:18]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-07-07 00:21:06]

lgtm

The core change starting on line 3275 looks good to me. I'll leave it up to you
whether you want to do the 'finalize' parameter change in this patch or pull it
out. If you do leave it in, I'd prefer if you gave a comment explaining its
function as an optimization (if I understand it correctly).

Do you have any tests where you make an iterable whose actual value getter
throws? That would be a nice addition, as that is the point of the patch. If
this is based on running incoming, not-yet-rolled test262 tests before and
after, it'd be nice to mention this in the commit message, and then I can verify
this when I do the roll.

https://codereview.chromium.org/2119353002/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2119353002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:5941: const bool finalize = false;
I see how in the spec, finalization is not done for spreads because it is
impossible to throw, but that's in a context of tons of duplication in the spec.
Are you carrying that forward to our code as an optimization to generate less
code? Leaving aside its value as an optimization, it feels slightly dangerous to
me--if spreads did somehow throw, you would expect it to close the iterator that
it is going over (this is an invariant all over where the spec does iteration).

[neis, 2016-07-07 06:59:07]

On 2016/07/07 00:21:06, Dan Ehrenberg wrote:
> lgtm
> 
> The core change starting on line 3275 looks good to me. I'll leave it up to
you
> whether you want to do the 'finalize' parameter change in this patch or pull
it
> out. If you do leave it in, I'd prefer if you gave a comment explaining its
> function as an optimization (if I understand it correctly).

You mean the 'finalize = false' in RewriteSpreads, right? I haven't actually
changed anything here, this for-loop never had the finalization code.

> Do you have any tests where you make an iterable whose actual value getter
> throws? That would be a nice addition, as that is the point of the patch. If
> this is based on running incoming, not-yet-rolled test262 tests before and
> after, it'd be nice to mention this in the commit message, and then I can
verify
> this when I do the roll.
> 
> https://codereview.chromium.org/2119353002/diff/40001/src/parsing/parser.cc
> File src/parsing/parser.cc (right):
> 
>
https://codereview.chromium.org/2119353002/diff/40001/src/parsing/parser.cc#n...
> src/parsing/parser.cc:5941: const bool finalize = false;
> I see how in the spec, finalization is not done for spreads because it is
> impossible to throw, but that's in a context of tons of duplication in the
spec.
> Are you carrying that forward to our code as an optimization to generate less
> code? Leaving aside its value as an optimization, it feels slightly dangerous
to
> me--if spreads did somehow throw, you would expect it to close the iterator
that
> it is going over (this is an invariant all over where the spec does
iteration).

All we do in that loop is call %AppendElement, which I suppose can only fail
here when we run out of memory. Does that clarify things?

[neis, 2016-07-07 06:59:40]

Regarding tests: yes, of course, they are part of this CL.

[Dan Ehrenberg, 2016-07-07 07:15:56]

lgtm

[neis, 2016-07-07 07:31:05]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-07-07 07:31:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 07:32:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 07:32:50]

Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/8634)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/18889)

[neis, 2016-07-07 07:46:29]

The CQ bit was checked by neis@chromium.org

[neis, 2016-07-07 07:46:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2119353002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-07-07 07:46:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 08:15:19]

Description was changed from

==========
[parser] Fix bug in for-of desugaring.

When reading the value property of an iterator result fails, we must not close
the iterator.
This was not discovered earlier because the tests had a subtle bug.

This CL fixes both the desugaring and the tests.

BUG=
==========

to

==========
[parser] Fix bug in for-of desugaring.

When reading the value property of an iterator result fails, we must not close
the iterator.
This was not discovered earlier because the tests had a subtle bug.

This CL fixes both the desugaring and the tests.

BUG=
==========

[commit-bot: I haz the power, 2016-07-07 08:15:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-07 08:16:23]

Description was changed from

==========
[parser] Fix bug in for-of desugaring.

When reading the value property of an iterator result fails, we must not close
the iterator.
This was not discovered earlier because the tests had a subtle bug.

This CL fixes both the desugaring and the tests.

BUG=
==========

to

==========
[parser] Fix bug in for-of desugaring.

When reading the value property of an iterator result fails, we must not close
the iterator.
This was not discovered earlier because the tests had a subtle bug.

This CL fixes both the desugaring and the tests.

BUG=

Committed: https://crrev.com/d3aefe8cc3e8bae9fd4e3fd308aa4a35d3829efa
Cr-Commit-Position: refs/heads/master@{#37571}
==========

[commit-bot: I haz the power, 2016-07-07 08:16:24]

Patchset 4 (id:??) landed as
https://crrev.com/d3aefe8cc3e8bae9fd4e3fd308aa4a35d3829efa
Cr-Commit-Position: refs/heads/master@{#37571}