dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.03k stars 1.16k forks source link

IMultiValueConverter receives {DisconnectedItem} with .NET Framework 4.8, KB4486153 #1706

Open vsfeedback opened 5 years ago

vsfeedback commented 5 years ago

This issue has been moved from a ticket on Developer Community.


We detected a possible regression, as customers with an updated Windows Version to 1903 (to be exact, it is the recent Windows-Update, which installs .NET Framework 4.8, KB4486153) are experiencing a crash (InvalidCastException in on of our MultiValueConverter).

We've got a WPF-Binding-Scenario where we use a MultiValueConverter in a DataGrid in a WPF-UserControl, which is embedded in a WinForm.

The ViewModel - which is bound to the dataGrid - clears all used BindingLists when disposing.
Before KB4486153, the bindingList.Clear() didnt trigger the multivalueconverter to convert anything.
After KB4486153, the bindingList.Clear() triggers the mutivalueconverter with one parameter to be of type {DisconnectedItem} (in my case, it is the Item Property, which is bound to DataGridRow).

This is suprising, because as one of the changelog-items in .NET Framework 4.8 was as followed:

"Fixed an issue involving bindings with DataContext explicitly on the binding path. When DataContext changes to {DisconnectedItem}, the binding should not pass that value to user code like converters, property-change handlers, etc. [801039, PresentationFramework.dll, Bug, Build:3761]" .

**EDIT**:

Just found out that this line is the problem in .NET 4.8. The Converter is called with {DisconnectedItem} if the bound item is getting removed, if the the problematic binding is specified as followed:

<Binding RelativeSource="{RelativeSource Mode=FindAncestor, AncestorType=DataGridRow, AncestorLevel=1}" Path="Item" />

If i change it to:

<Binding Path="." />

it works perfectly, even if we remove the item (or clearing the bindinglist) - the converter does not crash (or does not getting called).

But as this worked before 4.8 with a RelativeSource binding, this regression still should be checked.

I finally managed to create a small RePro which crashes with 4.8 (containing KB4486153) and does not Crash with 4.7.2 or earlier. Please have a look at it:disconnectedobject-repro.zip


Original Comments

Visual Studio Feedback System on 8/19/2019, 00:49 AM:

We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.

Visual Studio Feedback System on 8/22/2019, 01:53 PM:

Thank you for sharing your feedback! Our teams prioritize action on product issues with broad customer impact. See details at: https://docs.microsoft.com/en-us/visualstudio/ide/report-a-problem?view=vs-2017#faq. In case you need answers to common questions or need assisted support, be sure to use https://visualstudio.microsoft.com/vs/support/. We’ll keep you posted on any updates to this feedback.


Original Solutions

(no solutions)

SamBent commented 5 years ago

"Move to GitHub" failed to pick up my comments:

You've already discovered the solution: inside a DataGrid cell your second binding is better than the first. It's more efficient (avoiding ancestor lookup), and reacts better when the item is removed or goes out of view.

When the item is removed or goes out of view, the DataGridRow's DataContext is set to {DisconnectedItem} to indicate that it is no longer connected to an item. This is supposed to propagate into the subtree to disconnect all the UI elements and bindings therein (by "disconnect", I mean stop listening for property changes, writing new values, holding references, or interacting with the item in any way). In .NET 4.7.2, propagation into DataGrid cells wasn't happening, leading to memory leaks and subtle bugs when child elements continued to interact with the item in ways that interfered with the expected behavior. This was fixed in .NET 4.8 ("Fixed a memory leak arising when removing data items from their parent collections, when UIAutomation is present. [172291, PresentationFramework.dll, Bug, Build:3621]"), and part of the fix was to set DataGridRow.Item to {DisconnectedItem}.

We didn't consider your case - a binding whose path mentions "Item" - because there's a more natural alternative. Now that you've made me aware of it, I realize we should have suppressed the property-changed notification when changing Item to {DisconnectedItem}, just like we suppress when changing DataContext to {DisconnectedItem}. This would fix your problem - your converter wouldn't get called.

We'll fix this in .NETCore. If you really need this fixed in .NET 4.8, work with CSS (MS support) to open a servicing request. You'll need to provide business justification, which may be difficult as there is a simple workaround.

bschaeublin commented 5 years ago

Thank you for taking time and analysing this issue. I fixed it for now using the other binding - but I can't really be sure that I fixed it everywhere in our solution (we have many controls), as it needs some conditions for it to crash and so it's difficult to find the right spots.

If this would get bigger, we may contact the MS Support. But I don't think that this is necessary, as we do know the reason now and a solution for this problem.

weltkante commented 5 years ago

@bschaeublin just for the record, technically you are supposed to do type checks instead of plain casts in your IValueConverter and IMultiValueConverter implementations and return DependencyProperty.UnsetValue if your preconditions aren't met. Callers of the converter interfaces aren't required to do any typechecks and passing {DisconnectedItem} is explicitely allowed and can happen in various other situations when editing the visual tree (the binding engine never filtered them out, this was always "normal" behavior). This isn't documented super well but a correct implementation would have prevented this issue on your side.

bschaeublin commented 5 years ago

@weltkante you're right, i was wondering too, why there are no checks to these types. You know, code grows and sometimes gets old and neglected (unfortunately), so many converter still following samples which does not provide a hint to this or show best practices like you mentioned.

On the other side we wouldn't have realized that the behaviour changed - or end up with empty values or grids instead of failing fast.

shorasegui commented 5 years ago

Hi,

I have a similar issue. I have a datagrid which contains an autocomplete which is binded to an ObservableCollection based on ViewModel. I have implemented the DropDownClosed event to update some values to the object. After doing this everything seems to be ok but then I need to select the following row cell for edition by code and this cause that autocomplete datacontext appears as DisconnetedItem. I did not removed it, just updated some staff and selected the following cell in the row. Could you give me a clue about how can I resolve my issue?

Thank you in advance.

narayanmyidea commented 5 years ago

I have a similar issue. On observablecollection.Clear, It is invoking SelectionChanged command(I have only one relay command registered). Prior to 4.8 , it was receiving disconnected item as parameter in below line of Relay Command Implementation, so command was cancelled. bool CanExecute(object parameter)

After Installing KB4486153(OS WIN 10,Enterprise 1803) update, I am receiving T(My model class) as parameter so based on predicate result, command is executing. Since my solution is very big and in maintenance mode,It is difficult to identify the issues with respect to this Update#KB4486153