#16573 unwrap double-wrapped webdriver

[asolntsev]

User description

🔗 Related Issues

Fixes #16573

💥 What does this PR do?

Fix WebDriverDecorator to properly decorate WebDriver instance that is already decorated.

🔄 Types of changes

• Bug fix (backwards compatible)

Test

I have a test, but not sure where to put it in Selenium project codebase:

final class UnwrapWebdriverTest {
  private final WebDriver original = new ChromeDriver();

  @Test
  void webdriverDecorator() {
    WebDriver decorated1 = new WebDriverDecorator<>().decorate(original);
    WebDriver decorated2 = new WebDriverDecorator<>().decorate(decorated1);

    assertThat(((WrapsDriver) decorated1).getWrappedDriver()).isSameAs(original);
    assertThat(((WrapsDriver) decorated2).getWrappedDriver()).isSameAs(decorated1); // This failed

    new WebDriverWait(decorated2, Duration.ofSeconds(2))
      .until((d) -> "zopa".equalsIgnoreCase(d.getTitle())); // This hanged forever
  }

  @AfterEach
  void tearDown() {
    original.quit();
  }
}

---

PR Type

Bug fix

---

Description

• Fix WebDriverDecorator to properly unwrap double-wrapped WebDriver instances

• Add recursive unwrapping logic to handle nested WrapsDriver decorators

• Ensure getWrappedDriver returns the original unwrapped driver

• Add test coverage for multiple decorator wrapping scenarios

---

Diagram Walkthrough

flowchart LR
  A["Double-wrapped WebDriver"] -->|unwrapWebdriver| B["Recursive unwrapping"]
  B -->|Check WrapsDriver| C["Extract wrapped driver"]
  C -->|Repeat until base| D["Original WebDriver"]
  D -->|Return| E["Properly unwrapped driver"]

Loading

File Walkthrough

	Relevant files
Bug fix	WebDriverDecorator.java Add recursive unwrapping for nested WebDriver decorators  java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java Added unwrapOriginal() method to recursively unwrap decorated WebDriver instances Added unwrapWebdriver() method that recursively extracts the base driver from nested WrapsDriver wrappers Modified Definition constructor to use unwrapOriginal() instead of direct getOriginal() call Updated deriveAdditionalInterfaces() to return unwrapped driver for all WebDriver instances and handle WrapsDriver case properly +16/-2
Tests	DecoratedRemoteWebDriverTest.java Add test coverage for multiple decorator wrapping                java/test/org/openqa/selenium/support/decorators/DecoratedRemoteWebDriverTest.java Added assertion to verify wrapped driver is the original driver instance Added new test canWrapDriverMultipleTimes() to verify multiple decorator wrapping works correctly Test validates that deeply nested decorators properly unwrap to the original driver Ensures getWrappedDriver returns the same original driver instance regardless of wrapping depth +22/-0

---

[qodo-merge-pro]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #1234 🔴 Investigate and fix a regression where click() on a link with JavaScript in the href no longer triggers the JavaScript in Selenium 2.48.x (worked in 2.47.1), observed on Firefox 42.
🟡	🎫 #5678 🔴 Diagnose and resolve intermittent "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu with ChromeDriver 2.35 and Chrome 65 using Selenium 3.9.0.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No auditing: The new logic unwrapping and interface handling adds behavior changes without any auditing or logging of critical actions, but this utility likely should not log to avoid noise; verify if project conventions require audit logs here. Referred Code private static Object unwrapOriginal(Decorated<?> decorated) { Object original = decorated.getOriginal(); while (original instanceof WrapsDriver) { original = ((WrapsDriver) original).getWrappedDriver(); } return original; Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Edge cases unhandled: The unwrap loop assumes getWrappedDriver() never returns null and that unwrapping eventually terminates, lacking explicit safeguards or error context for cycles or null returns. Referred Code private static Object unwrapOriginal(Decorated<?> decorated) { Object original = decorated.getOriginal(); while (original instanceof WrapsDriver) { original = ((WrapsDriver) original).getWrappedDriver(); } return original; Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[selenium-ci]

