Merging internal commits for release/8.0

NuGet.config

@@ -5,8 +5,10 @@
     <clear />
     <!--Begin: Package sources managed by Dependency Flow automation. Do not edit the sources below.-->
     <!--  Begin: Package sources from dotnet-runtime -->
+    <add key="darc-int-dotnet-runtime-2d7eea2" value="https://pkgs.dev.azure.com/dnceng/internal/_packaging/darc-int-dotnet-runtime-2d7eea25/nuget/v3/index.json" />
     <!--  End: Package sources from dotnet-runtime -->
     <!--  Begin: Package sources from dotnet-winforms -->
+    <add key="darc-int-dotnet-winforms-41a4bd6" value="https://pkgs.dev.azure.com/dnceng/internal/_packaging/darc-int-dotnet-winforms-41a4bd69/nuget/v3/index.json" />
     <!--  End: Package sources from dotnet-winforms -->
     <!--End: Package sources managed by Dependency Flow automation. Do not edit the sources above.-->
     <add key="dotnet-eng" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json" />
@@ -20,8 +22,10 @@
   <disabledPackageSources>
     <!--Begin: Package sources managed by Dependency Flow automation. Do not edit the sources below.-->
     <!--  Begin: Package sources from dotnet-runtime -->
+    <add key="darc-int-dotnet-runtime-2d7eea2" value="true" />
     <!--  End: Package sources from dotnet-runtime -->
     <!--  Begin: Package sources from dotnet-winforms -->
+    <add key="darc-int-dotnet-winforms-41a4bd6" value="true" />
     <!--  End: Package sources from dotnet-winforms -->
     <!--End: Package sources managed by Dependency Flow automation. Do not edit the sources above.-->
   </disabledPackageSources>

eng/Version.Details.xml

@@ -1,13 +1,13 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Dependencies>
   <ProductDependencies>
-    <Dependency Name="Microsoft.Private.Winforms" Version="8.0.3-servicing.24116.3">
+    <Dependency Name="Microsoft.Private.Winforms" Version="8.0.4-servicing.24169.11">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-winforms</Uri>
-      <Sha>bd280bbb5c9699bb93097206f076ad2f330ea8e1</Sha>
+      <Sha>41a4bd690229661e3ec74276ce3f93863b22435b</Sha>
     </Dependency>
-    <Dependency Name="Microsoft.Dotnet.WinForms.ProjectTemplates" Version="8.0.3-servicing.24116.3">
+    <Dependency Name="Microsoft.Dotnet.WinForms.ProjectTemplates" Version="8.0.4-servicing.24169.11">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-winforms</Uri>
-      <Sha>bd280bbb5c9699bb93097206f076ad2f330ea8e1</Sha>
+      <Sha>41a4bd690229661e3ec74276ce3f93863b22435b</Sha>
     </Dependency>
     <Dependency Name="System.Reflection.MetadataLoadContext" Version="8.0.0" CoherentParentDependency="Microsoft.Private.Winforms">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
@@ -17,9 +17,9 @@
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
       <Sha>5535e31a712343a63f5d7d796cd874e563e5ac14</Sha>
     </Dependency>
-    <Dependency Name="Microsoft.NETCore.Platforms" Version="8.0.3-servicing.24114.23" CoherentParentDependency="Microsoft.Private.Winforms">
+    <Dependency Name="Microsoft.NETCore.Platforms" Version="8.0.4-servicing.24169.9" CoherentParentDependency="Microsoft.Private.Winforms">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
-      <Sha>9f4b1f5d664afdfc80e1508ab7ed099dff210fbd</Sha>
+      <Sha>2d7eea252964e69be94cb9c847b371b23e4dd470</Sha>
     </Dependency>
     <Dependency Name="Microsoft.DotNet.Wpf.DncEng" Version="8.0.0-rtm.24178.1">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-wpf-int</Uri>
@@ -29,17 +29,17 @@
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
       <Sha>5535e31a712343a63f5d7d796cd874e563e5ac14</Sha>
     </Dependency>
