Skip to content

Conversation

@sbushmelev
Copy link

@sbushmelev sbushmelev commented Dec 8, 2025

💥 What does this PR do?

Refactoring to 17 java style

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Modernize Java code to Java 17 style conventions

  • Convert switch statements to enhanced switch expressions

  • Reorganize imports to follow Java conventions

  • Refactor string formatting to use text blocks

  • Improve code indentation and formatting consistency


Diagram Walkthrough

flowchart LR
  A["Java Code"] -->|"Switch to enhanced switch expressions"| B["Modern Switch Syntax"]
  A -->|"Reorganize imports"| C["Proper Import Order"]
  A -->|"Use text blocks"| D["Multi-line String Formatting"]
  A -->|"Fix indentation"| E["Consistent Code Style"]
  B --> F["Java 17 Compliant Code"]
  C --> F
  D --> F
  E --> F
Loading

File Walkthrough

Relevant files
Enhancement
BrowsingContextInfo.java
Convert switch to enhanced switch expressions                       

java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInfo.java

  • Convert traditional switch statement with break statements to enhanced
    switch expressions using arrow syntax
  • Reduce code verbosity by eliminating explicit break statements and
    braces
  • Maintain identical functionality while improving readability
+8/-31   
ThreadGuard.java
Modernize imports, formatting, and string handling             

java/src/org/openqa/selenium/support/ThreadGuard.java

  • Reorganize imports to place Selenium imports before Java standard
    library imports
  • Refactor multi-line string concatenation to use Java 17 text blocks
  • Improve method indentation and alignment for better code consistency
  • Update error message formatting to use text block syntax
+14/-12 

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2025

CLA assistant check
All committers have signed the CLA.

@selenium-ci selenium-ci added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes labels Dec 8, 2025
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Information disclosure

Description: The new WebDriverException message is a multi-line text block that ends with a newline,
which may inadvertently leak thread names and IDs verbatim to logs; consider whether
exposing thread identifiers in exception messages is acceptable for your threat model.
ThreadGuard.java [86-92]

Referred Code
    String.format(
      """
        Thread safety error; this instance of WebDriver was constructed on
        thread %s (id %d) and is being accessed by thread %s (id %d)
        This is not permitted and *will* cause undefined behaviour
        """, threadName, threadId, currentThread.getName(), currentThread.getId()));
}
Ticket Compliance
🟡
🎫 #1234
🔴 Ensure clicking links with javascript in the href triggers the JavaScript as it did in
version 2.47.1, including on Firefox 42.0 (32-bit) on a 64-bit machine.
Provide compatibility so that behavior remains consistent across versions 2.48.0/2.48.2
where regression was observed.
Include any necessary handling in WebDriver to correctly trigger href-based JavaScript
alerts during click().
🟡
🎫 #5678
🔴 Diagnose and resolve "Error: ConnectFailure (Connection refused)" occurring when
instantiating multiple ChromeDriver instances on Ubuntu 16.04.4 with Chrome 65 and
ChromeDriver 2.35 using Selenium 3.9.0.
Ensure subsequent ChromeDriver instantiations after the first do not fail with connection
refused errors.
Provide a stable initialization flow for repeated ChromeDriver creation without console
errors.
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: 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: 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 code modifies error messaging and proxy creation but does not add or enhance any
audit logging for critical actions, making it unclear whether required audit trails are
maintained.

Referred Code
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
  try {
    if (Thread.currentThread().getId() != threadId) {
      Thread currentThread = Thread.currentThread();
      throw new WebDriverException(
        String.format(
          """
            Thread safety error; this instance of WebDriver was constructed on
            thread %s (id %d) and is being accessed by thread %s (id %d)
            This is not permitted and *will* cause undefined behaviour
            """, threadName, threadId, currentThread.getName(), currentThread.getId()));

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:
Input validation: The enhanced switch continues to read multiple JSON fields without visible validation or
sanitization, leaving uncertainty about handling of malformed or unexpected input.

Referred Code
case "context" -> id = input.read(String.class);
case "url" -> url = input.read(String.class);
case "children" -> children = input.read(LIST_OF_BROWSING_CONTEXT_INFO);
case "parent" -> parentBrowsingContext = input.read(String.class);
case "clientWindow" -> clientWindow = input.read(String.class);
case "originalOpener" -> originalOpener = input.read(String.class);
case "userContext" -> userContext = input.read(String.class);
default -> input.skipValue();

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

@selenium-ci
Copy link
Member

Thank you, @sbushmelev 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-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Refine text block for correct formatting

To preserve the original single-line error message format, use .stripIndent()
and line continuation characters (</code>) within the text block to remove unwanted
whitespace and newlines.

java/src/org/openqa/selenium/support/ThreadGuard.java [86-91]

 String.format(
   """
-    Thread safety error; this instance of WebDriver was constructed on
-    thread %s (id %d) and is being accessed by thread %s (id %d)
-    This is not permitted and *will* cause undefined behaviour
-    """, threadName, threadId, currentThread.getName(), currentThread.getId())
+    Thread safety error; this instance of WebDriver was constructed on \
+    thread %s (id %d) and is being accessed by thread %s (id %d). \
+    This is not permitted and *will* cause undefined behaviour.
+    """.stripIndent(), threadName, threadId, currentThread.getName(), currentThread.getId())

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the refactoring to a text block unintentionally alters the error message format by adding newlines and whitespace, and it provides a valid solution using .stripIndent() and line continuations to restore the original single-line format.

Low
Learned
best practice
Sanitize thread names in messages

Sanitize thread names before inserting them into the formatted message to
prevent control characters or unexpected formatting from being injected into
logs or UIs.

java/src/org/openqa/selenium/support/ThreadGuard.java [85-91]

+String safeThreadName = currentThread.getName() == null ? "<unknown>" : currentThread.getName().replaceAll("\\p{Cntrl}", "?");
+String safeConstructedName = threadName == null ? "<unknown>" : threadName.replaceAll("\\p{Cntrl}", "?");
 throw new WebDriverException(
-            String.format(
-              """
-                Thread safety error; this instance of WebDriver was constructed on
-                thread %s (id %d) and is being accessed by thread %s (id %d)
-                This is not permitted and *will* cause undefined behaviour
-                """, threadName, threadId, currentThread.getName(), currentThread.getId()));
+  String.format(
+    """
+      Thread safety error; this instance of WebDriver was constructed on
+      thread %s (id %d) and is being accessed by thread %s (id %d)
+      This is not permitted and *will* cause undefined behaviour
+      """, safeConstructedName, threadId, safeThreadName, currentThread.getId()));
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate and sanitize external inputs when constructing messages to avoid unintended formatting or injection; avoid embedding raw text into format strings that use placeholders.

Low
  • More

@cgoldberg
Copy link
Member

We still support Java 11:

# We target java 11 by default

@asolntsev
Copy link
Contributor

@sbushmelev Thank you. Unfortunately, currently Selenium must support Java 11.
But hopefully soon we will release Selenium 5.x which will run on Java17+.

@asolntsev asolntsev added this to the Selenium 5.0 milestone Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes C-java Java Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants