fixing issue where if a throw occurred in a promise without any attac…

…hed rejection handlers, we wouldn't notify the debugger that an unhandled exception occurred
chakra-core · Jun 26, 2018 · e97affd · e97affd
1 parent 756ee73
commit e97affd
Show file tree

Hide file tree

Showing 19 changed files with 707 additions and 4 deletions.
diff --git a/bin/ch/DbgController.js b/bin/ch/DbgController.js
@@ -383,6 +383,8 @@ var controllerObj = (function () {
                                 if (bpName == "none") {
                                     exceptionAttributes = 0; // JsDiagBreakOnExceptionAttributeNone
                                 } else if (bpName == "uncaught") {
+                                    exceptionAttributes = 0x1; // JsDiagBreakOnExceptionAttributeUncaught
+                                } else if (bpName == "firstchance") {
                                     exceptionAttributes = 0x2; // JsDiagBreakOnExceptionAttributeFirstChance
                                 } else if (bpName == "all") {
                                     exceptionAttributes = 0x1 | 0x2; // JsDiagBreakOnExceptionAttributeUncaught | JsDiagBreakOnExceptionAttributeFirstChance

diff --git a/lib/Runtime/Language/JavascriptExceptionOperators.cpp b/lib/Runtime/Language/JavascriptExceptionOperators.cpp
@@ -40,13 +40,22 @@ namespace Js
         }
     }
 
-    JavascriptExceptionOperators::AutoCatchHandlerExists::AutoCatchHandlerExists(ScriptContext* scriptContext)
+    JavascriptExceptionOperators::AutoCatchHandlerExists::AutoCatchHandlerExists(ScriptContext* scriptContext, bool isPromiseHandled)
     {
         Assert(scriptContext);
         m_threadContext = scriptContext->GetThreadContext();
         Assert(m_threadContext);
         m_previousCatchHandlerExists = m_threadContext->HasCatchHandler();
         m_threadContext->SetHasCatchHandler(TRUE);
+
+        if (!isPromiseHandled)
+        {
+            // If this is created from a promise-specific code path, and we don't have a rejection
+            // handler on the promise, then we want SetCatchHandler to be false so we report any
+            // unhandled exceptions to any detached debuggers.
+            m_threadContext->SetHasCatchHandler(false);
+        }
+
         m_previousCatchHandlerToUserCodeStatus = m_threadContext->IsUserCode();
         if (scriptContext->IsScriptContextInDebugMode())
         {

diff --git a/lib/Runtime/Language/JavascriptExceptionOperators.h b/lib/Runtime/Language/JavascriptExceptionOperators.h
@@ -39,7 +39,7 @@ namespace Js
             void FetchNonUserCodeStatus(ScriptContext *scriptContext);
 
           public:
-            AutoCatchHandlerExists(ScriptContext* scriptContext);
+            AutoCatchHandlerExists(ScriptContext* scriptContext, bool isPromiseHandled = true);
             ~AutoCatchHandlerExists();
         };
 

diff --git a/lib/Runtime/Library/JavascriptPromise.cpp b/lib/Runtime/Library/JavascriptPromise.cpp
@@ -932,7 +932,22 @@ namespace Js
         JavascriptExceptionObject* exception = nullptr;
 
         {
-            Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext);
+
+            bool isPromiseRejectionHandled = true;
+            if (scriptContext->IsScriptContextInDebugMode())
+            {
+                // only necessary to determine if false if debugger is attached.  This way we'll 
+                // correctly break on exceptions raised in promises that result in uhandled rejection
+                // notifications
+                Var promiseVar = promiseCapability->GetPromise();
+                if (JavascriptPromise::Is(promiseVar))
+                {
+                    JavascriptPromise* promise = JavascriptPromise::FromVar(promiseVar);
+                    isPromiseRejectionHandled = !promise->WillRejectionBeUnhandled();
+                }
+            }
+
+            Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext, isPromiseRejectionHandled);
             try
             {
                 BEGIN_SAFE_REENTRANT_CALL(scriptContext->GetThreadContext())
@@ -960,6 +975,80 @@ namespace Js
         return TryCallResolveOrRejectHandler(promiseCapability->GetResolve(), handlerResult, scriptContext);
     }
 
+
+    /**
+     * Determine if at the current point in time, the given promise has a path of reactions that result
+     * in an unhandled rejection.  This doesn't account for potential of a rejection handler added later
+     * in time.
+     */
+    bool JavascriptPromise::WillRejectionBeUnhandled()
+    {
+        bool willBeUnhandled = !this->GetIsHandled();
+        if (!willBeUnhandled)
+        {
+            // if this promise is handled, then we need to do a depth-first search over this promise's reject 
+            // reactions. If we find a reaction that 
+            //    - associated promise is "unhandled" (ie, it's never been "then'd")
+            //    - AND its rejection handler is our default "thrower function" 
+            // then this promise results in an unhandled rejection path.
+
+            JsUtil::Stack<JavascriptPromise*, HeapAllocator> stack(&HeapAllocator::Instance);
+            SimpleHashTable<JavascriptPromise*, int, HeapAllocator> visited(&HeapAllocator::Instance);
+            stack.Push(this);
+            visited.Add(this, 1);
+
+            while (!willBeUnhandled && !stack.Empty())
+            {
+                JavascriptPromise * curr = stack.Pop();
+                {
+                    JavascriptPromiseReactionList* reactions = curr->GetRejectReactions();
+                    for (int i = 0; i < reactions->Count(); i++)
+                    {
+                        JavascriptPromiseReaction* reaction = reactions->Item(i);
+                        Var promiseVar = reaction->GetCapabilities()->GetPromise();
+
+                        if (JavascriptPromise::Is(promiseVar))
+                        {
+                            JavascriptPromise* p = JavascriptPromise::FromVar(promiseVar);
+                            if (!p->GetIsHandled())
+                            {
+                                RecyclableObject* handler = reaction->GetHandler();
+                                if (JavascriptFunction::Is(handler))
+                                {
+                                    JavascriptFunction* func = JavascriptFunction::FromVar(handler);
+                                    FunctionInfo* functionInfo = func->GetFunctionInfo();
+
+#ifdef DEBUG
+                                    if (!func->IsCrossSiteObject())
+                                    {
+                                        // assert that Thrower function's FunctionInfo hasn't changed
+                                        AssertMsg(func->GetScriptContext()->GetLibrary()->GetThrowerFunction()->GetFunctionInfo() == &JavascriptPromise::EntryInfo::Thrower, "unexpected FunctionInfo for thrower function!");
+                                    }
+#endif
+
+                                    // If the function info is the default thrower function's function info, then assume that this is unhandled
+                                    // this will work across script contexts
+                                    if (functionInfo == &JavascriptPromise::EntryInfo::Thrower)
+                                    {
+                                        willBeUnhandled = true;
+                                        break;
+                                    }
+                                }
+                            }
+                            AssertMsg(visited.HasEntry(p) == false, "Unexecpted cycle in promise reaction tree!");
+                            if (!visited.HasEntry(p))
+                            {
+                                stack.Push(p);
+                                visited.Add(p, 1);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        return willBeUnhandled;
+    }
+
     Var JavascriptPromise::TryCallResolveOrRejectHandler(Var handler, Var value, ScriptContext* scriptContext)
     {
         Var undefinedVar = scriptContext->GetLibrary()->GetUndefined();
@@ -1092,7 +1181,15 @@ namespace Js
         JavascriptExceptionObject* exception = nullptr;
 
         {
-            Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext);
+            bool isPromiseRejectionHandled = true;
+            if (scriptContext->IsScriptContextInDebugMode())
+            {
+                // only necessary to determine if false if debugger is attached.  This way we'll 
+                // correctly break on exceptions raised in promises that result in uhandled rejections
+                isPromiseRejectionHandled = !promise->WillRejectionBeUnhandled();
+            }
+
+            Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext, isPromiseRejectionHandled);
             try
             {
                 BEGIN_SAFE_REENTRANT_CALL(scriptContext->GetThreadContext())

diff --git a/lib/Runtime/Library/JavascriptPromise.h b/lib/Runtime/Library/JavascriptPromise.h
@@ -563,6 +563,7 @@ namespace Js
 
     private :
         static void AsyncSpawnStep(JavascriptPromiseAsyncSpawnStepArgumentExecutorFunction* nextFunction, JavascriptGenerator* gen, Var resolve, Var reject);
+        bool WillRejectionBeUnhandled();
 
 #if ENABLE_TTD
     public:

diff --git a/test/Debugger/JsDiagBreakOnUncaughtException.baseline b/test/Debugger/JsDiagBreakOnUncaughtException.baseline
@@ -0,0 +1,3 @@
+Error: throw exception from throwFunction
+	at throwFunction() (JsDiagBreakOnUncaughtException.js:18:4)
+	at Global code (JsDiagBreakOnUncaughtException.js:20:1)
diff --git a/test/Debugger/JsDiagBreakOnUncaughtException.js b/test/Debugger/JsDiagBreakOnUncaughtException.js
@@ -0,0 +1,20 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+/**exception(uncaught):stack();**/
+
+function noThrowFunction() {
+  try {
+    throw new Error("throw exception from noThrowFunction");
+  } catch (err) {
+  }
+}
+noThrowFunction();
+
+// calling throwFunction() will terminate program, so this has to come last
+function throwFunction() { 
+   throw new Error("throw exception from throwFunction");
+}
+throwFunction();
diff --git a/test/Debugger/JsDiagBreakOnUncaughtException.js.dbg.baseline b/test/Debugger/JsDiagBreakOnUncaughtException.js.dbg.baseline
@@ -0,0 +1,18 @@
+[
+  {
+    "callStack": [
+      {
+        "line": 17,
+        "column": 3,
+        "sourceText": "throw new Error(\"throw exception from throwFunction\")",
+        "function": "throwFunction"
+      },
+      {
+        "line": 19,
+        "column": 0,
+        "sourceText": "throwFunction()",
+        "function": "Global code"
+      }
+    ]
+  }
+]
diff --git a/test/Debugger/JsDiagExceptionsInAsyncFunctions_BreakOnUncaughtExceptions.baseline b/test/Debugger/JsDiagExceptionsInAsyncFunctions_BreakOnUncaughtExceptions.baseline
diff --git a/test/Debugger/JsDiagExceptionsInAsyncFunctions_BreakOnUncaughtExceptions.js b/test/Debugger/JsDiagExceptionsInAsyncFunctions_BreakOnUncaughtExceptions.js
@@ -0,0 +1,35 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+/**exception(uncaught):stack();**/
+
+async function f1() {
+    await null;
+    throw new Error('error in f1');
+}
+f1();
+
+async function f2() {
+
+    async function f2a() {
+        throw "err";
+    }
+
+    async function f2b() {
+        try {
+            var p = f2a();
+        } catch (e) {
+            console.log("caught " + e);
+        }
+    }
+
+    async function f2c() {
+        var p = f2a();
+    }
+
+    f2b();
+    f2c();
+}
+f2();
diff --git a/test/Debugger/JsDiagExceptionsInAsyncFunctions_BreakOnUncaughtExceptions.js.dbg.baseline b/test/Debugger/JsDiagExceptionsInAsyncFunctions_BreakOnUncaughtExceptions.js.dbg.baseline
@@ -0,0 +1,40 @@
+[
+  {
+    "callStack": [
+      {
+        "line": 16,
+        "column": 8,
+        "sourceText": "throw \"err\"",
+        "function": "f2a"
+      },
+      {
+        "line": 28,
+        "column": 8,
+        "sourceText": "var p = f2a()",
+        "function": "f2c"
+      },
+      {
+        "line": 32,
+        "column": 4,
+        "sourceText": "f2c()",
+        "function": "f2"
+      },
+      {
+        "line": 34,
+        "column": 0,
+        "sourceText": "f2()",
+        "function": "Global code"
+      }
+    ]
+  },
+  {
+    "callStack": [
+      {
+        "line": 9,
+        "column": 4,
+        "sourceText": "throw new Error('error in f1')",
+        "function": "f1"
+      }
+    ]
+  }
+]
diff --git a/test/Debugger/JsDiagExceptionsInPromises_BreakOnFirstChanceExceptions.baseline b/test/Debugger/JsDiagExceptionsInPromises_BreakOnFirstChanceExceptions.baseline
diff --git a/test/Debugger/JsDiagExceptionsInPromises_BreakOnFirstChanceExceptions.crosscontext.js b/test/Debugger/JsDiagExceptionsInPromises_BreakOnFirstChanceExceptions.crosscontext.js
@@ -0,0 +1,20 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+var externalContextPromise = (function () {
+    let resolvePromise;
+    let promise = new Promise((resolve, reject) => {
+        resolvePromise = resolve;
+
+    });
+    promise = promise.then(()=> {
+        throw new Error("error from externalContextPromise1");
+    })
+
+    return {
+        promise,
+        resolvePromise
+    };
+})();