-    <Dependency Name="Microsoft.NETCore.App.Ref" Version="8.0.3" CoherentParentDependency="Microsoft.Private.Winforms">
+    <Dependency Name="Microsoft.NETCore.App.Ref" Version="8.0.4" CoherentParentDependency="Microsoft.Private.Winforms">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
-      <Sha>9f4b1f5d664afdfc80e1508ab7ed099dff210fbd</Sha>
+      <Sha>2d7eea252964e69be94cb9c847b371b23e4dd470</Sha>
     </Dependency>
-    <Dependency Name="Microsoft.NETCore.App.Runtime.win-x64" Version="8.0.3" CoherentParentDependency="Microsoft.Private.Winforms">
+    <Dependency Name="Microsoft.NETCore.App.Runtime.win-x64" Version="8.0.4" CoherentParentDependency="Microsoft.Private.Winforms">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
-      <Sha>9f4b1f5d664afdfc80e1508ab7ed099dff210fbd</Sha>
+      <Sha>2d7eea252964e69be94cb9c847b371b23e4dd470</Sha>
     </Dependency>
-    <Dependency Name="VS.Redist.Common.NetCore.SharedFramework.x64.8.0" Version="8.0.3-servicing.24114.23" CoherentParentDependency="Microsoft.Private.Winforms">
+    <Dependency Name="VS.Redist.Common.NetCore.SharedFramework.x64.8.0" Version="8.0.4-servicing.24169.9" CoherentParentDependency="Microsoft.Private.Winforms">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
-      <Sha>9f4b1f5d664afdfc80e1508ab7ed099dff210fbd</Sha>
+      <Sha>2d7eea252964e69be94cb9c847b371b23e4dd470</Sha>
     </Dependency>
     <!-- These dependencies are required by windowsdesktop for coherency. -->
     <Dependency Name="Microsoft.Win32.Registry.AccessControl" Version="8.0.0" CoherentParentDependency="Microsoft.Private.Winforms">
@@ -150,9 +150,9 @@
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
       <Sha>5535e31a712343a63f5d7d796cd874e563e5ac14</Sha>
     </Dependency>
-    <Dependency Name="System.Drawing.Common" Version="8.0.3">
+    <Dependency Name="System.Drawing.Common" Version="8.0.4">
       <Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-winforms</Uri>
-      <Sha>bd280bbb5c9699bb93097206f076ad2f330ea8e1</Sha>
+      <Sha>41a4bd690229661e3ec74276ce3f93863b22435b</Sha>
     </Dependency>
   </ProductDependencies>
   <ToolsetDependencies>

eng/Versions.props

@@ -22,18 +22,18 @@
   </PropertyGroup>
   <!-- Packages that come from https://github.com/dotnet/winforms -->
   <PropertyGroup>
-    <MicrosoftPrivateWinformsVersion>8.0.3-servicing.24116.3</MicrosoftPrivateWinformsVersion>
+    <MicrosoftPrivateWinformsVersion>8.0.4-servicing.24169.11</MicrosoftPrivateWinformsVersion>
   </PropertyGroup>
   <!-- Packages that come from https://github.com/dotnet/runtime -->
   <PropertyGroup>
