Fix exception handling while destructuring

When an exception happens during destructuring, IteratorClose
needs to be called, unless the exception happened inside the
IteratorNext call (in that case the iterator is assumed to be
invalid and we shouldn't call close on it).

Implement this, by ensuring that we set the done return variable
of IteratorNext to true whenever IteratorNext throws an exception.
IteratorClose will check the done state and not do anything in that
case.

Change-Id: I73a27f855f2c4d3134b8cc8980e64bf797d03886
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

9 files changed, 96 insertions, 86 deletions

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index 274917dd0a..1d6e8819f5 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -638,6 +638,7 @@ void Codegen::destructureElementList(const Codegen::Reference &array, PatternEle
     Reference iterator = Reference::fromStackSlot(this);
     Reference iteratorValue = Reference::fromStackSlot(this);
     Reference iteratorDone = Reference::fromStackSlot(this);
+    Reference::storeConstOnStack(this, Encode(false), iteratorDone.stackSlot());
 
     array.loadInAccumulator();
     Instruction::GetIterator iteratorObjInstr;
@@ -645,60 +646,58 @@ void Codegen::destructureElementList(const Codegen::Reference &array, PatternEle
     bytecodeGenerator->addInstruction(iteratorObjInstr);
     iterator.storeConsumeAccumulator();
 
-    bool hadNext = false;
     bool hasRest = false;
 
     BytecodeGenerator::Label end = bytecodeGenerator->newLabel();
-
-    for (PatternElementList *p = bindingList; p; p = p->next) {
-        PatternElement *e = p->element;
-        for (Elision *elision = p->elision; elision; elision = elision->next) {
+    {
+        auto cleanup = [this, iterator, iteratorDone]() {
             iterator.loadInAccumulator();
-            Instruction::IteratorNext next;
-            next.value = iteratorValue.stackSlot();
-            bytecodeGenerator->addInstruction(next);
-            hadNext = true;
-            bool last = !elision->next && !e && !p->next;
-            if (last)
-                iteratorDone.storeConsumeAccumulator();
-        }
+            Instruction::IteratorClose close;
+            close.done = iteratorDone.stackSlot();
+            bytecodeGenerator->addInstruction(close);
+        };
 
-        if (!e)
-            continue;
+        ControlFlowUnwindCleanup flow(this, cleanup);
 
-        hadNext = true;
-        RegisterScope scope(this);
-        iterator.loadInAccumulator();
+        for (PatternElementList *p = bindingList; p; p = p->next) {
+            PatternElement *e = p->element;
+            for (Elision *elision = p->elision; elision; elision = elision->next) {
+                iterator.loadInAccumulator();
+                Instruction::IteratorNext next;
+                next.value = iteratorValue.stackSlot();
+                next.done = iteratorDone.stackSlot();
+                bytecodeGenerator->addInstruction(next);
+            }
 
-        if (e->type == PatternElement::RestElement) {
-            bytecodeGenerator->addInstruction(Instruction::DestructureRestElement());
-            initializeAndDestructureBindingElement(e, Reference::fromAccumulator(this), isDefinition);
-            hasRest = true;
-        } else {
-            Instruction::IteratorNext next;
-            next.value = iteratorValue.stackSlot();
-            bytecodeGenerator->addInstruction(next);
-            bool last = !p->next || (!p->next->elision && !p->next->element);
-            if (last)
-                iteratorDone.storeConsumeAccumulator();
-            initializeAndDestructureBindingElement(e, iteratorValue, isDefinition);
-            if (hasError) {
-                end.link();
-                return;
+            if (!e)
+                continue;
+
+            RegisterScope scope(this);
+            iterator.loadInAccumulator();
+
+            if (e->type == PatternElement::RestElement) {
+                Reference::fromConst(this, Encode(true)).storeOnStack(iteratorDone.stackSlot());
+                bytecodeGenerator->addInstruction(Instruction::DestructureRestElement());
+                initializeAndDestructureBindingElement(e, Reference::fromAccumulator(this), isDefinition);
+                hasRest = true;
+            } else {
+                Instruction::IteratorNext next;
+                next.value = iteratorValue.stackSlot();
+                next.done = iteratorDone.stackSlot();
+                bytecodeGenerator->addInstruction(next);
+                initializeAndDestructureBindingElement(e, iteratorValue, isDefinition);
+                if (hasError) {
+                    end.link();
+                    return;
+                }
             }
         }
-    }
 
-    if (!hadNext) {
-        Reference::storeConstOnStack(this, Encode(false), iteratorDone.stackSlot());
+        if (hasRest)
+            // no need to close the iterator
+            bytecodeGenerator->jump().link(end);
     }
 
-    if (!hasRest) {
-        iterator.loadInAccumulator();
-        Instruction::IteratorClose close;
-        close.done = iteratorDone.stackSlot();
-        bytecodeGenerator->addInstruction(close);
-    }
 
     end.link();
 }
@@ -1112,6 +1111,7 @@ bool Codegen::visit(ArrayPattern *ast)
             RegisterScope scope(this);
 
             Reference iterator = Reference::fromStackSlot(this);
+            Reference iteratorDone = Reference::fromConst(this, Encode(false)).storeOnStack();
             Reference lhsValue = Reference::fromStackSlot(this);
 
             // There should be a temporal block, so that variables declared in lhs shadow outside vars.
@@ -1134,14 +1134,13 @@ bool Codegen::visit(ArrayPattern *ast)
             BytecodeGenerator::Label done = bytecodeGenerator->newLabel();
 
             {
-                auto unwind = [this, iterator]() {
-                    Reference iteratorDone = Reference::fromConst(this, Encode(false)).storeOnStack();
+                auto cleanup = [this, iterator, iteratorDone]() {
                     iterator.loadInAccumulator();
                     Instruction::IteratorClose close;
                     close.done = iteratorDone.stackSlot();
                     bytecodeGenerator->addInstruction(close);
                 };
-                ControlFlowLoop flow(this, &end, &in, unwind);
+                ControlFlowLoop flow(this, &end, &in, cleanup);
                 bytecodeGenerator->jump().link(in);
 
                 BytecodeGenerator::Label body = bytecodeGenerator->label();
@@ -1153,6 +1152,7 @@ bool Codegen::visit(ArrayPattern *ast)
                 iterator.loadInAccumulator();
                 Instruction::IteratorNext next;
                 next.value = lhsValue.stackSlot();
+                next.done = iteratorDone.stackSlot();
                 bytecodeGenerator->addInstruction(next);
                 bytecodeGenerator->addJumpInstruction(Instruction::JumpFalse()).link(body);
                 bytecodeGenerator->jump().link(done);
@@ -3155,6 +3155,7 @@ bool Codegen::visit(ForEachStatement *ast)
     RegisterScope scope(this);
 
     Reference iterator = Reference::fromStackSlot(this);
+    Reference iteratorDone = Reference::fromConst(this, Encode(false)).storeOnStack();
     Reference lhsValue = Reference::fromStackSlot(this);
 
     // There should be a temporal block, so that variables declared in lhs shadow outside vars.
@@ -3178,16 +3179,15 @@ bool Codegen::visit(ForEachStatement *ast)
     BytecodeGenerator::Label done = bytecodeGenerator->newLabel();
 
     {
-        auto unwind = [ast, this, iterator]() {
+        auto cleanup = [ast, iterator, iteratorDone, this]() {
             if (ast->type == ForEachType::Of) {
-                Reference iteratorDone = Reference::fromConst(this, Encode(false)).storeOnStack();
                 iterator.loadInAccumulator();
                 Instruction::IteratorClose close;
                 close.done = iteratorDone.stackSlot();
                 bytecodeGenerator->addInstruction(close);
             }
         };
-        ControlFlowLoop flow(this, &end, &in, unwind);
+        ControlFlowLoop flow(this, &end, &in, cleanup);
         bytecodeGenerator->jump().link(in);
 
         BytecodeGenerator::Label body = bytecodeGenerator->label();
@@ -3227,6 +3227,7 @@ bool Codegen::visit(ForEachStatement *ast)
         iterator.loadInAccumulator();
         Instruction::IteratorNext next;
         next.value = lhsValue.stackSlot();
+        next.done = iteratorDone.stackSlot();
         bytecodeGenerator->addInstruction(next);
         bytecodeGenerator->addJumpInstruction(Instruction::JumpFalse()).link(body);
         bytecodeGenerator->jump().link(done);
diff --git a/src/qml/compiler/qv4compilercontrolflow_p.h b/src/qml/compiler/qv4compilercontrolflow_p.h
index 1884b33588..e7f3a18a6d 100644
--- a/src/qml/compiler/qv4compilercontrolflow_p.h
+++ b/src/qml/compiler/qv4compilercontrolflow_p.h
@@ -188,33 +188,42 @@ struct ControlFlowUnwind : public ControlFlow
     }
 };
 
-struct ControlFlowLoop : public ControlFlowUnwind
+struct ControlFlowUnwindCleanup : public ControlFlowUnwind
 {
-    QString loopLabel;
-    BytecodeGenerator::Label *breakLabel = nullptr;
-    BytecodeGenerator::Label *continueLabel = nullptr;
-    std::function<void()> unwind = nullptr;
+    std::function<void()> cleanup = nullptr;
 
-    ControlFlowLoop(Codegen *cg, BytecodeGenerator::Label *breakLabel, BytecodeGenerator::Label *continueLabel = nullptr, std::function<void()> unwind = nullptr)
-        : ControlFlowUnwind(cg, Loop), loopLabel(ControlFlow::loopLabel()), breakLabel(breakLabel), continueLabel(continueLabel), unwind(unwind)
+    ControlFlowUnwindCleanup(Codegen *cg, std::function<void()> cleanup, Type type = Block)
+        : ControlFlowUnwind(cg, type), cleanup(cleanup)
     {
-        if (unwind != nullptr) {
+        if (cleanup) {
             setupUnwindHandler();
             generator()->setUnwindHandler(&unwindLabel);
         }
     }
 
-    ~ControlFlowLoop() {
-        if (unwind != nullptr) {
+    ~ControlFlowUnwindCleanup() {
+        if (cleanup) {
             unwindLabel.link();
             generator()->setUnwindHandler(parentUnwindHandler());
-            unwind();
+            cleanup();
             emitUnwindHandler();
         }
     }
 
     bool requiresUnwind() override {
-        return unwind != nullptr;
+        return cleanup != nullptr;
+    }
+};
+
+struct ControlFlowLoop : public ControlFlowUnwindCleanup
+{
+    QString loopLabel;
+    BytecodeGenerator::Label *breakLabel = nullptr;
+    BytecodeGenerator::Label *continueLabel = nullptr;
+
+    ControlFlowLoop(Codegen *cg, BytecodeGenerator::Label *breakLabel, BytecodeGenerator::Label *continueLabel = nullptr, std::function<void()> cleanup = nullptr)
+        : ControlFlowUnwindCleanup(cg, cleanup, Loop), loopLabel(ControlFlow::loopLabel()), breakLabel(breakLabel), continueLabel(continueLabel)
+    {
     }
 
     BytecodeGenerator::Label getUnwindTarget(UnwindType type, const QString &label) override {
diff --git a/src/qml/compiler/qv4instr_moth.cpp b/src/qml/compiler/qv4instr_moth.cpp
index e70b246181..b09f1d09d0 100644
--- a/src/qml/compiler/qv4instr_moth.cpp
+++ b/src/qml/compiler/qv4instr_moth.cpp
@@ -464,7 +464,7 @@ void dumpBytecode(const char *code, int len, int nLocals, int nFormals, int /*st
         MOTH_END_INSTR(GetIterator)
 
         MOTH_BEGIN_INSTR(IteratorNext)
-            d << dumpRegister(value, nFormals);
+            d << dumpRegister(value, nFormals) << ", " << dumpRegister(done, nFormals);
         MOTH_END_INSTR(IteratorNext)
 
         MOTH_BEGIN_INSTR(IteratorClose)
diff --git a/src/qml/compiler/qv4instr_moth_p.h b/src/qml/compiler/qv4instr_moth_p.h
index 2690151a23..40313551e8 100644
--- a/src/qml/compiler/qv4instr_moth_p.h
+++ b/src/qml/compiler/qv4instr_moth_p.h
@@ -132,7 +132,7 @@ QT_BEGIN_NAMESPACE
 #define INSTR_PopScriptContext(op) INSTRUCTION(op, PopScriptContext, 0)
 #define INSTR_PopContext(op) INSTRUCTION(op, PopContext, 0)
 #define INSTR_GetIterator(op) INSTRUCTION(op, GetIterator, 1, iterator)
-#define INSTR_IteratorNext(op) INSTRUCTION(op, IteratorNext, 1, value)
+#define INSTR_IteratorNext(op) INSTRUCTION(op, IteratorNext, 2, value, done)
 #define INSTR_IteratorClose(op) INSTRUCTION(op, IteratorClose, 1, done)
 #define INSTR_DestructureRestElement(op) INSTRUCTION(op, DestructureRestElement, 0)
 #define INSTR_DeleteProperty(op) INSTRUCTION(op, DeleteProperty, 2, base, index)
diff --git a/src/qml/jit/qv4baselinejit.cpp b/src/qml/jit/qv4baselinejit.cpp
index 9e9c586982..d0ac169e4f 100644
--- a/src/qml/jit/qv4baselinejit.cpp
+++ b/src/qml/jit/qv4baselinejit.cpp
@@ -682,7 +682,7 @@ void BaselineJIT::generate_GetIterator(int iterator)
     as->checkException();
 }
 
-void BaselineJIT::generate_IteratorNext(int value)
+void BaselineJIT::generate_IteratorNext(int value, int done)
 {
     as->saveAccumulatorInFrame();
     as->prepareCallWithArgCount(3);
@@ -690,6 +690,7 @@ void BaselineJIT::generate_IteratorNext(int value)
     as->passAccumulatorAsArg(1);
     as->passEngineAsArg(0);
     BASELINEJIT_GENERATE_RUNTIME_CALL(Runtime::method_iteratorNext, CallResultDestination::InAccumulator);
+    as->storeReg(done);
     as->checkException();
 }
 
diff --git a/src/qml/jit/qv4baselinejit_p.h b/src/qml/jit/qv4baselinejit_p.h
index 972e9d07b7..7e3fcfa5e6 100644
--- a/src/qml/jit/qv4baselinejit_p.h
+++ b/src/qml/jit/qv4baselinejit_p.h
@@ -148,7 +148,7 @@ public:
     void generate_PopScriptContext() override;
     void generate_PopContext() override;
     void generate_GetIterator(int iterator) override;
-    void generate_IteratorNext(int value) override;
+    void generate_IteratorNext(int value, int done) override;
     void generate_IteratorClose(int done) override;
     void generate_DestructureRestElement() override;
     void generate_DeleteProperty(int base, int index) override;
diff --git a/src/qml/jsruntime/qv4runtime.cpp b/src/qml/jsruntime/qv4runtime.cpp
index 3a1dabeae3..59fff91e7b 100644
--- a/src/qml/jsruntime/qv4runtime.cpp
+++ b/src/qml/jsruntime/qv4runtime.cpp
@@ -748,28 +748,38 @@ ReturnedValue Runtime::method_getIterator(ExecutionEngine *engine, const Value &
 
 ReturnedValue Runtime::method_iteratorNext(ExecutionEngine *engine, const Value &iterator, Value *value)
 {
+    // if we throw an exception from here, return true, not undefined. This is to ensure iteratorDone is set to true
+    // and the stack unwinding won't close the iterator
     Q_ASSERT(iterator.isObject());
 
     Scope scope(engine);
     ScopedFunctionObject f(scope, static_cast<const Object &>(iterator).get(engine->id_next()));
-    if (!f)
-        return engine->throwTypeError();
+    if (!f) {
+        engine->throwTypeError();
+        return Encode(true);
+    }
     JSCallData cData(scope, 0, nullptr, &iterator);
     ScopedObject o(scope, f->call(cData));
-    if (!o)
-        return engine->throwTypeError();
+    if (scope.hasException())
+        return Encode(true);
+    if (!o) {
+        engine->throwTypeError();
+        return Encode(true);
+    }
+
     ScopedValue d(scope, o->get(engine->id_done()));
     if (scope.hasException())
-        return Encode::undefined();
+        return Encode(true);
     bool done = d->toBoolean();
     if (done) {
         *value = Encode::undefined();
-    } else {
-        *value = o->get(engine->id_value());
-        if (scope.hasException())
-            return Encode::undefined();
+        return Encode(true);
     }
-    return Encode(done);
+
+    *value = o->get(engine->id_value());
+    if (scope.hasException())
+        return Encode(true);
+    return Encode(false);
 }
 
 ReturnedValue Runtime::method_iteratorClose(ExecutionEngine *engine, const Value &iterator, const Value &done)
diff --git a/src/qml/jsruntime/qv4vme_moth.cpp b/src/qml/jsruntime/qv4vme_moth.cpp
index 2d1f4c3e91..f38bd7b48e 100644
--- a/src/qml/jsruntime/qv4vme_moth.cpp
+++ b/src/qml/jsruntime/qv4vme_moth.cpp
@@ -875,6 +875,7 @@ QV4::ReturnedValue VME::interpret(CppStackFrame *frame, ExecutionEngine *engine,
     MOTH_BEGIN_INSTR(IteratorNext)
         STORE_ACC();
         acc = Runtime::method_iteratorNext(engine, accumulator, &STACK_VALUE(value));
+        STACK_VALUE(done) = acc;
         CHECK_EXCEPTION;
     MOTH_END_INSTR(IteratorNext)
 
diff --git a/tests/auto/qml/ecmascripttests/TestExpectations b/tests/auto/qml/ecmascripttests/TestExpectations
index 3a5ec521d3..e88bf14bf8 100644
--- a/tests/auto/qml/ecmascripttests/TestExpectations
+++ b/tests/auto/qml/ecmascripttests/TestExpectations
@@ -639,14 +639,9 @@ language/expressions/assignment/S11.13.1_A7_T3.js fails
 language/expressions/assignment/destructuring/iterator-destructuring-property-reference-target-evaluation-order.js fails
 language/expressions/assignment/destructuring/keyed-destructuring-property-reference-target-evaluation-order.js fails
 language/expressions/assignment/dstr-array-elem-iter-rtrn-close-err.js fails
-language/expressions/assignment/dstr-array-elem-iter-rtrn-close-null.js fails
-language/expressions/assignment/dstr-array-elem-iter-rtrn-close.js fails
 language/expressions/assignment/dstr-array-elem-iter-thrw-close-err.js fails
 language/expressions/assignment/dstr-array-elem-iter-thrw-close.js fails
 language/expressions/assignment/dstr-array-elem-put-let.js fails
-language/expressions/assignment/dstr-array-elem-trlg-iter-list-rtrn-close-err.js fails
-language/expressions/assignment/dstr-array-elem-trlg-iter-list-rtrn-close-null.js fails
-language/expressions/assignment/dstr-array-elem-trlg-iter-list-rtrn-close.js fails
 language/expressions/assignment/dstr-array-elem-trlg-iter-list-thrw-close-err.js fails
 language/expressions/assignment/dstr-array-elem-trlg-iter-list-thrw-close.js fails
 language/expressions/assignment/dstr-array-elem-trlg-iter-rest-rtrn-close-err.js fails
@@ -1002,14 +997,9 @@ language/statements/for-in/head-lhs-let.js sloppyFails
 language/statements/for-in/head-var-bound-names-let.js sloppyFails
 language/statements/for-in/identifier-let-allowed-as-lefthandside-expression-not-strict.js sloppyFails
 language/statements/for-of/dstr-array-elem-iter-rtrn-close-err.js fails
-language/statements/for-of/dstr-array-elem-iter-rtrn-close-null.js fails
-language/statements/for-of/dstr-array-elem-iter-rtrn-close.js fails
 language/statements/for-of/dstr-array-elem-iter-thrw-close-err.js fails
 language/statements/for-of/dstr-array-elem-iter-thrw-close.js fails
 language/statements/for-of/dstr-array-elem-put-let.js fails
-language/statements/for-of/dstr-array-elem-trlg-iter-list-rtrn-close-err.js fails
-language/statements/for-of/dstr-array-elem-trlg-iter-list-rtrn-close-null.js fails
-language/statements/for-of/dstr-array-elem-trlg-iter-list-rtrn-close.js fails
 language/statements/for-of/dstr-array-elem-trlg-iter-list-thrw-close-err.js fails
 language/statements/for-of/dstr-array-elem-trlg-iter-list-thrw-close.js fails
 language/statements/for-of/dstr-array-elem-trlg-iter-rest-rtrn-close-err.js fails
@@ -1029,9 +1019,7 @@ language/statements/for-of/dstr-obj-empty-undef.js fails
 language/statements/for-of/dstr-obj-id-put-let.js fails
 language/statements/for-of/dstr-obj-prop-put-let.js fails
 language/statements/for-of/head-var-bound-names-let.js sloppyFails
-language/statements/for-of/iterator-next-error.js fails
 language/statements/for-of/iterator-next-reference.js fails
-language/statements/for-of/iterator-next-result-value-attr-error.js fails
 language/statements/for-of/yield-star-from-catch.js fails
 language/statements/for-of/yield-star-from-finally.js fails
 language/statements/for-of/yield-star-from-try.js fails