1012480 - [x for (x in function*(){}())] hangs

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Open the JS shell
2. Type in the expression from the summary

Expected result: []

Actual result: JS shell goes into an infinite loop

Additional information: for ... of works correctly of course.

Comment 1

[[e-mail]]

Huh, I don't remember ticking the s-s flag... feel free to uncheck it.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Do not treat star generator's iterator as legacy generator's iterator in for..in loop.

Another testcases here.

  js> [x for (x in function*(){ throw StopIteration; }())];
  []
  js> [x for (x in function*(){ yield 1; yield 2; throw StopIteration; }())];
  [{value:1, done:false}, {value:2, done:false}]

Iteration can be stopped by throwing StopIteration, and the value pushed into the new born array is the returned value of next(). So, in the summary's case, next() returns `{value:undefined, done:true}` and it's pushed into the new born array infinitely.

If I understand correctly, for..in should not iterate over star generator's iterator by calling next(), and should simply iterate over its enumerable keys. This could be fixed by removing Class.ext.iteratorObject from StarGeneratorObject::class_.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=556f0af5eafc

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 8463448 [details] [diff] [review]
Do not treat star generator's iterator as legacy generator's iterator in for..in loop.

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

I'm not familiar with iterator semantics.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8463448 [details] [diff] [review]
Do not treat star generator's iterator as legacy generator's iterator in for..in loop.

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

Thank you again for the patch!

::: js/src/jit-test/tests/generators/star-generator-forin.js
@@ +6,5 @@
> +assertEqArray([x for (x in iter)],
> +              []);
> +assertEqArray([x for each (x in iter)],
> +              []);
> +iter = function*(){ yield 42; throw StopIteration; };

Please drop the `throw StopIteration;` from all these generators.

If you want to make absolutely sure that the generator isn't being called, the thing to do is

    var hit = 0;
    iter = function*(){ hit++; };

and then later assert that hit is still 0.

::: js/src/jsiter.cpp
@@ +1728,1 @@
>      }

You can just delete the line that says "JS_NULL_CLASS_SPEC," and everything after it.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Please drop the `throw StopIteration;` from all these generators.

The reason to drop this is because it's a "head fake" --- it looks like it's significant, but it really has nothing to do with what's being tested.

Great tests, though.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Do not treat star generator's iterator as legacy generator's iterator in for..in loop.

Thank you, here is updated patch.

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Please drop the `throw StopIteration;` from all these generators.
> 
> If you want to make absolutely sure that the generator isn't being called,
> the thing to do is
> 
>     var hit = 0;
>     iter = function*(){ hit++; };
> 
> and then later assert that hit is still 0.

Ah, I didn't think of that. Yes, as you wrote, yielded value and StopIteration have nothing to do with this test.
Now we can reduce the tests by half.

> ::: js/src/jsiter.cpp
> @@ +1728,1 @@
> >      }
> 
> You can just delete the line that says "JS_NULL_CLASS_SPEC," and everything
> after it.

Thank you for letting me know that :)


Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=9b9e5392bb0c

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8463448 [details] [diff] [review]
Do not treat star generator's iterator as legacy generator's iterator in for..in loop.

forgot to make old patch obsolete

Comment 8

[Tooru Fujisawa [:arai]]

Try run again: https://tbpl.mozilla.org/?tree=Try&rev=cdbfad4449ad

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3181a38a32c0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3181a38a32c0

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-esr31) Do not treat star generator's iterator as legacy generator's iterator in for..in loop. r=jorendorff

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  Malicious webpage can cause browser freeze with simple JavaScript code,
  and tracking flags ware set on bug 1053132.
User impact if declined:
  DoS attack possible.
Fix Landed on Version:
  34
Risk to taking this patch (and alternatives if risky):
  Low (simple fix, no regression happens for 4 versions).
String or UUID changes made by this patch:
  None.

Comment 14

[Tooru Fujisawa [:arai]]

Thank you :)

https://hg.mozilla.org/releases/mozilla-esr31/rev/f0e283a4a325

Comment 15

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build(s):
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/08/2014-08-20-03-02-02-mozilla-central/
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/01/2015-01-14-00-05-12-mozilla-esr31/

Went through verification using the following build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/31.5.0esr-candidates/build2/

Test Case Used:

- Opened the Web Console and inserted "[x for (x in function*(){}())]", ensured I received "Array [  ]" without the browser hanging. Tried this several times to ensure it's working as expected.