[dotnet] [bidi] Serialize base64 encoded string directly to bytes by nvborisenko · Pull Request #16203

[dotnet] [bidi] Serialize base64 encoded string directly to bytes by nvborisenko · Pull Request #16203 · SeleniumHQ/selenium

Possible issue

Preserve base64 on the wire

The JSON discriminator indicates type "base64", but the payload is now raw
bytes; ensure serialization still emits base64 as required by the protocol. Add
a custom converter to encode/decode the ReadOnlyMemory as base64 during JSON
serialization.

dotnet/src/webdriver/BiDi/Network/BytesValue.cs [28-36]

 public abstract record BytesValue
 {
     public static implicit operator BytesValue(string value) => new StringBytesValue(value);
     public static implicit operator BytesValue(byte[] value) => new Base64BytesValue(value);
 }
 
 public sealed record StringBytesValue(string Value) : BytesValue;
 
+[JsonConverter(typeof(Base64BytesValueJsonConverter))]
 public sealed record Base64BytesValue(ReadOnlyMemory<byte> Value) : BytesValue;
 
+internal sealed class Base64BytesValueJsonConverter : JsonConverter<Base64BytesValue>
+{
+    public override Base64BytesValue Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
+    {
+        if (reader.TokenType != JsonTokenType.String)
+        {
+            throw new JsonException("Expected base64 string.");
+        }
+        var bytes = reader.HasValueSequence
+            ? Convert.FromBase64String(reader.GetString()!)
+            : Convert.FromBase64String(reader.GetString()!);
+        return new Base64BytesValue(bytes);
+    }
+
+    public override void Write(Utf8JsonWriter writer, Base64BytesValue value, JsonSerializerOptions options)
+    {
+        writer.WriteStringValue(Convert.ToBase64String(value.Value.Span));
+    }
+}
+

Apply / Chat

Suggestion importance[1-10]: 10

Why: This suggestion correctly identifies a critical bug where changing the data type to ReadOnlyMemory<byte> would break JSON serialization, as the protocol expects a base64 string. The proposed custom JsonConverter is the correct and necessary fix.

High General

Avoid unnecessary byte array copies

Avoid copying the buffer when the underlying memory is already backed by an
array to prevent unnecessary allocations. Use TryGetArray on ReadOnlyMemory to
return the underlying array when possible, otherwise fall back to ToArray().

dotnet/src/webdriver/BiDi/BrowsingContext/CaptureScreenshotCommand.cs [60-65]

 public sealed record CaptureScreenshotResult(ReadOnlyMemory<byte> Data) : EmptyResult
 {
     public static implicit operator byte[](CaptureScreenshotResult captureScreenshotResult) => captureScreenshotResult.ToByteArray();
 
-    public byte[] ToByteArray() => Data.ToArray();
+    public byte[] ToByteArray()
+    {
+#if NETSTANDARD2_0 || NET461
+        return Data.ToArray();
+#else
+        if (MemoryMarshal.TryGetArray(Data, out ArraySegment<byte> segment) && segment.Offset == 0 && segment.Count == segment.Array!.Length)
+        {
+            return segment.Array!;
+        }
+        return Data.ToArray();
+#endif
+    }
 }

Apply / Chat

Suggestion importance[1-10]: 6

Why: The suggestion correctly identifies a potential performance improvement by avoiding an unnecessary array allocation and copy, which aligns with the PR's goal of optimizing byte handling.

Low

Optimize memory-to-array conversion

Mirror the optimized conversion to reduce allocations by returning the
underlying array when available. This keeps behavior consistent with screenshot
conversion and improves performance for large PDFs.

dotnet/src/webdriver/BiDi/BrowsingContext/PrintCommand.cs [115-118]

 public sealed record PrintResult(ReadOnlyMemory<byte> Data) : EmptyResult
 {
-    public byte[] ToByteArray() => Data.ToArray();
+    public byte[] ToByteArray()
+    {
+#if NETSTANDARD2_0 || NET461
+        return Data.ToArray();
+#else
+        if (MemoryMarshal.TryGetArray(Data, out ArraySegment<byte> segment) && segment.Offset == 0 && segment.Count == segment.Array!.Length)
+        {
+            return segment.Array!;
+        }
+        return Data.ToArray();
+#endif
+    }
 }

Apply / Chat

Suggestion importance[1-10]: 6

Why: This suggestion correctly applies the same valid performance optimization from the first suggestion to a similar record, PrintResult, improving both performance and code consistency.

Low Learned
best practice

Add file and data validation

Validate that path exists and bytes is non-empty before attempting to install
the extension. Add guards to surface clear test failures instead of file or
argument exceptions.

dotnet/test/common/BiDi/WebExtension/WebExtensionTest.cs [65-69]

 var path = LocateRelativePath("common/extensions/webextensions-selenium-example.zip");
+Assert.That(File.Exists(path), Is.True, $"Extension archive not found at '{path}'");
 
 var bytes = File.ReadAllBytes(path);
+Assert.That(bytes, Is.Not.Null.And.Length.GreaterThan(0), "Extension archive is empty");
 
 var result = await bidi.WebExtension.InstallAsync(new ExtensionBase64Encoded(bytes));

Apply / Chat

Suggestion importance[1-10]: 6

Why:
Relevant best practice - Add null/argument validation to prevent runtime errors when using external resources.

Low

Update