Skip to content
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

[dotnet][bidi] Remove AsBiDiContextAsync helper to avoid disposal issue #15279

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Feb 12, 2025

User description

Motivation and Context

Fixes the 4th point to contribute to #14530. Any helpers we can introduce later.

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

Enhancement, Bug fix


Description

  • Removed AsBiDiContextAsync helper to address disposal issues.

  • Updated BiDiFixture to use AsBiDiAsync and BrowsingContext.GetTreeAsync.

  • Simplified BiDi connection management by removing redundant helper methods.


Changes walkthrough 📝

Relevant files
Enhancement
WebDriver.Extensions.cs
Removed `AsBiDiContextAsync` helper method.                           

dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs

  • Removed AsBiDiContextAsync helper method.
  • Simplified BiDi connection management using AsBiDiAsync.
  • +0/-10   
    Bug fix
    BiDiFixture.cs
    Updated BiDiFixture to use `AsBiDiAsync`.                               

    dotnet/test/common/BiDi/BiDiFixture.cs

  • Replaced AsBiDiContextAsync with AsBiDiAsync in setup.
  • Updated context initialization using BrowsingContext.GetTreeAsync.
  • +3/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    14530 - Partially compliant

    Compliant requirements:

    • Remove AsBiDiContextAsync helper method to avoid disposal issues
    • Keep only one entry point: driver.AsBiDiAsync()

    Non-compliant requirements:

    • Provide a way to access BrowsingContext without disposal issues (no new method like bidi.AsBrowsingContext() was added)

    Requires further human verification:

    • Ensure safe BiDi connection management (needs runtime testing to verify proper disposal behavior)
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Race Condition

    The new setup uses two async calls in sequence (AsBiDiAsync and GetTreeAsync) where previously only one was used. This could potentially introduce timing issues in tests.

    bidi = await driver.AsBiDiAsync();
    
    context = (await bidi.BrowsingContext.GetTreeAsync())[0].Context;

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add error handling for empty contexts

    Add error handling around GetTreeAsync() call since it could return an empty
    array, leading to a potential null reference exception when accessing index [0]

    dotnet/test/common/BiDi/BiDiFixture.cs [49]

    -context = (await bidi.BrowsingContext.GetTreeAsync())[0].Context;
    +var contexts = await bidi.BrowsingContext.GetTreeAsync();
    +if (contexts.Length == 0) throw new InvalidOperationException("No browsing contexts available");
    +context = contexts[0].Context;
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion addresses a critical potential null reference exception by adding proper error handling when no browsing contexts are available. This is an important defensive programming practice that could prevent runtime crashes.

    High
    Learned
    best practice
    Add parameter validation to prevent null reference exceptions by checking required parameters at method entry points

    Add null validation for the webDriver parameter at the start of AsBiDiAsync
    method to prevent potential NullReferenceException when accessing its
    properties. Use ArgumentNullException.ThrowIfNull for clear error messaging.

    dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs [43-45]

     public static async Task<BiDi> AsBiDiAsync(this IWebDriver webDriver)
     {
    +    ArgumentNullException.ThrowIfNull(webDriver, nameof(webDriver));
         var webSocketUrl = webDriver.GetWebSocketUrl();
         if (webSocketUrl is null) throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 6
    Low

    @nvborisenko
    Copy link
    Member Author

    Definitely this is right decision. What is recommended replacement - it is another topic.

    @nvborisenko nvborisenko merged commit f02c68a into SeleniumHQ:trunk Feb 12, 2025
    9 of 10 checks passed
    @nvborisenko nvborisenko deleted the bidi-remove-as-context branch February 20, 2025 10:38
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 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.

    1 participant