-    <VSRedistCommonNetCoreSharedFrameworkx6480PackageVersion>8.0.3-servicing.24114.23</VSRedistCommonNetCoreSharedFrameworkx6480PackageVersion>
-    <MicrosoftNETCoreAppRefVersion>8.0.3</MicrosoftNETCoreAppRefVersion>
-    <MicrosoftNETCoreAppRuntimewinx64Version>8.0.3</MicrosoftNETCoreAppRuntimewinx64Version>
-    <MicrosoftNETCorePlatformsVersion>8.0.3-servicing.24114.23</MicrosoftNETCorePlatformsVersion>
+    <VSRedistCommonNetCoreSharedFrameworkx6480PackageVersion>8.0.4-servicing.24169.9</VSRedistCommonNetCoreSharedFrameworkx6480PackageVersion>
+    <MicrosoftNETCoreAppRefVersion>8.0.4</MicrosoftNETCoreAppRefVersion>
+    <MicrosoftNETCoreAppRuntimewinx64Version>8.0.4</MicrosoftNETCoreAppRuntimewinx64Version>
+    <MicrosoftNETCorePlatformsVersion>8.0.4-servicing.24169.9</MicrosoftNETCorePlatformsVersion>
     <SystemCodeDomPackageVersion>8.0.0</SystemCodeDomPackageVersion>
     <SystemConfigurationConfigurationManagerPackageVersion>8.0.0</SystemConfigurationConfigurationManagerPackageVersion>
     <SystemDiagnosticsEventLogPackageVersion>8.0.0</SystemDiagnosticsEventLogPackageVersion>
-    <SystemDrawingCommonVersion>8.0.3</SystemDrawingCommonVersion>
+    <SystemDrawingCommonVersion>8.0.4</SystemDrawingCommonVersion>
     <SystemDirectoryServicesVersion>8.0.0</SystemDirectoryServicesVersion>
     <SystemIOPackagingVersion>8.0.0</SystemIOPackagingVersion>
     <SystemReflectionMetadataLoadContextVersion>8.0.0</SystemReflectionMetadataLoadContextVersion>

src/Microsoft.DotNet.Wpf/src/PenImc/dll/ComLockableWrapper.hpp

@@ -40,6 +40,11 @@ namespace ComUtils
         // The apartment is verified during this call.
         HRESULT Unlock();
 
+        // Unlocking a wrapper permanently nulls out the server object pointer, so a
+        // wrapper contains a non-null server object pointer if and only if it is
+        // bound to a server object which has never been unlocked.
+        bool HasNotBeenUnlocked() { return (m_serverObject != nullptr); }
+
     private:
 
         IUnknown *m_serverObject;

src/Microsoft.DotNet.Wpf/src/PenImc/dll/PimcManager.cpp

