Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: evilpie, Assigned: shu)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
mozilla53
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

()

Attachments

(7 attachments, 4 obsolete attachments)

8.34 KB, patch
arai
: review+
Details | Diff | Splinter Review
11.30 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.01 KB, patch
arai
: review+
Details | Diff | Splinter Review
51.10 KB, patch
arai
: review+
Details | Diff | Splinter Review
33.92 KB, patch
arai
: review+
Details | Diff | Splinter Review
22.43 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.04 KB, patch
arai
: review+
Details | Diff | Splinter Review
Iterators in ES6 now have returning/closing semantics like we have for our old generators.
Depends on: 1180306
Some places where we need to close iterators:

- for-of break
- for-of throw
- array destructuring
- yield *
- yield * with throw()
- Map & Set constructors
- WeakMap & WeakSet constructors
- Array.from
Duplicate of this bug: 1163348
Somebody needs to start looking into this. We are missing quite a lot of points on kangax just because of this.
We haven't implemented because TC39 keeps hovering on the edge of removing it from the standard.
Can we get an update on this?
Depends on: 1115868
Is somebody interested in implementing this? This is definitely one of the biggest missing pieces that we have left for ES6.
Assignee: nobody → shu
(In reply to Tom Schuster [:evilpie] from comment #6)
> Is somebody interested in implementing this? This is definitely one of the
> biggest missing pieces that we have left for ES6.

Given this statement I've put P2 (fix in next release) on this bug. I'm not sure how much work is involved to get this fixed, therefore I didn't know if P1 (fix in this release) was possible.
Priority: -- → P2
(In reply to Hannes Verschore [:h4writer] from comment #7)
> (In reply to Tom Schuster [:evilpie] from comment #6)
> > Is somebody interested in implementing this? This is definitely one of the
> > biggest missing pieces that we have left for ES6.
> 
> Given this statement I've put P2 (fix in next release) on this bug. I'm not
> sure how much work is involved to get this fixed, therefore I didn't know if
> P1 (fix in this release) was possible.

I might be able to get to this before next train. I agree with P2 for now, given other deadlines I have.
Blocks: test262
(In reply to Shu-yu Guo [:shu] from comment #8)
> I might be able to get to this before next train. I agree with P2 for now,
> given other deadlines I have.

Can I move this to P1? As I understood you wanted to get this done in FF53 right?
Priority: P2 → P1
IteratorClose in built-in are implemented in bug 1180306.

remaining things are:
  * array destructuring
  * for-of
  * yield*

all those are now handled in bytecode.
See Also: → 1322069
Duplicate of this bug: 1326109
I was bitten by the difference between Firefox and Chrome, when I reused a generator after breaking out of a for...of loop.

To save others some trouble, I documented the expected (specified) behavior of generators in for-of loops, and added a compatibility note at
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of#Do_not_reuse_generators
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of#Browser_compatibility

Once this bug is fixed, please edit these sections to mention the Firefox version in which this bug is fixed.
(In reply to Rob Wu [:robwu] from comment #12)
> I was bitten by the difference between Firefox and Chrome, when I reused a
> generator after breaking out of a for...of loop.
> 
> To save others some trouble, I documented the expected (specified) behavior
> of generators in for-of loops, and added a compatibility note at
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> for...of#Do_not_reuse_generators
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> for...of#Browser_compatibility
> 
> Once this bug is fixed, please edit these sections to mention the Firefox
> version in which this bug is fixed.

Thank you for taking the time!
While doing this I ran into a snag implementing calling the "return" method for Generator.prototype.return() inside yield*. If we get a forced close inside a yield*, we need to call the "return" method on the iterator, if it exists. Moreover, the return value of calling "return" can result in iteration of yield* continuing. All in all, this means generator closing can no longer be implemented as an uncatchable exception.

My plan of implementation is:
  - Emit a new catch-like block in the yield* bytecode.
  - Make a new kind of try note, JSTRY_YIELD_STAR_RETURN, which will be the only kind of trynote that can intercept generator closing.
  - Implement the return logic from the spec in this new catch-like block.

Jan, you implemented Generator.return. Does the above sound reasonable?
Flags: needinfo?(jdemooij)
(In reply to Shu-yu Guo [:shu] from comment #14)
> Jan, you implemented Generator.return. Does the above sound reasonable?

You don't want to use a finally block for perf reasons? In a WIP patch for this I emitted a finally block where we checked whether the value on top of the stack was the generator-closing MagicValue: https://bug1115868.bmoattachments.org/attachment.cgi?id=8549529

If that doesn't work somehow or you think it's too slow, JSTRY_YIELD_STAR_RETURN seems reasonable.

FWIW, here's the V8 code desugaring yield*: https://github.com/v8/v8/blob/0663c225f8550ff8329c60a60f71038d9ae7bb74/src/parsing/parser.cc#L4236, not sure if they handle this correctly.
Flags: needinfo?(jdemooij)
Non-local jumps, i.e. |break| and |return| statements, are implemented
by emitting IteratorClose in bytecode.

On throws, the new trynote JSTRY_ITERCLOSE signals that there's an
iterator on the top of the stack that needs IteratorClose. Note that
this only applies to exceptions thrown from non-iterator code. If
iter.next or iter.return itself throws, IteratorClose should *not* be
called. This is why we can't use JSTRY_FOR_OF.
Attachment #8824291 - Flags: review?(arai.unmht)
This is to prepare for reimplementing several self-hosted methods to use
for-of.
Attachment #8824292 - Flags: review?(arai.unmht)
Since result.done is always needed now, always emit the code that pushes
it on the stack.

For throwing, like for-of, IteratorClose is only called when non-iterator
code throws.
Attachment #8824294 - Flags: review?(arai.unmht)
The forced return path is implemented as finally block. Unlike
IteratorClose, this path checks the result returned by the "return"
method. If !result.done, the yield loop continues.

This also changes checking for the "throw" method with a JSOP_CALLPROP
instead of a JSOP_IN to be in line with current spec.
Attachment #8824295 - Flags: review?(jdemooij)
Jan, the bytecode emitted for the yield* is found in emitIteratorClose in the for-of patch. Forgot to split it out, sorry.
Comment on attachment 8824295 [details] [diff] [review]
Implement calling IteratorClose and "return" on iterators in yield*.

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

looks like the JIT implementation doesn't match to interpreter implementation.

::: js/src/jit/BaselineCompiler.cpp
@@ +4006,5 @@
>  
>  bool
> +BaselineCompiler::emit_JSOP_ISGENCLOSING()
> +{
> +    return emit_JSOP_ISNOITER();

ISNOITER uses the top of the stack value, and ISGENCLOSING uses the second value.
so they should be different implementation. (or helper function that takes stack depth?)

::: js/src/jit/IonBuilder.cpp
@@ +2190,5 @@
>        case JSOP_MOREITER:
>          return jsop_itermore();
>  
>        case JSOP_ISNOITER:
> +      case JSOP_ISGENCLOSING:

here too, they're different.

::: js/src/vm/Opcodes.h
@@ +1929,1 @@
>      /*

stack convention should be `val, ignored => val, ignored, res`, and nuses=2, ndefs=3.
Attachment #8824296 - Flags: review?(arai.unmht) → review+
Comment on attachment 8824291 [details] [diff] [review]
Implement IteratorClose for for-of.

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

Looks great :)

only some comment fixes.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +289,5 @@
> +    // IteratorClose. Since non-local jumps like break and return call
> +    // IteratorClose, whenever a non-local jump is emitted, we must terminate
> +    // the current JSTRY_ITERCLOSE note to skip the non-local jump code, then
> +    // start a new one.
> +    ptrdiff_t iterCloseTryStart_;

This needs some diagram or example that describes what "terminate the current note" and "start a new one" mean, also, something that clarifies that startNewIterCloseTryNote should be called after finishIterCloseTryNote.

how about following?


  for (x of y) {
      ...             instantiate ForOfLoopControl
      ...         +   <-- iterCloseTryStart_ points here
      ...         ^
      ...         |
      if (...)    v
                  +   call finishIterCloseTryNote before |break|
                      above range is noted with JSTRY_ITERCLOSE

        break;    <-- break and IteratorClose are not inside JSTRY_ITERCLOSE note

                      call startNewIterCloseTryNote after |break|
                  +   <-- next iterCloseTryStart_ points here
      ...         |
      ...         ~
  }

@@ +2313,5 @@
>            case StatementKind::ForOfLoop:
> +            hasForOfLoops = true;
> +            if (!control->as<ForOfLoopControl>().finishIterCloseTryNote(bce_))
> +                return false;
> +            // The iterator and the current value are on the stack.

Can we use stack comment (including initial condition) like other places?
  // ITER VALUE

@@ +2322,4 @@
>              break;
>  
>            case StatementKind::ForInLoop:
> +            // The iterator and the current value are on the stack.

here too.

@@ +2338,5 @@
> +        hasForOfLoops = true;
> +        if (!target->as<ForOfLoopControl>().finishIterCloseTryNote(bce_))
> +            return false;
> +
> +        // The iterator and the current value are on the stack.

and here.
so that it clarifies we're doing different thing for target and other for-of.

::: js/src/jsiter.cpp
@@ +1227,5 @@
>              // Nothing sensible to do.
>              return true;
>          }
>          return LegacyGeneratorObject::close(cx, obj);
> +    } else {

LegacyGeneratorObject that implements ES6 iterator protocol will be caught by previous branch |obj->is<LegacyGeneratorObject>()| even if ES6 iterator protocol was used until here.

  var g = (function() { yield 1; })();
  var iterCalled = false;
  var nextCalled = false;
  var returnCalled = false;
  Object.defineProperty(g, Symbol.iterator, {
    value: () => {
      iterCalled = true;
      return g;
    }
  });
  Object.defineProperty(g, "next", {
    value: () => {
      nextCalled = true;
      return { done: false, value: 10 };
    }
  });
  Object.defineProperty(g, "return", {
    value: () => {
      returnCalled = true;
      return {};
    }
  });
  try {
    for (let a of g)
      throw 10;
  } catch (e) {}
  assertEq(iterCalled, true);
  assertEq(nextCalled, true);
  assertEq(returnCalled, true); // fails

We could just ignore the case, since it won't happen in wild tho, adding some comment will be nice.
Attachment #8824291 - Flags: review?(arai.unmht) → review+
Attachment #8824292 - Flags: review?(arai.unmht) → review+
Comment on attachment 8824293 [details] [diff] [review]
Convert self-hosted code that need to call IteratorClose to use for-of.

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

r+ with the comment addressed :)

::: js/src/builtin/Array.js
@@ +824,5 @@
>  
> +            // Steps 5.e.ii (reordered), 5.e.viii.
> +            _DefineDataProperty(A, k++, mappedValue);
> +
> +            k++;

k is already incremented in _DefineDataProperty arguments.
please remove this extra one.
Attachment #8824293 - Flags: review?(arai.unmht) → review+
Comment on attachment 8824294 [details] [diff] [review]
Implement IteratorClose for array destructuring.

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

it's nice to see this fixed without adding extra many branches and try-finally :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5060,5 @@
>      //   SetOrInitialize(lref, value);
> +    //
> +    //   // === emitted after loop ===
> +    //   if (!done)
> +    //      IteratorClose(iter);

Can you update the comments for each element to mention which expression is noted by ITERCLOSE ?
it would make it clear that IteratorClose is also handled there, not only after loop.

@@ +5181,2 @@
>              }
>              if (!ifAlreadyDone.emitIfElse())                      // ... OBJ ITER ?DONE *LREF

like this one, "?DONE" should be "DONE", since it's always there.

@@ +5269,5 @@
> +                                                                  // ... OBJ ITER
> +        if (!emitIteratorClose())                                 // ... OBJ
> +            return false;
> +    } else {
> +        // Otherwise, the last ?DONE value is on top of the stack. If not ?DONE,

"DONE" is always on the stack, so please remove "?" before "DONE", here and other stack comments in this method.
Attachment #8824294 - Flags: review?(arai.unmht) → review+
Comment on attachment 8824291 [details] [diff] [review]
Implement IteratorClose for for-of.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4788,5 @@
> +    // Do nothing if "return" is undefined.
> +    IfThenElseEmitter ifReturnMethodIsDefined(this);
> +    if (!emit1(JSOP_DUP))                                 // ... ITER RET RET
> +        return false;
> +    if (!emit1(JSOP_UNDEFINED))                           // ... ITER RET RET UNDEFINED

IteratorClose calls GetMethod to retrieve the "return" method, so we also need to test for |ret !== null|.

::: js/src/jsiter.cpp
@@ +1241,5 @@
> +
> +        // Step 4.
> +        //
> +        // Do nothing if "return" is undefined.
> +        if (returnMethod.isUndefined())

s/isUndefined/isNullOrUndefined/

::: js/src/tests/ecma_6/Statements/for-of-iterator-return.js
@@ +11,5 @@
> +            return {};
> +        }
> +    });
> +
> +    // break calls iter.return

Maybe also add a test to test continue calls return()? |L: do for (var x of iterable) continue L; while(false);|
The following test262 (still) fail with all patches applied:
    test262/built-ins/Map/iterator-close-after-set-failure.js
    test262/built-ins/Map/iterator-item-first-entry-returns-abrupt.js
    test262/built-ins/Map/iterator-item-second-entry-returns-abrupt.js
    test262/built-ins/Set/set-iterator-close-after-add-failure.js
    test262/built-ins/Array/from/iter-map-fn-err.js
    test262/built-ins/Array/from/iter-map-fn-return.js
    test262/built-ins/Array/from/from-array.js
    test262/built-ins/Array/from/iter-set-length.js
    test262/built-ins/Array/from/elements-updated-after.js
    test262/built-ins/Array/from/iter-set-elem-prop.js
    test262/built-ins/Array/from/iter-map-fn-args.js
    test262/built-ins/Array/from/elements-deleted-after.js
    test262/built-ins/Array/from/from-string.js
    test262/built-ins/Array/from/source-object-iterator-2.js
    test262/built-ins/Array/from/source-array-boundary.js
    test262/built-ins/WeakMap/iterator-close-after-set-failure.js
    test262/built-ins/WeakMap/iterator-item-first-entry-returns-abrupt.js
    test262/built-ins/WeakMap/iterator-item-second-entry-returns-abrupt.js
    test262/built-ins/WeakSet/iterator-close-after-add-failure.js
    test262/language/statements/for-of/iterator-next-result-value-attr-error.js
    test262/language/statements/for-of/continue-from-try.js
    test262/language/statements/for-of/dstr-array-elem-trlg-iter-rest-thrw-close-err.js
    test262/language/statements/for-of/dstr-array-rest-nested-array-iter-thrw-close-skip.js
    test262/language/statements/for-of/continue-from-finally.js
    test262/language/statements/for-of/body-put-error.js
    test262/language/statements/for-of/dstr-array-elem-iter-thrw-close-err.js
    test262/language/statements/for-of/dstr-array-elem-trlg-iter-list-thrw-close-err.js
    test262/language/statements/for-of/dstr-array-rest-put-prop-ref-user-err-iter-close-skip.js
    test262/language/statements/for-of/continue-from-catch.js
    test262/language/statements/for-of/continue.js
    test262/language/statements/for-of/dstr-array-rest-iter-thrw-close-err.js
    test262/language/statements/for-of/dstr-array-rest-lref-err.js
    test262/language/statements/for-of/dstr-array-elem-iter-thrw-close.js
    test262/language/statements/for-of/dstr-array-elem-trlg-iter-list-thrw-close.js
    test262/language/statements/for-of/body-dstr-assign-error.js
    test262/language/statements/for-of/dstr-array-rest-iter-thrw-close.js
    test262/language/statements/for-of/dstr-array-elem-trlg-iter-rest-thrw-close.js
    test262/language/expressions/yield/star-rhs-iter-thrw-violation-no-rtrn.js
    test262/language/expressions/yield/star-rhs-iter-rtrn-res-value-final.js
    test262/language/expressions/yield/star-rhs-iter-rtrn-rtrn-invoke.js
    test262/language/expressions/yield/star-rhs-iter-rtrn-res-done-no-value.js
    test262/language/expressions/yield/star-rhs-iter-thrw-thrw-call-non-obj.js
    test262/language/expressions/yield/star-rhs-iter-rtrn-res-value-err.js
    test262/language/expressions/assignment/dstr-array-elem-trlg-iter-rest-thrw-close-err.js
    test262/language/expressions/assignment/dstr-array-rest-nested-array-iter-thrw-close-skip.js
    test262/language/expressions/assignment/dstr-array-elem-iter-thrw-close-err.js
    test262/language/expressions/assignment/dstr-array-elem-trlg-iter-list-thrw-close-err.js
    test262/language/expressions/assignment/dstr-array-rest-put-prop-ref-user-err-iter-close-skip.js
    test262/language/expressions/assignment/dstr-array-rest-iter-thrw-close-err.js
    test262/language/expressions/assignment/dstr-array-rest-lref-err.js
    test262/language/expressions/assignment/dstr-array-elem-iter-thrw-close.js
    test262/language/expressions/assignment/dstr-array-elem-trlg-iter-list-thrw-close.js
    test262/language/expressions/assignment/dstr-array-rest-iter-thrw-close.js
    test262/language/expressions/assignment/dstr-array-elem-trlg-iter-rest-thrw-close.js
(In reply to Tooru Fujisawa [:arai] from comment #24)
> Comment on attachment 8824291 [details] [diff] [review]
> Implement IteratorClose for for-of.
> 
> Review of attachment 8824291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great :)
> 
> only some comment fixes.
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +289,5 @@
> > +    // IteratorClose. Since non-local jumps like break and return call
> > +    // IteratorClose, whenever a non-local jump is emitted, we must terminate
> > +    // the current JSTRY_ITERCLOSE note to skip the non-local jump code, then
> > +    // start a new one.
> > +    ptrdiff_t iterCloseTryStart_;
> 
> This needs some diagram or example that describes what "terminate the
> current note" and "start a new one" mean, also, something that clarifies
> that startNewIterCloseTryNote should be called after finishIterCloseTryNote.
> 
> how about following?
> 
> 
>   for (x of y) {
>       ...             instantiate ForOfLoopControl
>       ...         +   <-- iterCloseTryStart_ points here
>       ...         ^
>       ...         |
>       if (...)    v
>                   +   call finishIterCloseTryNote before |break|
>                       above range is noted with JSTRY_ITERCLOSE
> 
>         break;    <-- break and IteratorClose are not inside JSTRY_ITERCLOSE
> note
> 
>                       call startNewIterCloseTryNote after |break|
>                   +   <-- next iterCloseTryStart_ points here
>       ...         |
>       ...         ~
>   }
> 

Beautiful, thank you for the diagram. :)
Non-local jumps, i.e. |break| and |return| statements, are implemented
by emitting IteratorClose in bytecode.

On throws, the new trynote JSTRY_ITERCLOSE signals that there's an
iterator on the top of the stack that needs IteratorClose. Note that
this only applies to exceptions thrown from non-iterator code. If
iter.next or iter.return itself throws, IteratorClose should *not* be
called. This is why we can't use JSTRY_FOR_OF.
Attachment #8824622 - Flags: review?(arai.unmht)
Attachment #8824291 - Attachment is obsolete: true
This and the for-of parts could use re-review. Had to fix some correctness bugs
in |continue| and forced return that anba pointed out.
Attachment #8824623 - Flags: review?(arai.unmht)
Attachment #8824295 - Attachment is obsolete: true
Attachment #8824295 - Flags: review?(jdemooij)
I haven't gotten through all the test262 failures yet -- early next week at the latest.
Comment on attachment 8824622 [details] [diff] [review]
Implement IteratorClose for for-of.

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

Great :D

::: js/src/frontend/BytecodeEmitter.cpp
@@ +301,5 @@
> +    //     if (...)    v
> +    //                 +   call finishIterCloseTryNote before |break|
> +    //                     above range is noted with JSTRY_ITERCLOSE
> +    //
> +    //       break;    <-- break and IteratorClose are not inside JSTRY_ITERCLOSE note

sorry, can you wrap this line to 79 chars?

@@ +2242,5 @@
> +
> +        // A 'break' or 'return' statement does call IteratorClose for the
> +        // loop it is breaking out of or returning from, i.e. including the
> +        // target loop.
> +        EmitIteratorCloseExcludingAtTarget

Looks like the comments are swapped between EmitIteratorCloseIncludingAtTarget and EmitIteratorCloseExcludingAtTarget.

How about using throw/continue/break-or-return in these names instead?
with enum NonLocalExitKind typename or something, so it's clear that when to specify which kind.

and also maybe adding helper function/method like shouldEmitIteratorCloseForTarget/shouldEmitIteratorCloseForInnerLoop that receive the enum value and returns bool, to use in prepareForNonLocalJump.

@@ +2361,5 @@
> +                    return false;
> +            } else {
> +                if (!bce_->emit1(JSOP_POP))               // ... ITER
> +                    return false;
> +                if (!bce_->emit1(JSOP_POP))               // ...

emitUint16Operand(JSOP_POPN, 2) maybe?
Attachment #8824622 - Flags: review?(arai.unmht) → review+
Comment on attachment 8824623 [details] [diff] [review]
Implement calling IteratorClose and "return" on iterators in yield*.

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

looks almost good, except the stack depth in checkThrow branch.
also forwarding review to jandem.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8060,1 @@
>      // THROW? = 'throw' in ITER

this comment is obsolete.

@@ +8075,2 @@
>      JumpList checkThrow;
> +    if (!emitJump(JSOP_IFEQ, &checkThrow))                       // ITER RESULT EXCEPTION ITER THROW

it would be nice if we can use IfThenElseEmitter here.

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

This branch hits `Assertion failure: stackDepth == depth` in js/src/jit/BytecodeAnalysis.h:30.

since the stack doesn't balance.
we may need to push undefined or something to balance the stack depth (or perhaps there's some trick to avoid that?)

@@ +8084,1 @@
>      if (!emitJumpTargetAndPatch(checkThrow))                     // delegate:

since "delegate" comment is removed above, we could move the stack comment " // ITER OLDRESULT EXCEPTION ITER THROW" from the next line to here.
Attachment #8824623 - Flags: review?(jdemooij)
Attachment #8824623 - Flags: review?(arai.unmht)
Attachment #8824623 - Flags: feedback+
Comment on attachment 8824623 [details] [diff] [review]
Implement calling IteratorClose and "return" on iterators in yield*.

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

JSOP_ISGENCLOSING is added here and used in the next patch, but that's fine.

::: js/src/jit/BaselineCompiler.cpp
@@ +4006,5 @@
>  
>  bool
> +BaselineCompiler::emit_JSOP_ISGENCLOSING()
> +{
> +    return emit_JSOP_ISNOITER();

Since JSOP_ISGENCLOSING and JSOP_ISNOITER are pretty much unrelated, it would be nice to refactor this so we have emitIsMagicValue() or something, and then call that for both JSOP_ISGENCLOSING and JSOP_ISNOITER.

::: js/src/jit/IonBuilder.cpp
@@ +2195,1 @@
>          return jsop_isnoiter();

This will fail the following assert in IonBuilder::jsop_isnoiter:

    MOZ_ASSERT(def->isIteratorMore());

We should also rename the MIsNoIter stuff etc... Can we actually compile scripts with this op in Ion?
Attachment #8824623 - Flags: review?(jdemooij)
(In reply to Tooru Fujisawa [:arai] from comment #33)
> Comment on attachment 8824622 [details] [diff] [review]
> Implement IteratorClose for for-of.
> 
> Review of attachment 8824622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great :D
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +301,5 @@
> > +    //     if (...)    v
> > +    //                 +   call finishIterCloseTryNote before |break|
> > +    //                     above range is noted with JSTRY_ITERCLOSE
> > +    //
> > +    //       break;    <-- break and IteratorClose are not inside JSTRY_ITERCLOSE note
> 
> sorry, can you wrap this line to 79 chars?
> 
> @@ +2242,5 @@
> > +
> > +        // A 'break' or 'return' statement does call IteratorClose for the
> > +        // loop it is breaking out of or returning from, i.e. including the
> > +        // target loop.
> > +        EmitIteratorCloseExcludingAtTarget
> 
> Looks like the comments are swapped between
> EmitIteratorCloseIncludingAtTarget and EmitIteratorCloseExcludingAtTarget.
> 
> How about using throw/continue/break-or-return in these names instead?
> with enum NonLocalExitKind typename or something, so it's clear that when to
> specify which kind.
> 
> and also maybe adding helper function/method like
> shouldEmitIteratorCloseForTarget/shouldEmitIteratorCloseForInnerLoop that
> receive the enum value and returns bool, to use in prepareForNonLocalJump.

Sure, I'll add a Kind enum.

> 
> @@ +2361,5 @@
> > +                    return false;
> > +            } else {
> > +                if (!bce_->emit1(JSOP_POP))               // ... ITER
> > +                    return false;
> > +                if (!bce_->emit1(JSOP_POP))               // ...
> 
> emitUint16Operand(JSOP_POPN, 2) maybe?

I chose |pop; pop| because it's 2 bytes, and |popn 2| is 3 bytes.
(In reply to Jan de Mooij [:jandem] from comment #35)
> Comment on attachment 8824623 [details] [diff] [review]
> Implement calling IteratorClose and "return" on iterators in yield*.
> 
> Review of attachment 8824623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> JSOP_ISGENCLOSING is added here and used in the next patch, but that's fine.
> 
> ::: js/src/jit/BaselineCompiler.cpp
> @@ +4006,5 @@
> >  
> >  bool
> > +BaselineCompiler::emit_JSOP_ISGENCLOSING()
> > +{
> > +    return emit_JSOP_ISNOITER();
> 
> Since JSOP_ISGENCLOSING and JSOP_ISNOITER are pretty much unrelated, it
> would be nice to refactor this so we have emitIsMagicValue() or something,
> and then call that for both JSOP_ISGENCLOSING and JSOP_ISNOITER.
> 

Will do this refactoring.

> ::: js/src/jit/IonBuilder.cpp
> @@ +2195,1 @@
> >          return jsop_isnoiter();
> 
> This will fail the following assert in IonBuilder::jsop_isnoiter:
> 
>     MOZ_ASSERT(def->isIteratorMore());
> 
> We should also rename the MIsNoIter stuff etc... Can we actually compile
> scripts with this op in Ion?

Good point. GENCLOSING is only emitted inside a finally block with this patch, so Ion will never compile it anyways. I'll remove that line from IonBuilder.
destructuring was wrong. The new implementation always keeps the iter obj and
the done value on the stack. There's a new JSTRY_DESTRUCTURING_ITERCLOSE that
unlike the for-of JSTRY_ITERCLOSE, only calls IteratorClose if !done.
Attachment #8825589 - Flags: review?(arai.unmht)
Attachment #8824294 - Attachment is obsolete: true
generator, since we implement that as an uncatchable magic exception.
Attachment #8825591 - Flags: review?(jdemooij)
Attachment #8824623 - Attachment is obsolete: true
Locally, all the previously failing IteratorClose test262 tests now pass.
Attachment #8825591 - Flags: review?(jdemooij) → review+
Comment on attachment 8825589 [details] [diff] [review]
As pointed out by test262 failures, the previous implementation for

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

Thank you for cleanup on array destructuring code :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5123,5 @@
>      //       value = result.value;
>      //   }
>      //
>      //   if (value === undefined)
>      //     value = y;

evaluating default (y here) is covered by trynote.
(only comment fix)
Attachment #8825589 - Flags: review?(arai.unmht) → review+
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b28ef89ebda2
Implement IteratorClose for for-of. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1304f6c59821
Rename allowContentSpread to allowContentIter. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/314efb239b64
Convert self-hosted code that need to call IteratorClose to use for-of. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7872a2456195
Implement IteratorClose for array destructuring. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9eef2331ae6
Implement calling IteratorClose and "return" on iterators in yield*. (r=jandem)
https://hg.mozilla.org/integration/mozilla-inbound/rev/408a37107c7f
Implement JSOP_PICK and JSOP_UNPICK in the expression decompiler. (r=arai)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6584da54c13d
Backed out changeset 408a37107c7f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/076bdd3f1f7a
Backed out changeset d9eef2331ae6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5fc270c79c
Backed out changeset 7872a2456195 
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db3783de0f2
Backed out changeset 314efb239b64 
https://hg.mozilla.org/integration/mozilla-inbound/rev/831c8cfd2512
Backed out changeset 1304f6c59821 
https://hg.mozilla.org/integration/mozilla-inbound/rev/feb27da5b04c
Backed out changeset b28ef89ebda2 since it seems this made browser_console_addonsdk_loader_exception.js | Test timed out - more worse
backed out because seems this somehow caused https://treeherder.mozilla.org/logviewer.html#?job_id=68732808&repo=mozilla-inbound failing a lot
Flags: needinfo?(shu)
The devtools failure was caused by the conversion of self-hosted code from
using manual while loops to for-of. In the manual while loop version, we'd
throw JSMSG_NOT_ITERABLE with DecompileArg(0, iterable), which decompiles the
argument name correctly. In for-of, the error is generated in bytecode on the
argument, resulting in a different error message.

For example, |new Map(42)| used to throw "42 is not iterable", and now throws
"iterable is not iterable", because the 1st argument in the self-hosted
MapConstructorInit is named |iterable|.

This patch changes the behavior of the expression decompiler in self-hosted
scripts to *always* try decompiling argument slots one frame up.
Attachment #8826786 - Flags: review?(arai.unmht)
Flags: needinfo?(shu)
Comment on attachment 8826786 [details] [diff] [review]
Always decompile argument names in self-hosted code in the caller frame.

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

::: js/src/jsopcode.cpp
@@ +1272,5 @@
> +            // not succeed.
> +            if (result) {
> +                if (!write(result))
> +                    return false;
> +                js_free(result);

`result` should be freed even if `write` fails.
Attachment #8826786 - Flags: review?(arai.unmht) → review+
(In reply to Tooru Fujisawa [:arai] from comment #46)
> Comment on attachment 8826786 [details] [diff] [review]
> Always decompile argument names in self-hosted code in the caller frame.
> 
> Review of attachment 8826786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsopcode.cpp
> @@ +1272,5 @@
> > +            // not succeed.
> > +            if (result) {
> > +                if (!write(result))
> > +                    return false;
> > +                js_free(result);
> 
> `result` should be freed even if `write` fails.

Good catch. Thanks!
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/757b50c0ee48
Implement IteratorClose for for-of. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad6c93e5162
Rename allowContentSpread to allowContentIter. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d332a5b2e8
Convert self-hosted code that need to call IteratorClose to use for-of. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce293b3c0a8b
Implement IteratorClose for array destructuring. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0dc4150f8ac
Implement calling IteratorClose and "return" on iterators in yield*. (r=jandem)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9c4069ed0f
Implement JSOP_PICK and JSOP_UNPICK in the expression decompiler. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/142dbb4bffc0
Always decompile argument names in self-hosted code in the caller frame. (r=arai)
Depends on: 1331444
Depends on: 1332155
Depends on: 1332881
Depends on: 1333946
Depends on: 1334799
Depends on: 1338796
Depends on: 1342049
Depends on: 1346862
Depends on: 1360839
Depends on: 1357075
You need to log in before you can comment on or make changes to this bug.