Fixing stack pointer when destructuring at call expression.

The destructuring pattern is wrapped around try..finally, as it has to do iteratorclose when the pattern exhaust. However that pattern can appear at call expression. The finally block resetOut the stack pointer. This created the problem as we were middle of the call expression and the stack pointer (m_outSp and m_outParams) got reseted. In order to fix that we need to cache the m_outSp when we enter the try and restore that when we enter the finally block. We need to restore the m_outParams as well - so we are storing that information in the stack itself. There is no problem in the JIT (or bailout code path) so no fix required.
Added the tests for more clarity.

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

@@ -11470,6 +11470,9 @@ void Emit(ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator, FuncInfo *func
             byteCodeGenerator->Writer()->Br(Js::OpCode::TryFinally, finallyLabel);
         }
 
+        // Increasing the stack as we will be storing the additional values when we enter try..finally.
+        funcInfo->StartRecordingOutArgs(1);
+
         Emit(pnodeTry->sxTry.pnodeBody, byteCodeGenerator, funcInfo, fReturnValue);
         funcInfo->ReleaseLoc(pnodeTry->sxTry.pnodeBody);
 
@@ -11505,6 +11508,8 @@ void Emit(ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator, FuncInfo *func
             funcInfo->ReleaseTmpRegister(regException);
         }
 
+        funcInfo->EndRecordingOutArgs(1);
+
         byteCodeGenerator->Writer()->RecordCrossFrameEntryExitRecord(false);
 
         byteCodeGenerator->Writer()->Empty(Js::OpCode::LeaveNull);

lib/Runtime/Language/InterpreterStackFrame.cpp

@@ -1073,6 +1073,7 @@ namespace Js
         newInstance->m_callFlags    = this->callFlags;
         newInstance->m_outParams    = newInstance->m_localSlots + localCount;
         newInstance->m_outSp        = newInstance->m_outParams;
+        newInstance->m_outSpCached  = nullptr;
         newInstance->m_arguments    = NULL;
         newInstance->function       = this->function;
         newInstance->m_functionBody = this->executeFunction;
@@ -2202,6 +2203,36 @@ namespace Js
         *(AsmJsSIMDValue*)(&(m_outParams[outRegisterID])) = val;
     }
 
+    // This will be called in the beginning of the try_finally.
+    inline void InterpreterStackFrame::CacheSp()
+    {
+        // Before caching the current m_outSp, we will be storing the previous the previously stored value in the m_outSpCached.
+        *m_outSp++ = (Var)m_outSpCached;
+        *m_outSp++ = (Var)m_outParams;
+        m_outSpCached = m_outSp - 2;
+    }
+
+    inline void InterpreterStackFrame::RestoreSp()
+    {
+        // This will be called in the Finally block to restore from the previous SP cached.
+
+        // m_outSpCached can be null if the catch block is called.
+        if (m_outSpCached != nullptr)
+        {
+            Assert(m_outSpCached < m_outSp);
+            m_outSp = m_outSpCached;
+
+            m_outSpCached = (Var*)*m_outSp;
+            Assert(m_outSpCached == nullptr || m_outSpCached <= m_outSp);
+
+            m_outParams = (Var*)*(m_outSp + 1);
+        }
+        else
+        {
+            ResetOut();
+        }
+    }
+
     inline void InterpreterStackFrame::PushOut(Var aValue)
     {
         *m_outSp++ = aValue;
@@ -2225,6 +2256,7 @@ namespace Js
         m_outParams    = m_localSlots + this->m_functionBody->GetLocalsCount();
 
         m_outSp        = m_outParams;
+        m_outSpCached  = nullptr;
     }
 
     _NOINLINE
@@ -6707,6 +6739,8 @@ const byte * InterpreterStackFrame::OP_ProfiledLoopBodyStart(const byte * ip)
             // mark the stackFrame as 'in try block'
             this->m_flags |= InterpreterStackFrameFlags_WithinTryBlock;
 
+            CacheSp();
+
             if (this->IsInDebugMode())
             {
                 result = this->ProcessWithDebugging();
@@ -6735,6 +6769,8 @@ const byte * InterpreterStackFrame::OP_ProfiledLoopBodyStart(const byte * ip)
 
         if (skipFinallyBlock)
         {
+            RestoreSp();
+
             // A leave occurred due to a yield
             return;
         }
@@ -6768,7 +6804,7 @@ const byte * InterpreterStackFrame::OP_ProfiledLoopBodyStart(const byte * ip)
         // Call into the finally by setting the IP, consuming the Finally, and letting the interpreter recurse.
         m_reader.SetCurrentRelativeOffset(ip, jumpOffset);
 
-        ResetOut();
+        RestoreSp();
 
         newOffset = this->ProcessFinally();
 

lib/Runtime/Language/InterpreterStackFrame.h

@@ -78,6 +78,7 @@ namespace Js
         Var* m_inParams;                // Range of 'in' parameters
         Var* m_outParams;               // Range of 'out' parameters (offset in m_localSlots)
         Var* m_outSp;                   // Stack pointer for next outparam
+        Var* m_outSpCached;             // Stack pointer for caching previos SP (in order to assist in try..finally)
         Var  m_arguments;               // Dedicated location for this frame's arguments object
         StackScriptFunction * stackNestedFunctions;
         FrameDisplay * localFrameDisplay;
@@ -167,6 +168,8 @@ namespace Js
         void SetOut(ArgSlot_OneByte outRegisterID, Var bValue);
         void PushOut(Var aValue);
         void PopOut(ArgSlot argCount);
+        void CacheSp();
+        void RestoreSp();
 
         FrameDisplay * GetLocalFrameDisplay() const;
         FrameDisplay * GetFrameDisplayForNestedFunc() const;

test/es6/destructuring_bugs.js

@@ -313,6 +313,53 @@ var tests = [
         }
         foo1([]);
     }
+  },
+  {
+    name: "Destructuring - array pattern at call expression (which causes iterator close)",
+    body: function () {
+        function foo(x1, x2, x3, x4) {
+            assert.areEqual(x1, 'first');
+            assert.areEqual(x2, 'second');
+            assert.areEqual(x3[0][0], 'third');
+            assert.areEqual(x4[0][0][0][0], 'fourth');
+        }
+        var a1;
+        var a2;
+        foo([{}] = 'first', 'second', [[a1]] = [['third']], [[[[a2]]]] = [[[['fourth']]]]);
+        assert.areEqual(a1, 'third');
+        assert.areEqual(a2, 'fourth');
+    }
+  },
+  {
+    name: "Destructuring - array patten at call expression - validating the next/return functions are called.",
+    body: function () {
+        var nextCount = 0;
+        var returnCount = 0;
+        var iterable = {};
+        iterable[Symbol.iterator] = function () {
+            return {
+                next: function() {
+                    nextCount++;
+                    return {value : 1, done:false};
+                },
+                return: function (value) {
+                    returnCount++;
+                    return {done:true};
+                }
+            };
+        };
+
+        var obj = {
+          set prop(val) {
+            throw new Error('From setter');
+          }
+        };
+        
+        var foo = function () { };
+        assert.throws(function () { foo([[obj.prop]] = [iterable]); }, Error, 'pattern at call expression throws and return function is called', 'From setter');
+        assert.areEqual(nextCount, 1);
+        assert.areEqual(returnCount, 1);
+    }
   }
 ];