Revert "[dotnet] [bidi] Make cookie expiry as TimeSpan"

[nvborisenko]

User description

Reverts #16204

Firefox is still good, expiry is just DateTime. Chrome works unexpectedly, needs further investigation or post an issue.

---

PR Type

Bug fix

---

Description

• Reverts cookie expiry from TimeSpan to DateTimeOffset

• Removes TimeSpanConverter from BiDi communication

• Fixes Chrome compatibility issues with cookie handling

• Enhances DateTimeOffsetConverter to handle double values

---

Diagram Walkthrough

flowchart LR
  A["TimeSpan Cookie Expiry"] -- "revert" --> B["DateTimeOffset Cookie Expiry"]
  C["TimeSpanConverter"] -- "remove" --> D["Enhanced DateTimeOffsetConverter"]
  B --> E["Chrome Compatibility Fixed"]

Loading

File Walkthrough

Relevant files

Bug fix

Broker.cs Remove TimeSpanConverter registration                                        dotnet/src/webdriver/BiDi/Communication/Broker.cs Removes TimeSpanConverter from converter list +0/-1      DateTimeOffsetConverter.cs Enhanced DateTimeOffset converter for Chrome                          dotnet/src/webdriver/BiDi/Communication/Json/Converters/DateTimeOffsetConverter.cs Adds handling for double values in JSON reading Implements workaround for Chrome's double expiry format +10/-1    TimeSpanConverter.cs Delete TimeSpanConverter implementation                                    dotnet/src/webdriver/BiDi/Communication/Json/Converters/TimeSpanConverter.cs Completely removes TimeSpanConverter class file +0/-44    Cookie.cs Revert cookie expiry to DateTimeOffset                                      dotnet/src/webdriver/BiDi/Network/Cookie.cs Changes Expiry from TimeSpan to DateTimeOffset property Adds JsonInclude attribute for serialization +6/-1

---

[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 Possible Issue The converter treats numeric JSON values as Unix milliseconds, but Chrome's example looks like Unix seconds with fractional component. Converting the double to Int64 and then calling FromUnixTimeMilliseconds may produce incorrect dates (1000x off). Confirm the expected unit from BiDi/Chrome and convert accordingly (seconds vs milliseconds). // Workaround: it should be Int64, chrome uses double for `expiry` like "expiry":1737379944.308351 if (reader.TryGetInt64(out long unixTime) is false) { var doubleValue = reader.GetDouble(); unixTime = Convert.ToInt64(doubleValue); } return DateTimeOffset.FromUnixTimeMilliseconds(unixTime); } Serialization Shape Switching to a record with an internal-set nullable DateTimeOffset Expiry and [JsonInclude] relies on property-based deserialization. Verify other properties are still serialized/deserialized as expected and that Expiry naming matches the BiDi protocol ('expiry' vs 'Expiration') and timezone semantics. public sealed record Cookie(string Name, BytesValue Value, string Domain, string Path, long Size, bool HttpOnly, bool Secure, SameSite SameSite) { [JsonInclude] public DateTimeOffset? Expiry { get; internal set; } }

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix seconds vs milliseconds parsing The converter assumes the incoming number is milliseconds, but the comment and example value suggest seconds with fractional part. Converting the double directly to Int64 will misinterpret seconds as milliseconds, shifting dates by orders of magnitude. Detect doubles and convert seconds to milliseconds before constructing the DateTimeOffset. dotnet/src/webdriver/BiDi/Communication/Json/Converters/DateTimeOffsetConverter.cs [30-39] -// Workaround: it should be Int64, chrome uses double for `expiry` like "expiry":1737379944.308351 +// Workaround: Chrome sometimes uses double seconds for `expiry` like 1737379944.308351. +// Accept both integer milliseconds and double seconds. +if (reader.TokenType == JsonTokenType.Number) +{ + if (reader.TryGetInt64(out long millis)) + { + return DateTimeOffset.FromUnixTimeMilliseconds(millis); + } -if (reader.TryGetInt64(out long unixTime) is false) -{ - var doubleValue = reader.GetDouble(); - - unixTime = Convert.ToInt64(doubleValue); + // Treat as seconds with fractional component; convert to milliseconds. + double seconds = reader.GetDouble(); + long millisFromSeconds = Convert.ToInt64(Math.Round(seconds * 1000.0, MidpointRounding.AwayFromZero)); + return DateTimeOffset.FromUnixTimeMilliseconds(millisFromSeconds); } -return DateTimeOffset.FromUnixTimeMilliseconds(unixTime); +throw new JsonException("Expected numeric Unix time for DateTimeOffset."); Apply / Chat Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical bug where the PR code misinterprets a Unix timestamp in seconds as milliseconds, leading to an incorrect date. The proposed fix correctly handles the conversion, which is essential for the feature's correctness.	High
	Ensure Expiry deserializes correctly Using an init-only or public setter is required for JsonSerializer to set the property during deserialization; an internal setter with only JsonInclude may prevent population depending on options. Expose a public setter limited to init-time to keep immutability while enabling deserialization. dotnet/src/webdriver/BiDi/Network/Cookie.cs [25-29] public sealed record Cookie(string Name, BytesValue Value, string Domain, string Path, long Size, bool HttpOnly, bool Secure, SameSite SameSite) { [JsonInclude] - public DateTimeOffset? Expiry { get; internal set; } + public DateTimeOffset? Expiry { get; init; } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that using init is a more idiomatic and robust way to handle deserialization for immutable record properties, improving maintainability and aligning with modern C# practices.	Medium
Learned best practice	Validate JSON token types Validate the JSON token type and handle non-numeric tokens gracefully to avoid exceptions. Use a switch on TokenType and throw a JsonException with context when the token is invalid. Also guard against negative or out-of-range values before converting. dotnet/src/webdriver/BiDi/Communication/Json/Converters/DateTimeOffsetConverter.cs [32-39] -if (reader.TryGetInt64(out long unixTime) is false) +if (!reader.TryGetInt64(out long unixTime)) { - var doubleValue = reader.GetDouble(); - - unixTime = Convert.ToInt64(doubleValue); + if (reader.TokenType == JsonTokenType.Number) + { + var doubleValue = reader.GetDouble(); + if (doubleValue < 0) + { + throw new JsonException("Expected non-negative Unix time in milliseconds."); + } + unixTime = Convert.ToInt64(doubleValue); + } + else + { + throw new JsonException($"Expected number token for Unix time, but got {reader.TokenType}."); + } } -return DateTimeOffset.FromUnixTimeMilliseconds(unixTime); +try +{ + return DateTimeOffset.FromUnixTimeMilliseconds(unixTime); +} +catch (ArgumentOutOfRangeException ex) +{ + throw new JsonException($"Invalid Unix time in milliseconds: {unixTime}.", ex); +} Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Add input validation and handle unexpected JSON token types to prevent runtime exceptions.	Low
More

[nvborisenko]

Cookie expiry is unix epoch seconds. Let me prepare the fix.