[1.4>master] [MERGE #2315 @tcare] Remove InternalErrorException and FailFast instead

Merge pull request #2315 from tcare:bait

There are a series of bugs we are getting where the dump has happened after we have thrown a InternalErrorException and returned E_FAIL. We don't even really need an InternalErrorException anyway, so we can FailFast in this case and get a more useful dump.

Author: tcare

lib/Common/Common.h

@@ -74,7 +74,6 @@ template<> struct IntMath<int64> { using Type = Int64Math; };
 
 // Exceptions
 #include "Exceptions/ExceptionBase.h"
-#include "Exceptions/InternalErrorException.h"
 #include "Exceptions/JavascriptException.h"
 #include "Exceptions/OutOfMemoryException.h"
 #include "Exceptions/OperationAbortedException.h"

lib/Common/Exceptions/Chakra.Common.Exceptions.vcxproj

@@ -45,7 +45,6 @@
     <ClInclude Include="EvalDisabledException.h" />
     <ClInclude Include="ExceptionBase.h" />
     <ClInclude Include="ExceptionCheck.h" />
-    <ClInclude Include="InternalErrorException.h" />
     <ClInclude Include="JavascriptException.h" />
     <ClInclude Include="NotImplementedException.h" />
     <ClInclude Include="OperationAbortedException.h" />

lib/Common/Exceptions/InternalErrorException.h

@@ -15,7 +15,6 @@
 
 #include "StackOverflowException.h"
 #include "AsmJsParseException.h"
-#include "InternalErrorException.h"
 #include "OutOfMemoryException.h"
 #include "NotImplementedException.h"
 
@@ -84,8 +83,7 @@ namespace Js {
 
     void Throw::InternalError()
     {
-        AssertMsg(false, "Internal error!!");
-        throw InternalErrorException();
+        AssertOrFailFastMsg(false, "Internal error!!");
     }
 
     void Throw::OutOfMemory()

lib/Common/Exceptions/Throw.cpp

@@ -110,10 +110,6 @@ namespace Js {
 
 #define END_TRANSLATE_KNOWN_EXCEPTION_TO_HRESULT(hr) \
     } \
-    catch (Js::InternalErrorException)   \
-    {   \
-        hr = E_FAIL;    \
-    }   \
     catch (Js::OutOfMemoryException) \
     {   \
         hr = E_OUTOFMEMORY; \
@@ -135,14 +131,43 @@ namespace Js {
         hr = JSERR_AsmJsCompileError; \
     }
 
+// This should be the inverse of END_TRANSLATE_KNOWN_EXCEPTION_TO_HRESULT, and catch all the same cases.
+#define THROW_KNOWN_HRESULT_EXCEPTIONS(hr, scriptContext) \
+    if (hr == E_OUTOFMEMORY) \
+    { \
+        JavascriptError::ThrowOutOfMemoryError(scriptContext); \
+    } \
+    else if (hr == VBSERR_OutOfStack) \
+    { \
+        JavascriptError::ThrowStackOverflowError(scriptContext); \
+    } \
+    else if (hr == E_NOTIMPL) \
+    { \
+        throw Js::NotImplementedException(); \
+    } \
+    else if (hr == E_ABORT) \
+    { \
+        throw Js::ScriptAbortException(); \
+    } \
+    else if (hr == JSERR_AsmJsCompileError) \
+    { \
+        throw Js::AsmJsParseException(); \
+    } \
+    else if (FAILED(hr)) \
+    { \
+        /* Intended to be the inverse of E_FAIL in CATCH_UNHANDLED_EXCEPTION */ \
+        AssertOrFailFast(false); \
+    }
+
 #define CATCH_UNHANDLED_EXCEPTION(hr) \
     catch (...) \
-    {   \
-        AssertMsg(FALSE, "invalid exception thrown and didn't get handled");    \
-        hr = E_FAIL;    \
+    { \
+        AssertOrFailFastMsg(FALSE, "invalid exception thrown and didn't get handled"); \
+        hr = E_FAIL; /* Suppress C4701 */ \
     } \
     }
 
+
 #define END_TRANSLATE_EXCEPTION_TO_HRESULT(hr) \
     END_TRANSLATE_KNOWN_EXCEPTION_TO_HRESULT(hr)\
     CATCH_UNHANDLED_EXCEPTION(hr)

lib/Common/Exceptions/Throw.h

@@ -5,7 +5,6 @@
 #pragma once
 #include "PageAllocatorDefines.h"
 #include "Exceptions/ExceptionBase.h"
-#include "Exceptions/InternalErrorException.h"
 
 #ifdef PROFILE_MEM
 struct PageMemoryData;
@@ -400,7 +399,7 @@ class MemoryOperationLastError
         if (MemOpLastError == 0)
         {
             MemOpLastError = GetLastError();
-            throw Js::InternalErrorException();
+            AssertOrFailFast(false);
         }
     }
     static void CheckProcessAndThrowFatalError(HANDLE hProcess)

lib/Common/Memory/PageAllocator.h

@@ -833,17 +833,12 @@ HRESULT ServerCallWrapper(ServerThreadContext* threadContextInfo, Fn fn)
     {
         hr = E_ABORT;
     }
-    catch (Js::InternalErrorException)
-    {
-        hr = E_FAIL;
-    }
     catch (...)
     {
-        AssertMsg(false, "Unknown exception caught in JIT server call.");
-        hr = E_FAIL;
+        AssertOrFailFastMsg(false, "Unknown exception caught in JIT server call.");
     }
 
-    if (hr == E_OUTOFMEMORY || hr == E_FAIL)
+    if (hr == E_OUTOFMEMORY)
     {
         if (HRESULT_FROM_WIN32(MemoryOperationLastError::GetLastError()) != S_OK)
         {

lib/JITServer/JITServer.cpp

@@ -2343,22 +2343,7 @@ namespace Js
             }
             END_LEAVE_SCRIPT_INTERNAL(m_scriptContext);
 
-            if (hr == E_OUTOFMEMORY)
-            {
-                JavascriptError::ThrowOutOfMemoryError(m_scriptContext);
-            }
-            else if(hr == VBSERR_OutOfStack)
-            {
-                JavascriptError::ThrowStackOverflowError(m_scriptContext);
-            }
-            else if(hr == E_ABORT)
-            {
-                throw Js::ScriptAbortException();
-            }
-            else if(FAILED(hr))
-            {
-                throw Js::InternalErrorException();
-            }
+            THROW_KNOWN_HRESULT_EXCEPTIONS(hr, m_scriptContext);
 
             Assert(hr == NO_ERROR);
 

lib/Runtime/Base/FunctionBody.cpp

@@ -925,22 +925,7 @@ namespace Js
 #ifdef PROFILE_EXEC
         scriptContext->ProfileEnd(Js::EvalCompilePhase);
 #endif
-        if (hr == E_OUTOFMEMORY)
-        {
-            JavascriptError::ThrowOutOfMemoryError(scriptContext);
-        }
-        else if(hr == VBSERR_OutOfStack)
-        {
-            JavascriptError::ThrowStackOverflowError(scriptContext);
-        }
-        else if(hr == E_ABORT)
-        {
-            throw Js::ScriptAbortException();
-        }
-        else if(FAILED(hr))
-        {
-            throw Js::InternalErrorException();
-        }
+        THROW_KNOWN_HRESULT_EXCEPTIONS(hr, scriptContext);
 
         if (!SUCCEEDED(hrParser))
         {
@@ -1087,22 +1072,7 @@ namespace Js
 #ifdef PROFILE_EXEC
         scriptContext->ProfileEnd(Js::EvalCompilePhase);
 #endif
-        if (hr == E_OUTOFMEMORY)
-        {
-            JavascriptError::ThrowOutOfMemoryError(scriptContext);
-        }
-        else if(hr == VBSERR_OutOfStack)
-        {
-            JavascriptError::ThrowStackOverflowError(scriptContext);
-        }
-        else if(hr == E_ABORT)
-        {
-            throw Js::ScriptAbortException();
-        }
-        else if(FAILED(hr))
-        {
-            throw Js::InternalErrorException();
-        }
+        THROW_KNOWN_HRESULT_EXCEPTIONS(hr);
 
         if (!SUCCEEDED(hrParser))
         {

lib/Runtime/Library/GlobalObject.cpp

@@ -709,7 +709,7 @@ namespace Js
         {
             // Just don't execute anything if we are in an assert
             // throw the exception directly to avoid additional assert in Js::Throw::InternalError
-            throw Js::InternalErrorException();
+            AssertOrFailFast(false);
         }
 #endif