First stab at ComWrappers usage in the WPF #6606
Conversation
This is minimal change which replace usage built-in COM for IThThreadMgr instance. Having ComWrappers is important to make dotnet#3811 works.
ghost
assigned 
ghost
requested review from
src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/Threading/Dispatcher.cs
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public void AdviseKeyEventSink(int clientId, [MarshalAs(UnmanagedType.Interface)] object obj, [MarshalAs(UnmanagedType.Bool)] bool fForeground) | ||
| { | ||
| IntPtr unknownPtr = Marshal.GetIUnknownForObject(obj); |
There was a problem hiding this comment.
How it is leaking here? Are you mean in case of any failures inside unmanaged part, we should release unknownPtr ?
There was a problem hiding this comment.
Or do you mean, once we pass COM pointer to unamanged side we can safely release all instances from unmanaged part?
There was a problem hiding this comment.
The Marshal.GetIUnknownForObject and Marshal.QueryInterface APIs both should perform an AddRef. If the API being called with the IUnknown instances needs to retain ownership it should call its own AddRef. Which means unless it can be confirmed that the calling API takes over ownership of the IUnknown and will call Release it is a leak.
There was a problem hiding this comment.
Then I would say I take care of issues. At least I think so
There was a problem hiding this comment.
Should this be this?
IntPtr unknownPtr = default;
try
{
unknownPtr = Marshal.GetIUnknownForObject(obj);
....
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/TextEditor.cs
Show resolved
Hide resolved
|
|
||
| public void AdviseKeyEventSink(int clientId, [MarshalAs(UnmanagedType.Interface)] object obj, [MarshalAs(UnmanagedType.Bool)] bool fForeground) | ||
| { | ||
| IntPtr unknownPtr = Marshal.GetIUnknownForObject(obj); |
There was a problem hiding this comment.
Should this be this?
IntPtr unknownPtr = default;
try
{
unknownPtr = Marshal.GetIUnknownForObject(obj);
....
src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/WpfComWrappers.TfThreadMgrWrapper.cs
Outdated
Show resolved
Hide resolved
|
Any movement on this? |
Description
This is minimal change which replace usage built-in COM for IThThreadMgr instance.
Having ComWrappers is important to make #3811 works.
Additionally I remove sharing headers from
UnsafeNativeMethodsTextServices.cssince this file used just inWindowsBase.csproj.Customer Impact
Less chance to have trimming inside WPF
Testing
Manually testing by opening/closing WPF application and doing actions
Risk
Potentially can introduce some new bugs if RCW written incorrectly. Given that I already have some experience making this kind of changes for WinForms I have doubts. I always can try ask @RussKie or @AaronRobinsonMSFT review this as additional safeguard. Hopefully they would agree, and their help is welcome.
Microsoft Reviewers: Open in CodeFlow