Revert previous commit, use something more reliable

The previous fix would only fix the leaks of specific handles that were
being leaked. This one should work on any handles allocated with its
LoaderAllocator.

src/vm/appdomain.cpp

@@ -1345,32 +1345,6 @@ OBJECTREF* BaseDomain::AllocateObjRefPtrsInLargeTable(int nRequested, OBJECTREF*
         return result;
     }
 }
-
-#ifdef FEATURE_COLLECTIBLE_ALC
-void BaseDomain::ReleaseObjRefPtrsInLargeTable(OBJECTREF *pObjRef, DWORD nReleased)
-{
-    CONTRACTL
-    {
-        NOTHROW;
-        GC_TRIGGERS;
-        MODE_ANY;
-        PRECONDITION((nReleased > 0));
-        INJECT_FAULT(COMPlusThrowOM(););
-    }
-    CONTRACTL_END;
-
-    {
-        CrstHolder ch(&m_LargeHeapHandleTableCrst);
-        GCX_COOP();
-
-        if (!m_pLargeHeapHandleTable)
-            _ASSERTE(!"Unreachable");
-
-        m_pLargeHeapHandleTable->ReleaseHandles(pObjRef, nReleased);
-    }
-}
-#endif // FEATURE_COLLECTIBLE_ALC
-
 #endif // CROSSGEN_COMPILE
 
 #endif // !DACCESS_COMPILE
