[1.10>master] [MERGE #5497 @irinayat-MS] Move re-throw of exception o…

…ut of catch Merge pull request #5497 from irinayat-MS:StackOverflow Fixes OS.18246154 Re-throw from catch doesn't unwind the stack (only resets the active frame pointer) so, if there are nested try/catch blocks that refrow, the stack keeps growing, which is unhelpful if the initial exception is trying to prevent Stack Overflow. To avoid this, move re-throw outside of the catch (so the stack fully unwinds on each handling site). Took the opportunity to add more test coverage for typeof and fix exception handling deviations from the spec (and V8).
chakra-core · Jul 27, 2018 · 1158355 · 1158355
2 parents e0f049c + a3c6bac
commit 1158355
Show file tree

Hide file tree

Showing 5 changed files with 169 additions and 100 deletions.
diff --git a/lib/Runtime/Language/JavascriptOperators.h b/lib/Runtime/Language/JavascriptOperators.h
@@ -22,36 +22,47 @@ namespace Js
     CONTEXT                 ep##c;                  \
     EXCEPTION_POINTERS      ep = {&ep##er, &ep##c};
 
+    // Typeof operator should return 'undefined' for undeclared or hoisted vars
+    // and propagate all other exceptions.
+    //
+    // NB: Re-throw from catch unwinds the active frame but doesn't clear the stack
+    // (catch clauses keep accumulating at the top of the stack until a catch 
+    // that doesn't re-throw). This is problematic if we've detected potential
+    // stack overflow and report it via exceptions: the handling of throw
+    // might actually overflow the stack and cause system SO exception.
+    // Thus, use catch to cache the exception, and re-throw it outside of the catch.
 #define TYPEOF_ERROR_HANDLER_CATCH(scriptContext, var) \
     } \
     catch (const JavascriptException& err) \
     { \
-        JavascriptExceptionObject* exceptionObject = err.GetAndClear(); \
+        exceptionObject = err.GetAndClear(); \
+        var = scriptContext->GetLibrary()->GetUndefined(); \
+
+#define TYPEOF_ERROR_HANDLER_THROW(scriptContext, var) \
+    } \
+    if (exceptionObject != nullptr) \
+    { \
         Js::Var errorObject = exceptionObject->GetThrownObject(nullptr); \
-        if (errorObject != nullptr && Js::JavascriptError::Is(errorObject)) \
+        HRESULT hr = (errorObject != nullptr && Js::JavascriptError::Is(errorObject)) \
+                     ? Js::JavascriptError::GetRuntimeError(Js::RecyclableObject::FromVar(errorObject), nullptr) \
+                     : S_OK; \
+        if (JavascriptError::GetErrorNumberFromResourceID(JSERR_UndefVariable) != (int32)hr) \
         { \
-            HRESULT hr = Js::JavascriptError::GetRuntimeError(Js::RecyclableObject::FromVar(errorObject), nullptr); \
-            if (JavascriptError::GetErrorNumberFromResourceID(JSERR_Property_CannotGet_NullOrUndefined) == (int32)hr \
-                || JavascriptError::GetErrorNumberFromResourceID(JSERR_UseBeforeDeclaration) == (int32)hr) \
+            if (scriptContext->IsScriptContextInDebugMode()) \
             { \
-                if (scriptContext->IsScriptContextInDebugMode()) \
-                { \
-                    JavascriptExceptionOperators::ThrowExceptionObject(exceptionObject, scriptContext, true); \
-                } \
-                else \
-                { \
-                    JavascriptExceptionOperators::DoThrow(exceptionObject, scriptContext); \
-                } \
+                JavascriptExceptionOperators::ThrowExceptionObject(exceptionObject, scriptContext, true); \
+            } \
+            else \
+            { \
+                JavascriptExceptionOperators::DoThrowCheckClone(exceptionObject, scriptContext); \
             } \
         } \
-        var = scriptContext->GetLibrary()->GetUndefined();
-
-#define TYPEOF_ERROR_HANDLER_THROW(scriptContext, var) \
     } \
     if (scriptContext->IsUndeclBlockVar(var)) \
     { \
         JavascriptError::ThrowReferenceError(scriptContext, JSERR_UseBeforeDeclaration); \
-    }
+    } \
+}
 
 #ifdef ENABLE_SCRIPT_DEBUGGING
 #define BEGIN_TYPEOF_ERROR_HANDLER_DEBUGGER_THROW_IS_INTERNAL \
