Skip to content

[java] Fix SpotBugs findings in ChromiumDriver and PortProber#14589

Merged
pujagani merged 2 commits intoSeleniumHQ:trunkfrom
mk868:fix-ChromiumDriver-and-PortProber
Oct 14, 2024
Merged

[java] Fix SpotBugs findings in ChromiumDriver and PortProber#14589
pujagani merged 2 commits intoSeleniumHQ:trunkfrom
mk868:fix-ChromiumDriver-and-PortProber

Conversation

@mk868
Copy link
Copy Markdown
Contributor

@mk868 mk868 commented Oct 11, 2024 •
edited by qodo-code-review Bot
Loading

User description

Description

As described in this comment, SpotBugs found some additional bugs in the code.
In this PR I fix part of the found problems.

ChromiumDriver

SpotBugs error:

H C RV: Bad attempt to compute absolute value of signed 32-bit hashcode in org.openqa.selenium.chromium.ChromiumDriver.pin(String)  At ChromiumDriver.java:[line 215]

documentation

Yes there is an actual bug here.
When script.hashCode() == Integer.MIN_VALUE then the generated code is not valid JavaScript code, and will cause the script to crash.

  String.format(
      "window.seleniumPinnedScript%s = function(){%s}",
      Math.abs(Integer.MIN_VALUE), "...");
// output: window.seleniumPinnedScript-2147483648 = function(){...}

Solution: cast hashCode to long

  String.format(
      "window.seleniumPinnedScript%s = function(){%s}",
      Math.abs((long) Integer.MIN_VALUE), "...");
// output: window.seleniumPinnedScript2147483648 = function(){...}

PortProber

SpotBugs error:

M D REC: Exception is caught when Exception is not thrown in org.openqa.selenium.net.PortProber.isFree(String, int)  At PortProber.java:[line 122]

documentation

Actually the code in the try block declares that it can throw exceptions of types:

  • IOException
  • SocketException
  • SecurityException
  • IllegalArgumentException

This can be shortened to IOException | RuntimeException union.

Motivation and Context

Fixing the actual problems is necessary before enabling full SpotBugs analysis, in order to not break the build.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix

Description

  • Fixed a bug in ChromiumDriver by casting the hash code to long to prevent invalid JavaScript code when the hash code equals Integer.MIN_VALUE.
  • Simplified exception handling in PortProber by catching specific exceptions (IOException and RuntimeException) instead of a generic Exception.

Changes walkthrough 📝

Relevant files
Bug fix
ChromiumDriver.java
Fix hashCode handling in `ChromiumDriver` pin method         

java/src/org/openqa/selenium/chromium/ChromiumDriver.java

  • Fixed potential bug by casting hashCode to long to avoid invalid
    JavaScript code.
  • Ensured the script hash code is correctly handled even for
    Integer.MIN_VALUE.
    		•  +2/-1     
    PortProber.java
    Simplify exception handling in `PortProber` isFree method

    java/src/org/openqa/selenium/net/PortProber.java

  • Simplified exception handling by catching IOException and
    RuntimeException.
  • Improved code clarity and reduced unnecessary catch-all exception
    handling.
    		•  +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @qodo-code-review qodo-code-review Bot added P-bug fix PR addresses a known issue Review effort [1-5]: 2 labels Oct 11, 2024
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 11, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Overflow
    The fix for the hashCode issue is correct, but it's worth considering if there are any other places in the code where similar issues might occur. A thorough review of hashCode usage throughout the codebase could be beneficial.

    Exception Handling
    The change from catching Exception to IOException | RuntimeException is more specific, which is good. However, it's worth considering if there are any specific exceptions that should be handled differently or logged for debugging purposes.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 11, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Log exceptions for better debugging and issue tracking

    Consider logging the exception for debugging purposes, as silently catching
    exceptions may hide important issues.

    java/src/org/openqa/selenium/net/PortProber.java [122-124]

     } catch (IOException | RuntimeException e) {
+  logger.debug("Port {} is not free: {}", port, e.getMessage());
   return false;
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding logging for caught exceptions can significantly aid in debugging and tracking issues, making the code more maintainable. This enhancement is practical and improves the code's robustness by providing more visibility into potential problems.

    		7
    Best practice
    Use a more consistent method for converting int to long in hash code calculation

    Consider using Long.hashCode(script.hashCode()) instead of casting to long to ensure
    consistent behavior across different JVM implementations.

    java/src/org/openqa/selenium/chromium/ChromiumDriver.java [216]

    -Math.abs((long) script.hashCode())
+Math.abs(Long.hashCode(script.hashCode()))
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to use Long.hashCode(script.hashCode()) instead of casting to long is a valid approach to ensure consistent behavior across JVM implementations. However, the impact of this change is moderate as the current casting method is not necessarily problematic.

    		5

    💡 Need additional feedback ? start a PR chat

    @pujagani
    Copy link
    Copy Markdown
    Contributor

    pujagani commented Oct 14, 2024

    Thank you! @mk868

    @pujagani pujagani merged commit c7daf73 into SeleniumHQ:trunk Oct 14, 2024
    @mk868 mk868 deleted the fix-ChromiumDriver-and-PortProber branch October 14, 2024 15:38
    @mk868 mk868 mentioned this pull request Dec 9, 2024
    Merged
    8 tasks
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @mk868 @pujagani @M1troll
    [java] Fix SpotBugs findings in ChromiumDriver and PortProber (Se…
    9894795 
    …leniumHQ#14589)

Co-authored-by: Puja Jagani <[email protected]>
    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    P-bug fix PR addresses a known issue Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @mk868 @pujagani