Skip to content

[js] add --websocket-port flag when not connecting to existing connection on firefox #15513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Mar 26, 2025

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

By having the --websocket-port parameter by default we avoid port collisions when using BiDi with firefox

Motivation and Context

Fixes #15451 for JS binding

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, Enhancement


Description

  • Added --websocket-port flag for Firefox to avoid port conflicts.

  • Integrated portprober to dynamically allocate free ports.

  • Ensured the flag is only applied when not connecting to an existing instance.


Changes walkthrough 📝

Relevant files
Enhancement
index.js
Add dynamic `--websocket-port` allocation for Firefox       

javascript/selenium-webdriver/index.js

  • Imported portprober for dynamic port allocation.
  • Added logic to append --websocket-port with a free port for Firefox.
  • Ensured the flag is skipped when connecting to an existing instance.
  • +7/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Sorry, something went wrong.

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres
    …tion on firefox
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15451 - PR Code Verified

    Compliant requirements:

    • Added support for the websocket-port argument to avoid port collisions when using BiDi with Firefox

    Requires further human verification:

    • Verify that the implementation properly fixes the issue with multiple sessions on the same geckodriver
    • Confirm that the solution is consistent with how Python and .NET bindings handle this

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

    Method Error

    The code uses service?.args.append() but the correct method for arrays in JavaScript is likely push() not append(). This will cause runtime errors.

    service?.args.append('--websocket-port')
    service?.args.append(`${portprober.findFreePort()}`)
    Race Condition

    The port is allocated but not reserved, which could lead to a race condition where another process takes the port between finding it and starting Firefox.

    service?.args.append(`${portprober.findFreePort()}`)

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 26, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use correct array method
    Suggestion Impact:The commit directly implemented the suggestion by changing the incorrect 'append' method to the correct 'push' method for adding elements to JavaScript arrays, fixing a critical bug that would have caused runtime errors

    code diff:

    -          service?.args.append('--websocket-port')
    -          service?.args.append(`${portprober.findFreePort()}`)
    +          service?.args.push('--websocket-port')
    +          service?.args.push(`${portprober.findFreePort()}`)

    The args property likely doesn't have an append method. You should use push
    instead, which is the standard method for adding elements to arrays in
    JavaScript.

    javascript/selenium-webdriver/index.js [685-688]

     if (!service?.args.includes('--connect-existing')) {
    -  service?.args.append('--websocket-port')
    -  service?.args.append(`${portprober.findFreePort()}`)
    +  service?.args.push('--websocket-port')
    +  service?.args.push(`${portprober.findFreePort()}`)
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 10

    __

    Why: The suggestion correctly identifies a critical bug. JavaScript arrays use the push() method to add elements, not append(). Using append() would cause a runtime error that would break the Firefox driver initialization.

    High
    • Update

    Sorry, something went wrong.

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres
    @pujagani
    Copy link
    Contributor

    I think all the Firefox tests are failing. Can you help take a look? @Delta456
    Thank you!

    Copy link
    Contributor

    @pujagani pujagani left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    It looks fine but there must be a reason for tests failing. Do they pass on your local machine? Wondering if this is RBE specific

    @Delta456
    Copy link
    Member Author

    Delta456 commented Mar 26, 2025

    It looks fine but there must be a reason for tests failing. Do they pass on your local machine? Wondering if this is RBE specific

    Yes, they are passing locally. I am trying to figure out why it is failing on CI only.

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres
    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you @Delta456 !

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres
    Fix race condition!

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres

    Verified

    This commit was signed with the committer’s verified signature.
    Zerpet Aitor Pérez Cedres
    @harsha509
    Copy link
    Member

    Hi @Delta456,

    Thank you for your PR. The fix appeared correct, but it didn't replace the port as expected.

    I've updated the implementation in PR #15557.
    I'm closing this one—apologies for the oversight.

    Thank you @pujagani for taking a look.

    @harsha509 harsha509 closed this Apr 3, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Add argument for websocket-port
    3 participants