[MERGE #5449 @atulkatti] MSFT:18139538 Remove the use of Guest Arena from parser code to avoid ScriptContext leak.

Merge pull request #5449 from atulkatti:Bug18139538.RemoveGuestArenaUsage.1

MSFT:18171347 Parser's Temporary Guest Arena leaks in case of modules with Regex patterns.

Author: Atul Katti

lib/Common/Memory/RecyclerRootPtr.h

@@ -51,7 +51,7 @@ class AutoRecyclerRootPtr : public RecyclerRootPtr<T>
     }
     void Unroot()
     {
-        if (ptr != nullptr)
+        if (this->ptr != nullptr)
         {
             __super::Unroot(recycler);
         }

lib/Parser/Parse.cpp

@@ -82,8 +82,10 @@ Parser::Parser(Js::ScriptContext* scriptContext, BOOL strictMode, PageAllocator
     m_doingFastScan(false),
 #endif
     m_nextBlockId(0),
+    m_tempGuestArenaReleased(false),
+    m_tempGuestArena(scriptContext->GetTemporaryGuestAllocator(_u("ParserRegex")), scriptContext->GetRecycler()),
     // use the GuestArena directly for keeping the RegexPattern* alive during byte code generation
-    m_registeredRegexPatterns(scriptContext->GetGuestArena()),
+    m_registeredRegexPatterns(m_tempGuestArena->GetAllocator()),
 
     m_scriptContext(scriptContext),
     m_token(), // should initialize to 0/nullptrs
@@ -154,12 +156,7 @@ Parser::Parser(Js::ScriptContext* scriptContext, BOOL strictMode, PageAllocator
 
 Parser::~Parser(void)
 {
-    if (m_scriptContext == nullptr || m_scriptContext->GetGuestArena() == nullptr)
-    {
-        // If the scriptContext or guestArena have gone away, there is no point clearing each item of this list.
-        // Just reset it so that destructor of the SList will be no-op
-        m_registeredRegexPatterns.Reset();
-    }
+    this->ReleaseTemporaryGuestArena();
 
 #if ENABLE_BACKGROUND_PARSING
     if (this->m_hasParallelJob)
@@ -1925,7 +1922,7 @@ void Parser::RegisterRegexPattern(UnifiedRegex::RegexPattern *const regexPattern
     Assert(regexPattern);
 
     // ensure a no-throw add behavior here, to catch out of memory exceptions, using the guest arena allocator
-    if (!m_registeredRegexPatterns.PrependNoThrow(m_scriptContext->GetGuestArena(), regexPattern))
+    if (!m_registeredRegexPatterns.PrependNoThrow(m_tempGuestArena->GetAllocator(), regexPattern))
     {
         Parser::Error(ERRnoMemory);
     }
@@ -13909,6 +13906,23 @@ void Parser::ProcessCapturedNames(ParseNodeFnc* pnodeFnc)
     }
 }
 
+void Parser::ReleaseTemporaryGuestArena()
+{
+    // In case of modules the Parser lives longer than the temporary Guest Arena. We may have already released the arena explicitly.
+    if (!m_tempGuestArenaReleased)
+    {
+        // The regex patterns list has references to the temporary Guest Arena. Reset it first.
+        m_registeredRegexPatterns.Reset();
+
+        if (this->m_scriptContext != nullptr)
+        {
+            this->m_scriptContext->ReleaseTemporaryGuestAllocator(m_tempGuestArena);
+        }
+
+        m_tempGuestArenaReleased = true;
+    }
+}
+
 bool Parser::IsCreatingStateCache()
 {
     return (((this->m_grfscr & fscrCreateParserState) == fscrCreateParserState)

lib/Parser/Parse.h

@@ -177,6 +177,8 @@ namespace Js
 {
     class ParseableFunctionInfo;
     class FunctionBody;
+    template <bool isGuestArena>
+    class TempArenaAllocatorWrapper;
 };
 
 class Parser
@@ -192,7 +194,7 @@ class Parser
     ~Parser(void);
 
     Js::ScriptContext* GetScriptContext() const { return m_scriptContext; }
-
+    void ReleaseTemporaryGuestArena();
     bool IsCreatingStateCache();
 
 #if ENABLE_BACKGROUND_PARSING
@@ -271,10 +273,12 @@ class Parser
     bool                m_isInBackground;
     bool                m_doingFastScan;
 #endif
+    bool                m_tempGuestArenaReleased;
     int                 m_nextBlockId;
 
+    AutoRecyclerRootPtr<Js::TempArenaAllocatorWrapper<true>> m_tempGuestArena;
     // RegexPattern objects created for literal regexes are recycler-allocated and need to be kept alive until the function body
-    // is created during byte code generation. The RegexPattern pointer is stored in the script context's guest
+    // is created during byte code generation. The RegexPattern pointer is stored in a temporary guest
     // arena for that purpose. This list is then unregistered from the guest arena at the end of parsing/scanning.
     SList<UnifiedRegex::RegexPattern *, ArenaAllocator> m_registeredRegexPatterns;
 
@@ -1150,5 +1154,4 @@ class Parser
 public:
     charcount_t GetSourceIchLim() { return m_sourceLim; }
     static BOOL NodeEqualsName(ParseNodePtr pnode, LPCOLESTR sz, uint32 cch);
-
 };

lib/Runtime/Base/ScriptContext.cpp

@@ -88,7 +88,6 @@ namespace Js
         integerStringMapCacheMissCount(0),
         integerStringMapCacheUseCount(0),
 #endif
-        guestArena(nullptr),
 #ifdef ENABLE_SCRIPT_DEBUGGING
         diagnosticArena(nullptr),
         raiseMessageToDebuggerFunctionType(nullptr),
@@ -798,12 +797,6 @@ namespace Js
             interpreterArena = nullptr;
         }
 
-        if (this->guestArena)
-        {
-            ReleaseGuestArena();
-            guestArena = nullptr;
-        }
-
         builtInLibraryFunctions = nullptr;
 
         pActiveScriptDirect = nullptr;
@@ -1304,8 +1297,6 @@ namespace Js
 
     void ScriptContext::InitializePreGlobal()
     {
-        this->guestArena = this->GetRecycler()->CreateGuestArena(_u("Guest"), Throw::OutOfMemory);
-
 #if ENABLE_BACKGROUND_PARSING
         if (PHASE_ON1(Js::ParallelParsePhase))
         {
@@ -2680,17 +2671,6 @@ namespace Js
         }
     }
 
-
-    void ScriptContext::ReleaseGuestArena()
-    {
-        AssertMsg(this->guestArena, "No guest arena to release");
-        if (this->guestArena)
-        {
-            this->GetRecycler()->DeleteGuestArena(this->guestArena);
-            this->guestArena = nullptr;
-        }
-    }
-
     void ScriptContext::SetScriptStartEventHandler(ScriptContext::EventHandler eventHandler)
     {
         AssertMsg(this->scriptStartEventHandler == nullptr, "Do not support multi-cast yet");
@@ -4895,7 +4875,6 @@ namespace Js
     void ScriptContext::BindReference(void * addr)
     {
         Assert(!this->isClosed);
-        Assert(this->guestArena);
         Assert(recycler->IsValidObject(addr));
 #if DBG
         Assert(!bindRef.ContainsKey(addr));     // Make sure we don't bind the same pointer twice
@@ -5062,7 +5041,6 @@ namespace Js
         {
             return;
         }
-        Assert(this->guestArena);
 
         if (EnableEvalMapCleanup())
         {

lib/Runtime/Base/ScriptContext.h

@@ -578,7 +578,6 @@ namespace Js
         CacheAllocator enumeratorCacheAllocator;
 
         ArenaAllocator* interpreterArena;
-        ArenaAllocator* guestArena;
 
 #ifdef ENABLE_SCRIPT_DEBUGGING
         ArenaAllocator* diagnosticArena;
@@ -1378,13 +1377,6 @@ namespace Js
         bool EnsureInterpreterArena(ArenaAllocator **);
         void ReleaseInterpreterArena();
 
-        ArenaAllocator* GetGuestArena() const
-        {
-            return guestArena;
-        }
-
-        void ReleaseGuestArena();
-
         Recycler* GetRecycler() const { return recycler; }
         RecyclerJavascriptNumberAllocator * GetNumberAllocator() { return &numberAllocator; }
 #if ENABLE_NATIVE_CODEGEN

lib/Runtime/Language/SourceTextModuleRecord.cpp

@@ -915,6 +915,11 @@ namespace Js
                 childModuleRecord->GenerateRootFunction();
             });
         }
+
+        if (this->parser != nullptr)
+        {
+            this->parser->ReleaseTemporaryGuestArena();
+        }
     }
 
     Var SourceTextModuleRecord::ModuleEvaluation()

test/es6module/module-functionality.js

@@ -350,6 +350,19 @@ var tests = [
             testRunner.LoadModule(functionBody, 'samethread');
         }
     },
+    {
+        name: "OS18171347 - Module's parser leaks temporary Guest Arena allocator when module has a regex pattern.",
+        body: function() {
+            testRunner.LoadModule(` /x/ ;`, 'samethread');
+
+            try
+            {
+                // syntax error
+                testRunner.LoadModule(` /x/ ; for(i=0);`, 'samethread', {shouldFail:true});
+            }
+            catch(e){}
+        }
+    },
 ];
 
 testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });