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 c1b94aab24d66f0f10d5f1d622845a50def03dcf..efab923aa3a6f361f129d5f943400d9646b2fd45 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,23 +398,41 @@ private void DisconnectHostedVisualOnAllChannels() 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 (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) { @@ -407,6 +441,101 @@ private void DisconnectHostedVisualOnAllChannels() } } + /// + /// 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) + { + 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. @@ -443,7 +572,49 @@ private void Invalidate() /// /// This field is free-threaded and should be accessed from under a lock. /// - private List _connectedChannels = new List(); + 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 and HostVisual. 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 5f67d5575a41b3f0589c1e765be37d4f8462a755..ebeae5054abebedc18624e3026d4f3d84f819956 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 @@ internal bool IsOutOfBandChannel int cSize, bool sendInSeparateBatch) { + #if ENFORCE_CHANNEL_THREAD_ACCESS + VerifyAccess(); + #endif + checked { Invariant.Assert(pCommandData != (byte*)0 && cSize > 0); @@ -808,6 +821,10 @@ internal bool IsOutOfBandChannel int cbSize, int cbExtra) { + #if ENFORCE_CHANNEL_THREAD_ACCESS + VerifyAccess(); + #endif + checked { Invariant.Assert(cbSize > 0);