From 5be1d2f8f09d281eed2923d480e51d121aa139bb Mon Sep 17 00:00:00 2001 From: Sam Bent Date: Tue, 22 Sep 2020 17:09:57 -0700 Subject: [PATCH 1/3] disconnect HostVisual on its own thread --- .../System/Windows/Media/HostVisual.cs | 243 +++++++++++++++--- .../src/WpfGfx/include/exports.cs | 17 ++ 2 files changed, 229 insertions(+), 31 deletions(-) diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs index c1b94aab24d..980fdda4b7f 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs @@ -125,9 +125,16 @@ internal override void FreeContent(DUCE.Channel channel) using (CompositionEngineLock.Acquire()) { - DisconnectHostedVisual( - channel, - /* removeChannelFromCollection */ true); + // if there's a pending disconnect, do it now preemptively; + // otherwise do the disconnect the normal way. + // This ensures we do the disconnect before calling base, + // as required. + if (!DoPendingDisconnect(channel)) + { + DisconnectHostedVisual( + channel, + /* removeChannelFromCollection */ true); + } } base.FreeContent(channel); @@ -252,7 +259,7 @@ private void EnsureHostedVisualConnected(DUCE.Channel channel) // if (!(channel.IsSynchronous) && _target != null - && !_connectedChannels.Contains(channel)) + && !_connectedChannels.ContainsKey(channel)) { Debug.Assert(IsOnChannel(channel)); @@ -323,7 +330,15 @@ private void EnsureHostedVisualConnected(DUCE.Channel channel) channel); } - _connectedChannels.Add(channel); + // remember what channel we connected to, and which thread + // did the connection, so that we can disconnect on the + // same thread. Earlier comments imply this is the HostVisual's + // dispatcher thread, which we assert here. Even if it's not, + // the code downstream should work, or at least not crash + // (even if channelDispatcher is set to null). + Dispatcher channelDispatcher = Dispatcher.FromThread(Thread.CurrentThread); + Debug.Assert(channelDispatcher == this.Dispatcher, "HostVisual connecting on a second thread"); + _connectedChannels.Add(channel, channelDispatcher); // // Indicate that that content composition root has been @@ -364,10 +379,11 @@ private void EnsureHostedVisualConnected(DUCE.Channel channel) /// private void DisconnectHostedVisualOnAllChannels() { - foreach (DUCE.Channel channel in _connectedChannels) + IDictionaryEnumerator ide = _connectedChannels.GetEnumerator() as IDictionaryEnumerator; + while (ide.MoveNext()) { DisconnectHostedVisual( - channel, + (DUCE.Channel)ide.Key, /* removeChannelFromCollection */ false); } @@ -382,31 +398,154 @@ private void DisconnectHostedVisual( DUCE.Channel channel, bool removeChannelFromCollection) { - if (_target != null && _connectedChannels.Contains(channel)) + Dispatcher channelDispatcher; + if (_target != null && _connectedChannels.TryGetValue(channel, out channelDispatcher)) { - DUCE.CompositionNode.RemoveChild( - _proxy.GetHandle(channel), - _target._contentRoot.GetHandle(channel), - channel - ); - - // - // Release the targets handle. If we had duplicated the handle, - // then this removes the duplicated handle, otherwise just decrease - // the ref count for VisualTarget. - // - - _target._contentRoot.ReleaseOnChannel(channel); - - SetFlags(channel, false, VisualProxyFlags.IsContentNodeConnected); + // Adding commands to a channel is not thread-safe, + // we must do the actual work on the same dispatcher thread + // where the connection happened. + if (CoreAppContextSwitches.HostVisualDisconnectsOnWrongThread || + (channelDispatcher != null && channelDispatcher.CheckAccess())) + { + Disconnect(channel, + channelDispatcher, + _proxy.GetHandle(channel), + _target._contentRoot.GetHandle(channel), + _target._contentRoot); + } + else + { + // marshal to the right thread + if (channelDispatcher != null) + { + DispatcherOperation op = channelDispatcher.BeginInvoke( + DispatcherPriority.Normal, + new DispatcherOperationCallback(DoDisconnectHostedVisual), + channel); + + _disconnectData = new DisconnectData( + op: op, + channel: channel, + dispatcher: channelDispatcher, + hostVisual: this, + hostHandle: _proxy.GetHandle(channel), + targetHandle: _target._contentRoot.GetHandle(channel), + contentRoot: _target._contentRoot, + next: _disconnectData); + } + } if (removeChannelFromCollection) { _connectedChannels.Remove(channel); } } + + if (removeChannelFromCollection) + { + _connectedChannels.Remove(channel); + } + } } + /// + /// Callback to disconnect on the right thread + /// + private object DoDisconnectHostedVisual(object arg) + { + using (CompositionEngineLock.Acquire()) + { + DoPendingDisconnect((DUCE.Channel)arg); + } + + return null; + } + + /// + /// Perform a pending disconnect for the given channel. + /// This method should be called under the CompositionEngineLock, + /// on the thread that owns the channel. It can be called either + /// from the dispatcher callback DoDisconnectHostedVisual or + /// from FreeContent, whichever happens to occur first. + /// + /// + /// True if a matching request was found and processed. False if not. + /// + private bool DoPendingDisconnect(DUCE.Channel channel) + { + DisconnectData disconnectData = _disconnectData; + DisconnectData previous = null; + + // search the list for an entry matching the given channel + while (disconnectData != null && (disconnectData.HostVisual != this || disconnectData.Channel != channel)) + { + previous = disconnectData; + disconnectData = disconnectData.Next; + } + + // if no match found, do nothing + if (disconnectData == null) + { + return false; + } + + // remove the matching entry from the list + if (previous == null) + { + _disconnectData = disconnectData.Next; + } + else + { + previous.Next = disconnectData.Next; + } + + // cancel the dispatcher callback, (if we're already in it, + // this call is a no-op) + disconnectData.DispatcherOperation.Abort(); + + // do the actual disconnect + Disconnect(disconnectData.Channel, + disconnectData.ChannelDispatcher, + disconnectData.HostHandle, + disconnectData.TargetHandle, + disconnectData.ContentRoot); + + return true; + } + + /// + /// Do the actual work to disconnect the VisualTarget. + /// This is called (on the channel's thread) either from + /// DisconnectHostedVisual or from DoPendingDisconnect, + /// depending on which thread the request arrived on. + /// + private void Disconnect(DUCE.Channel channel, + Dispatcher channelDispatcher, + DUCE.ResourceHandle hostHandle, + DUCE.ResourceHandle targetHandle, + DUCE.MultiChannelResource contentRoot) + { + if (!CoreAppContextSwitches.HostVisualDisconnectsOnWrongThread) + { + channelDispatcher.VerifyAccess(); + } + + DUCE.CompositionNode.RemoveChild( + hostHandle, + targetHandle, + channel + ); + + // + // Release the targets handle. If we had duplicated the handle, + // then this removes the duplicated handle, otherwise just decrease + // the ref count for VisualTarget. + // + + contentRoot.ReleaseOnChannel(channel); + + SetFlags(channel, false, VisualProxyFlags.IsContentNodeConnected); + } /// /// Invalidate this visual. @@ -437,15 +576,57 @@ private void Invalidate() /// private VisualTarget _target; - /// - /// The channels we have marshalled the visual target composition root. - /// - /// - /// This field is free-threaded and should be accessed from under a lock. - /// - private List _connectedChannels = new List(); + /// + /// The channels we have marshalled the visual target composition root. + /// + /// + /// This field is free-threaded and should be accessed from under a lock. + /// + private Dictionary _connectedChannels = new Dictionary(); - #endregion Private Fields + /// + /// Data needed to disconnect the visual target. + /// + /// + /// This field is free-threaded and should be accessed from under a lock. + /// It's the head of a singly-linked list of pending disconnect requests, + /// each identified by the channel. In practice, the list is either empty + /// or has only one entry. + /// + private static DisconnectData _disconnectData; + + private class DisconnectData + { + public DispatcherOperation DispatcherOperation { get; private set; } + public DUCE.Channel Channel { get; private set; } + public Dispatcher ChannelDispatcher { get; private set; } + public HostVisual HostVisual { get; private set; } + public DUCE.ResourceHandle HostHandle { get; private set; } + public DUCE.ResourceHandle TargetHandle { get; private set; } + public DUCE.MultiChannelResource ContentRoot { get; private set; } + public DisconnectData Next { get; set; } + + public DisconnectData(DispatcherOperation op, + DUCE.Channel channel, + Dispatcher dispatcher, + HostVisual hostVisual, + DUCE.ResourceHandle hostHandle, + DUCE.ResourceHandle targetHandle, + DUCE.MultiChannelResource contentRoot, + DisconnectData next) + { + DispatcherOperation = op; + Channel = channel; + ChannelDispatcher = dispatcher; + HostVisual = hostVisual; + HostHandle = hostHandle; + TargetHandle = targetHandle; + ContentRoot = contentRoot; + Next = next; + } } + + #endregion Private Fields +} } diff --git a/src/Microsoft.DotNet.Wpf/src/WpfGfx/include/exports.cs b/src/Microsoft.DotNet.Wpf/src/WpfGfx/include/exports.cs index 5f67d5575a4..ebeae5054ab 100644 --- a/src/Microsoft.DotNet.Wpf/src/WpfGfx/include/exports.cs +++ b/src/Microsoft.DotNet.Wpf/src/WpfGfx/include/exports.cs @@ -365,6 +365,15 @@ internal struct ChannelSet /// channel. /// internal sealed partial class Channel + #if ENFORCE_CHANNEL_THREAD_ACCESS + : System.Windows.Threading.DispatcherObject + // "Producer" operations - adding commands et al. - should only be done + // on the thread that created the channel. These operations are on the + // hot path, so we don't add the cost of enforcement. To detect + // violations (which can lead to render-thread failures that + // are very difficult to diagnose), build + // PresentationCore with ENFORCE_CHANNEL_THREAD_ACCESS defined. + #endif { /// /// Primary channel. @@ -768,6 +777,10 @@ unsafe internal void SendCommand( int cSize, bool sendInSeparateBatch) { + #if ENFORCE_CHANNEL_THREAD_ACCESS + VerifyAccess(); + #endif + checked { Invariant.Assert(pCommandData != (byte*)0 && cSize > 0); @@ -808,6 +821,10 @@ unsafe internal void BeginCommand( int cbSize, int cbExtra) { + #if ENFORCE_CHANNEL_THREAD_ACCESS + VerifyAccess(); + #endif + checked { Invariant.Assert(cbSize > 0); From 7d20243937d6d8465085a8ff9dece509f29584ba Mon Sep 17 00:00:00 2001 From: Sam Bent Date: Wed, 23 Sep 2020 11:46:27 -0700 Subject: [PATCH 2/3] repair bad copy/paste --- .../System/Windows/Media/HostVisual.cs | 102 +++++++++--------- 1 file changed, 48 insertions(+), 54 deletions(-) diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs index 980fdda4b7f..5983145c785 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs @@ -440,12 +440,6 @@ private void DisconnectHostedVisual( _connectedChannels.Remove(channel); } } - - if (removeChannelFromCollection) - { - _connectedChannels.Remove(channel); - } - } } /// @@ -576,57 +570,57 @@ private void Invalidate() /// private VisualTarget _target; - /// - /// The channels we have marshalled the visual target composition root. - /// - /// - /// This field is free-threaded and should be accessed from under a lock. - /// - private Dictionary _connectedChannels = new Dictionary(); + /// + /// The channels we have marshalled the visual target composition root. + /// + /// + /// This field is free-threaded and should be accessed from under a lock. + /// + private Dictionary _connectedChannels = new Dictionary(); - /// - /// Data needed to disconnect the visual target. - /// - /// - /// This field is free-threaded and should be accessed from under a lock. - /// It's the head of a singly-linked list of pending disconnect requests, - /// each identified by the channel. In practice, the list is either empty - /// or has only one entry. - /// - private static DisconnectData _disconnectData; - - private class DisconnectData - { - public DispatcherOperation DispatcherOperation { get; private set; } - public DUCE.Channel Channel { get; private set; } - public Dispatcher ChannelDispatcher { get; private set; } - public HostVisual HostVisual { get; private set; } - public DUCE.ResourceHandle HostHandle { get; private set; } - public DUCE.ResourceHandle TargetHandle { get; private set; } - public DUCE.MultiChannelResource ContentRoot { get; private set; } - public DisconnectData Next { get; set; } - - public DisconnectData(DispatcherOperation op, - DUCE.Channel channel, - Dispatcher dispatcher, - HostVisual hostVisual, - DUCE.ResourceHandle hostHandle, - DUCE.ResourceHandle targetHandle, - DUCE.MultiChannelResource contentRoot, - DisconnectData next) + /// + /// Data needed to disconnect the visual target. + /// + /// + /// This field is free-threaded and should be accessed from under a lock. + /// It's the head of a singly-linked list of pending disconnect requests, + /// each identified by the channel and HostVisual. In practice, the list + /// is either empty or has only one entry. + /// + private static DisconnectData _disconnectData; + + private class DisconnectData { - DispatcherOperation = op; - Channel = channel; - ChannelDispatcher = dispatcher; - HostVisual = hostVisual; - HostHandle = hostHandle; - TargetHandle = targetHandle; - ContentRoot = contentRoot; - Next = next; + public DispatcherOperation DispatcherOperation { get; private set; } + public DUCE.Channel Channel { get; private set; } + public Dispatcher ChannelDispatcher { get; private set; } + public HostVisual HostVisual { get; private set; } + public DUCE.ResourceHandle HostHandle { get; private set; } + public DUCE.ResourceHandle TargetHandle { get; private set; } + public DUCE.MultiChannelResource ContentRoot { get; private set; } + public DisconnectData Next { get; set; } + + public DisconnectData(DispatcherOperation op, + DUCE.Channel channel, + Dispatcher dispatcher, + HostVisual hostVisual, + DUCE.ResourceHandle hostHandle, + DUCE.ResourceHandle targetHandle, + DUCE.MultiChannelResource contentRoot, + DisconnectData next) + { + DispatcherOperation = op; + Channel = channel; + ChannelDispatcher = dispatcher; + HostVisual = hostVisual; + HostHandle = hostHandle; + TargetHandle = targetHandle; + ContentRoot = contentRoot; + Next = next; + } } - } - #endregion Private Fields -} + #endregion Private Fields + } } From e628574672ef188fe6c44843397462ef0d2b6f30 Mon Sep 17 00:00:00 2001 From: Sam Bent Date: Thu, 24 Sep 2020 10:07:06 -0700 Subject: [PATCH 3/3] remove AppContext switch --- .../PresentationCore/System/Windows/Media/HostVisual.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs index 5983145c785..efab923aa3a 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/HostVisual.cs @@ -404,8 +404,7 @@ private void DisconnectHostedVisual( // Adding commands to a channel is not thread-safe, // we must do the actual work on the same dispatcher thread // where the connection happened. - if (CoreAppContextSwitches.HostVisualDisconnectsOnWrongThread || - (channelDispatcher != null && channelDispatcher.CheckAccess())) + if (channelDispatcher != null && channelDispatcher.CheckAccess()) { Disconnect(channel, channelDispatcher, @@ -519,10 +518,7 @@ private void Disconnect(DUCE.Channel channel, DUCE.ResourceHandle targetHandle, DUCE.MultiChannelResource contentRoot) { - if (!CoreAppContextSwitches.HostVisualDisconnectsOnWrongThread) - { - channelDispatcher.VerifyAccess(); - } + channelDispatcher.VerifyAccess(); DUCE.CompositionNode.RemoveChild( hostHandle,