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] Switch DevTools response JSON parsing to JsonElement #14990

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

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 30, 2024

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

Switches JSON response parsing in DevTools from mutable JsonNode to immutable JsonElement.

Motivation and Context

In general, JsonElement is preferred if the JSON is not being mutated. But the true motivation is from #14972. JsonElement is a lot more forgiving of imperfect json (it allows multiple properties with the same name, it doesn't throw exceptions on invalid UTF-16 before the deserialization converters apply, etc).

This will allow us to Eventually fortify the code against invalid JSON, which is the root cause of #14903.

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

  • Switches JSON response parsing in DevTools from mutable JsonNode to immutable JsonElement across multiple files
  • Improves JSON handling robustness by using JsonElement which is more forgiving of imperfect JSON
  • Updates error handling to use proper JsonElement property access methods
  • Fixes file upload issues when using Network.StartMonitoring() by addressing JSON parsing issues
  • Adds better handling of invalid UTF-16 and duplicate property names in JSON responses

Changes walkthrough 📝

Relevant files
Enhancement
DevToolsCommandData.cs
Switch Result property to JsonElement type                             

dotnet/src/webdriver/DevTools/DevToolsCommandData.cs

  • Changed Result property type from JsonNode to JsonElement
  • Added System.Text.Json namespace import
  • +2/-1     
    DevToolsSession.cs
    Refactor JSON handling to use JsonElement                               

    dotnet/src/webdriver/DevTools/DevToolsSession.cs

  • Changed JSON parsing to use JsonElement instead of JsonNode
  • Updated error handling to use JsonElement methods
  • Modified message processing to use JsonDocument and JsonElement
  • +20/-16 
    DevToolsSessionEventReceivedEventArgs.cs
    Update EventData to use JsonElement                                           

    dotnet/src/webdriver/DevTools/DevToolsSessionEventReceivedEventArgs.cs

  • Changed EventData property type from JsonNode to JsonElement
  • Updated constructor parameter type to match
  • +3/-2     
    IDevToolsSession.cs
    Update SendCommand return type to JsonElement                       

    dotnet/src/webdriver/DevTools/IDevToolsSession.cs

  • Changed return type of SendCommand from JsonNode to JsonElement?
  • Added System.Text.Json namespace import
  • +2/-1     

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    14903 - Partially compliant

    Compliant requirements:

    • None yet, this PR is a preparatory refactoring step

    Non-compliant requirements:

    • Fix file upload failures when IWebDriver.Manage().Network.StartMonitoring() is called
    • Support chunked file uploads (files > 4MB)

    Requires further human verification:

    • Testing with large file uploads (>4MB) to verify the fix works
    • Testing with Chrome browser version 131.0.6778.140
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The new JsonElement error handling code needs careful review to ensure all error cases are properly handled, especially around null checks and property access

    var errorMessage = modified.Result.GetProperty("message").GetString();
    var errorData = modified.Result.TryGetProperty("data", out var data) ? data.GetString() : null;
    
    var exceptionMessage = $"{commandName}: {errorMessage}";
    if (!string.IsNullOrWhiteSpace(errorData))
    {
        exceptionMessage = $"{exceptionMessage} - {errorData}";
    }
    
    LogTrace("Recieved Error Response {0}: {1} {2}", modified.CommandId, message, errorData);
    throw new CommandResponseException(exceptionMessage)
    {
        Code = modified.Result.TryGetProperty("code", out var code) ? code.GetInt64() : -1
    };
    Memory Management

    Verify proper disposal of JsonDocument in ProcessMessage method to prevent memory leaks

    using (var doc = JsonDocument.Parse(message))
    {
        messageObject = doc.RootElement.Clone();
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add defensive programming to handle missing JSON properties safely

    Add null checks before accessing properties of JsonElement to prevent potential
    InvalidOperationException when properties don't exist.

    dotnet/src/webdriver/DevTools/DevToolsSession.cs [301-302]

    -var errorMessage = modified.Result.GetProperty("message").GetString();
    +var errorMessage = modified.Result.TryGetProperty("message", out var messageElement) ? messageElement.GetString() ?? string.Empty : string.Empty;
     var errorData = modified.Result.TryGetProperty("data", out var data) ? data.GetString() : null;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves error handling by using TryGetProperty for the 'message' property, preventing potential runtime exceptions when processing malformed JSON responses. This is critical for maintaining application stability.

    8
    Add input validation to prevent processing of invalid JSON messages

    Add validation for empty or malformed JSON messages before parsing to prevent
    potential exceptions.

    dotnet/src/webdriver/DevTools/DevToolsSession.cs [562-563]

    +if (string.IsNullOrWhiteSpace(message))
    +{
    +    LogError("Received empty or invalid message");
    +    return;
    +}
     JsonElement messageObject;
     using (var doc = JsonDocument.Parse(message))
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding validation for empty or malformed messages before parsing is a good defensive programming practice that can prevent crashes and provide better error handling for invalid input.

    7
    General
    Ensure proper resource cleanup with structured exception handling

    Dispose of JsonDocument using a try-finally block to ensure proper cleanup even if
    an exception occurs during processing.

    dotnet/src/webdriver/DevTools/DevToolsSession.cs [563-566]

    -using (var doc = JsonDocument.Parse(message))
    -{
    +JsonDocument doc = null;
    +try {
    +    doc = JsonDocument.Parse(message);
         messageObject = doc.RootElement.Clone();
     }
    +finally {
    +    doc?.Dispose();
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While the existing using statement already ensures proper disposal, the suggested try-finally block is redundant as the using statement provides the same functionality. The improvement is minimal.

    4

    @@ -53,6 +54,6 @@ public DevToolsEventReceivedEventArgs(string domainName, string eventName, JsonN
    /// <summary>
    /// Gets the data with which the event is to be raised.
    /// </summary>
    public JsonNode EventData { get; private set; }
    public JsonElement EventData { get; private set; }
    Copy link
    Contributor Author

    @RenderMichael RenderMichael Dec 30, 2024

    Choose a reason for hiding this comment

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

    This is used all across cdp generated code. Luckily, the generated code looks like var eventArgs = e.EventData.Deserialize(eventData.EventArgsType); which is exactly the same when EventData is either JsonNode or JsonElement.

    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