[MERGE #5933 @wyrichte] Fixes bug where ImplicitCallFlags and DisableImplicitFlags were not restored to previous values held.

Merge pull request #5933 from wyrichte:build/wyrichte/disableImplicitFlagBug

InlineCache::TrySetProperty changes threadContext->ImplicitCallFlags and threadContext->DisableImplicitFlags because "We need to disable implicit call to ensure the check doesn't cause unwanted side effects...", calls CheckPrototypesForAccessorOrNonWritablePropertySlow, and then restores the Flags. The issue is CheckPrototypesForAccessorOrNonWritablePropertySlow can lead to an exception (StackOverflow) which then bypasses the restoration of the flags in TrySetProperty.

Author: wyrichte

lib/Runtime/Base/ThreadContext.h

@@ -348,6 +348,25 @@ class ThreadContext sealed :
         WorkerThread(HANDLE handle = nullptr) :threadHandle(handle){};
     };
 
+    struct AutoRestoreImplicitFlags
+    {
+        ThreadContext * threadContext;
+        Js::ImplicitCallFlags savedImplicitCallFlags;
+        DisableImplicitFlags savedDisableImplicitFlags;
+        AutoRestoreImplicitFlags(ThreadContext *threadContext, Js::ImplicitCallFlags implicitCallFlags, DisableImplicitFlags disableImplicitFlags) :
+            threadContext(threadContext),
+            savedImplicitCallFlags(implicitCallFlags),
+            savedDisableImplicitFlags(disableImplicitFlags)
+        {
+        }
+
+        ~AutoRestoreImplicitFlags()
+        {
+            threadContext->SetImplicitCallFlags((Js::ImplicitCallFlags)(savedImplicitCallFlags));
+            threadContext->SetDisableImplicitFlags((DisableImplicitFlags)savedDisableImplicitFlags);
+        }
+    };
+
     void SetCurrentThreadId(DWORD threadId) { this->currentThreadId = threadId; }
     DWORD GetCurrentThreadId() const { return this->currentThreadId; }
     void SetIsThreadBound()

lib/Runtime/Language/InlineCache.inl

@@ -184,13 +184,12 @@ namespace Js
         bool canSetField; // To verify if we can set a field on the object
         Var setterValue = nullptr;
         {
-            // We need to disable implicit call to ensure the check doesn't cause unwanted side effects in debug code
-            // Save old disableImplicitFlags and implicitCallFlags and disable implicit call and exception
+            // We need to disable implicit call to ensure the check doesn't cause unwanted side effects in debug
+            // code Save old disableImplicitFlags and implicitCallFlags and disable implicit call and exception.
             ThreadContext * threadContext = requestContext->GetThreadContext();
-            DisableImplicitFlags disableImplicitFlags = *threadContext->GetAddressOfDisableImplicitFlags();
-            Js::ImplicitCallFlags implicitCallFlags = threadContext->GetImplicitCallFlags();
+            ThreadContext::AutoRestoreImplicitFlags autoRestoreImplicitFlags(threadContext, threadContext->GetImplicitCallFlags(), threadContext->GetDisableImplicitFlags());
             threadContext->ClearImplicitCallFlags();
-            *threadContext->GetAddressOfDisableImplicitFlags() = DisableImplicitCallAndExceptionFlag;
+            threadContext->SetDisableImplicitFlags(DisableImplicitCallAndExceptionFlag);
 
             DescriptorFlags flags = DescriptorFlags::None;
             canSetField = !JavascriptOperators::CheckPrototypesForAccessorOrNonWritablePropertySlow(object, propertyId, &setterValue, &flags, isRoot, requestContext);
@@ -199,15 +198,12 @@ namespace Js
                 canSetField = true; // If there was an implicit call, inconclusive. Disable debug check.
                 setterValue = nullptr;
             }
-            else
-                if ((flags & Accessor) == Accessor)
+            else if ((flags & Accessor) == Accessor)
             {
                 Assert(setterValue != nullptr);
             }
 
-            // Restore old disableImplicitFlags and implicitCallFlags
-            *threadContext->GetAddressOfDisableImplicitFlags() = disableImplicitFlags;
-            threadContext->SetImplicitCallFlags(implicitCallFlags);
+            // ImplicitCallFlags and DisableImplicitFlags restored by AutoRestoreImplicitFlags' destructor.
         }
 #endif
 

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

@@ -131,26 +131,6 @@ namespace Js
             return;
         }
 
-        struct AutoRestoreFlags
-        {
-            ThreadContext * ctx;
-            ImplicitCallFlags savedImplicitCallFlags;
-            DisableImplicitFlags savedDisableImplicitFlags;
-            AutoRestoreFlags(ThreadContext *ctx, Js::ImplicitCallFlags implFlags, DisableImplicitFlags disableImplFlags) :
-                ctx(ctx),
-                savedImplicitCallFlags(implFlags),
-                savedDisableImplicitFlags(disableImplFlags)
-            {
-                ctx->ClearDisableImplicitFlags();
-            }
-
-            ~AutoRestoreFlags()
-            {
-                ctx->SetImplicitCallFlags((Js::ImplicitCallFlags)(savedImplicitCallFlags));
-                ctx->SetDisableImplicitFlags((DisableImplicitFlags)savedDisableImplicitFlags);
-            }
-        };
-
         try {
             EnsureJsBuiltInByteCode(scriptContext);
             Assert(jsBuiltInByteCode != nullptr);
@@ -189,7 +169,8 @@ namespace Js
 #endif
             // Clear disable implicit call bit as initialization code doesn't have any side effect
             {
-                AutoRestoreFlags autoRestoreFlags(scriptContext->GetThreadContext(), scriptContext->GetThreadContext()->GetImplicitCallFlags(), scriptContext->GetThreadContext()->GetDisableImplicitFlags());
+                ThreadContext::AutoRestoreImplicitFlags autoRestoreImplicitFlags(scriptContext->GetThreadContext(), scriptContext->GetThreadContext()->GetImplicitCallFlags(), scriptContext->GetThreadContext()->GetDisableImplicitFlags());
+                scriptContext->GetThreadContext()->ClearDisableImplicitFlags();
                 JavascriptFunction::CallRootFunctionInScript(functionGlobal, Js::Arguments(callInfo, args));
             }
 
@@ -198,7 +179,8 @@ namespace Js
 
             // Clear disable implicit call bit as initialization code doesn't have any side effect
             {
-                AutoRestoreFlags autoRestoreFlags(scriptContext->GetThreadContext(), scriptContext->GetThreadContext()->GetImplicitCallFlags(), scriptContext->GetThreadContext()->GetDisableImplicitFlags());
+                ThreadContext::AutoRestoreImplicitFlags autoRestoreImplicitFlags(scriptContext->GetThreadContext(), scriptContext->GetThreadContext()->GetImplicitCallFlags(), scriptContext->GetThreadContext()->GetDisableImplicitFlags());
+                scriptContext->GetThreadContext()->ClearDisableImplicitFlags();
                 JavascriptFunction::CallRootFunctionInScript(functionBuiltins, Js::Arguments(callInfo, args));
             }
 

test/TryCatch/TryCatchStackOverflow.js

@@ -0,0 +1,16 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+var a = [];
+function f() {
+    try {
+        f();
+    } catch (e) {
+        a.foo = 100;
+    }
+}
+f();
+
+print("pass");

test/TryCatch/rlexe.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="utf-8"?>
+<regress-exe>
+  <test>
+    <default>
+      <files>TryCatchStackOverflow.js</files>
+    </default>
+  </test>
+</regress-exe>