Crash in MS.Internal.Data.AccessorTable.CleanupOperation

[IngeborgAtRS]

Description

I had an unexpected crash in my application (see crash log below). It's large, with many WPF views - this may have contributed to triggering this crash. The application had been running for over 4 hours when the crash occurred.

Reproduction Steps

I was not able to reproduce. It may be related to how long the application has been executing (over 4 hours).

Expected behavior

No crash :).

Actual behavior

The crash log is as follows:

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.Dictionary`2.Enumerator.MoveNext()
   at MS.Internal.Data.AccessorTable.CleanupOperation(Object arg)
   at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs)
   at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler)
   at System.Windows.Threading.DispatcherOperation.InvokeImpl()
   at MS.Internal.CulturePreservingExecutionContext.CallbackWrapper(Object obj)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at MS.Internal.CulturePreservingExecutionContext.Run(CulturePreservingExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Windows.Threading.DispatcherOperation.Invoke()
   at System.Windows.Threading.Dispatcher.ProcessQueue()
   at System.Windows.Threading.Dispatcher.WndProcHook(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled)
   at MS.Win32.HwndWrapper.WndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled)
   at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs)
   at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler)
   at System.Windows.Threading.Dispatcher.LegacyInvokeImpl(DispatcherPriority priority, TimeSpan timeout, Delegate method, Object args, Int32 numArgs)
   at MS.Win32.HwndSubclass.SubclassWndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam)
   at MS.Win32.UnsafeNativeMethods.DispatchMessage(MSG& msg)
   at System.Windows.Threading.Dispatcher.PushFrameImpl(DispatcherFrame frame)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)

Regression?

Uncertain since repro steps are unknown. We recently moved our application from .Net framework 4.8 to .Net 8, could be related.

Known Workarounds

None

Impact

Uncertain. I have so far only seen this once.

Configuration

.Net 8, Windows 11, x64. I doubt that it's specific to the configuration.

Other information

I find this code suspicious:

             // Remove entries that are sufficiently old
            foreach (KeyValuePair<AccessorTableKey, AccessorInfo> entry in _table)
            {
                int age = _generation - entry.Value.Generation;
                if (age >= AgeLimit)
                {
                    _table.Remove(entry.Key);
                }
            }

Removing an entry from something you're enumerating always looks suspicious, but maybe the dictionary enumerator can handle it.
Otherwise, is it possible that _table could be updated from multiple threads by the DataBinding engine?

[h3xds1nz]

Removal/Clear during enumeration on Dictionary<K, V> is perfectly safe on .NET Core 3.0+.

This was converted from Hashtable to Dictionary in #6500 but I find that hardly to change the conditions, as Hashtable's only additional gurantee is multiple-readers/1-writer which won't really change the semantics here. Cleanups are done on the Dispatcher thread and only other write to the accessor table should be via PropertyPathWorker iirc.

Are you using collection synchronization by any chance in your app?

[IngeborgAtRS]

There is only one call to EnableCollectionSynchronization in the entire repo, and that particular piece of code was not in use when the crash occurred (it's not part of the application that was running at the time).

There are some calls to SetBinding/ClearBinding/GetBinding however.

[amarinov-msft]

@h3xds1nz I think I was able to reproduce the underlying issue. Can you check the attached sample.

If I understand the scenario correctly, the PropertyPathWorker captures its engine at construction, then uses Engine.AccessorTable on whatever thread evaluates the binding. So, a binding created on the UI thread is permanently bound to the UI thread's AccessorTable. If that binding is later evaluated on a worker thread, the worker thread writes into the UI thread's _table (with the indexer setter, which increments Dictionary._version) while the UI thread's dispatcher runs CleanupOperation, which is enumerating that same _table.

repro11665.zip

[h3xds1nz]

@amarinov-msft Yeah, that would be correct and what is more than likely happening in the original scenario.

[IngeborgAtRS]

So, what should I be looking for as a workaround? Check that NotifyPropertyChanged, CollectionChanged etc is always triggered on the Dispatcher thread?

[amarinov-msft]

@IngeborgAtRS
Yes, what you suggest looks like a valid workaround. CleanupOperation is always queued back to the Dispatcher, but the write side - PropertyPathWorker.ReplaceItem calling Engine.AccessorTable[...] = ... - runs on whatever thread re-evaluates the binding. If those notifications fire on the Dispatcher, the write and the cleanup sweep are serialized by the Dispatcher and the race goes away.

I would propose the following approach:
Raise PropertyChanged on the Dispatcher from your ViewModelBase. In OnPropertyChanged, if Dispatcher.CheckAccess() returns false, invoke the handler using Dispatcher.BeginInvoke instead of calling it directly. This might be the smallest change with the broadest coverage.

I tested this in a console repro (updated the one I attached in my previous response). Happy to share it if useful.

[IngeborgAtRS]

Thanks, but there is a small problem with that solution, unfortunately. Our ViewModelBase class is currently unaware of WPF, including the Dispatcher. This is by design.

But it looks to me like ClrBindingWorker.OnSourcePropertyChanged already deals with marshaling to the correct thread?
Have to think a bit longer about how to deal with this.

[h3xds1nz]

@amarinov-msft I would be hesitant to mark this as a WPF bug to be honest.

@IngeborgAtRS Yeah, PropertyChanged notifications are marshaled, CollectionChanged, if connected to a collection view, would throw an exception. I do not know how your app is structured WPF-wise, whether you use different UI threads for your views, etc. but it leads me to believe that you might be evaluating a binding on some of your paths from a different thread than the one that constructed it.

As for dealing for VM without being aware of WPF, there are multiple ways, from the top of my head:

1. If your VM runs on a different thread and you do not use a special SynchronizationContext, based on checking Current property, you can decide whether to marshal or not.
2. If you do and just use multiple threads and you're view-first, you can capture the SynchronizationContext into a TaskScheduler during the construction of your VM and then posting tasks onto the Dispatcher queue via the TaskScheduler.

That being said, it is hard to provide guidance not knowing the scale and architecture.

[IngeborgAtRS]

Of course it is hard to provide guidance without the code, and unfortunately it is proprietary so I cannot show it to you. We don't use multiple UI threads in general, except for a couple of transient progress dialogs. However, we do trigger property updates in the VMs from worker threads. Using SynchronizationContext is possible, we do use it elsewhere.

I looked at PropertyPathWorker and it's hard to make a guess as to what triggered PropertyPathWorker.ReplaceItem. I will probably wait and see if this reproduces. We're talking about a repo with ~4 million lines of code here so guesswork is not feasible.

I just wish AccessorTable was threadsafe.