1364608 - Stash rval in AsyncIteratorClose

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Shu-yu Guo [:shu]]

AsyncIteratorClose awaits the result of calling iter.return(). This clobbers rval, which results in crashes when performing AsyncIteratorClose due to "return" in for-await-of.

Test case courtesy of leobalter:

  let iter = function* () { yield 42; yield 43; }()
  async function *fn() { for await (let [...x] of [iter]) { return; } }
  fn().next().then(_ => print("resolved"), _ => print("rejected"))

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Stash rval in AsyncIteratorClose.

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8867405 [details] [diff] [review]
Stash rval in AsyncIteratorClose.

Review of attachment 8867405 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

Comment 3

[Leo Balter]

Thanks for this patch! While I'm not familiar with the patch code, I can tell the test captures the bug well.

I'll write a quick Test262 test for it to make sure this is captured.

Test262 is not yet 100% covered for for-await-of, so it's good if I already add this part, anyway.

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Leo Balter from comment #3)
> Thanks for this patch! While I'm not familiar with the patch code, I can
> tell the test captures the bug well.
> 
> I'll write a quick Test262 test for it to make sure this is captured.
> 
> Test262 is not yet 100% covered for for-await-of, so it's good if I already
> add this part, anyway.

Sure, please add a test262 test.

Comment 5

[Shu-yu Guo [:shu]]

I'll commit the testcase to test262/local in the meantime and wait for you to do the upstream test262 PR.

Comment 6

[Leo Balter]

Cool, you can use the tests from this PR. I'm planning a new sync tomorrow in the morning, if that's ok. If we have the local files, it's gonna be just about replacing them, otherwise I'll add the tests to the skip list.

Comment 7

[Leo Balter]

I forgot to link the PR: https://github.com/tc39/test262/pull/1036/files

Comment 9

[Shu-yu Guo [:shu]]

Going to land in the meantime because of becoming a fuzzblocker due to recent test262 sync

Marking leave-open as until [1] is resolved we still have to skip some tests.

[1] https://github.com/tc39/test262/issues/1043

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8084d6ac8a07
Stash rval in AsyncIteratorClose. (r=arai)

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/0b11639e4085
user:        Tooru Fujisawa
date:        Mon Mar 27 23:20:19 2017 +0900
summary:     Bug 1331092 - Part 9: Implement for-await-of. r=shu

The dupe bug 1366399 had this regression range.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8084d6ac8a07

Comment 13

[André Bargull [:anba]]

Removing the leave-open flag because the blocker mentioned in comment #9 has been resolved.