1180306 - new Map(iterable) should close the argument iterator after a value is not Object error.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Leo Balter]

When creating a new Map object, if a iterable item is caught on the `not Object` error, it should close the iterator:

Ref: 23.1.1.1 item 9.f.ii

  23.1.1.1 Map ( [ iterable ] )

  ...
  9. Repeat
    ...
    d. Let nextItem be IteratorValue(next).
    e. ReturnIfAbrupt(nextItem).
    f. If Type(nextItem) is not Object,
      i. Let error be Completion{[[type]]: throw, [[value]]: a newly created
      TypeError object, [[target]]:empty}.
      ii. Return IteratorClose(iter, error).

Example Code:

```
var closed = false;
var iterable = {};
iterable[Symbol.iterator] = function() {
  return {
    next: function() {
      return { value: 1, done: false };
    },
    return: function() {
      closed = true;
    }
  };
};

try {
  new Map(iterable);
} catch() {
  // it will throw a TypeError
}
```

Expects `closed` to be `true`, as the iterable argument should be closed.

Comment 1

[Tooru Fujisawa [:arai]]

Will fix the following here.
  Map
  Set
  WeakMap
  WeakSet
  Promise.all
  Promise.race
  Array.from

Comment 2

[Tooru Fujisawa [:arai]]

Here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c22537aae06b28f6eaab991400469a322cf154bc

to be clear, I'll leave the following to bug 1147371.
  * array destructuring
  * for-of
  * yield*

all those are now handled in bytecode.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Close iterator after error in {Map,Set,WeakMap,WeakSet} constructors.

Since IteratorClose receives completion, we'll need different functions for normal completion and abrupt completion.
fortunately there's only abrupt completion case in this bug's scope.

So, added IteratorCloseThrow that is called when we're going to throw.
It basically does IteratorClose, but it returns when it's supposed to throw, so that we can throw after calling it.

Comment 4

[Tooru Fujisawa [:arai]]

forgot to check the performance impact.
will check it and post remaining patches after that.

Comment 5

[Tooru Fujisawa [:arai]]

here's performance comparison for Part 1 (and code for 1 and 2)

code:
  var start = Date.now();
  let a = [];
  for (let i = 0; i < 1000; i ++) {
    a.push([{}, {}]);
  }
  function f() {
    for (let i = 0; i < 10000; i ++) {
      // Uncomment one of them.
      // new Map(a);
      // new Set(a);
      // new WeakMap(a);
      // new WeakSet(a);
      // Array.from(a);
    }
  }
  f();
var end = Date.now();
print(end - start, "ms");


[Map]

before: 1157 ms
after:  1265 ms : +108 ms (8.5%)

[Set]

before: 413 ms
after:  412 ms : no change

[WeakMap]

before: 1359 ms
after:  1438 ms : +79 ms (5.5%)

[WeakSet]

before: 1133 ms
after:  1122 ms : no change

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Close iterator after error in Array.from.

Step 5.e.i is problematic.
It makes the code slower, but it won't hit in almost all cases.
So, in this patch, I don't add it.


[Array.from]

before: 792 ms
after:  819 ms : +27 ms (3.2%)
after:  877 ms : +85 ms (9.7%) -- When uncomment step 5.e.i

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Close iterator after error in Promise.{all,race}.

Now PerformPromiseAll and PerformPromiseRace return |done| value, that is basically the output of |iterator.next|, but it's also set to true when |iterator.next| fails.

Like Part 1, we need IteratorClose for throwing case.
So I added ForOfIterator::closeThrow, that basically does IteratorClose, but skips normal completion case, and also save/recovers pending exception instead of receiving completion value.

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8811605 [details] [diff] [review]
Part 3: Close iterator after error in Promise.{all,race}.

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

\o/, this is great! I have a couple of concerns I'd like to see addressed, but by and large this is very nice.

