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] Make the devtools SendCommand method AOT-safe #15159

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 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.

Description

This uses source-generated JSON serialization on more devtools-related types.

Motivation and Context

Contributes to #14480

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


Description

  • Introduced source-generated JSON serialization for DevTools types.

  • Added JsonSerializerContext for improved serialization performance.

  • Ignored DomMutationData.Element during JSON operations.

  • Replaced generic serialization calls with context-specific serialization.


Changes walkthrough 📝

Relevant files
Enhancement
DevToolsSession.cs
Use context-specific JSON serialization in DevToolsSession

dotnet/src/webdriver/DevTools/DevToolsSession.cs

  • Replaced generic JSON serialization with context-specific
    serialization.
  • Added DevToolsSessionSerializerContext for DevTools types.
  • Improved logging and serialization performance.
  • +7/-2     
    DomMutationData.cs
    Ignore `Element` property during JSON serialization           

    dotnet/src/webdriver/DomMutationData.cs

  • Added JsonIgnore attribute to Element property.
  • Prevented serialization of Element during JSON operations.
  • +1/-0     
    JavaScriptEngine.cs
    Use context-specific JSON deserialization in JavaScriptEngine

    dotnet/src/webdriver/JavaScriptEngine.cs

  • Replaced generic JSON deserialization with context-specific
    deserialization.
  • Added JsonEngineSerializerContext for DomMutationData.
  • +5/-1     

    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

    qodo-merge-pro bot commented Jan 26, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit de38b25)

    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

    Error Handling

    The JSON serialization/deserialization operations do not have explicit error handling. Consider adding try-catch blocks to handle potential serialization exceptions gracefully.

    string contents = JsonSerializer.Serialize(message, DevToolsSessionSerializerContext.Default.DevToolsCommandData);
    Null Check

    Missing null check for valueChangeData after deserialization before accessing its properties. This could lead to NullReferenceException if deserialization fails.

    var locator = By.CssSelector($"*[data-__webdriver_id='{valueChangeData.TargetId}']");
    valueChangeData.Element = driver.FindElements(locator).FirstOrDefault();

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 26, 2025

    PR Code Suggestions ✨

    Latest suggestions up to de38b25
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add JSON serialization error handling

    Add error handling for JSON serialization to handle potential serialization
    exceptions that could occur during command serialization.

    dotnet/src/webdriver/DevTools/DevToolsSession.cs [287-289]

    -string contents = JsonSerializer.Serialize(message, DevToolsSessionSerializerContext.Default.DevToolsCommandData);
    -this.pendingCommands.TryAdd(message.CommandId, message);
    -await this.connection.SendData(contents).ConfigureAwait(false);
    +try {
    +    string contents = JsonSerializer.Serialize(message, DevToolsSessionSerializerContext.Default.DevToolsCommandData);
    +    this.pendingCommands.TryAdd(message.CommandId, message);
    +    await this.connection.SendData(contents).ConfigureAwait(false);
    +} catch (JsonException ex) {
    +    LogError("Failed to serialize DevTools command: {0}", ex.Message);
    +    throw;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Adding error handling for JSON serialization is crucial for robustness, as serialization failures could occur with malformed data. The suggestion properly logs the error and maintains the exception chain.

    Medium
    Add null check after deserialization
    Suggestion Impact:The commit implements null safety check for DomMutationData but in a different way - using null-coalescing operator with throw instead of an if-check

    code diff:

    +                DomMutationData valueChangeData = JsonSerializer.Deserialize(e.Payload, JsonEngineSerializerContext.Default.DomMutationData) ?? throw new JsonException("DomMutationData returned null");

    Add null check for the deserialized DomMutationData before accessing its
    properties to prevent potential NullReferenceException.

    dotnet/src/webdriver/JavaScriptEngine.cs [402-403]

     DomMutationData valueChangeData = JsonSerializer.Deserialize(e.Payload, JsonEngineSerializerContext.Default.DomMutationData);
    +if (valueChangeData == null) {
    +    return;
    +}
     var locator = By.CssSelector($"*[data-__webdriver_id='{valueChangeData.TargetId}']");
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion adds important null-safety check to prevent NullReferenceException when deserialization fails. This is a critical defensive programming practice for handling potentially invalid JSON data.

    Medium
    Learned
    best practice
    Add null validation checks before deserializing JSON data and accessing its properties to prevent NullReferenceExceptions

    Add null validation for rawVersionInfo before deserializing it to prevent
    potential NullReferenceException. Use ArgumentNullException.ThrowIfNull() to
    validate the input.

    dotnet/src/webdriver/DevTools/DevToolsSession.cs [414-415]

    +ArgumentNullException.ThrowIfNull(rawVersionInfo, nameof(rawVersionInfo));
     var versionInfo = JsonSerializer.Deserialize(rawVersionInfo, DevToolsSessionSerializerContext.Default.DevToolsVersionInfo);
    +ArgumentNullException.ThrowIfNull(versionInfo, nameof(versionInfo));
     this.websocketAddress = versionInfo.WebSocketDebuggerUrl;
    • Apply this suggestion
    Low

    Previous suggestions

    Suggestions up to commit 2d241d0
    CategorySuggestion                                                                                                                                    Score
    General
    Add JSON serialization options

    The DevToolsSerializerContext should be marked with JsonSourceGenerationOptions to
    ensure consistent serialization behavior across the DevTools types.

    dotnet/src/webdriver/DevTools/Json/DevToolsJsonOptions.cs [37-40]

     [JsonSerializable(typeof(DevToolsCommandData))]
     [JsonSerializable(typeof(DevToolsVersionInfo))]
     [JsonSerializable(typeof(DomMutationData))]
    +[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)]
     internal sealed partial class DevToolsSerializerContext : JsonSerializerContext;
    Suggestion importance[1-10]: 7

    Why: Adding JsonSourceGenerationOptions with CamelCase naming policy would ensure consistent JSON property naming across all DevTools types, improving the robustness of the serialization process and maintaining compatibility with the DevTools protocol.

    7

    @RenderMichael RenderMichael changed the title [dotnet] Use source-generated JSON serialization for DevTools types [dotnet] Make the devtools SendCommand method AOT-safe Jan 29, 2025
    @RenderMichael RenderMichael changed the base branch from trunk to dotnet-devtools-aot January 31, 2025 16:23
    @RenderMichael RenderMichael changed the base branch from dotnet-devtools-aot to trunk January 31, 2025 22:45
    @RenderMichael RenderMichael deleted the devtools-json branch February 1, 2025 05:41
    @RenderMichael RenderMichael restored the devtools-json branch February 10, 2025 16:23
    @RenderMichael RenderMichael reopened this Feb 10, 2025
    @RenderMichael
    Copy link
    Contributor Author

    @nvborisenko re-opening this because of a remark I noticed in #13994:

    We'll still allow access to lower level CDP functionality (see #13990), but the high level approach will be removed.

    This is the lower-level CDP functionality: sending string commandName + JsonNode parameters and getting back JsonElement? response. For that reason, this method will not be depreciated and should support AOT (minimal effort to do so, as this PR shows).

    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.

    2 participants