Thank you, @asolntsev for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect driver unwrapping logic Correct the WrapsDriver invocation handler to return the decorated instance instead of unwrapping it, ensuring it adheres to the WrapsDriver contract. java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java [496-501] -if (sample instanceof WrapsDriver) { - return ((WrapsDriver) sample).getWrappedDriver(); -} if ("getWrappedDriver".equals(method.getName())) { return instance; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a logic error in the PR that violates the WrapsDriver contract by unwrapping one level too deep, and it provides the correct fix.	Medium
	Prevent infinite loop in unwrap Add cycle detection to the unwrapOriginal method by tracking visited driver instances in a Set to prevent potential infinite loops. java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java [201-207] private static Object unwrapOriginal(Decorated<?> decorated) { Object original = decorated.getOriginal(); + Set<Object> seen = new HashSet<>(); while (original instanceof WrapsDriver) { + if (!seen.add(original)) { + throw new IllegalStateException("Circular reference detected in WrapsDriver chain"); + } original = ((WrapsDriver) original).getWrappedDriver(); } return original; } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential infinite loop in the newly added unwrapOriginal method and proposes a standard, robust solution to prevent it by detecting circular references.	Medium
Learned best practice	Add null validation guards Validate that decorated and decorated.getOriginal() are non-null and that unwrapping produces a non-null driver; throw a clear IllegalArgumentException otherwise. java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java [196-207] public Definition(Decorated<?> decorated) { + if (decorated == null) { + throw new IllegalArgumentException("decorated must not be null"); + } this.decoratedClass = decorated.getClass(); - this.originalClass = unwrapOriginal(decorated).getClass(); + Object original = unwrapOriginal(decorated); + if (original == null) { + throw new IllegalArgumentException("Original driver must not be null after unwrapping"); + } + this.originalClass = original.getClass(); } private static Object unwrapOriginal(Decorated<?> decorated) { Object original = decorated.getOriginal(); while (original instanceof WrapsDriver) { - original = ((WrapsDriver) original).getWrappedDriver(); + Object next = ((WrapsDriver) original).getWrappedDriver(); + if (next == null) { + break; + } + original = next; } return original; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Guard external API interactions and inputs with validation to avoid ambiguous behavior and surface clear errors.	Low
Update

[qodo-merge-pro]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Naming consistency: The new helper names mix camelCase with lowercase 'webdriver' (unwrapWebdriver) which may be inconsistent with existing naming conventions and could reduce readability. Referred Code private static Object unwrapWebdriver(Object webDriver) { if (webDriver instanceof WebDriver && webDriver instanceof WrapsDriver) { return unwrapWebdriver(((WrapsDriver) webDriver).getWrappedDriver()); } return webDriver; } Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent stack overflow in recursion To prevent a potential StackOverflowError from circular wrapping, modify the recursive unwrapWebdriver method to track visited drivers using a Set to detect cycles. java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java [224-229] private static Object unwrapWebdriver(Object webDriver) { + return unwrapWebdriver(webDriver, new HashSet<>()); +} + +private static Object unwrapWebdriver(Object webDriver, Set<Object> alreadySeen) { + if (webDriver == null || !alreadySeen.add(webDriver)) { + return webDriver; + } + if (webDriver instanceof WebDriver && webDriver instanceof WrapsDriver) { - return unwrapWebdriver(((WrapsDriver) webDriver).getWrappedDriver()); + return unwrapWebdriver(((WrapsDriver) webDriver).getWrappedDriver(), alreadySeen); } return webDriver; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential StackOverflowError in the recursive unwrapWebdriver method due to circular dependencies and provides a robust solution using a Set to track visited objects.	Medium
Learned best practice	Fix inconsistent method capitalization Rename the helper to use the correct "WebDriver" capitalization for consistency with Selenium types and method names. java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java [224-229] -private static Object unwrapWebdriver(Object webDriver) { +private static Object unwrapWebDriver(Object webDriver) { if (webDriver instanceof WebDriver && webDriver instanceof WrapsDriver) { - return unwrapWebdriver(((WrapsDriver) webDriver).getWrappedDriver()); + return unwrapWebDriver(((WrapsDriver) webDriver).getWrappedDriver()); } return webDriver; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Enforce accurate and consistent naming to reflect actual behavior and avoid confusion.	Low
More