::: js/src/builtin/Promise.cpp
@@ +1402,5 @@
>      if (!result) {
>          // Step 8.a.
> +        if (!done && cx->isExceptionPending()) {
> +            if (!iter.closeThrow())
> +                return false;

I don't think this is right: even if IteratorClose results in an abrupt completion, we should still reject the promise in step b instead of just bailing. We also should close the iterator after uncatchable exceptions, if possible, I think.

@@ +1657,5 @@
>      RootedId indexId(cx);
>      RootedValue rejectFunVal(cx, ObjectOrNullValue(reject));
>  
>      while (true) {
> +        // Steps a, c, e, f, g.

Change this to
Steps a-c, e-g.

@@ +1665,1 @@
>              return false;

And add a "Step c." comment above this.

@@ +1866,5 @@
>      if (!result) {
>          // Step 8.a.
> +        if (!done && cx->isExceptionPending()) {
> +            if (!iter.closeThrow())
> +                return false;

Same comment as in Promise.all.

@@ +1901,1 @@
>              return false;

And same comments as in PerformPromiseAll

::: js/src/vm/ForOfIterator.cpp
@@ +153,5 @@
>  
> +// ES 2017 draft 0f10dba4ad18de92d47d421f378233a2eae8f077 7.4.6.
> +// When completion.[[Type]] is throw.
> +MOZ_MUST_USE bool
> +ForOfIterator::closeThrow()

It feels pretty weird that a return value of `true` means that an exception is pending here. Really, we should always have a return value of `false` to match expectations. Given that, perhaps we should just make this void?

@@ +191,5 @@
> +    // Step 5.
> +    RootedValue innerResultValue(cx_);
> +    if (!js::Call(cx_, returnVal, iterator, &innerResultValue)) {
> +        if (!cx_->isExceptionPending())
> +            return false;

Ok, so we have an uncatchable exception here. Given that we never intended to use this completion value at all, shouldn't we just ignore that and continue as we would otherwise? We do have the original exception that we'll set back to pending in step 6, which should work just fine, right?

(Chances are this is an OOM situation and we'll crash right afterwards anyway, but that's not our problem here.)

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8811590 [details] [diff] [review]
Part 1: Close iterator after error in {Map,Set,WeakMap,WeakSet} constructors.

the testcase should be updated to check more about IteratorClose steps.
will update them.

Comment 10

[Tom S [:evilpie]]

Comment on attachment 8811590 [details] [diff] [review]
Part 1: Close iterator after error in {Map,Set,WeakMap,WeakSet} constructors.

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

Nice work. I like the tests. I find the difference between Map and Set confusing, I would have assumed the try..catch slows it down, but it looks like the function call in the !IsObject case is to blame.

::: js/src/tests/ecma_6/Map/constructor-iterator-close.js
@@ +131,5 @@
> +// == Successful cases ==
> +
> +test([Map, WeakMap], {
> +    nextVal: { value: [{}, {}], done: false },
> +    closed: false,

TIL return() is not invoked when there is no error.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Close iterator after error in {Map,Set,WeakMap,WeakSet} constructors.

Updated testcase to be more descriptive about exception, and also added the testcase for IteratorClose and which exception to be used.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Close iterator after error in Array.from.

same changes

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Close iterator after error in Promise.{all,race}.

We should return more information from IteratorClose, since `completion` value is ignored when GetMethod fails at IteratorClose step 3.
So, I changed closeThrow not to handle pending exception.
Now returning true means there's no pending exception.

Now, caller saves exception, and if closeThrow suceeds, restores it.
that corresponds to IteratorClose step 4 and step 6.

Comment 14

[Till Schneidereit [:till]]

Comment on attachment 8811898 [details] [diff] [review]
Part 3: Close iterator after error in Promise.{all,race}.

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

This looks good as far as functionality is concerned. I'm still not fond of how closeThrow has to be used though, and would like to see all the exception handling moved inside it. Setting r+ in case I'm overlooking something that makes that infeasible. If it does work, I'd like to take another quick look.

::: js/src/builtin/Promise.cpp
@@ +1403,5 @@
>          // Step 8.a.
> +        if (!done && cx->isExceptionPending()) {
> +            RootedValue exc(cx);
> +            if (!cx->getPendingException(&exc))
> +                return false;

Uncatchable exceptions shouldn't prevent iterator closing, I think.

@@ +1404,5 @@
> +        if (!done && cx->isExceptionPending()) {
> +            RootedValue exc(cx);
> +            if (!cx->getPendingException(&exc))
> +                return false;
> +            cx->clearPendingException();

This can use js::GetAndClearException, I think

@@ +1407,5 @@
> +                return false;
> +            cx->clearPendingException();
> +
> +            if (iter.closeThrow())
> +                cx->setPendingException(exc);

I don't understand why we can't do all of this inside iter.closeThrow. Ideally we'd just have `if (!done) iter.closeThrow()` here and have closeThrow deal with uncatchable exceptions and restoring exception state before returning.

Comment 15

[Tom S [:evilpie]]

Comment on attachment 8811895 [details] [diff] [review]
Part 2: Close iterator after error in Array.from.

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

::: js/src/builtin/Array.js
@@ +816,5 @@
> +            // Step 5.e.i.
> +            // Disabled for performance reason.  We won't hit this case on
> +            // normal array, since _DefineDataProperty will throw before it.
> +            // We could hit this when |A| is a proxy and it ignores
> +            // |_DefineDataProperty|, but it happens only after too long loop.

I figure we should at least open a bug so somebody can maybe optimize this away in the JITs.

::: js/src/tests/ecma_6/Array/from-iterator-close.js
@@ +84,5 @@
> +// ES 2017 draft 7.4.6 step 3.
> +// if GetMethod fails, the thrown value should be used.
> +test(MyArray, {
> +    nextVal: { value: 1, done: false },
> +    modifier: (iterator, iterable) => {

You can just use the method syntax if you want.

Comment 16

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Till Schneidereit [:till] from comment #14)
> This looks good as far as functionality is concerned. I'm still not fond of
> how closeThrow has to be used though, and would like to see all the
> exception handling moved inside it. Setting r+ in case I'm overlooking
> something that makes that infeasible. If it does work, I'd like to take
> another quick look.

I'll put everything in closeThrow.
I thought it needs to be done separately, but apparently it doesn't.


(In reply to Tom Schuster [:evilpie] from comment #15)
> @@ +816,5 @@
> > +            // Step 5.e.i.
> > +            // Disabled for performance reason.  We won't hit this case on
> > +            // normal array, since _DefineDataProperty will throw before it.
> > +            // We could hit this when |A| is a proxy and it ignores
> > +            // |_DefineDataProperty|, but it happens only after too long loop.
> 
> I figure we should at least open a bug so somebody can maybe optimize this
> away in the JITs.

I'll file a bug after landing this patch.


> ::: js/src/tests/ecma_6/Array/from-iterator-close.js
> @@ +84,5 @@
> > +// ES 2017 draft 7.4.6 step 3.
> > +// if GetMethod fails, the thrown value should be used.
> > +test(MyArray, {
> > +    nextVal: { value: 1, done: false },
> > +    modifier: (iterator, iterable) => {
> 
> You can just use the method syntax if you want.

I don't think method is appropriate there, since the function is not called on the object,
so I'd prefer explicitly decouple the function from the object.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Close iterator after error in Promise.{all,race}.

moved exception handling to closeThrow.

Comment 18

[Till Schneidereit [:till]]

Comment on attachment 8811978 [details] [diff] [review]
Part 3: Close iterator after error in Promise.{all,race}.

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

Much nicer, thank you! r=me with or without the last bit of feedback addressed.

::: js/src/vm/ForOfIterator.cpp
@@ +160,5 @@
> +
> +    RootedValue completionException(cx_);
> +    if (cx_->isExceptionPending()) {
> +        if (!GetAndClearException(cx_, &completionException))
> +            return;

This should still not be required: we can't restore uncatchable exceptions, and that sucks, but it shouldn't stop us from closing the iterator. I think just setting completionException to `undefined` here (so we don't trigger the same-compartment assert in setPendingException) should work.

Comment 19

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aae03265dbef5d2d80ccfff095071fda5d1e3673
Bug 1180306 - Part 1: Close iterator after error in {Map,Set,WeakMap,WeakSet} constructors. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/f861cf587ad19b39da9d1676e32e31df366dc606
Bug 1180306 - Part 2: Close iterator after error in Array.from. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/cad8259d02777425759f822e4f1c9221c0fc7ceb
Bug 1180306 - Part 3: Close iterator after error in Promise.{all,race}. r=till

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aae03265dbef
https://hg.mozilla.org/mozilla-central/rev/f861cf587ad1
https://hg.mozilla.org/mozilla-central/rev/cad8259d0277