Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AccessViolation on grouped ListViewBase with Drag+Drop #9119 #9932

Conversation

lukasf
Copy link

@lukasf lukasf commented Aug 29, 2024

This PR should fix the AccessViolation that happens when using DragDrop on grouped ListView or GridView, see Issue #9119.

Description

As can be seen from the strack trace of the crashes, the error comes from CInputServices::InitializeDirectManipulationViewportValues. This method has a parameter pDirectManipulationService which is marked as optional. It is called with a NULL parameter in case of beginning of mouse edge scrolling here:

// Set the CDMViewport's initial and current transformation values, as well as for its potential secondary contents,

                            // Set the CDMViewport's initial and current transformation values, as well as for its potential secondary contents,
                            // for mouse-driven auto scrolling. Values were already initialized for touch scenarios and must not be re-initialized
                            // when an auto scroll was interrupted.
                            IFC(InitializeDirectManipulationViewportValues(
                                pViewport,
                                NULL /*pDirectManipulationService*/,
                                contentTranslationX,
                                contentTranslationY,
                                contentZoomFactor));

There are multiple places where pDirectManipulationService is accessed inside InitializeDirectManipulationViewportValues, each time with a check against null. But in case of clip content, there is no check. Looks like grouping will result in clip content being used, and this in turn will lead to the AccessViolation when the framework tries to begin an edge scroll animation (where NULL is passed for pDirectManipulationService). There is no other place in this method where unchecked parameters are accessed, so I am very confident that this is the exact place where the AccessViolation occurs.

Like in all other places where pDirectManipulationService is checked against null, the provided translations from the method parameters are used as a replacement, when the pDirectManipulationService is not available. See similar calls here:

if (pDirectManipulationService)

Or here:

if (pDirectManipulationService)

Motivation and Context

Both Drag+Drop and Grouping are critical core features. Both can be used easily together in all XAML platforms except for WinUI 3. This PR aims to make it possible to use these important features together in WinUI 3 as well.

How Has This Been Tested?

Since Microsoft has not yet made public all build dependencies, I could not fully test this myself. But the access violation definitely comes from this method, and this is the only place in the method where an unchecked access to an optional parameter happens. A simpe repro is available in the linked issue.

@microsoft-github-policy-service microsoft-github-policy-service bot added the needs-triage Issue needs to be triaged by the area owners label Aug 29, 2024
@lukasf lukasf changed the base branch from main to winui3/release/1.6-stable August 30, 2024 18:21
@lukasf
Copy link
Author

lukasf commented Aug 30, 2024

This was created against the wrong base branch. Closing in favor of #9938.

@lukasf lukasf closed this Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Issue needs to be triaged by the area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants