Home Original page

[dotnet] [bidi] Make cookie expiry as TimeSpan by nvborisenko · Pull Request #16204 · SeleniumHQ/selenium

@nvborisenko

@nvborisenko nvborisenko commented

Aug 18, 2025

•

edited by qodo-code-review bot

Loading

User description

Finally Firefox and Chromium are aligned.

💥 What does this PR do?

Expiry means how long cookie lives in milliseconds.

🔧 Implementation Notes

Added new TimeSpan converter.

🔄 Types of changes

  • Bug fix (backwards compatible)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Bug fix, Enhancement

Description

  • Change cookie expiry from DateTimeOffset to TimeSpan

  • Add TimeSpan JSON converter for BiDi communication

  • Align Firefox and Chromium cookie handling

Diagram Walkthrough

flowchart LR
  A["Cookie.Expiry Property"] --> B["DateTimeOffset → TimeSpan"]
  B --> C["TimeSpanConverter"]
  C --> D["Broker Registration"]
  D --> E["Aligned Browser Support"]
Loading

File Walkthrough
Relevant files
Configuration changes
Broker.cs
Register TimeSpan converter                                                           

dotnet/src/webdriver/BiDi/Communication/Broker.cs

  • Register new TimeSpanConverter in the broker's converter list
 +1/-0     
Enhancement
TimeSpanConverter.cs
Add TimeSpan JSON converter                                                           

dotnet/src/webdriver/BiDi/Communication/Json/Converters/TimeSpanConverter.cs

  • Create new JSON converter for TimeSpan type
  • Handle milliseconds conversion from/to JSON
  • Support both integer and double value parsing
 +44/-0   
Bug fix
Cookie.cs
Update cookie expiry type                                                               

dotnet/src/webdriver/BiDi/Network/Cookie.cs

  • Change Expiry property type from DateTimeOffset to TimeSpan
 +1/-1     

@nvborisenko

@qodo-code-review

PR Reviewer Guide 🔍

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
Precision Loss

Converting a JSON double to milliseconds via Convert.ToInt64 will truncate sub-millisecond precision; verify that protocol values never rely on fractional milliseconds or consider rounding behavior explicitly.

 
public override TimeSpan Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
    if (reader.TryGetInt64(out long milliseconds) is false)
    {
        var doubleValue = reader.GetDouble();

        milliseconds = Convert.ToInt64(doubleValue);
    }

    return TimeSpan.FromMilliseconds(milliseconds);
Semantics Change

Changing cookie expiry from DateTimeOffset? to TimeSpan? alters meaning from absolute time to duration; ensure all serialization, deserialization, and downstream usages (comparisons, logging) are updated accordingly to avoid misinterpretation.

 
public sealed record Cookie(string Name, BytesValue Value, string Domain, string Path, long Size, bool HttpOnly, bool Secure, SameSite SameSite)
{
    [JsonInclude]
    public TimeSpan? Expiry { get; internal set; }
}
Read Validation

Read assumes numeric token; consider handling unexpected token types or nulls to produce clearer errors and align with protocol robustness.

 
public override TimeSpan Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
    if (reader.TryGetInt64(out long milliseconds) is false)
    {
        var doubleValue = reader.GetDouble();

        milliseconds = Convert.ToInt64(doubleValue);
    }

    return TimeSpan.FromMilliseconds(milliseconds);

@qodo-code-review

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Clarify TimeSpan units/precision

The new TimeSpanConverter assumes expiry is milliseconds, but silently converts
doubles to long, potentially truncating sub-millisecond precision and risking
misinterpretation if servers send seconds. Explicitly document and enforce the
expected unit (ms) and consider validating or rejecting non-integer/alternative
units to avoid cross-browser inconsistencies. Ensure all BiDi message
producers/consumers and existing DateTimeOffset usages are migrated to this
contract to prevent subtle expiry drift.

Examples:

dotnet/src/webdriver/BiDi/Communication/Json/Converters/TimeSpanConverter.cs [28-38] 
    public override TimeSpan Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        if (reader.TryGetInt64(out long milliseconds) is false)
        {
            var doubleValue = reader.GetDouble();

            milliseconds = Convert.ToInt64(doubleValue);
        }

        return TimeSpan.FromMilliseconds(milliseconds);

 ... (clipped 1 lines)

Solution Walkthrough:

Before:

public override TimeSpan Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
    if (reader.TryGetInt64(out long milliseconds) is false)
    {
        var doubleValue = reader.GetDouble();

        milliseconds = Convert.ToInt64(doubleValue);
    }

    return TimeSpan.FromMilliseconds(milliseconds);
}

After:

public override TimeSpan Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
    // Per spec, expiry is in whole milliseconds. Rejecting floating-point
    // values prevents silent precision loss and potential unit confusion.
    if (reader.TryGetInt64(out long milliseconds))
    {
        return TimeSpan.FromMilliseconds(milliseconds);
    }

    throw new JsonException("Expected cookie expiry to be an integer representing milliseconds.");
}
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a critical issue where silent truncation of floating-point values in TimeSpanConverter could lead to incorrect cookie expiry times and cross-browser inconsistencies.

Medium
Possible issue
Validate JSON token types

Guard against unexpected token types to avoid throwing when the JSON value is
not a number. Explicitly validate reader.TokenType and throw a descriptive
JsonException if the token isn't an integer or floating-point number.

dotnet/src/webdriver/BiDi/Communication/Json/Converters/TimeSpanConverter.cs [28-38]

 public override TimeSpan Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
 {
-    if (reader.TryGetInt64(out long milliseconds) is false)
+    if (reader.TokenType == JsonTokenType.Number)
     {
-        var doubleValue = reader.GetDouble();
+        if (reader.TryGetInt64(out long milliseconds))
+        {
+            return TimeSpan.FromMilliseconds(milliseconds);
+        }
 
-        milliseconds = Convert.ToInt64(doubleValue);
+        if (reader.TryGetDouble(out double doubleValue))
+        {
+            return TimeSpan.FromMilliseconds(doubleValue);
+        }
     }
 
-    return TimeSpan.FromMilliseconds(milliseconds);
+    throw new JsonException($"Invalid JSON token for TimeSpan: expected number, got {reader.TokenType}.");
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the Read method lacks validation for the JSON token type, which could lead to an InvalidOperationException. The proposed change improves robustness and provides more descriptive error messages by checking the token type and throwing a JsonException for unexpected types.

Medium
General
Normalize milliseconds precision

Ensure consistent numeric precision by writing integral milliseconds when the
value is whole to avoid unnecessary floating-point serialization differences.
This reduces potential protocol mismatches where integers are expected.

dotnet/src/webdriver/BiDi/Communication/Json/Converters/TimeSpanConverter.cs [40-43]

 public override void Write(Utf8JsonWriter writer, TimeSpan value, JsonSerializerOptions options)
 {
-    writer.WriteNumberValue(value.TotalMilliseconds);
+    double totalMs = value.TotalMilliseconds;
+    if (Math.Abs(totalMs % 1) < double.Epsilon)
+    {
+        writer.WriteNumberValue((long)totalMs);
+    }
+    else
+    {
+        writer.WriteNumberValue(totalMs);
+    }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that serializing whole-number milliseconds as integers instead of doubles can improve consistency with protocols that might expect an integer. This is a good practice for ensuring predictable JSON output, making the implementation more robust.

Low
  • Update

@nvborisenko

@nvborisenko

nvborisenko added a commit that referenced this pull request

Aug 20, 2025

This was referenced

Oct 2, 2025

This was referenced

Oct 27, 2025

This was referenced

Dec 9, 2025

This was referenced

Dec 16, 2025

This was referenced

Jan 19, 2026

This was referenced

Feb 16, 2026

This was referenced

Feb 23, 2026