Home Original page

[dotnet] [bidi] Support network collectors and get response body by nvborisenko · Pull Request #16192 · SeleniumHQ/selenium

Possible issue
Validate JSON token for id

Validate the JSON token type before reading to prevent invalid JSON from causing
unexpected behavior. Throw a JsonException when the token is not a string or
when the id is null/empty.

dotnet/src/webdriver/BiDi/Communication/Json/Converters/CollectorConverter.cs [36-41]

 public override Collector? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
 {
+    if (reader.TokenType != JsonTokenType.String)
+    {
+        throw new JsonException("Expected string token for Collector id.");
+    }
+
     var id = reader.GetString();
+    if (string.IsNullOrEmpty(id))
+    {
+        throw new JsonException("Collector id cannot be null or empty.");
+    }
 
-    return new Collector(_bidi, id!);
+    return new Collector(_bidi, id);
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion significantly improves the robustness of the JSON converter by adding validation for the token type and for a null or empty id, preventing potential exceptions from malformed JSON.

Medium
Make dispose safe and idempotent

Dispose should be resilient and idempotent. Wrap the removal in a try/catch to
avoid throwing during dispose paths and guard against double-dispose to prevent
multiple removal attempts.

dotnet/src/webdriver/BiDi/Network/Collector.cs [42-45]

+private bool _disposed;
+
 public async ValueTask DisposeAsync()
 {
-    await RemoveAsync();
+    if (_disposed) return;
+    _disposed = true;
+    try
+    {
+        await RemoveAsync().ConfigureAwait(false);
+    }
+    catch
+    {
+        // Suppress exceptions on dispose
+    }
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly implements the standard disposable pattern, making DisposeAsync idempotent and preventing exceptions from being thrown, which is a crucial best practice for resource management.

Medium General
Add null-safe explicit cast

Handle null inputs defensively to avoid a NullReferenceException during explicit
cast. Also use the Encoding property directly and consider malformed base64 data
to avoid hard crashes.

dotnet/src/webdriver/BiDi/Network/BytesValue.cs [33-38]

-public static explicit operator string(BytesValue value) => value switch
+public static explicit operator string?(BytesValue? value) => value switch
 {
+    null => null,
     StringBytesValue stringBytesValue => stringBytesValue.Value,
     Base64BytesValue base64BytesValue => System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(base64BytesValue.Value)),
     _ => throw new InvalidCastException($"Cannot cast '{value.GetType()}' to '{typeof(string)}'.")
 };
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly improves the robustness of the explicit cast operator by handling null input, which aligns with standard C# casting behavior and prevents potential NullReferenceException.

Low Learned
best practice
Validate constructor parameters

Validate constructor arguments to prevent invalid state and clearer errors.
Check for null bidi and null/empty id and throw ArgumentNullException or
ArgumentException.

dotnet/src/webdriver/BiDi/Network/Collector.cs [29-33]

 internal Collector(BiDi bidi, string id)
 {
-    _bidi = bidi;
-    Id = id;
+    _bidi = bidi ?? throw new ArgumentNullException(nameof(bidi));
+    Id = !string.IsNullOrEmpty(id) ? id : throw new ArgumentException("Collector id cannot be null or empty.", nameof(id));
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add null checks and validation for parameters before using them to prevent runtime errors.

Low
  • More