@@ -80,6 +91,8 @@ namespace Js
 #endif
 
 #define BEGIN_TYPEOF_ERROR_HANDLER(scriptContext)  \
+{ \
+    JavascriptExceptionObject* exceptionObject = nullptr; \
     try { \
         Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext); \
         BEGIN_TYPEOF_ERROR_HANDLER_DEBUGGER_THROW_IS_INTERNAL

diff --git a/test/Basics/rlexe.xml b/test/Basics/rlexe.xml
@@ -148,7 +148,6 @@
   <test>
     <default>
       <files>typeof.js</files>
-      <baseline>typeof.baseline</baseline>
       <compile-flags>-Intl-</compile-flags>
     </default>
   </test>

diff --git a/test/Basics/typeof.baseline b/test/Basics/typeof.baseline
diff --git a/test/Basics/typeof.js b/test/Basics/typeof.js
@@ -3,63 +3,96 @@
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 
-WScript.Echo("typeof (new String()) : " + typeof (new String("")));
-WScript.Echo("typeof () : " + typeof (""));
+WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
 
-WScript.Echo("typeof (new Boolean(false)) : " + typeof (new Boolean(false)));
-WScript.Echo("typeof (false) : " + typeof (false));
+var tests = [
+    {
+        name: "typeof of literals, built-in types and object wrappers",
+        body: function () {
+            var arr = [42];
 
-WScript.Echo("typeof (new Number(0)) : " + typeof (new Number(0)));
-WScript.Echo("typeof (0) : " + typeof (0));
+            assert.areEqual("object", typeof null, "typeof null");
+            assert.areEqual("undefined", typeof undefined, "typeof undefined");
+            assert.areEqual("string", typeof "", "typeof empty string");
+            assert.areEqual("boolean", typeof false, "typeof false");
+            assert.areEqual("number", typeof 0, "typeof 0");
+            assert.areEqual("number", typeof 12345.678, "typeof 12345.678");
+            assert.areEqual("object", typeof {}, "typeof {}");
+            assert.areEqual("object", typeof arr, "typeof array");
+            assert.areEqual("number", typeof arr[0], "typeof arr[0]");
+            assert.areEqual("undefined", typeof arr[1], "typeof arr[1]");
+            assert.areEqual("object", typeof /abc/, "typeof /abc/");
+            assert.areEqual("function", typeof (function (){}), "typeof (function (){})");
+            assert.areEqual("function", typeof (() => 42), "typeof (() => 42)");
+            assert.areEqual("symbol", typeof Symbol(), "typeof Symbol()");
 
+            // built-in object and object wrappers
+            assert.areEqual("object", typeof (new String("")), "typeof empty string object wrapper");
+            assert.areEqual("object", typeof (new Boolean(false)), "typeof (new Boolean(false))");
+            assert.areEqual("object", typeof (new Number(0)), "typeof (new Number(0))");
+            assert.areEqual("object", typeof (new Number(12345.678)), "typeof (new Number(12345.678))");
+            assert.areEqual("object", typeof (new Object()), "typeof (new Object())");
+            assert.areEqual("object", typeof (new Array()), "typeof (new Array())");
+            assert.areEqual("object", typeof (new Date(123)), "typeof (new Date(123))");
+        }
+    },
+    {
+        name: "typeof of expressions",
+        body: function () {
+            function f() {
+                function g() { }
+                return g;
+            }
+            assert.areEqual("function", typeof f(), "typeof of function call");
+            assert.areEqual("number", typeof eval(7*6), "typeof of direct eval");
+        }
+    },
+    {
+        name: "typeof handling of undefined variables",
+        body: function () {
+            assert.areEqual("undefined", typeof x, "typeof of undeclaired var");
+            assert.areEqual("undefined", typeof {}.x, "typeof {}.x");
 
-WScript.Echo("typeof (new Number(12345.678)) : " + typeof (new Number(12345.678)));
-WScript.Echo("typeof (12345.678) : " + typeof (12345.678));
+            assert.areEqual("undefined", typeof hoisted, "typeof of hoisted var");
+            var hoisted = 42;
 
-function f() {
-    function g() { }
-    return g;
-}
+            function inner() { var scoped = 42; }
+            assert.areEqual("undefined", typeof scoped, "typeof of var when it's out of scope");
 
-WScript.Echo("typeof function : " + typeof (f));
-WScript.Echo("typeof function returning function : " + typeof (f()));
+            assert.throws(()=>{ typeof x.y; }, ReferenceError, "typeof of property on undefined obj", "'x' is not defined");
+            assert.throws(()=>{ typeof x[0]; }, ReferenceError, "typeof of property on undefined obj", "'x' is not defined");
+            assert.throws(()=>{ typeof (()=>x)(); }, ReferenceError, "reference error in the function invoked by typeof", "'x' is not defined");
 
-function exc() {
-    try {
-        WScript.Echo(q);
-    }
-    catch (e) {
-        WScript.Echo("Caught JS error on undefined var");
-        WScript.Echo(typeof (q));
-    }
-}
-exc();
+            assert.throws(()=>{ typeof x_let; }, ReferenceError, "typeof of 'let' variable in its dead zone", "Use before declaration");
+            let x_let = 42;
 
-var x = {};
-WScript.Echo("typeof empty obj : " + typeof (x));
-WScript.Echo("typeof obj : " + typeof (new Object()));
-
-var y = [];
-y[0] = 0;
-WScript.Echo("typeof array element with number : " + typeof (y[0]));
-WScript.Echo("typeof undef element array : " + typeof (y[1]));
-WScript.Echo("typeof array : " + typeof (y));
-
-var verr = new Error("aaa");
-WScript.Echo("typeof (err) : " + typeof (verr));
-
-var vDate = new Date(123);
-WScript.Echo("typeof ( new Date) : " + typeof (vDate));
-
-WScript.Echo("typeof (new Array()) : " + typeof (new Array()));
-
-var regx = /abc/;
-WScript.Echo("typeof(regex) : " + typeof (regx));
-
-var s;
-var map = {};
-function CEvent() {
-    do { 
-    } while(typeof (s = map.x) != "undefined");
-}
-CEvent();
+            assert.throws(()=>{ typeof x_const; }, ReferenceError, "typeof of 'const' variable in its dead zone", "Use before declaration");
+            const x_const = 42;
+        }
+    },
+    {
+        name: "typeof should propagate user exceptions",
+        body: function () {
+            function foo() { throw new Error("abc"); }
+            assert.throws(()=>{typeof foo()}, Error, "exception raised from the invoked function", "abc");
+
+            var obj = { get x() { throw new Error("xyz")}};
+            assert.throws(()=>{typeof obj.x}, Error, "exception raised from the getter", "xyz");
+        }
+    },
+    {
+        name: "typeof should propagate stack overflow",
+        body: function () {
+            var obj = {};
+            var handler = {
+                get: function () {
+                    return obj.x;
+                }
+            };
+            obj = new Proxy(obj, handler);
+            assert.throws(()=>{typeof obj.x}, Error, "recursive proxy should hit SO", "Out of stack space");
+        }
+    },
+];
+
+testRunner.runTests(tests, { verbose: false /*so no need to provide baseline*/ });
diff --git a/test/EH/StackOverflow.js b/test/EH/StackOverflow.js
@@ -2,14 +2,59 @@
 // // Copyright (C) Microsoft. All rights reserved.
 // // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 // //-------------------------------------------------------------------------------------------------------
+WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
 
-function foo(a)
-{ 
-    try { a[0] } finally {}
-    try { foo(0) } catch(e) {}
-    try { foo() } catch(e) {}
-}
+var tests = [
+    {
+        name: "plain recursive call with modified arguments",
+        body: function () {
+            function recursive(a) {
+                recursive(a + 1);
+            }
+            try {
+                recursive(42);
+                assert(false); // should never reach this line
+            }
+            catch (e) {
+                assert.areNotEqual(-1, e.message.indexOf("Out of stack space"), "Should be SO exception");
+            }
+        }
+    },
+    {
+        name: "plain recursive call with no arguments",
+        body: function () {
+            function recursive() {
+                recursive();
+            }
+            try {
+                recursive();
+                assert(false); // should never reach this line
+            }
+            catch (e) {
+                assert.areNotEqual(-1, e.message.indexOf("Out of stack space"), "Should be SO exception");
+            }
+        }
+    },
+    {
+        name: "recursive call to getter via proxy",
+        body: function () {
+            var obj = {};
+            var handler = {
+                get: function () {
+                    return obj.x;
+                }
+            };
+            obj = new Proxy(obj, handler);
 
-foo(0)
-
-WScript.Echo("PASS");
+            try {
+                var y = obj.x;
+                assert(false); // should never reach this line
+            }
+            catch (e) {
+                assert.areNotEqual(-1, e.message.indexOf("Out of stack space"), "Should be SO exception");
+            }
+        }
+    },
+];
+
+testRunner.runTests(tests, { verbose: false /*so no need to provide baseline*/ });