@@ -6814,11 +6788,60 @@ DomainAssembly *AppDomain::LoadDomainAssemblyInternal(AssemblySpec* pIdentity,
 
     if (result == NULL)
     {
+#if defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
+        BOOL fIsCollectible = FALSE;
+        SafeComHolder<IAssemblyLoadContext> pAssemblyLoadContext = NULL;
+
+        ICLRPrivBinder *pFileBinder = pFile->GetBindingContext();
+        if (pFileBinder != NULL)
+        {
+            ICLRPrivBinder *pBinder = reinterpret_cast<BINDER_SPACE::Assembly *>(pFileBinder)->GetBinder();
+
+            // Assemblies loaded with AssemblyLoadContext need to use a different LoaderAllocator if
+            // marked as collectible
+            if (SUCCEEDED(pBinder->QueryInterface<IAssemblyLoadContext>(&pAssemblyLoadContext)))
+            {
+                pAssemblyLoadContext->GetIsCollectible(&fIsCollectible);
+            }
+        }
+
+        AssemblyLoaderAllocator *pLoaderAllocatorOverride = NULL;
+
+        if (fIsCollectible && pAssemblyLoadContext != NULL)
+        {
+            GCX_COOP();
+
+            LOADERALLOCATORREF pManagedLoaderAllocator = NULL;
+            GCPROTECT_BEGIN(pManagedLoaderAllocator);
+
+            {
+                GCX_PREEMP();
+
+                pLoaderAllocatorOverride = new AssemblyLoaderAllocator();
+
+                // Some of the initialization functions are not virtual. Call through the derived class
+                // to prevent calling the base class version.
+                pLoaderAllocatorOverride->Init(this);
+
+                // Setup the managed proxy now, but do not actually transfer ownership to it.
+                // Once everything is setup and nothing can fail anymore, the ownership will be
+                // atomically transfered by call to LoaderAllocator::ActivateManagedTracking().
+                pLoaderAllocatorOverride->SetupManagedTracking(&pManagedLoaderAllocator);
+            }
+
+            GCPROTECT_END();
+        }
+
+        LoaderAllocator *pLoaderAllocator = pLoaderAllocatorOverride == NULL ? this->GetLoaderAllocator() : pLoaderAllocatorOverride;
+#else // defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
+        LoaderAllocator *pLoaderAllocator = this->GetLoaderAllocator();
+#endif
+
         // Allocate the DomainAssembly a bit early to avoid GC mode problems. We could potentially avoid
         // a rare redundant allocation by moving this closer to FileLoadLock::Create, but it's not worth it.
 
         NewHolder<DomainAssembly> pDomainAssembly;
-        pDomainAssembly = new DomainAssembly(this, pFile, pLoadSecurity, this->GetLoaderAllocator());
+        pDomainAssembly = new DomainAssembly(this, pFile, pLoadSecurity, pLoaderAllocator);
 
         LoadLockHolder lock(this);
 

src/vm/appdomain.hpp

@@ -1260,10 +1260,6 @@ class BaseDomain
     // will be properly serialized)
     OBJECTREF *AllocateObjRefPtrsInLargeTable(int nRequested, OBJECTREF** ppLazyAllocate = NULL, BOOL bCrossAD = FALSE);
 
-#ifdef FEATURE_COLLECTIBLE_ALC
-    void ReleaseObjRefPtrsInLargeTable(OBJECTREF *pObjRef, DWORD nReleased);
-#endif // FEATURE_COLLECTIBLE_ALC
-
 #ifdef FEATURE_PREJIT
     // Ensures that the file for logging profile data is open (we only open it once)
     // return false on failure

src/vm/assembly.cpp

@@ -575,57 +575,25 @@ Assembly * Assembly::Create(
 
 #if defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
     SafeComHolder<IAssemblyLoadContext> pAssemblyLoadContext = NULL;
+    BOOL fCollectibleALC = FALSE;
 
     ICLRPrivBinder *pFileBinder = pFile->GetBindingContext();
     if (pFileBinder != NULL)
     {
         ICLRPrivBinder *pBinder = reinterpret_cast<BINDER_SPACE::Assembly *>(pFileBinder)->GetBinder();
-
-        // Assemblies loaded with AssemblyLoadContext need to use a different LoaderAllocator if
-        // marked as collectible
-        if (SUCCEEDED(pBinder->QueryInterface<IAssemblyLoadContext>(&pAssemblyLoadContext)))
-        {
-            BOOL isCollectible;
-            pAssemblyLoadContext->GetIsCollectible(&isCollectible);
-
-            if (isCollectible)
-            {
-                _ASSERTE(!fIsCollectible && pLoaderAllocator == NULL);
-                fIsCollectible = TRUE;
-            }
-        }
-    }
-
-    NewHolder<LoaderAllocator> pNewLoaderAllocator;
-    AssemblyLoaderAllocator *pNewAssemblyLoaderAllocator;
-
-    if (fIsCollectible && pAssemblyLoadContext != NULL)
-    {
-        GCX_COOP();
-
-        // TODO: This should probably be stored in the managed AssemblyLoadContext.
-        LOADERALLOCATORREF pManagedLoaderAllocator = NULL;
-        GCPROTECT_BEGIN(pManagedLoaderAllocator);
 
+        if (SUCCEEDED(pBinder->QueryInterface<IAssemblyLoadContext>(&pAssemblyLoadContext)))
         {
-            GCX_PREEMP();
+            pAssemblyLoadContext->GetIsCollectible(&fCollectibleALC);
 
-            pNewAssemblyLoaderAllocator = new AssemblyLoaderAllocator();
-            pNewLoaderAllocator = pNewAssemblyLoaderAllocator;
+            _ASSERTE(!fIsCollectible && pLoaderAllocator == NULL);
 
-            // Some of the initialization functions are not virtual. Call through the derived class
-            // to prevent calling the base class version.
-            pNewAssemblyLoaderAllocator->Init(reinterpret_cast<AppDomain *>(pDomain));
+            fIsCollectible = fCollectibleALC;
+            pLoaderAllocator = pDomainAssembly->GetLoaderAllocator();
 
-            // Setup the managed proxy now, but do not actually transfer ownership to it.
-            // Once everything is setup and nothing can fail anymore, the ownership will be
-            // atomically transfered by call to LoaderAllocator::ActivateManagedTracking().
-            pNewAssemblyLoaderAllocator->SetupManagedTracking(&pManagedLoaderAllocator);
+            if (fCollectibleALC)
+                _ASSERTE(pLoaderAllocator->IsCollectible());
         }
-
-        GCPROTECT_END();
-
-        pLoaderAllocator = pNewLoaderAllocator;
     }
 #endif // defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
 
@@ -665,14 +633,15 @@ Assembly * Assembly::Create(
     END_INTERIOR_STACK_PROBE;
 
 #if defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
-    if (fIsCollectible && pAssemblyLoadContext != NULL)
+    if (fCollectibleALC)
     {
-        pNewAssemblyLoaderAllocator->SetDomainAssembly(pDomainAssembly);
+        AssemblyLoaderAllocator *pAssemblyLoaderAllocator = static_cast<AssemblyLoaderAllocator *>(pLoaderAllocator);
+
+        pAssemblyLoaderAllocator->SetDomainAssembly(pDomainAssembly);
 
-        pNewLoaderAllocator->ActivateManagedTracking();
-        pNewLoaderAllocator.SuppressRelease();
+        pAssemblyLoaderAllocator->ActivateManagedTracking();
 
-        VERIFY(SUCCEEDED(pAssemblyLoadContext->ReferenceLoaderAllocator(pNewLoaderAllocator)));
+        VERIFY(SUCCEEDED(pAssemblyLoadContext->ReferenceLoaderAllocator(pLoaderAllocator)));
     }
 #endif // defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
 

src/vm/domainfile.cpp

@@ -1688,6 +1688,9 @@ DomainAssembly::DomainAssembly(AppDomain *pDomain, PEFile *pFile, AssemblyLoadSe
     m_MissingDependenciesCheckStatus(CMD_Unknown),
     m_fSkipPolicyResolution(pLoadSecurity != NULL && !pLoadSecurity->ShouldResolvePolicy()),
     m_fDebuggerUnloadStarted(FALSE),
+#ifdef FEATURE_COLLECTIBLE_ALC
+    m_pLoaderAllocator(pLoaderAllocator),
+#endif // FEATURE_COLLECTIBLE_ALC
     m_fCollectible(pLoaderAllocator->IsCollectible()),
     m_fHostAssemblyPublished(false),
     m_fCalculatedShouldLoadDomainNeutral(false),

src/vm/domainfile.h

@@ -759,6 +759,10 @@ class DomainAssembly : public DomainFile
     void NotifyDebuggerUnload();
     BOOL IsUnloading();
 
+#ifdef FEATURE_COLLECTIBLE_ALC
+    inline PTR_LoaderAllocator GetLoaderAllocator();
+#endif // FEATURE_COLLECTIBLE_ALC
+
     inline BOOL IsCollectible();
     // 
     //  GC API
@@ -841,6 +845,9 @@ class DomainAssembly : public DomainFile
     Volatile<bool>                          m_fHostAssemblyPublished;
     Volatile<bool>                          m_fCalculatedShouldLoadDomainNeutral;
     Volatile<bool>                          m_fShouldLoadDomainNeutral;
+#ifdef FEATURE_COLLECTIBLE_ALC
+    PTR_LoaderAllocator                     m_pLoaderAllocator;
+#endif // FEATURE_COLLECTIBLE_ALC
 
   public:
     // Indicates if the assembly can be cached in a binding cache such as AssemblySpecBindingCache.

src/vm/domainfile.inl

@@ -129,6 +129,14 @@ inline ULONG DomainAssembly::HashIdentity()
     return GetFile()->HashIdentity();
 }
 
+#ifdef FEATURE_COLLECTIBLE_ALC
+inline PTR_LoaderAllocator DomainAssembly::GetLoaderAllocator()
+{
+    LIMITED_METHOD_CONTRACT;
+    return m_pLoaderAllocator;
+}
+#endif // FEATURE_COLLECTIBLE_ALC
+
 inline BOOL DomainAssembly::IsCollectible()
 {
     LIMITED_METHOD_CONTRACT;

src/vm/securitydescriptor.cpp

@@ -476,20 +476,4 @@ BOOL PEFileSecurityDescriptor::AllowBindingRedirects()
 
 #endif // FEATURE_CORECLR
 
-#if defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
-SecurityDescriptor::~SecurityDescriptor()
-{
-    // When the AppDomain's LoaderAllocator is not collectible, its LargeHeapHandleTable will
-    // be used for the permission handles
-    if (!m_pLoaderAllocator->IsCollectible())
-    {
-        if (m_hGrantedPermissionSet != NULL)
-            m_pAppDomain->ReleaseObjRefPtrsInLargeTable((OBJECTREF*)((UINT_PTR)m_hGrantedPermissionSet - 1), 1);
-
-        if (m_hGrantDeniedPermissionSet != NULL)
-            m_pAppDomain->ReleaseObjRefPtrsInLargeTable((OBJECTREF*)((UINT_PTR)m_hGrantDeniedPermissionSet - 1), 1);
-    }
-}
-#endif // defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
-
 #endif // !DACCESS_COMPILE

src/vm/securitydescriptor.h

@@ -137,11 +137,6 @@ class SecurityDescriptor
 #ifdef FEATURE_PAL
     SecurityDescriptor() {}
 #endif // FEATURE_PAL
-
-#if defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
-    ~SecurityDescriptor();
-#endif // defined(FEATURE_COLLECTIBLE_ALC) && !defined(CROSSGEN_COMPILE)
-
 #endif // !DACCESS_COMPILE
 };