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

Add check for UIObject circles resulting in infinite loops #3516

Conversation

marcelwgn
Copy link
Collaborator

Description

When the EventsSource property of an element is set to its parent, the test framework gets in an infnite loop returning the same element over and over again resulting in finding elements not terminating.

This PR adds a check if the last processed descendant is equal to the current descendant to detect whether we are in an infinite loop and break if that is the case.

Motivation and Context

Closes #3491

How Has This Been Tested?

Here is the test code:

C#

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

using Windows.UI.Xaml.Automation.Peers;
using Windows.UI.Xaml.Controls;

namespace MUXControlsTestApp
{
    public sealed class TestControl : ContentControl
    {
        protected override AutomationPeer OnCreateAutomationPeer()
        {
            return new TestControlAutomationPeer(this);
        }
    }

    public sealed class TestControlAutomationPeer : FrameworkElementAutomationPeer
    {
        public TestControlAutomationPeer(TestControl owner) : base(owner) { }
        protected override AutomationControlType GetAutomationControlTypeCore()
        {
            return AutomationControlType.Custom;
        }
    }

    [TopLevelTestPage(Name = "BorderThickness")]
    public sealed partial class BorderThicknessPage : TestPage
    {


        public BorderThicknessPage()
        {
            this.InitializeComponent();

            var customControlPeer = FrameworkElementAutomationPeer.FromElement(CustomControl);
            var buttonPeer = FrameworkElementAutomationPeer.FromElement(Button1);

            buttonPeer.EventsSource = customControlPeer;
        }
    }
}

XAML

<!-- Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT License. See LICENSE in the project root for license information. -->
<local:TestPage
    xmlns:local="using:MUXControlsTestApp"
    x:Class="MUXControlsTestApp.BorderThicknessPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    xmlns:controls="using:Microsoft.UI.Xaml.Controls"
    mc:Ignorable="d"
    xmlns:contract5Present="http://schemas.microsoft.com/winfx/2006/xaml/presentation?IsApiContractPresent(Windows.Foundation.UniversalApiContract, 5)"
    Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">

    <ScrollViewer>
        <local:TestControl x:Name="CustomControl" AutomationProperties.Name="CustomControl">
            <Button x:Name="Button1" Content="Button"/>
        </local:TestControl>
    </ScrollViewer>
</local:TestPage>

Interaction test

        [TestMethod]
        public void MyTestMethod()
        {
            using (var setup = new TestSetupHelper("BorderThickness Tests"))
            {
                FindElement.ByName("CustomControl");
            }
        }

Screenshots (if appropriate):

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Oct 29, 2020
@ranjeshj
Copy link
Contributor

@ejserna as FYI

@ranjeshj
Copy link
Contributor

This looks like it might fix the link from child to immediate parent but not if there is a loop across say child and grandparent ?

@ranjeshj ranjeshj added area-DevInternal Internal build infrastructure, code cleanup, engineering efficiency team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Oct 29, 2020
@marcelwgn
Copy link
Collaborator Author

This looks like it might fix the link from child to immediate parent but not if there is a loop across say child and grandparent ?

Yes, we can only circumvent direct parent/child relations, a loop across multiple items can't be found. The issue is that to allow any depth of items would require us to store all items previously visited and looking if the item was found. I tried that and even starting the app with that takes a long time as app startup already contains over a hundred items.

@ranjeshj
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ranjeshj
Copy link
Contributor

This looks like it might fix the link from child to immediate parent but not if there is a loop across say child and grandparent ?

Yes, we can only circumvent direct parent/child relations, a loop across multiple items can't be found. The issue is that to allow any depth of items would require us to store all items previously visited and looking if the item was found. I tried that and even starting the app with that takes a long time as app startup already contains over a hundred items.

ok. This seems like a reasonable fix for the immediate problem at hand.

@marcelwgn
Copy link
Collaborator Author

It seems that the changes broke the RS2/RS3 interaction tests entirely, though I am not sure what exactly the issue is. According to the logs, this is about passing null where it shouldn't be null. An example log can be downloaded here.

@marcelwgn
Copy link
Collaborator Author

I'm inclined to say that the test failure was CI related, not because of this PR. PR #3498 also encountered an issue with WUXC.dll crashing in release/x86. It's quite strange that both CI runs failed on release/86 with WUXC issues.

@ranjeshj
Copy link
Contributor

ranjeshj commented Nov 3, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcelwgn
Copy link
Collaborator Author

I think the issue is that we prematurely break out of the for loop and as such, the down level, some elements can not be found. Updated the logic to only break when we encounter an item 10 times in a row.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcelwgn
Copy link
Collaborator Author

Anything missing in this PR or can we merge this?

@StephenLPeters StephenLPeters merged commit d7c672d into microsoft:master Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-DevInternal Internal build infrastructure, code cleanup, engineering efficiency team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FindElement.Refresh stuck in infinite loop when setting a child's events source to the parent's
3 participants