#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

• Recursively unwrap WrapsDriver to find the original driver implementation

• Handle WrapsDriver interface in deriveAdditionalInterfaces method

• Prevent infinite loops and hanging when decorating already-decorated drivers

---

Diagram Walkthrough

flowchart LR
  A["Double-wrapped WebDriver"] -->|unwrapOriginal| B["Recursively unwrap WrapsDriver"]
  B -->|find original| C["Original driver instance"]
  D["deriveAdditionalInterfaces"] -->|check WrapsDriver| E["Return wrapped driver or instance"]
  C -->|proper decoration| F["Correctly decorated WebDriver"]

Loading

File Walkthrough

Relevant files

Bug fix

WebDriverDecorator.java Unwrap double-wrapped WebDriver instances recursively        java/src/org/openqa/selenium/support/decorators/WebDriverDecorator.java Added unwrapOriginal() method to recursively unwrap WrapsDriver instances until reaching the original driver Modified Definition constructor to use unwrapOriginal() instead of directly calling getOriginal() Updated deriveAdditionalInterfaces() to handle already-wrapped drivers by checking if sample is WrapsDriver Added conditional logic in WrapsDriver handler to return wrapped driver when sample is already wrapped +13/-2

---

[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

[joerg1985]

Why should the method name be ignored for a WrapsDriver instance? WrapsDriver does only has this one method, but this might change over time. Or some generic methods e.g. .hashCode() could be called?