@@ -396,6 +396,94 @@ void CPimcManager::TerminateHookThread(__inout CHookThreadItem * pThread)
 void CPimcManager::FinalRelease() 
 {
     m_wispManagerLock.RevokeIfValid();
+
+    //
+    // The CComObject<CPimcManager> destructor is the only function which calls into this
+    // FinalRelease code.
+    //
+    // In all successful usage of CPimcManager: 1) Managed WPF code uses CoCreateInstance
+    // to acquire an IPimcManager2 interface to a brand-new CPimcManager instance (created by
+    // the ATL CComCreator<T>::CreateInstance machinery), meaning FinalConstruct by-definition
+    // completes successfully, meaning "m_managerLock" is therefore guaranteed to be locked;
+    // 2) Managed WPF code then runs through its full end-to-end usage of the CPimcManager
+    // object (generally managed by the code in PenThreadWorker.cs); 3) When/if the managed WPF
+    // code determines that the CPimcManager object is no longer needed, it sends a
+    // RELEASE_MANAGER_EXT message (see UnsafeNativeMethods.ReleaseManagerExternalLock()) which
+    // unlocks "m_managerLock"; 4) Now that it is unlocked, the CComObject<CPimcManager> object
+    // can be destroyed when/if its refcount drops to zero, and this FinalRelease function will
+    // run at that time.
+    //
+    // So in all successful usage cases, it is guaranteed that "m_managerLock" is already
+    // unlocked when this code runs (because if it was still locked, the lock itself would have
+    // prevented the refcount from reaching zero, and would have prevented this function from
+    // ever running).
+    //
+    // That said, in unsuccessful usage cases, the ATL CComCreator<T>::CreateInstance machinery
+    // can fail, meaning it will destroy the brand-new CPimcManager instance before returning
+    // an error back to the CreateInstance caller.  Destroying the brand-new instance triggers
+    // the CComObject<CPimcManager> destructor and therefore calls into this function during
+    // the CComCreator<T>::CreateInstance operation itself.
+    //
+    // The final step in CComCreator<T>::CreateInstance is a QI which queries the newly-created
+    // object for whatever interface has been requested by the caller.  This operation is the
+    // main way that CComCreator<T>::CreateInstance can fail.  For example, this QI is
+    // guaranteed to fail whenever the CoCreateInstance caller targets the CPimcManager CLSID
+    // but passes in a "random" IID that has nothing to do with IPimcManager2 or anything else
+    // that CPimcManager implements.
+    //
+    // (In CPimcManager construction, outside of pathological cases (e.g., where a small heap
+    // allocation in OS code fails due to out-of-memory), there are no other known ways that
+    // the CComCreator<T>::CreateInstance sequence can fail; so the QI failure is the only
+    // failure mode that is known to be of general interest.)
+    //
+    // The QI failure can only occur after the preceding FinalConstruct call has completed
+    // successfully (since any FinalConstruct failure would have caused
+    // CComCreator<T>::CreateInstance to abort without ever trying the QI); since
+    // CPimcManager::FinalConstruct always locks the "m_managerLock", this implies that the
+    // "m_managerLock" is guaranteed to be locked when this code runs (which is exactly
+    // opposite to what happens in all successful usage cases as discussed above).
+    //
+    // In this case, it is crucial to unlock "m_managerLock" before allowing this CPimcManager
+    // object to be destroyed.  Without the unlock, this CPimcManager object would be destroyed
+    // while the associated CStdIdentity in the OS code still holds a reference to it; during
+    // any future apartment unload, the OS code would release this reference, and the release
+    // would be a use-after-free at that point.
+    //
+    // Note that the crucial unlock causes overactive ATL debug asserts to fire if a chk build
+    // of this DLL is used; specifically:
+    //
+    //    - The code in the CComObject<CPimcManager> destructor always stomps the refcount to
+    //      0xc0000001 (i.e., "-(LONG_MAX/2)"), meaning this CPimcManager object's refcount is
+    //      always 0xc0000001 when this code runs; unlocking "m_managerLock" will cause the refcount
+    //      to drop by one (because, as discussed above, the crucial operation which prevents
+    //      use-after-free problems will release the associated CStdIdentity's reference to this
+    //      CPimcManager object, and in this way releases the reference that was added when
+    //      "managerLock" was locked during FinalConstruct); as a result, unlocking "m_managerLock"
+    //      will move this CPimcManager object's refcount through a "0xc0000001 -> 0xc0000000"
+    //      transition.
+    //
+    //    - Both of the CComObjectRootEx<T>::InternalRelease specializations contain debug asserts
+    //      which will fire whenever the refcount drops below 0xc0000001, so this transition always
+    //      triggers a debug assert when using a chk build of this DLL.
+    //
+    //    - That said, all evidence strongly suggests that this is just an overactive assert in
+    //      the ATL code (probably just indicating that it is rare for FinalConstruct to add
+    //      "self-references" like it does for CPimcManager (since these self-references generally
+    //      prevent the server object from being destroyed unless a manual action like the
+    //      RELEASE_MANAGER_EXT message is taken later on), meaning it is rare to have a situation
+    //      where FinalRelease needs to release self-references that were acquired in
+    //      FinalConstruct, meaning this is a rare enough case that the ATL authors either didn't
+    //      test it or didn't think it was common enough to warrant adjusting the assert).
+    //
+    // Since this change is being made in servicing, attempt to change behavior as little as
+    // possible in the "successful usage" cases where "m_managerLock" is already unlocked,
+    // while still ensuring that FinalRelease will always run the crucial unlock in all
+    // "unsuccessful usage" cases.
+    //
+    if (m_managerLock.HasNotBeenUnlocked())
+    {
+        m_managerLock.Unlock();
+    }
 }
 
 /////////////////////////////////////////////////////////////////////////////