[1.9>1.10] [1.8>1.9] [MERGE #5417 @sethbrenith] JsRunSerialized crash

Merge pull request #5417 from sethbrenith:user/sethb/script-buffer-lifetime

JsrtSourceHolder needs to ensure that the input ArrayBuffer's data stays alive until after all related FunctionEntryPointInfos have finalized (because they fetch their name for ETW logging during Finalize).

Fixes OS:18095136

Author: sethbrenith

lib/Jsrt/ChakraCore.h

@@ -694,8 +694,8 @@ CHAKRA_API
 ///     Requires an active script context.
 ///     </para>
 ///     <para>
-///     The runtime will hold on to the buffer until all instances of any functions created from
-///     the buffer are garbage collected.
+///     The runtime will detach the data from the buffer and hold on to it until all
+///     instances of any functions created from the buffer are garbage collected.
 ///     </para>
 /// </remarks>
 /// <param name="buffer">The serialized script as an ArrayBuffer (preferably ExternalArrayBuffer).</param>

lib/Jsrt/Jsrt.cpp

@@ -470,6 +470,26 @@ CHAKRA_API JsPrivateCollectGarbageSkipStack(_In_ JsRuntimeHandle runtimeHandle)
 {
     return JsCollectGarbageCommon<CollectNowExhaustiveSkipStack>(runtimeHandle);
 }
+
+CHAKRA_API JsPrivateDetachArrayBuffer(_In_ JsValueRef ref, _Out_ void** detachedState)
+{
+    return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode
+    {
+        VALIDATE_JSREF(ref);
+        *detachedState = Js::JavascriptOperators::DetachVarAndGetState(ref);
+        return JsNoError;
+    });
+}
+
+CHAKRA_API JsPrivateFreeDetachedArrayBuffer(_In_ void* detachedState)
+{
+    return GlobalAPIWrapper_NoRecord([&]() -> JsErrorCode
+    {
+        auto state = reinterpret_cast<Js::ArrayBufferDetachedStateBase*>(detachedState);
+        state->CleanUp();
+        return JsNoError;
+    });
+}
 #endif
 
 CHAKRA_API JsDisposeRuntime(_In_ JsRuntimeHandle runtimeHandle)
@@ -3855,7 +3875,7 @@ template <typename TLoadCallback, typename TUnloadCallback>
 JsErrorCode RunSerializedScriptCore(
     TLoadCallback scriptLoadCallback, TUnloadCallback scriptUnloadCallback,
     JsSourceContext scriptLoadSourceContext, // only used by scriptLoadCallback
-    unsigned char *buffer, JsValueRef bufferVal,
+    unsigned char *buffer, Js::ArrayBuffer* bufferVal,
     JsSourceContext sourceContext, const WCHAR *sourceUrl,
     bool parseOnly, bool useParserStateCache, JsValueRef *result,
     uint sourceIndex)
@@ -3910,7 +3930,7 @@ JsErrorCode RunSerializedScriptCore(
         }
 
         HRESULT hr;
-        
+
         Field(Js::FunctionBody*) functionBody = nullptr;
 
         uint32 flags = 0;
@@ -5127,12 +5147,13 @@ CHAKRA_API JsParseSerialized(
         return JsErrorInvalidArgument;
     }
 
-    byte* buffer = Js::ArrayBuffer::FromVar(bufferVal)->GetBuffer();
+    Js::ArrayBuffer* arrayBuffer = Js::ArrayBuffer::FromVar(bufferVal);
+    byte* buffer = arrayBuffer->GetBuffer();
 
     return RunSerializedScriptCore(
       scriptLoadCallback, DummyScriptUnloadCallback,
       sourceContext,// use the same user provided sourceContext as scriptLoadSourceContext
-      buffer, bufferVal, sourceContext, url, true, false, result, Js::Constants::InvalidSourceIndex);
+      buffer, arrayBuffer, sourceContext, url, true, false, result, Js::Constants::InvalidSourceIndex);
 }
 
 CHAKRA_API JsRunSerialized(
@@ -5160,12 +5181,13 @@ CHAKRA_API JsRunSerialized(
         return JsErrorInvalidArgument;
     }
 
-    byte* buffer = Js::ArrayBuffer::FromVar(bufferVal)->GetBuffer();
+    Js::ArrayBuffer* arrayBuffer = Js::ArrayBuffer::FromVar(bufferVal);
+    byte* buffer = arrayBuffer->GetBuffer();
 
     return RunSerializedScriptCore(
         scriptLoadCallback, DummyScriptUnloadCallback,
         sourceContext, // use the same user provided sourceContext as scriptLoadSourceContext
-        buffer, bufferVal, sourceContext, url, false, false, result, Js::Constants::InvalidSourceIndex);
+        buffer, arrayBuffer, sourceContext, url, false, false, result, Js::Constants::InvalidSourceIndex);
 }
 
 CHAKRA_API JsCreatePromise(_Out_ JsValueRef *promise, _Out_ JsValueRef *resolve, _Out_ JsValueRef *reject)
