Skip to content

Commit 246a8a8

Browse files
author
Mike Kaufman
committed
[MERGE #5328 @mike-kaufman] fixing issue where "uncaught" exceptions in promises wouldn't notify debugger
Merge pull request #5328 from mike-kaufman:build/mkaufman/support-uncaught-exception-handler-in-promises If an exception was raised inside a promise and the promise didn't have any rejection handlers, we wouldn't notify the debugger that an "unhandled" exception occurred. Fixed this up and added some simple tests for it. This addresses the common cases for promises, but doesn't yet address async/await constructs. Will leave #4630 open for that.
2 parents d0d083b + 543e0e1 commit 246a8a8

19 files changed

+707
-4
lines changed

‎bin/ch/DbgController.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,8 @@ var controllerObj = (function () {
383383
if (bpName == "none") {
384384
exceptionAttributes = 0; // JsDiagBreakOnExceptionAttributeNone
385385
} else if (bpName == "uncaught") {
386+
exceptionAttributes = 0x1; // JsDiagBreakOnExceptionAttributeUncaught
387+
} else if (bpName == "firstchance") {
386388
exceptionAttributes = 0x2; // JsDiagBreakOnExceptionAttributeFirstChance
387389
} else if (bpName == "all") {
388390
exceptionAttributes = 0x1 | 0x2; // JsDiagBreakOnExceptionAttributeUncaught | JsDiagBreakOnExceptionAttributeFirstChance

‎lib/Runtime/Language/JavascriptExceptionOperators.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,22 @@ namespace Js
4040
}
4141
}
4242

43-
JavascriptExceptionOperators::AutoCatchHandlerExists::AutoCatchHandlerExists(ScriptContext* scriptContext)
43+
JavascriptExceptionOperators::AutoCatchHandlerExists::AutoCatchHandlerExists(ScriptContext* scriptContext, bool isPromiseHandled)
4444
{
4545
Assert(scriptContext);
4646
m_threadContext = scriptContext->GetThreadContext();
4747
Assert(m_threadContext);
4848
m_previousCatchHandlerExists = m_threadContext->HasCatchHandler();
4949
m_threadContext->SetHasCatchHandler(TRUE);
50+
51+
if (!isPromiseHandled)
52+
{
53+
// If this is created from a promise-specific code path, and we don't have a rejection
54+
// handler on the promise, then we want SetCatchHandler to be false so we report any
55+
// unhandled exceptions to any detached debuggers.
56+
m_threadContext->SetHasCatchHandler(false);
57+
}
58+
5059
m_previousCatchHandlerToUserCodeStatus = m_threadContext->IsUserCode();
5160
if (scriptContext->IsScriptContextInDebugMode())
5261
{

‎lib/Runtime/Language/JavascriptExceptionOperators.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ namespace Js
3939
void FetchNonUserCodeStatus(ScriptContext *scriptContext);
4040

4141
public:
42-
AutoCatchHandlerExists(ScriptContext* scriptContext);
42+
AutoCatchHandlerExists(ScriptContext* scriptContext, bool isPromiseHandled = true);
4343
~AutoCatchHandlerExists();
4444
};
4545

‎lib/Runtime/Library/JavascriptPromise.cpp

Lines changed: 99 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -932,7 +932,22 @@ namespace Js
932932
JavascriptExceptionObject* exception = nullptr;
933933

934934
{
935-
Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext);
935+
936+
bool isPromiseRejectionHandled = true;
937+
if (scriptContext->IsScriptContextInDebugMode())
938+
{
939+
// only necessary to determine if false if debugger is attached. This way we'll
940+
// correctly break on exceptions raised in promises that result in uhandled rejection
941+
// notifications
942+
Var promiseVar = promiseCapability->GetPromise();
943+
if (JavascriptPromise::Is(promiseVar))
944+
{
945+
JavascriptPromise* promise = JavascriptPromise::FromVar(promiseVar);
946+
isPromiseRejectionHandled = !promise->WillRejectionBeUnhandled();
947+
}
948+
}
949+
950+
Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext, isPromiseRejectionHandled);
936951
try
937952
{
938953
BEGIN_SAFE_REENTRANT_CALL(scriptContext->GetThreadContext())
@@ -960,6 +975,80 @@ namespace Js
960975
return TryCallResolveOrRejectHandler(promiseCapability->GetResolve(), handlerResult, scriptContext);
961976
}
962977

978+
979+
/**
980+
* Determine if at the current point in time, the given promise has a path of reactions that result
981+
* in an unhandled rejection. This doesn't account for potential of a rejection handler added later
982+
* in time.
983+
*/
984+
bool JavascriptPromise::WillRejectionBeUnhandled()
985+
{
986+
bool willBeUnhandled = !this->GetIsHandled();
987+
if (!willBeUnhandled)
988+
{
989+
// if this promise is handled, then we need to do a depth-first search over this promise's reject
990+
// reactions. If we find a reaction that
991+
// - associated promise is "unhandled" (ie, it's never been "then'd")
992+
// - AND its rejection handler is our default "thrower function"
993+
// then this promise results in an unhandled rejection path.
994+
995+
JsUtil::Stack<JavascriptPromise*, HeapAllocator> stack(&HeapAllocator::Instance);
996+
SimpleHashTable<JavascriptPromise*, int, HeapAllocator> visited(&HeapAllocator::Instance);
997+
stack.Push(this);
998+
visited.Add(this, 1);
999+
1000+
while (!willBeUnhandled && !stack.Empty())
1001+
{
1002+
JavascriptPromise * curr = stack.Pop();
1003+
{
1004+
JavascriptPromiseReactionList* reactions = curr->GetRejectReactions();
1005+
for (int i = 0; i < reactions->Count(); i++)
1006+
{
1007+
JavascriptPromiseReaction* reaction = reactions->Item(i);
1008+
Var promiseVar = reaction->GetCapabilities()->GetPromise();
1009+
1010+
if (JavascriptPromise::Is(promiseVar))
1011+
{
1012+
JavascriptPromise* p = JavascriptPromise::FromVar(promiseVar);
1013+
if (!p->GetIsHandled())
1014+
{
1015+
RecyclableObject* handler = reaction->GetHandler();
1016+
if (JavascriptFunction::Is(handler))
1017+
{
1018+
JavascriptFunction* func = JavascriptFunction::FromVar(handler);
1019+
FunctionInfo* functionInfo = func->GetFunctionInfo();
1020+
1021+
#ifdef DEBUG
1022+
if (!func->IsCrossSiteObject())
1023+
{
1024+
// assert that Thrower function's FunctionInfo hasn't changed
1025+
AssertMsg(func->GetScriptContext()->GetLibrary()->GetThrowerFunction()->GetFunctionInfo() == &JavascriptPromise::EntryInfo::Thrower, "unexpected FunctionInfo for thrower function!");
1026+
}
1027+
#endif
1028+
1029+
// If the function info is the default thrower function's function info, then assume that this is unhandled
1030+
// this will work across script contexts
1031+
if (functionInfo == &JavascriptPromise::EntryInfo::Thrower)
1032+
{
1033+
willBeUnhandled = true;
1034+
break;
1035+
}
1036+
}
1037+
}
1038+
AssertMsg(visited.HasEntry(p) == false, "Unexpected cycle in promise reaction tree!");
1039+
if (!visited.HasEntry(p))
1040+
{
1041+
stack.Push(p);
1042+
visited.Add(p, 1);
1043+
}
1044+
}
1045+
}
1046+
}
1047+
}
1048+
}
1049+
return willBeUnhandled;
1050+
}
1051+
9631052
Var JavascriptPromise::TryCallResolveOrRejectHandler(Var handler, Var value, ScriptContext* scriptContext)
9641053
{
9651054
Var undefinedVar = scriptContext->GetLibrary()->GetUndefined();
@@ -1092,7 +1181,15 @@ namespace Js
10921181
JavascriptExceptionObject* exception = nullptr;
10931182

10941183
{
1095-
Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext);
1184+
bool isPromiseRejectionHandled = true;
1185+
if (scriptContext->IsScriptContextInDebugMode())
1186+
{
1187+
// only necessary to determine if false if debugger is attached. This way we'll
1188+
// correctly break on exceptions raised in promises that result in uhandled rejections
1189+
isPromiseRejectionHandled = !promise->WillRejectionBeUnhandled();
1190+
}
1191+
1192+
Js::JavascriptExceptionOperators::AutoCatchHandlerExists autoCatchHandlerExists(scriptContext, isPromiseRejectionHandled);
10961193
try
10971194
{
10981195
BEGIN_SAFE_REENTRANT_CALL(scriptContext->GetThreadContext())

‎lib/Runtime/Library/JavascriptPromise.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,7 @@ namespace Js
563563

564564
private :
565565
static void AsyncSpawnStep(JavascriptPromiseAsyncSpawnStepArgumentExecutorFunction* nextFunction, JavascriptGenerator* gen, Var resolve, Var reject);
566+
bool WillRejectionBeUnhandled();
566567

567568
#if ENABLE_TTD
568569
public:

‎test/Debugger/JsDiagBreakOnUncaughtException.baseline

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Error: throw exception from throwFunction
2+
at throwFunction() (JsDiagBreakOnUncaughtException.js:18:4)
3+
at Global code (JsDiagBreakOnUncaughtException.js:20:1)

‎test/Debugger/JsDiagBreakOnUncaughtException.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
/**exception(uncaught):stack();**/
7+
8+
function noThrowFunction() {
9+
try {
10+
throw new Error("throw exception from noThrowFunction");
11+
} catch (err) {
12+
}
13+
}
14+
noThrowFunction();
15+
16+
// calling throwFunction() will terminate program, so this has to come last
17+
function throwFunction() {
18+
throw new Error("throw exception from throwFunction");
19+
}
20+
throwFunction();

‎test/Debugger/JsDiagBreakOnUncaughtException.js.dbg.baseline

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
[
2+
{
3+
"callStack": [
4+
{
5+
"line": 17,
6+
"column": 3,
7+
"sourceText": "throw new Error(\"throw exception from throwFunction\")",
8+
"function": "throwFunction"
9+
},
10+
{
11+
"line": 19,
12+
"column": 0,
13+
"sourceText": "throwFunction()",
14+
"function": "Global code"
15+
}
16+
]
17+
}
18+
]

‎test/Debugger/JsDiagExceptionsInAsyncFunctions_BreakOnUncaughtExceptions.baseline

Whitespace-only changes.

‎test/Debugger/JsDiagExceptionsInAsyncFunctions_BreakOnUncaughtExceptions.js

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
/**exception(uncaught):stack();**/
7+
8+
async function f1() {
9+
await null;
10+
throw new Error('error in f1');
11+
}
12+
f1();
13+
14+
async function f2() {
15+
16+
async function f2a() {
17+
throw "err";
18+
}
19+
20+
async function f2b() {
21+
try {
22+
var p = f2a();
23+
} catch (e) {
24+
console.log("caught " + e);
25+
}
26+
}
27+
28+
async function f2c() {
29+
var p = f2a();
30+
}
31+
32+
f2b();
33+
f2c();
34+
}
35+
f2();

0 commit comments

Comments
 (0)