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

Merge pull request #5441 from atulkatti:Revert.ParserGuestArenaRemoval.1

This reverts commit 3fb7fdd.

This introduced a leak in the modules that will need non-trivial work to get rid of.

Author: Atul Katti

lib/Common/Memory/RecyclerRootPtr.h

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

lib/Parser/Parse.cpp

@@ -82,9 +82,8 @@ Parser::Parser(Js::ScriptContext* scriptContext, BOOL strictMode, PageAllocator
     m_doingFastScan(false),
 #endif
     m_nextBlockId(0),
-    m_tempGuestArena(scriptContext->GetTemporaryGuestAllocator(_u("ParserRegex")), scriptContext->GetRecycler()),
     // use the GuestArena directly for keeping the RegexPattern* alive during byte code generation
-    m_registeredRegexPatterns(m_tempGuestArena->GetAllocator()),
+    m_registeredRegexPatterns(scriptContext->GetGuestArena()),
 
     m_scriptContext(scriptContext),
     m_token(), // should initialize to 0/nullptrs
@@ -155,10 +154,11 @@ Parser::Parser(Js::ScriptContext* scriptContext, BOOL strictMode, PageAllocator
 
 Parser::~Parser(void)
 {
-    m_registeredRegexPatterns.Reset();
-    if (m_scriptContext != nullptr)
+    if (m_scriptContext == nullptr || m_scriptContext->GetGuestArena() == nullptr)
     {
-        m_scriptContext->ReleaseTemporaryGuestAllocator(m_tempGuestArena);
+        // 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();
     }
 
 #if ENABLE_BACKGROUND_PARSING
@@ -1925,7 +1925,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_tempGuestArena->GetAllocator(), regexPattern))
+    if (!m_registeredRegexPatterns.PrependNoThrow(m_scriptContext->GetGuestArena(), regexPattern))
     {
         Parser::Error(ERRnoMemory);
     }

lib/Parser/Parse.h

@@ -177,8 +177,6 @@ namespace Js
 {
     class ParseableFunctionInfo;
     class FunctionBody;
-    template <bool isGuestArena>
-    class TempArenaAllocatorWrapper;
 };
 
 class Parser
@@ -275,9 +273,8 @@ class Parser
 #endif
     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 a temporary guest
+    // is created during byte code generation. The RegexPattern pointer is stored in the script context's 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;
 

lib/Runtime/Base/ScriptContext.cpp

@@ -88,6 +88,7 @@ namespace Js
         integerStringMapCacheMissCount(0),
         integerStringMapCacheUseCount(0),
 #endif
+        guestArena(nullptr),
 #ifdef ENABLE_SCRIPT_DEBUGGING
         diagnosticArena(nullptr),
         raiseMessageToDebuggerFunctionType(nullptr),
@@ -797,6 +798,12 @@ namespace Js
             interpreterArena = nullptr;
         }
 
+        if (this->guestArena)
+        {
+            ReleaseGuestArena();
+            guestArena = nullptr;
+        }
+
         builtInLibraryFunctions = nullptr;
 
         pActiveScriptDirect = nullptr;
@@ -1297,6 +1304,8 @@ namespace Js
 
     void ScriptContext::InitializePreGlobal()
     {
+        this->guestArena = this->GetRecycler()->CreateGuestArena(_u("Guest"), Throw::OutOfMemory);
+
 #if ENABLE_BACKGROUND_PARSING
         if (PHASE_ON1(Js::ParallelParsePhase))
         {
@@ -2671,6 +2680,17 @@ 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");
@@ -4875,6 +4895,7 @@ 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
@@ -5041,6 +5062,7 @@ namespace Js
         {
             return;
         }
+        Assert(this->guestArena);
 
         if (EnableEvalMapCleanup())
         {

lib/Runtime/Base/ScriptContext.h

@@ -578,6 +578,7 @@ namespace Js
         CacheAllocator enumeratorCacheAllocator;
 
         ArenaAllocator* interpreterArena;
+        ArenaAllocator* guestArena;
 
 #ifdef ENABLE_SCRIPT_DEBUGGING
         ArenaAllocator* diagnosticArena;
@@ -1377,6 +1378,13 @@ 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