@@ -5744,13 +5766,14 @@ CHAKRA_API JsRunScriptWithParserState(
         return JsErrorInvalidArgument;
     }
 
-    byte* buffer = Js::ArrayBuffer::FromVar(parserState)->GetBuffer();
+    Js::ArrayBuffer* arrayBuffer = Js::ArrayBuffer::FromVar(parserState);
+    byte* buffer = arrayBuffer->GetBuffer();
     JsSerializedLoadScriptCallback dummy = DummyScriptLoadSourceCallbackForRunScriptWithParserState;
 
     return RunSerializedScriptCore(
         dummy, DummyScriptUnloadCallback,
         sourceContext, // use the same user provided sourceContext as scriptLoadSourceContext
-        buffer, parserState, sourceContext, url, false, true, result, sourceIndex);
+        buffer, arrayBuffer, sourceContext, url, false, true, result, sourceIndex);
 }
 
 #endif // _CHAKRACOREBUILD

lib/Jsrt/JsrtExternalArrayBuffer.cpp

@@ -19,10 +19,41 @@ namespace Js
     }
 
     void JsrtExternalArrayBuffer::Finalize(bool isShutdown)
+    {
+        if (finalizeCallback != nullptr && !isDetached)
+        {
+            finalizeCallback(callbackState);
+        }
+    }
+
+    ArrayBufferDetachedStateBase* JsrtExternalArrayBuffer::CreateDetachedState(BYTE* buffer, DECLSPEC_GUARD_OVERFLOW uint32 bufferLength)
+    {
+        return HeapNew(JsrtExternalArrayBufferDetachedState, buffer, bufferLength, finalizeCallback, callbackState);
+    };
+
+    JsrtExternalArrayBuffer::JsrtExternalArrayBufferDetachedState::JsrtExternalArrayBufferDetachedState(
+        BYTE* buffer, uint32 bufferLength, JsFinalizeCallback finalizeCallback, void *callbackState)
+        : ExternalArrayBufferDetachedState(buffer, bufferLength), finalizeCallback(finalizeCallback), callbackState(callbackState)
+    {}
+
+    void JsrtExternalArrayBuffer::JsrtExternalArrayBufferDetachedState::ClearSelfOnly()
+    {
+        HeapDelete(this);
+    }
+
+    void JsrtExternalArrayBuffer::JsrtExternalArrayBufferDetachedState::DiscardState()
     {
         if (finalizeCallback != nullptr)
         {
             finalizeCallback(callbackState);
         }
+        finalizeCallback = nullptr;
+    }
+
+    ArrayBuffer* JsrtExternalArrayBuffer::JsrtExternalArrayBufferDetachedState::Create(JavascriptLibrary* library)
+    {
+        ArrayBuffer* arr = JsrtExternalArrayBuffer::New(buffer, bufferLength, finalizeCallback, callbackState, library->GetArrayBufferType());
+        JS_ETW(EventWriteJSCRIPT_RECYCLER_ALLOCATE_OBJECT(arr));
+        return arr;
     }
 }

lib/Jsrt/JsrtExternalArrayBuffer.h

@@ -12,6 +12,7 @@ namespace Js {
         DEFINE_MARSHAL_OBJECT_TO_SCRIPT_CONTEXT(JsrtExternalArrayBuffer);
 
         JsrtExternalArrayBuffer(byte *buffer, uint32 length, JsFinalizeCallback finalizeCallback, void *callbackState, DynamicType *type);
+        virtual ArrayBufferDetachedStateBase* CreateDetachedState(BYTE* buffer, DECLSPEC_GUARD_OVERFLOW uint32 bufferLength) override;
 
     public:
         static JsrtExternalArrayBuffer* New(byte *buffer, uint32 length, JsFinalizeCallback finalizeCallback, void *callbackState, DynamicType *type);
@@ -20,6 +21,17 @@ namespace Js {
     private:
         FieldNoBarrier(JsFinalizeCallback) finalizeCallback;
         Field(void *) callbackState;
+
+        class JsrtExternalArrayBufferDetachedState : public ExternalArrayBufferDetachedState
+        {
+            FieldNoBarrier(JsFinalizeCallback) finalizeCallback;
+            Field(void *) callbackState;
+        public:
+            JsrtExternalArrayBufferDetachedState(BYTE* buffer, uint32 bufferLength, JsFinalizeCallback finalizeCallback, void *callbackState);
+            virtual void ClearSelfOnly() override;
+            virtual void DiscardState() override;
+            virtual ArrayBuffer* Create(JavascriptLibrary* library) override;
+        };
     };
 }
 AUTO_REGISTER_RECYCLER_OBJECT_DUMPER(Js::JsrtExternalArrayBuffer, &Js::RecyclableObject::DumpObjectFunction);

lib/Jsrt/JsrtSourceHolder.cpp

@@ -299,7 +299,6 @@ namespace Js
             this->mappedSource = nullptr;
         }
         this->mappedScriptValue = nullptr;
-        this->mappedSerializedScriptValue = nullptr;
 
         // Don't allow load or unload again after told to unload.
         scriptLoadCallback = nullptr;

lib/Jsrt/JsrtSourceHolder.h

@@ -19,7 +19,7 @@ namespace Js
 
 #ifndef NTBUILD
         Field(JsValueRef) mappedScriptValue;
-        Field(JsValueRef) mappedSerializedScriptValue;
+        Field(DetachedStateBase*) mappedSerializedScriptValue;
 #endif
         Field(utf8char_t const *) mappedSource;
         Field(size_t) mappedSourceByteLength;
@@ -46,13 +46,13 @@ namespace Js
         JsrtSourceHolder(_In_ TLoadCallback scriptLoadCallback,
             _In_ TUnloadCallback scriptUnloadCallback,
             _In_ JsSourceContext sourceContext,
-            JsValueRef serializedScriptValue = nullptr) :
+            Js::ArrayBuffer* serializedScriptValue = nullptr) :
             scriptLoadCallback(scriptLoadCallback),
             scriptUnloadCallback(scriptUnloadCallback),
             sourceContext(sourceContext),
 #ifndef NTBUILD
             mappedScriptValue(nullptr),
-            mappedSerializedScriptValue(serializedScriptValue),
+            mappedSerializedScriptValue(serializedScriptValue == nullptr ? nullptr : serializedScriptValue->DetachAndGetState()),
 #endif
             mappedSourceByteLength(0),
             mappedSource(nullptr)
@@ -89,6 +89,15 @@ namespace Js
 
         virtual void Dispose(bool isShutdown) override
         {
+#ifndef NTBUILD
+            if (this->mappedSerializedScriptValue != nullptr)
+            {
+                // We have to extend the buffer data's lifetime until Dispose because
+                // it might be used during finalization of other objects, such as
+                // FunctionEntryPointInfo which wants to log its name.
+                this->mappedSerializedScriptValue->CleanUp();
+            }
+#endif
         }
 
         virtual void Mark(Recycler * recycler) override

lib/Runtime/DetachedStateBase.h

@@ -47,7 +47,8 @@ namespace Js
     {
         Heap = 0x0,
         CoTask = 0x1,
-        MemAlloc = 0x02
+        MemAlloc = 0x02,
+        External = 0x03,
     } ArrayBufferAllocationType;
 
     class ArrayBufferDetachedStateBase : public DetachedStateBase

lib/Runtime/Library/ArrayBuffer.cpp

@@ -37,6 +37,9 @@ namespace Js
         case ArrayBufferAllocationType::MemAlloc:
             toReturn = library->CreateArrayBuffer(arrayBufferState->buffer, arrayBufferState->bufferLength);
             break;
+        case ArrayBufferAllocationType::External:
+            toReturn = static_cast<ExternalArrayBufferDetachedState*>(state)->Create(library);
+            break;
         default:
             AssertMsg(false, "Unknown allocationType of ArrayBufferDetachedStateBase ");
         }
@@ -1066,11 +1069,27 @@ namespace Js
         /* See ProjectionArrayBuffer::Finalize */
     }
 
+    ArrayBuffer* ExternalArrayBufferDetachedState::Create(JavascriptLibrary* library)
+    {
+        return library->CreateExternalArrayBuffer(buffer, bufferLength);
+    }
+
     ExternalArrayBuffer::ExternalArrayBuffer(byte *buffer, uint32 length, DynamicType *type)
         : ArrayBuffer(buffer, length, type, true)
     {
     }
 
+    ExternalArrayBuffer* ExternalArrayBuffer::Create(byte* buffer, uint32 length, DynamicType * type)
+    {
+        // This type does not own the external memory, so don't AddExternalMemoryUsage like other ArrayBuffer types do
+        return RecyclerNewFinalized(type->GetScriptContext()->GetRecycler(), ExternalArrayBuffer, buffer, length, type);
+    }
+
+    ArrayBufferDetachedStateBase* ExternalArrayBuffer::CreateDetachedState(BYTE* buffer, DECLSPEC_GUARD_OVERFLOW uint32 bufferLength)
+    {
+        return HeapNew(ExternalArrayBufferDetachedState, buffer, bufferLength);
+    };
+
 #if ENABLE_TTD
     TTD::NSSnapObjects::SnapObjectType ExternalArrayBuffer::GetSnapTag_TTD() const
     {
@@ -1096,4 +1115,23 @@ namespace Js
         TTD::NSSnapObjects::StdExtractSetKindSpecificInfo<TTD::NSSnapObjects::SnapArrayBufferInfo*, TTD::NSSnapObjects::SnapObjectType::SnapArrayBufferObject>(objData, sabi);
     }
 #endif
+
+    ExternalArrayBufferDetachedState::ExternalArrayBufferDetachedState(BYTE* buffer, uint32 bufferLength)
+        : ArrayBufferDetachedStateBase(TypeIds_ArrayBuffer, buffer, bufferLength, ArrayBufferAllocationType::External)
+    {}
+
+    void ExternalArrayBufferDetachedState::ClearSelfOnly()
+    {
+        HeapDelete(this);
+    }
+
+    void ExternalArrayBufferDetachedState::DiscardState()
+    {
+        // Nothing to do as buffer is external
+    }
+
+    void ExternalArrayBufferDetachedState::Discard()
+    {
+        ClearSelfOnly();
+    }
 }

lib/Runtime/Library/ArrayBuffer.h

@@ -337,15 +337,27 @@ namespace Js
     protected:
         DEFINE_VTABLE_CTOR(ExternalArrayBuffer, ArrayBuffer);
         DEFINE_MARSHAL_OBJECT_TO_SCRIPT_CONTEXT(ExternalArrayBuffer);
+
     public:
         ExternalArrayBuffer(byte *buffer, DECLSPEC_GUARD_OVERFLOW uint32 length, DynamicType *type);
+        static ExternalArrayBuffer* Create(byte* buffer, DECLSPEC_GUARD_OVERFLOW uint32 length, DynamicType * type);
     protected:
-        virtual ArrayBufferDetachedStateBase* CreateDetachedState(BYTE* buffer, DECLSPEC_GUARD_OVERFLOW uint32 bufferLength) override { Assert(UNREACHED); Throw::InternalError(); };
+        virtual ArrayBufferDetachedStateBase* CreateDetachedState(BYTE* buffer, DECLSPEC_GUARD_OVERFLOW uint32 bufferLength) override;
 
 #if ENABLE_TTD
     public:
         virtual TTD::NSSnapObjects::SnapObjectType GetSnapTag_TTD() const override;
         virtual void ExtractSnapObjectDataInto(TTD::NSSnapObjects::SnapObject* objData, TTD::SlabAllocator& alloc) override;
 #endif
     };
+
+    class ExternalArrayBufferDetachedState : public ArrayBufferDetachedStateBase
+    {
+    public:
+        ExternalArrayBufferDetachedState(BYTE* buffer, uint32 bufferLength);
+        virtual void ClearSelfOnly() override;
+        virtual void DiscardState() override;
+        virtual void Discard() override;
+        virtual ArrayBuffer* Create(JavascriptLibrary* library);
+    };
 }

lib/Runtime/Library/JavascriptLibrary.cpp

@@ -5750,6 +5750,13 @@ namespace Js
         return arr;
     }
 
+    ArrayBuffer* JavascriptLibrary::CreateExternalArrayBuffer(byte* buffer, uint32 length)
+    {
+        ArrayBuffer* arr = ExternalArrayBuffer::Create(buffer, length, arrayBufferType);
+        JS_ETW(EventWriteJSCRIPT_RECYCLER_ALLOCATE_OBJECT(arr));
+        return arr;
+    }
+
     DataView* JavascriptLibrary::CreateDataView(ArrayBufferBase* arrayBuffer, uint32 offset, uint32 length)
     {
         DataView* dataView = RecyclerNew(this->GetRecycler(), DataView, arrayBuffer, offset, length, dataViewType);