From c9ae1d0eea701b2b03c5c9adf2449196e0bab015 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Wed, 4 Jan 2023 13:55:32 -0800 Subject: [PATCH 1/2] Fix assembly unloading for DispatchInfo Create DispatchInfo functions for manipulating loader handles instead of sharing the LoaderAllocator. Make the DispatchMemberInfo look more regular and make all fields private. --- src/coreclr/vm/dispatchinfo.cpp | 92 +++++++++++-------- src/coreclr/vm/dispatchinfo.h | 56 +++++++---- src/coreclr/vm/loaderallocator.hpp | 2 +- src/coreclr/vm/stdinterfaces.cpp | 10 +- .../PInvoke/Delegate/DelegateTest.csproj | 2 - .../Variant/VariantTestComWrappers.csproj | 2 +- 6 files changed, 99 insertions(+), 65 deletions(-) diff --git a/src/coreclr/vm/dispatchinfo.cpp b/src/coreclr/vm/dispatchinfo.cpp index 59aa001f2d4174..d1031f3fada861 100644 --- a/src/coreclr/vm/dispatchinfo.cpp +++ b/src/coreclr/vm/dispatchinfo.cpp @@ -138,7 +138,7 @@ DispatchMemberInfo::~DispatchMemberInfo() delete [] m_pParamInOnly; if (m_hndMemberInfo) - m_pDispInfo->GetLoaderAllocator()->FreeHandle(m_hndMemberInfo); + m_pDispInfo->FreeHandle(m_hndMemberInfo); // Clear the name of the member. m_strName.Clear(); @@ -340,7 +340,8 @@ PTRARRAYREF DispatchMemberInfo::GetParameters() OBJECTREF DispatchMemberInfo::GetMemberInfoObject() { - return m_pDispInfo->GetLoaderAllocator()->GetHandleValue(m_hndMemberInfo); + WRAPPER_NO_CONTRACT; + return m_pDispInfo->GetHandleValue(m_hndMemberInfo); } void DispatchMemberInfo::MarshalParamNativeToManaged(int iParam, VARIANT *pSrcVar, OBJECTREF *pDestObj) @@ -1059,7 +1060,7 @@ DispatchInfo::~DispatchInfo() while (pCurrMember) { // Retrieve the next member. - DispatchMemberInfo* pNextMember = pCurrMember->m_pNext; + DispatchMemberInfo* pNextMember = pCurrMember->GetNext(); // Delete the current member. delete pCurrMember; @@ -1124,9 +1125,10 @@ DispatchMemberInfo* DispatchInfo::FindMember(SString& strName, BOOL bCaseSensiti if (pCurrMemberInfo->GetMemberInfoObject() != NULL) { // Compare the 2 strings. - if (bCaseSensitive ? - pCurrMemberInfo->m_strName.Equals(strName) : - pCurrMemberInfo->m_strName.EqualsCaseInsensitive(strName)) + SString& name = pCurrMemberInfo->GetName(); + if (bCaseSensitive + ? name.Equals(strName) + : name.EqualsCaseInsensitive(strName)) { // We have found the member, so ensure it is initialized and return it. pCurrMemberInfo->EnsureInitialized(); @@ -1136,7 +1138,7 @@ DispatchMemberInfo* DispatchInfo::FindMember(SString& strName, BOOL bCaseSensiti } // Process the next member. - pCurrMemberInfo = pCurrMemberInfo->m_pNext; + pCurrMemberInfo = pCurrMemberInfo->GetNext(); } // No member has been found with the corresponding name. @@ -1145,7 +1147,7 @@ DispatchMemberInfo* DispatchInfo::FindMember(SString& strName, BOOL bCaseSensiti // Helper method used to create DispatchMemberInfo's. This is only here because // we can't call new inside a method that has a EX_TRY statement. -DispatchMemberInfo* DispatchInfo::CreateDispatchMemberInfoInstance(DISPID DispID, SString& strMemberName, OBJECTREF MemberInfoObj) +DispatchMemberInfo* DispatchInfo::CreateDispatchMemberInfoInstance(DISPID dispID, SString& strMemberName, OBJECTREF memberInfoObj) { CONTRACT (DispatchMemberInfo*) { @@ -1157,8 +1159,8 @@ DispatchMemberInfo* DispatchInfo::CreateDispatchMemberInfoInstance(DISPID DispID } CONTRACT_END; - DispatchMemberInfo* pInfo = new DispatchMemberInfo(this, DispID, strMemberName); - pInfo->SetHandle(GetLoaderAllocator()->AllocateHandle(MemberInfoObj)); + DispatchMemberInfo* pInfo = new DispatchMemberInfo(this, dispID, strMemberName); + pInfo->SetHandle(AllocateHandle(memberInfoObj)); RETURN pInfo; } @@ -1756,7 +1758,7 @@ void DispatchInfo::InvokeMemberWorker(DispatchMemberInfo* pDispMemberInfo, } else { - pObjs->MemberName = (OBJECTREF)StringObject::NewString(pDispMemberInfo->m_strName.GetUnicode()); + pObjs->MemberName = (OBJECTREF)StringObject::NewString(pDispMemberInfo->GetName().GetUnicode()); } // If there are named arguments, then set up the array of named arguments @@ -2730,7 +2732,7 @@ BOOL DispatchInfo::SynchWithManagedView() // Go through the list of member info's and find the end. DispatchMemberInfo **ppNextMember = &m_pFirstMemberInfo; while (*ppNextMember) - ppNextMember = &((*ppNextMember)->m_pNext); + ppNextMember = (*ppNextMember)->GetNextField(); // Retrieve the member info map. pMemberMap = GetMemberInfoMap(); @@ -2785,7 +2787,7 @@ BOOL DispatchInfo::SynchWithManagedView() } // Check the next member. - pCurrMemberInfo = pCurrMemberInfo->m_pNext; + pCurrMemberInfo = pCurrMemberInfo->GetNext(); } // If we have not found a match then we need to add the member info to the @@ -2846,7 +2848,7 @@ BOOL DispatchInfo::SynchWithManagedView() *ppNextMember = pMemberToAdd; // Update ppNextMember to be ready for the next new member. - ppNextMember = &((*ppNextMember)->m_pNext); + ppNextMember = (*ppNextMember)->GetNextField(); // Add the member to the map. Note, the hash is unsynchronized, but we already have our lock // so we're okay. @@ -2902,6 +2904,41 @@ BOOL DispatchInfo::VariantIsMissing(VARIANT *pOle) return (V_VT(pOle) == VT_ERROR) && (V_ERROR(pOle) == DISP_E_PARAMNOTFOUND); } +LOADERHANDLE DispatchInfo::AllocateHandle(OBJECTREF objRef) +{ + WRAPPER_NO_CONTRACT; + + return m_pMT->GetLoaderAllocator()->AllocateHandle(objRef); +} + +void DispatchInfo::FreeHandle(LOADERHANDLE handle) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + PRECONDITION(handle != NULL); + } + CONTRACTL_END; + + PTR_LoaderAllocator loaderAllocator = m_pMT->GetLoaderAllocator(); + + // If the loader isn't alive, we can't free the handle. + if (loaderAllocator->AddReferenceIfAlive()) + { + loaderAllocator->FreeHandle(handle); + loaderAllocator->Release(); + } +} + +OBJECTREF DispatchInfo::GetHandleValue(LOADERHANDLE handle) +{ + WRAPPER_NO_CONTRACT; + + return m_pMT->GetLoaderAllocator()->GetHandleValue(handle); +} + PTRARRAYREF DispatchInfo::RetrievePropList() { CONTRACTL @@ -3272,27 +3309,6 @@ HRESULT DispatchExInfo::SynchInvokeMember(SimpleComCallWrapper *pSimpleWrap, DIS return hr; } -// Helper method used to create DispatchMemberInfo's. This is only here because -// we can't call new inside a method that has a EX_TRY statement. -DispatchMemberInfo* DispatchExInfo::CreateDispatchMemberInfoInstance(DISPID DispID, SString& strMemberName, OBJECTREF MemberInfoObj) -{ - CONTRACT (DispatchMemberInfo*) - { - THROWS; - GC_TRIGGERS; - MODE_ANY; - INJECT_FAULT(COMPlusThrowOM()); - POSTCONDITION(CheckPointer(RETVAL)); - } - CONTRACT_END; - - DispatchMemberInfo* pInfo = new DispatchMemberInfo(this, DispID, strMemberName); - pInfo->SetHandle(GetLoaderAllocator()->AllocateHandle(MemberInfoObj)); - - RETURN pInfo; -} - - DispatchMemberInfo* DispatchExInfo::GetFirstMember() { CONTRACT (DispatchMemberInfo*) @@ -3323,7 +3339,7 @@ DispatchMemberInfo* DispatchExInfo::GetFirstMember() // Now we need to make sure we skip any members that are deleted. while ((*ppNextMemberInfo) && !(*ppNextMemberInfo)->GetMemberInfoObject()) - ppNextMemberInfo = &((*ppNextMemberInfo)->m_pNext); + ppNextMemberInfo = (*ppNextMemberInfo)->GetNextField(); RETURN *ppNextMemberInfo; } @@ -3345,7 +3361,7 @@ DispatchMemberInfo* DispatchExInfo::GetNextMember(DISPID CurrMemberDispID) RETURN NULL; // Start from the next member. - DispatchMemberInfo **ppNextMemberInfo = &pDispMemberInfo->m_pNext; + DispatchMemberInfo **ppNextMemberInfo = pDispMemberInfo->GetNextField(); // If the next member is not set we need to sink up with the expando object // itself to make sure that this member is really the last member and that @@ -3363,7 +3379,7 @@ DispatchMemberInfo* DispatchExInfo::GetNextMember(DISPID CurrMemberDispID) // Now we need to make sure we skip any members that are deleted. while ((*ppNextMemberInfo) && !(*ppNextMemberInfo)->GetMemberInfoObject()) - ppNextMemberInfo = &((*ppNextMemberInfo)->m_pNext); + ppNextMemberInfo = (*ppNextMemberInfo)->GetNextField(); RETURN *ppNextMemberInfo; } diff --git a/src/coreclr/vm/dispatchinfo.h b/src/coreclr/vm/dispatchinfo.h index dfe3d0facc411c..59566bb83a0ddc 100644 --- a/src/coreclr/vm/dispatchinfo.h +++ b/src/coreclr/vm/dispatchinfo.h @@ -52,18 +52,47 @@ enum CultureAwareStates }; // This structure represents a dispatch member. -struct DispatchMemberInfo +class DispatchMemberInfo { - DispatchMemberInfo(DispatchInfo *pDispInfo, DISPID DispID, SString& strName); +private: // static + static MethodTable* s_pMemberTypes[NUM_MEMBER_TYPES]; + static EnumMemberTypes s_memberTypes[NUM_MEMBER_TYPES]; + static int s_iNumMemberTypesKnown; + +public: + DispatchMemberInfo(DispatchInfo* pDispInfo, DISPID DispID, SString& strName); ~DispatchMemberInfo(); // Helper method to ensure the entry is initialized. void EnsureInitialized(); - BOOL IsNeutered() + DISPID GetDISPID() + { + LIMITED_METHOD_CONTRACT; + return m_DispID; + } + + DispatchMemberInfo* GetNext() + { + LIMITED_METHOD_CONTRACT; + return m_pNext; + } + + DispatchMemberInfo** GetNextField() + { + LIMITED_METHOD_CONTRACT; + return &m_pNext; + } + + SString& GetName() { LIMITED_METHOD_CONTRACT; + return m_strName; + } + BOOL IsNeutered() + { + LIMITED_METHOD_CONTRACT; return (m_bNeutered) ? TRUE : FALSE; } @@ -172,7 +201,7 @@ struct DispatchMemberInfo void SetUpMethodMarshalerInfo(MethodDesc *pMeth, BOOL bReturnValueOnly); void SetUpDispParamMarshalerForMarshalInfo(int iParam, MarshalInfo *pInfo); void SetUpDispParamAttributes(int iParam, MarshalInfo* Info); -public: + DISPID m_DispID; LOADERHANDLE m_hndMemberInfo; DispParamMarshaler** m_apParamMarshaler; @@ -187,14 +216,8 @@ struct DispatchMemberInfo BOOL m_bNeutered; DispatchInfo* m_pDispInfo; BOOL m_bLastParamOleVarArg; - -private: - static MethodTable* s_pMemberTypes[NUM_MEMBER_TYPES]; - static EnumMemberTypes s_memberTypes[NUM_MEMBER_TYPES]; - static int s_iNumMemberTypesKnown; }; - struct InvokeObjects { PTRARRAYREF ParamArray; @@ -293,10 +316,10 @@ class DispatchInfo // Returns TRUE if the argument is "Missing" static BOOL VariantIsMissing(VARIANT *pOle); - LoaderAllocator* GetLoaderAllocator() - { - return m_pMT->GetLoaderAllocator(); - } + // Functions for loader handle manipulation. + LOADERHANDLE AllocateHandle(OBJECTREF objRef); + void FreeHandle(LOADERHANDLE handle); + OBJECTREF GetHandleValue(LOADERHANDLE handle); protected: // Parameter marshaling helpers. @@ -332,7 +355,7 @@ class DispatchInfo DISPID GenerateDispID(); // Helper method to create an instance of a DispatchMemberInfo. - virtual DispatchMemberInfo* CreateDispatchMemberInfoInstance(DISPID DispID, SString& strMemberName, OBJECTREF MemberInfoObj); + DispatchMemberInfo* CreateDispatchMemberInfoInstance(DISPID dispID, SString& strMemberName, OBJECTREF memberInfoObj); // Helper function to fill in an EXCEPINFO for an InvocationException. static void GetExcepInfoForInvocationExcep(OBJECTREF objException, EXCEPINFO *pei); @@ -372,9 +395,6 @@ class DispatchExInfo : public DispatchInfo // with the managed view if they fail to find the method. HRESULT SynchInvokeMember(SimpleComCallWrapper *pSimpleWrap, DISPID id, LCID lcid, WORD wFlags, DISPPARAMS *pdp, VARIANT *pVarRes, EXCEPINFO *pei, IServiceProvider *pspCaller, unsigned int *puArgErr); - // Helper method to create an instance of a DispatchMemberInfo. - virtual DispatchMemberInfo* CreateDispatchMemberInfoInstance(DISPID DispID, SString& strMemberName, OBJECTREF MemberInfoObj); - // These methods return the first and next non deleted members. DispatchMemberInfo* GetFirstMember(); DispatchMemberInfo* GetNextMember(DISPID CurrMemberDispID); diff --git a/src/coreclr/vm/loaderallocator.hpp b/src/coreclr/vm/loaderallocator.hpp index b943ea37ad4ddf..9b6f17f0533fa6 100644 --- a/src/coreclr/vm/loaderallocator.hpp +++ b/src/coreclr/vm/loaderallocator.hpp @@ -358,7 +358,7 @@ class LoaderAllocator // - Other LoaderAllocator can have this LoaderAllocator in its reference list // (code:m_LoaderAllocatorReferences), but without call to code:AddRef. // - LoaderAllocator cannot ever go back to phase #1 or #2, but it can skip this phase if there are - // not any LCG method references keeping it alive at the time of manged scout finalization. + // no LCG method references keeping it alive at the time of managed scout finalization. // Detection: // code:IsAlive ... TRUE // code:IsManagedScoutAlive ... FALSE (change from phase #2) diff --git a/src/coreclr/vm/stdinterfaces.cpp b/src/coreclr/vm/stdinterfaces.cpp index cffce984a04a3c..08af895c3baec9 100644 --- a/src/coreclr/vm/stdinterfaces.cpp +++ b/src/coreclr/vm/stdinterfaces.cpp @@ -1266,7 +1266,7 @@ InternalDispatchImpl_GetIDsOfNames ( if (pDispMemberInfo) { // Get the DISPID of the member. - rgdispid[0] = pDispMemberInfo->m_DispID; + rgdispid[0] = pDispMemberInfo->GetDISPID(); // Get the ID's of the named arguments. if (cNames > 1) @@ -1487,7 +1487,7 @@ HRESULT __stdcall DispatchEx_GetIDsOfNames ( if (pDispMemberInfo) { // Get the DISPID of the member. - rgdispid[0] = pDispMemberInfo->m_DispID; + rgdispid[0] = pDispMemberInfo->GetDISPID(); // Get the ID's of the named arguments. if (cNames > 1) @@ -1661,7 +1661,7 @@ HRESULT __stdcall DispatchEx_GetDispID ( // Set the return DISPID if the member has been found. if (pDispMemberInfo) - *pid = pDispMemberInfo->m_DispID; + *pid = pDispMemberInfo->GetDISPID(); } END_EXTERNAL_ENTRYPOINT; @@ -1714,7 +1714,7 @@ HRESULT __stdcall DispatchEx_GetMemberName ( else { // Copy the name into the output string. - *pbstrName = SysAllocString(pDispMemberInfo->m_strName); + *pbstrName = SysAllocString(pDispMemberInfo->GetName().GetUnicode()); } } END_EXTERNAL_ENTRYPOINT; @@ -1936,7 +1936,7 @@ HRESULT __stdcall DispatchEx_GetNextDispID ( // If we have found a member that has not been deleted then return its DISPID. if (pNextMember) { - *pid = pNextMember->m_DispID; + *pid = pNextMember->GetDISPID(); hr = S_OK; } else diff --git a/src/tests/Interop/PInvoke/Delegate/DelegateTest.csproj b/src/tests/Interop/PInvoke/Delegate/DelegateTest.csproj index d722c4eba90493..a99f25c8f8c6aa 100644 --- a/src/tests/Interop/PInvoke/Delegate/DelegateTest.csproj +++ b/src/tests/Interop/PInvoke/Delegate/DelegateTest.csproj @@ -1,8 +1,6 @@ Exe - - true diff --git a/src/tests/Interop/PInvoke/Variant/VariantTestComWrappers.csproj b/src/tests/Interop/PInvoke/Variant/VariantTestComWrappers.csproj index 75dea8cd15799c..278d46b8cb58b4 100644 --- a/src/tests/Interop/PInvoke/Variant/VariantTestComWrappers.csproj +++ b/src/tests/Interop/PInvoke/Variant/VariantTestComWrappers.csproj @@ -2,7 +2,7 @@ Exe true - + true true From a102e4d2d5a11bd8a03caf57b8ce02299138321b Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 5 Jan 2023 08:46:11 -0800 Subject: [PATCH 2/2] Feedback --- src/coreclr/vm/dispatchinfo.cpp | 10 +++++----- src/coreclr/vm/dispatchinfo.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/dispatchinfo.cpp b/src/coreclr/vm/dispatchinfo.cpp index d1031f3fada861..3e9e5866abbcee 100644 --- a/src/coreclr/vm/dispatchinfo.cpp +++ b/src/coreclr/vm/dispatchinfo.cpp @@ -2732,7 +2732,7 @@ BOOL DispatchInfo::SynchWithManagedView() // Go through the list of member info's and find the end. DispatchMemberInfo **ppNextMember = &m_pFirstMemberInfo; while (*ppNextMember) - ppNextMember = (*ppNextMember)->GetNextField(); + ppNextMember = (*ppNextMember)->GetNextPtr(); // Retrieve the member info map. pMemberMap = GetMemberInfoMap(); @@ -2848,7 +2848,7 @@ BOOL DispatchInfo::SynchWithManagedView() *ppNextMember = pMemberToAdd; // Update ppNextMember to be ready for the next new member. - ppNextMember = (*ppNextMember)->GetNextField(); + ppNextMember = (*ppNextMember)->GetNextPtr(); // Add the member to the map. Note, the hash is unsynchronized, but we already have our lock // so we're okay. @@ -3339,7 +3339,7 @@ DispatchMemberInfo* DispatchExInfo::GetFirstMember() // Now we need to make sure we skip any members that are deleted. while ((*ppNextMemberInfo) && !(*ppNextMemberInfo)->GetMemberInfoObject()) - ppNextMemberInfo = (*ppNextMemberInfo)->GetNextField(); + ppNextMemberInfo = (*ppNextMemberInfo)->GetNextPtr(); RETURN *ppNextMemberInfo; } @@ -3361,7 +3361,7 @@ DispatchMemberInfo* DispatchExInfo::GetNextMember(DISPID CurrMemberDispID) RETURN NULL; // Start from the next member. - DispatchMemberInfo **ppNextMemberInfo = pDispMemberInfo->GetNextField(); + DispatchMemberInfo **ppNextMemberInfo = pDispMemberInfo->GetNextPtr(); // If the next member is not set we need to sink up with the expando object // itself to make sure that this member is really the last member and that @@ -3379,7 +3379,7 @@ DispatchMemberInfo* DispatchExInfo::GetNextMember(DISPID CurrMemberDispID) // Now we need to make sure we skip any members that are deleted. while ((*ppNextMemberInfo) && !(*ppNextMemberInfo)->GetMemberInfoObject()) - ppNextMemberInfo = (*ppNextMemberInfo)->GetNextField(); + ppNextMemberInfo = (*ppNextMemberInfo)->GetNextPtr(); RETURN *ppNextMemberInfo; } diff --git a/src/coreclr/vm/dispatchinfo.h b/src/coreclr/vm/dispatchinfo.h index 59566bb83a0ddc..59485847eab997 100644 --- a/src/coreclr/vm/dispatchinfo.h +++ b/src/coreclr/vm/dispatchinfo.h @@ -78,7 +78,7 @@ class DispatchMemberInfo return m_pNext; } - DispatchMemberInfo** GetNextField() + DispatchMemberInfo** GetNextPtr() { LIMITED_METHOD_CONTRACT; return &m_pNext;