Skip to content

[dotnet] Switch DevTools response JSON parsing to JsonElement #14990

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 7, 2025
Merged

[dotnet] Switch DevTools response JSON parsing to JsonElement #14990

merged 3 commits into from
Jan 7, 2025
+27 −20

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 30, 2024 •
edited by qodo-merge-pro bot
Loading

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

    @qodo-merge-pro Qodo Merge Pro
    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();
}

    @qodo-merge-pro Qodo Merge Pro
    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

    @nvborisenko
    Copy link
    Member

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

    I still don't see how, can you please elaborate more?

    it doesn't throw exceptions on invalid UTF-16 before the deserialization converters apply

    00:29:37.899  WARN DevToolsSession: CDP VNT ^^ Unhandled error occured in event handler of 'Fetch.requestPaused' method. System.Text.Json.JsonException: The JSON value could not be converted to System.String. Path: $.request.postData | LineNumber: 0 | BytePositionInLine: 419706.
 ---> System.InvalidOperationException: Cannot read incomplete UTF-16 JSON text as string with missing low surrogate.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ReadIncompleteUTF16()
   at System.Text.Json.JsonReaderHelper.TryUnescape(ReadOnlySpan`1 source, Span`1 destination, Int32 idx, Int32& written)
   at System.Text.Json.JsonReaderHelper.GetUnescapedString(ReadOnlySpan`1 utf8Source)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value, Boolean& isPopulatedValue)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value, Boolean& isPopulatedValue)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   --- End of inner exception stack trace ---
   at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& state, Utf8JsonReader& reader, Exception ex)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.DeserializeAsObject(Utf8JsonReader& reader, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadFromSpanAsObject(ReadOnlySpan`1 utf8Json, JsonTypeInfo jsonTypeInfo, Nullable`1 actualByteCount)
   at System.Text.Json.JsonSerializer.Deserialize(JsonElement element, Type returnType, JsonSerializerOptions options)
   at OpenQA.Selenium.DevTools.V131.Fetch.FetchAdapter.OnDevToolsEventReceived(Object sender, DevToolsEventReceivedEventArgs e) in D:\SeleniumHQ\selenium\bazel-bin\dotnet\src\webdriver\cdp\v131\Fetch\FetchAdapter.cs:line 151
   at OpenQA.Selenium.DevTools.DevToolsSession.OnDevToolsEventReceived(DevToolsEventReceivedEventArgs e) in D:\SeleniumHQ\selenium\dotnet\src\webdriver\DevTools\DevToolsSession.cs:line 641
   at OpenQA.Selenium.DevTools.DevToolsSession.<>c__DisplayClass52_0.<ProcessMessage>b__0() in D:\SeleniumHQ\selenium\dotnet\src\webdriver\DevTools\DevToolsSession.cs:line 618

    @RenderMichael
    Copy link
    Contributor Author

    JsonElement with invalid JSON contents will allow custom converters to run before the exception, JsonNode does not.

    This PR is a first step towards handling the invalid JSON which CDP sometimes returns.

    @nvborisenko
    Copy link
    Member

    When I was migrating from Newtonsoft.Json to System.Text.Json I chose JsonNode as more similar class to avoid big code changes. Seems JsonElement is more fundamental, let's try to use it.

    nvborisenko
    nvborisenko approved these changes Jan 7, 2025
    View reviewed changes
    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

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

    Thx @RenderMichael for your contribution!

    @nvborisenko nvborisenko merged commit c8aa015 into SeleniumHQ:trunk Jan 7, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the devtools-json branch January 8, 2025 02:54
    yvsvarma pushed a commit to yvsvarma/selenium that referenced this pull request Jan 9, 2025
    @RenderMichael @yvsvarma
    [dotnet] Switch DevTools response JSON parsing to JsonElement (Sele…
    b4bd0f5 
    …niumHQ#14990)
    yvsvarma pushed a commit to yvsvarma/selenium that referenced this pull request Jan 11, 2025
    @RenderMichael @yvsvarma
    [dotnet] Switch DevTools response JSON parsing to JsonElement (Sele…
    1144969 
    …niumHQ#14990)
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @RenderMichael @sandeepsuryaprasad
    [dotnet] Switch DevTools response JSON parsing to JsonElement (Sele…
    72ddde3 
    …niumHQ#14990)
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @nvborisenko nvborisenko nvborisenko approved these changes

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 3
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @RenderMichael @nvborisenko