You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In many places, we only unwrap IWrapsElement once. This, however, has led to user issues.
This PR fixes that in a pain point from a user report. However, we should audit our usage of IWrapsElement and make sure this problem does not exist elsewhere.
There's also a lot of code duplication (or code that should be duplicated, but is implemented differently). We should centralize the element unwrapping logic.
The recursive unwrapping while loop could potentially enter an infinite loop if there's a circular reference between wrapped elements that isn't caught by the ReferenceEquals check
while(elementWrapper!=null){elementReference=elementWrapper.WrappedElementasIWebDriverObjectReference;if(ReferenceEquals(elementWrapper,elementWrapper.WrappedElement)){thrownewInvalidOperationException("Cannot determine root element: element wrapper wraps itself");}elementWrapper=elementWrapper.WrappedElementasIWrapsElement;}
The ConvertElement method is duplicated between PointerInputDevice and WheelInputDevice. Consider extracting to a shared utility class
privateDictionary<string,object>ConvertElement(){IWebDriverObjectReference?elementReference=this.targetasIWebDriverObjectReference;if(elementReference==null){IWrapsElement?elementWrapper=this.targetasIWrapsElement;while(elementWrapper!=null){elementReference=elementWrapper.WrappedElementasIWebDriverObjectReference;if(ReferenceEquals(elementWrapper,elementWrapper.WrappedElement)){thrownewInvalidOperationException("Cannot determine root element: element wrapper wraps itself");}elementWrapper=elementWrapper.WrappedElementasIWrapsElement;}}if(elementReference==null){thrownewArgumentException($"Target element cannot be converted to {nameof(IWebDriverObjectReference)}");}Dictionary<string,object>elementDictionary=elementReference.ToDictionary();returnelementDictionary;
+const int MaxRecursionDepth = 100;+int depth = 0;
while (elementWrapper != null)
{
+ if (depth++ > MaxRecursionDepth)+ {+ throw new InvalidOperationException("Maximum element wrapper depth exceeded");+ }
elementReference = elementWrapper.WrappedElement as IWebDriverObjectReference;
if (ReferenceEquals(elementWrapper, elementWrapper.WrappedElement))
{
throw new InvalidOperationException("Cannot determine root element: element wrapper wraps itself");
}
elementWrapper = elementWrapper.WrappedElement as IWrapsElement;
}
Apply this suggestion
Suggestion importance[1-10]: 8
__
Why: Adding a maximum recursion depth check is crucial for preventing stack overflow errors in production code, especially when dealing with potentially deeply nested element wrappers. This is a significant safety improvement.
Medium
Handle null wrapped elements
Add null check for WrappedElement to prevent NullReferenceException if an IWrapsElement implementation returns null.
while (elementWrapper != null)
{
- elementReference = elementWrapper.WrappedElement as IWebDriverObjectReference;- if (ReferenceEquals(elementWrapper, elementWrapper.WrappedElement))+ var wrappedElement = elementWrapper.WrappedElement;+ if (wrappedElement == null)+ {+ throw new InvalidOperationException("Element wrapper returned null for WrappedElement");+ }+ elementReference = wrappedElement as IWebDriverObjectReference;+ if (ReferenceEquals(elementWrapper, wrappedElement))
{
throw new InvalidOperationException("Cannot determine root element: element wrapper wraps itself");
}
- elementWrapper = elementWrapper.WrappedElement as IWrapsElement;+ elementWrapper = wrappedElement as IWrapsElement;
}
Apply this suggestion
Suggestion importance[1-10]: 7
__
Why: The suggestion adds important null checking for WrappedElement to prevent potential NullReferenceException, which could occur if an IWrapsElement implementation incorrectly returns null. This improves error handling and debugging.
Medium
Learned best practice
Add parameter validation to fail fast when required parameters are null
Add a null check for the target parameter at the start of the ConvertElement method to fail fast if the target is null, rather than letting it fail later when trying to use it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Description
In many places, we only unwrap
IWrapsElementonce. This, however, has led to user issues.This PR fixes that in a pain point from a user report. However, we should audit our usage of
IWrapsElementand make sure this problem does not exist elsewhere.There's also a lot of code duplication (or code that should be duplicated, but is implemented differently). We should centralize the element unwrapping logic.
Motivation and Context
Fixes #14513
Types of changes
Checklist
PR Type
Bug fix, Tests
Description
Fixed recursive unwrapping of
IWrapsElementto handle nested wrappers.Added error handling for self-referencing element wrappers.
Introduced new tests to validate double-wrapped element handling.
Added
WebElementWrapperclass for testing wrapped elements.Changes walkthrough 📝
PointerInputDevice.cs
Enhance `ConvertElement` to handle nested wrappersdotnet/src/webdriver/Interactions/PointerInputDevice.cs
ConvertElementto recursively unwrapIWrapsElement.WheelInputDevice.cs
Enhance `ConvertElement` to handle nested wrappersdotnet/src/webdriver/Interactions/WheelInputDevice.cs
ConvertElementto recursively unwrapIWrapsElement.BasicMouseInterfaceTest.cs
Add test for double-wrapped elements in mouse actionsdotnet/test/common/Interactions/BasicMouseInterfaceTest.cs
MoveToElementandClickfunctionality with wrapped elements.BasicWheelInterfaceTest.cs
Add test for double-wrapped elements in wheel actionsdotnet/test/common/Interactions/BasicWheelInterfaceTest.cs
ScrollToElementfunctionality with wrapped elements.WebElementWrapper.cs
Introduce `WebElementWrapper` for testingdotnet/test/common/WebElementWrapper.cs
WebElementWrapperclass implementingIWebElementandIWrapsElement.