[1.6>1.7] [MERGE #3447 @dilijev] Fix #3438: AutoInitLibraryCodeScope:…

… hide Intl.js initialization from debugger in addition to profiler. Merge pull request #3447 from dilijev:debugger-suspend Fxies #3438
chakra-core · Jul 28, 2017 · 786f348 · 786f348
2 parents 76a2210 + ed7ef5a
commit 786f348
Show file tree

Hide file tree

Showing 14 changed files with 109 additions and 47 deletions.
diff --git a/lib/Runtime/Base/ScriptContext.cpp b/lib/Runtime/Base/ScriptContext.cpp
@@ -3999,7 +3999,6 @@ namespace Js
             }
         }
 
-
         __TRY_FINALLY_BEGIN // SEH is not guaranteed, see the implementation
         {
             Assert(!function->IsScriptFunction() || function->GetFunctionProxy());
@@ -4016,7 +4015,7 @@ namespace Js
             OUTPUT_VERBOSE_TRACE(Js::DebuggerPhase, _u("DebugProfileProbeThunk: calling function: %s isWrapperRegistered=%d useDebugWrapper=%d\n"),
                 function->GetFunctionInfo()->HasBody() ? function->GetFunctionBody()->GetDisplayName() : _u("built-in/library"), AutoRegisterIgnoreExceptionWrapper::IsRegistered(scriptContext->GetThreadContext()), useDebugWrapper);
 
-            if (scriptContext->IsScriptContextInDebugMode())
+            if (scriptContext->IsDebuggerRecording())
             {
                 scriptContext->GetDebugContext()->GetProbeContainer()->StartRecordingCall();
             }
@@ -4089,7 +4088,7 @@ namespace Js
             }
 #endif
 
-            if (scriptContext->IsScriptContextInDebugMode())
+            if (scriptContext->IsDebuggerRecording())
             {
                 scriptContext->GetDebugContext()->GetProbeContainer()->EndRecordingCall(aReturn, function);
             }
@@ -5819,6 +5818,23 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
         return false;
     }
 
+    bool ScriptContext::IsDebuggerRecording() const
+    {
+        if (this->debugContext != nullptr)
+        {
+            return this->GetDebugContext()->IsDebuggerRecording();
+        }
+        return false;
+    }
+
+    void ScriptContext::SetIsDebuggerRecording(bool isDebuggerRecording)
+    {
+        if (this->debugContext != nullptr)
+        {
+            this->GetDebugContext()->SetIsDebuggerRecording(isDebuggerRecording);
+        }
+    }
+
     bool ScriptContext::IsIntlEnabled()
     {
 #ifdef ENABLE_INTL_OBJECT

diff --git a/lib/Runtime/Base/ScriptContext.h b/lib/Runtime/Base/ScriptContext.h
@@ -500,6 +500,10 @@ namespace Js
         bool IsScriptContextInNonDebugMode() const;
         bool IsScriptContextInDebugMode() const;
         bool IsScriptContextInSourceRundownOrDebugMode() const;
+
+        bool IsDebuggerRecording() const;
+        void SetIsDebuggerRecording(bool isDebuggerRecording);
+
         bool IsRunningScript() const { return this->threadContext->GetScriptEntryExit() != nullptr; }
 
         typedef JsUtil::List<RecyclerWeakReference<Utf8SourceInfo>*, Recycler, false, Js::WeakRefFreeListedRemovePolicy> CalleeSourceList;
@@ -1879,8 +1883,34 @@ namespace Js
         Js::Phase phase;
         bool isPhaseComplete;
     };
-}
 
+    // Set up a scope in which we will initialize library JS code (like Intl.js),
+    // which should not be treated as user-level JS code.
+    // We should not profile and should not log debugger information in such a scope.
+    class AutoInitLibraryCodeScope
+    {
+    private:
+        ScriptContext * const scriptContext;
+        const bool oldIsProfilingUserCode;
+        const bool oldIsDebuggerRecording;
+
+    public:
+        AutoInitLibraryCodeScope(ScriptContext *scriptContext) :
+            scriptContext(scriptContext),
+            oldIsProfilingUserCode(scriptContext->GetThreadContext()->IsProfilingUserCode()),
+            oldIsDebuggerRecording(scriptContext->IsDebuggerRecording())
+        {
+            this->scriptContext->GetThreadContext()->SetIsProfilingUserCode(false);
+            this->scriptContext->SetIsDebuggerRecording(false);
+        }
+
+        ~AutoInitLibraryCodeScope()
+        {
+            this->scriptContext->GetThreadContext()->SetIsProfilingUserCode(this->oldIsProfilingUserCode);
+            this->scriptContext->SetIsDebuggerRecording(this->oldIsDebuggerRecording);
+        }
+    };
+}
 
 #define BEGIN_TEMP_ALLOCATOR(allocator, scriptContext, name) \
     Js::TempArenaAllocatorObject *temp##allocator = scriptContext->GetTemporaryAllocator(name); \

diff --git a/lib/Runtime/Base/ThreadContext.h b/lib/Runtime/Base/ThreadContext.h
@@ -1800,27 +1800,6 @@ class ThreadContext sealed :
 
 extern void(*InitializeAdditionalProperties)(ThreadContext *threadContext);
 
-// Temporarily set script profiler isProfilingUserCode state, restore at destructor
-class AutoProfilingUserCode
-{
-private:
-    ThreadContext* threadContext;
-    const bool oldIsProfilingUserCode;
-
-public:
-    AutoProfilingUserCode(ThreadContext* threadContext, bool isProfilingUserCode) :
-        threadContext(threadContext),
-        oldIsProfilingUserCode(threadContext->IsProfilingUserCode())
-    {
-        threadContext->SetIsProfilingUserCode(isProfilingUserCode);
-    }
-
-    ~AutoProfilingUserCode()
-    {
-        threadContext->SetIsProfilingUserCode(oldIsProfilingUserCode);
-    }
-};
-
 #if ENABLE_JS_REENTRANCY_CHECK
 class JsReentLock
 {

diff --git a/lib/Runtime/Debug/DebugContext.cpp b/lib/Runtime/Debug/DebugContext.cpp
@@ -11,6 +11,7 @@ namespace Js
         hostDebugContext(nullptr),
         diagProbesContainer(nullptr),
         debuggerMode(DebuggerMode::NotDebugging),
+        isDebuggerRecording(true),
         isReparsingSource(false)
     {
         Assert(scriptContext != nullptr);

diff --git a/lib/Runtime/Debug/DebugContext.h b/lib/Runtime/Debug/DebugContext.h
@@ -51,11 +51,15 @@ namespace Js
         void SetHostDebugContext(HostDebugContext * hostDebugContext);
 
         void SetDebuggerMode(DebuggerMode mode);
+
         bool IsDebugContextInNonDebugMode() const { return this->debuggerMode == DebuggerMode::NotDebugging; }
         bool IsDebugContextInDebugMode() const { return this->debuggerMode == DebuggerMode::Debugging; }
         bool IsDebugContextInSourceRundownMode() const { return this->debuggerMode == DebuggerMode::SourceRundown; }
         bool IsDebugContextInSourceRundownOrDebugMode() const { return IsDebugContextInSourceRundownMode() || IsDebugContextInDebugMode(); }
 
+        bool IsDebuggerRecording() const { return this->isDebuggerRecording; }
+        void SetIsDebuggerRecording(bool isDebuggerRecording) { this->isDebuggerRecording = isDebuggerRecording; }
+
         ProbeContainer* GetProbeContainer() const { return this->diagProbesContainer; }
 
         HostDebugContext * GetHostDebugContext() const { return hostDebugContext; }
@@ -65,8 +69,9 @@ namespace Js
     private:
         ScriptContext * scriptContext;
         HostDebugContext* hostDebugContext;
-        DebuggerMode debuggerMode;
         ProbeContainer* diagProbesContainer;
+        DebuggerMode debuggerMode;
+        bool isDebuggerRecording;
         bool isReparsingSource;
 
         // Private Functions

diff --git a/lib/Runtime/Debug/ProbeContainer.cpp b/lib/Runtime/Debug/ProbeContainer.cpp
@@ -75,11 +75,13 @@ namespace Js
 
     void ProbeContainer::StartRecordingCall()
     {
+        Assert(this->pScriptContext->GetDebugContext() && this->pScriptContext->GetDebugContext()->IsDebuggerRecording());
         this->debugManager->stepController.StartRecordingCall();
     }
 
     void ProbeContainer::EndRecordingCall(Js::Var returnValue, Js::JavascriptFunction * function)
     {
+        Assert(this->pScriptContext->GetDebugContext() && this->pScriptContext->GetDebugContext()->IsDebuggerRecording());
         this->debugManager->stepController.EndRecordingCall(returnValue, function);
     }
 

diff --git a/lib/Runtime/Library/IntlEngineInterfaceExtensionObject.cpp b/lib/Runtime/Library/IntlEngineInterfaceExtensionObject.cpp
@@ -466,8 +466,9 @@ namespace Js
             }
 #endif
 
-            // Mark we are profiling library code already, so that any initialization library code called here won't be reported to profiler
-            AutoProfilingUserCode autoProfilingUserCode(scriptContext->GetThreadContext(), /*isProfilingUserCode*/false);
+            // Mark we are profiling library code already, so that any initialization library code called here won't be reported to profiler.
+            // Also tell the debugger not to record events during intialization so that we don't leak information about initialization.
+            AutoInitLibraryCodeScope autoInitLibraryCodeScope(scriptContext);
 
             Js::Var args[] = { scriptContext->GetLibrary()->GetUndefined(), scriptContext->GetLibrary()->GetEngineInterfaceObject(), initType };
             Js::CallInfo callInfo(Js::CallFlags_Value, _countof(args));

diff --git a/test/Debugger/rlexe.xml b/test/Debugger/rlexe.xml
@@ -26,7 +26,7 @@
   </test>
   <test>
     <default>
-      <compile-flags>-debuglaunch -dbgbaseline:JsDiagGetFunctionPositionIntl.js.dbg.baseline</compile-flags>
+      <compile-flags>-debuglaunch -dbgbaseline:JsDiagGetFunctionPositionIntl.js.dbg.baseline -Intl</compile-flags>
       <files>JsDiagGetFunctionPositionIntl.js</files>
       <!-- xplat-todo: enable on xplat when Intl is supported on xplat (Microsoft/ChakraCore#2919) -->
       <tags>exclude_xplat,Intl</tags>

diff --git a/test/DebuggerCommon/IntlInit.js b/test/DebuggerCommon/IntlInit.js
@@ -0,0 +1,11 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+let x = 0;
+Intl.getCanonicalLocales("en-us") /**bp:resume('step_over');locals();**/
+x++;
+Intl.getCanonicalLocales("en-us") /**bp:resume('step_over');locals();**/
+x++;
+print("PASS");
diff --git a/test/DebuggerCommon/IntlInit.js.dbg.baseline b/test/DebuggerCommon/IntlInit.js.dbg.baseline
@@ -0,0 +1,20 @@
+[
+  {
+    "this": "Object {...}",
+    "functionCallsReturn": {
+      "[Intl.getCanonicalLocales returned]": "Array [en-US]"
+    },
+    "locals": {
+      "x": "number 0"
+    }
+  },
+  {
+    "this": "Object {...}",
+    "functionCallsReturn": {
+      "[Intl.getCanonicalLocales returned]": "Array [en-US]"
+    },
+    "locals": {
+      "x": "number 1"
+    }
+  }
+]
diff --git a/test/DebuggerCommon/returnedvaluetests1.js.dbg.baseline b/test/DebuggerCommon/returnedvaluetests1.js.dbg.baseline
@@ -11,7 +11,6 @@
     "this": "Object {...}",
     "arguments": "Object {...}",
     "functionCallsReturn": {
-      "[Anonymous function returned]": "undefined undefined",
       "[Intl.NumberFormat returned]": "Object {...}"
     },
     "locals": {

diff --git a/test/DebuggerCommon/rlexe.xml b/test/DebuggerCommon/rlexe.xml
@@ -28,6 +28,14 @@
       <baseline>blockscope_func_insidescopes.js.baseline</baseline>
     </default>
   </test>
+  <test>
+    <default>
+      <compile-flags>-debuglaunch -dbgbaseline:IntlInit.js.dbg.baseline -Intl</compile-flags>
+      <files>IntlInit.js</files>
+      <!-- xplat-todo: enable on xplat when Intl is supported on xplat (Microsoft/ChakraCore#2919) -->
+      <tags>exclude_winglob,exclude_xplat,Intl</tags>
+    </default>
+  </test>
   <test>
     <default>
       <files>ES6_intl_stepinto.js</files>
@@ -213,7 +221,7 @@
       <compile-flags>-dbgbaseline:ES6_intl_simple_attach.js.dbg.baseline -Intl</compile-flags>
       <baseline>ES6_intl_simple_attach.js.baseline</baseline>
       <!-- xplat-todo: enable on xplat when Intl is supported on xplat (Microsoft/ChakraCore#2919) -->
-      <tags>exclude_winglob,exclude_xplat</tags>
+      <tags>exclude_winglob,exclude_xplat,Intl</tags>
     </default>
   </test>
   <test>
@@ -286,7 +294,7 @@
       <files>ES6_intl_stepinto_libexpandos.js</files>
       <compile-flags>-debuglaunch -dbgbaseline:ES6_intl_stepinto_libexpandos.js.dbg.baseline -Intl</compile-flags>
       <!-- xplat-todo: enable on xplat when Intl is supported on xplat (Microsoft/ChakraCore#2919) -->
-      <tags>exclude_winglob,exclude_xplat</tags>
+      <tags>exclude_winglob,exclude_xplat,Intl</tags>
     </default>
   </test>
   <test>

diff --git a/test/Intl/IntlReturnedValueTests.js b/test/Intl/IntlReturnedValueTests.js
@@ -6,23 +6,14 @@
 // Return values from Intl function calls show the function name correctly in debugger.
 
 /////////////////// DateFormat ////////////////////
-var options = { ca: "gregory", hour12: true, timeZone:"UTC" }; 
-// Bug: GitHub Issue#3438: Intl.DateTimeFormat constructor call has 2 entries in the debugger functionCallsReturn.
-// https://github.com/Microsoft/ChakraCore/issues/3438
-// Actual: 
-    // "functionCallsReturn": {
-    //   "[Anonymous function returned]": "undefined undefined",
-    //   "[Intl.DateTimeFormat returned]": "Object {...}"
-// Expected:
-    // "functionCallsReturn": {
-    //   "[Intl.DateTimeFormat returned]": "Object {...}"
+var options = { ca: "gregory", hour12: true, timeZone:"UTC" };
 var dateFormat1 = new Intl.DateTimeFormat("en-US", options);    /**bp:resume('step_over');locals();**/
 WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
 var date1 = new Date(2000, 1, 1);
 
 // Bug: GitHub Issue#3439: Intl function call has 2 entries in the debugger functionCallsReturn.
 // https://github.com/Microsoft/ChakraCore/issues/3439
-// Actual: 
+// Actual:
     // "functionCallsReturn": {
     //   "[get format returned]": "function <large string>",
     //   "[Intl.DateTimeFormat.prototype.format returned]": "string ‎2‎/‎1‎/‎2000"
@@ -41,7 +32,7 @@ var numberFormat1 = new Intl.NumberFormat();                    /**bp:resume('st
 WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
 // Bug: GitHub Issue#3439: Intl function call has 2 entries in the debugger functionCallsReturn.
 // https://github.com/Microsoft/ChakraCore/issues/3439
-// Actual: 
+// Actual:
     // "functionCallsReturn": {
     //   "[get format returned]": "function <large string>",
     //   "[Intl.DateTimeFormat.prototype.format returned]": "string ‎2‎/‎1‎/‎2000"
@@ -58,7 +49,7 @@ WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Requi
 var collator1 = Intl.Collator();                                /**bp:resume('step_over');locals();resume('step_over');locals();**/
 // Bug: GitHub Issue#3439: Intl function call has 2 entries in the debugger functionCallsReturn.
 // https://github.com/Microsoft/ChakraCore/issues/3439
-// Actual: 
+// Actual:
     // "functionCallsReturn": {
     //   "[get compare returned]": "function <large string>",
     //   "[Intl.Collator.prototype.compare returned]": "number -1"

diff --git a/test/Intl/IntlReturnedValueTests.js.dbg.baseline b/test/Intl/IntlReturnedValueTests.js.dbg.baseline
@@ -2,7 +2,6 @@
   {
     "this": "Object {...}",
     "functionCallsReturn": {
-      "[Anonymous function returned]": "undefined undefined",
       "[Intl.DateTimeFormat returned]": "Object {...}"
     },
     "locals": {
@@ -224,4 +223,4 @@
       "compare1": "number -1"
     }
   }
-]
+]