[dotnet] [bidi] Stateless converters with hydration

[nvborisenko]

User description

Finally resolving issues with BiDi AOT trimming.

🔗 Related Issues

Related to #16095

💥 What does this PR do?

• Removes stateful json converters
• Keeping continuation feature as designed initially

🔧 Implementation Notes

• Removed BiDi instance in DTO ctor

💡 Additional Considerations

Now assembly is fully trimmable, as we wanted.

🔄 Types of changes

• Cleanup (formatting, renaming)
• Bug fix (backwards compatible)
• New feature (non-breaking change which adds functionality and tests!)

---

PR Type

Bug fix, Enhancement

---

Description

• Removes stateful JSON converters to enable AOT trimming

• Implements hydration pattern for BiDi property injection

• Converts JsonSerializerContext to static singletons

• Adds IBiDiHydratable interface for post-deserialization setup

---

Diagram Walkthrough

flowchart LR
  A["Stateful Converters<br/>with BiDi instance"] -->|Remove| B["Stateless Converters"]
  B -->|Hydrate after| C["IBiDiHydratable.Hydrate"]
  C -->|Set BiDi property| D["DTO instances"]
  E["JsonSerializerContext<br/>instance field"] -->|Convert to| F["Static singleton"]
  F -->|Use Default| G["AOT-trimmed assembly"]

Loading

File Walkthrough

	Relevant files
Enhancement	20 files IBiDiHydratable.cs New interface for post-deserialization hydration                  +25/-0    Broker.cs Hydrate command results and event args                                      +10/-0    EventArgs.cs Implement IBiDiHydratable for event args                                  +6/-1      BrowserModule.cs Convert context to static singleton with attributes            +10/-7    CreateUserContextCommand.cs Implement IBiDiHydratable for result                                          +7/-1      BrowsingContextModule.cs Convert context to static singleton with attributes            +10/-7    CreateCommand.cs Implement IBiDiHydratable for result                                          +7/-1      GetTreeCommand.cs Implement IBiDiHydratable for tree result                                +10/-1    AddDataCollectorCommand.cs Implement IBiDiHydratable for result                                          +7/-1      AddInterceptCommand.cs Implement IBiDiHydratable for result                                          +7/-1      NetworkModule.cs Convert context to static singleton with attributes            +10/-7    AddPreloadScriptCommand.cs Implement IBiDiHydratable for result                                          +7/-2      ScriptModule.cs Convert context to static singleton with attributes            +10/-7    EmulationModule.cs Convert context to static singleton with attributes            +10/-7    InputModule.cs Convert context to static singleton with attributes            +10/-7    LogModule.cs Convert context to static singleton with attributes            +10/-7    PermissionsModule.cs Convert context to static singleton with attributes            +10/-7    SessionModule.cs Convert context to static singleton with attributes            +10/-7    StorageModule.cs Convert context to static singleton with attributes            +10/-7    WebExtensionModule.cs Convert context to static singleton with attributes            +10/-7
Bug fix	20 files BiDi.cs Remove JsonOptions from module creation                                    +13/-13  Module.cs Remove Initialize method and JsonOptions parameter              +1/-7      UserContext.cs Remove BiDi constructor parameter, add hydration                  +8/-5      BrowsingContext.cs Remove BiDi constructor parameter, add hydration                  +4/-3      Collector.cs Remove BiDi constructor parameter, add hydration                  +8/-5      Intercept.cs Remove BiDi constructor parameter, add hydration                  +11/-8    Handle.cs Remove BiDi constructor parameter, add hydration                  +8/-4      InternalId.cs Remove BiDi constructor parameter, add hydration                  +8/-4      PreloadScript.cs Remove BiDi constructor parameter, add hydration                  +8/-5      Realm.cs Remove BiDi constructor parameter, add hydration                  +8/-4      BrowsingContextConverter.cs Remove stateful BiDi instance from converter                          +1/-8      BrowserUserContextConverter.cs Remove stateful BiDi instance from converter                          +1/-8      CollectorConverter.cs Remove stateful BiDi instance from converter                          +1/-8      HandleConverter.cs Remove stateful BiDi instance from converter                          +1/-8      InterceptConverter.cs Remove stateful BiDi instance from converter                          +1/-8      InternalIdConverter.cs Remove stateful BiDi instance from converter                          +1/-8      PreloadScriptConverter.cs Remove stateful BiDi instance from converter                          +1/-8      RealmConverter.cs Remove stateful BiDi instance from converter                          +1/-8      WebExtensionConverter.cs Remove stateful BiDi instance from converter                          +1/-8      Extension.cs Remove BiDi constructor parameter, add hydration                  +8/-5

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Object hydration trust Description: Hydration of IBiDiHydratable types after deserialization sets internal BiDi references based solely on wire data, so if an attacker can influence payloads they could create objects that gain access to the BiDi instance; ensure only trusted channels feed deserialization and that hydration applies only to expected result types. Broker.cs [282-286] Referred Code if (commandResult is IBiDiHydratable bidiHydratable) { bidiHydratable.Hydrate(_bidi); }
Ticket Compliance
🟡	🎫 #16095 🟢 Remove stateful JSON converters that depend on BiDi instance. Use EmptyResult as valid return type for all methods instead of void. 🔴 Move BiDi access to AnyDriver/IWebDriver via AsBiDiAsync, making AnyDriver own BiDi lifecycle and dependency direction Selenium.WebDriver -> Selenium.WebDriver.BiDi. Expose previously hidden methods like AddIntercept as public. Revisit InterceptRequestAsync and related helpers to be extensions in a dedicated namespace, leveraging .NET 10 extension capabilities. Use normal types as event args with appropriate inheritance model. Remove overcomplicated syntax sugar like Result implementing IReadOnlyList, prefer extensions like .GetTreeAsync().AsContexts(), and remove strange JSON converters for these types. Commands should return exact types (e.g., SetCookieAsync returns exact type) instead of wrapper Result objects. Make command options immutable objects. Decide on scoped event handlers (remove or keep). Catch-all for any further finalizations to stabilize public API. ⚪ None
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing auditing: Critical command/result processing lacks explicit audit logging of actions, user/context, and outcomes in the new hydration path. Referred Code if (commandResult is IBiDiHydratable bidiHydratable) { bidiHydratable.Hydrate(_bidi); } command.TaskCompletionSource.SetResult((EmptyResult)commandResult); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Hydration errors: The new IBiDiHydratable.Hydrate call is inside the try block but lacks dedicated null/exception handling or fallback, which may swallow hydration-specific context. Referred Code if (commandResult is IBiDiHydratable bidiHydratable) { bidiHydratable.Hydrate(_bidi); } command.TaskCompletionSource.SetResult((EmptyResult)commandResult); } catch (Exception ex) { command.TaskCompletionSource.SetException(ex); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Minimal parsing: New stateless converters accept string IDs without visible validation or sanitization, relying on upstream trust; security relevance depends on external input sources. Referred Code public override BrowsingContext.BrowsingContext? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { var id = reader.GetString(); return new BrowsingContext.BrowsingContext(id!); } public override void Write(Utf8JsonWriter writer, BrowsingContext.BrowsingContext value, JsonSerializerOptions options) { Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Hydrate event arguments after deserialization Add hydration logic for deserialized event arguments in the ProcessReceivedMessage method, similar to how command results are handled, to prevent runtime errors. dotnet/src/webdriver/BiDi/Broker.cs [282-285] +if (commandResult is IBiDiHydratable bidiHydratable) +{ + bidiHydratable.Hydrate(_bidi); +} - Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug where event arguments are not hydrated, which would lead to NullReferenceException at runtime, defeating a major goal of the PR.	High
General	Allow derived classes to extend hydration Make the Hydrate method in the EventArgs base class virtual and protected to allow derived classes to override it and hydrate their own nested properties. dotnet/src/webdriver/BiDi/EventArgs.cs [24-33] public abstract record EventArgs : IBiDiHydratable { [JsonIgnore] public BiDi BiDi { get; internal set; } - void IBiDiHydratable.Hydrate(BiDi bidi) + protected virtual void Hydrate(BiDi bidi) { BiDi = bidi; } + + void IBiDiHydratable.Hydrate(BiDi bidi) + { + Hydrate(bidi); + } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion provides a robust and extensible pattern for hydrating nested objects within event arguments, which is a necessary improvement to the hydration mechanism introduced in the PR.	Medium
Learned best practice	Eliminate dead JSON options Remove the now-unused JsonSerializerOptions method and inline config; prefer the new source-generated contexts to avoid dead code and duplication. dotnet/src/webdriver/BiDi/BiDi.cs [95-117] -return new JsonSerializerOptions -{ - //PropertyNameCaseInsensitive = true, - //PropertyNamingPolicy = JsonNamingPolicy.CamelCase, - //DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, +// Remove GetJsonOptions() entirely and references to it. +// Modules now use source-generated contexts (e.g., BrowsingContextJsonSerializerContext.Default) - // BiDi returns special numbers such as "NaN" as strings - // Additionally, -0 is returned as a string "-0" - NumberHandling = JsonNumberHandling.AllowNamedFloatingPointLiterals | JsonNumberHandling.AllowReadingFromString, - Converters = - { - //new BrowsingContextConverter(), - //new BrowserUserContextConverter(), - //new CollectorConverter(), - //new InterceptConverter(), - //new HandleConverter(), - //new InternalIdConverter(), - //new PreloadScriptConverter(), - //new RealmConverter(), - new DateTimeOffsetConverter(), - //new WebExtensionConverter(), - } -}; - Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Pattern 4: Centralize shared configuration logic to avoid duplication and ensure consistency (e.g., JSON options/converters).	Low
Update

[RenderMichael]

Better [ClsCompliant(false)] I think. You’ll probably run into dotnet/roslyn#68560 though.

[nvborisenko]

We are CLS compliant:

selenium/dotnet/src/webdriver/GlobalSuppressions.cs

Line 29 in a94820f

[assembly: System.CLSCompliant(true)]

[RenderMichael]

Do we have any types with properties that are BiDi hydratable? We might need to handle nested structures.

[nvborisenko]

Yes, provided small example: https://github.com/SeleniumHQ/selenium/pull/16670/files#diff-af620e59fd946547606bfa939dfd78078129c6ebd737e8b24d958fcf0ff738a9

[nvborisenko]

I hydrated couple just to make tests green. If we like the approach, I will review all top-level CommndResults/EventArgs to include nested properties.

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Deserialization hydration risk Description: The hydration hook allows any deserialized type implementing IBiDiHydratable to receive a BiDi instance, which could be abused if untrusted payloads were ever deserialized; ensure inputs come only from trusted BiDi endpoints and IBiDiHydratable is confined to internal types. Broker.cs [279-288] Referred Code var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo) ?? throw new JsonException("Remote end returned null command result in the 'result' property."); if (commandResult is IBiDiHydratable bidiHydratable) { bidiHydratable.Hydrate(_bidi); } command.TaskCompletionSource.SetResult((EmptyResult)commandResult); }
	Event hydration surface Description: Event args deserialization hydrates any IBiDiHydratable instance from incoming events; if event payloads were spoofable this could lead to unintended object graph mutation, so validate source authenticity and restrict IBiDiHydratable to SDK types. Broker.cs [310-316] Referred Code var eventArgs = (EventArgs)JsonSerializer.Deserialize(ref paramsReader, eventInfo)!; if (eventArgs is IBiDiHydratable bidiHydratable) { bidiHydratable.Hydrate(_bidi); }
Ticket Compliance
🟡	🎫 #16095 🟢 Remove stateful JSON converters; avoid converters depending on BiDi instance. Use EmptyResult as a valid return type instead of void across methods. 🔴 Move BiDi extension method AsBiDiAsync() to IWebDriver/AnyDriver as the owner of BiDi, ensuring lifecycle ownership and dependency direction Selenium.WebDriver -> Selenium.WebDriver.BiDi. Revisit InterceptRequestAsync and related APIs; move helpers/extensions to a dedicated namespace using .NET 10 extension capabilities. Use normal types as event args (inheritance topic). Remove IReadOnlyList sugar from Result types and associated custom JSON converters; provide extensions like .GetTreeAsync().AsContexts(). Commands should return the exact type (Result) instead of wrapping/other objects; review all commands accordingly. Make command options immutable after construction. Decide on scoped event handlers: remove or keep. Address any additional big design changes as needed to finalize the public API. ⚪ Expose previously hidden internal methods like AddIntercept() as public.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Event Hydration Unlogged: New hydration steps for results and events add critical post-deserialization actions without any accompanying audit logging, which may obscure tracing of these critical BiDi interactions. Referred Code if (commandResult is IBiDiHydratable bidiHydratable) { bidiHydratable.Hydrate(_bidi); } command.TaskCompletionSource.SetResult((EmptyResult)commandResult); } catch (Exception ex) { command.TaskCompletionSource.SetException(ex); } finally { _pendingCommands.TryRemove(id.Value, out _); } } else { throw new BiDiException($"The remote end responded with 'success' message type, but no pending command with id {id} was found."); } ... (clipped 14 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Null Result Handling: While null results throw JsonException, the new hydration step does not guard against partial/invalid object graphs (e.g., missing IDs) before setting BiDi, which could lead to late failures without explicit handling. Referred Code var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo) ?? throw new JsonException("Remote end returned null command result in the 'result' property."); if (commandResult is IBiDiHydratable bidiHydratable) { bidiHydratable.Hydrate(_bidi); } command.TaskCompletionSource.SetResult((EmptyResult)commandResult); } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Minimal Validation: New stateless converters and constructors accept string IDs without validating format or emptiness, which could allow invalid external input to propagate if upstream validation is absent. Referred Code public override BrowsingContext.BrowsingContext? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { var id = reader.GetString(); return new BrowsingContext.BrowsingContext(id!); } public override void Write(Utf8JsonWriter writer, BrowsingContext.BrowsingContext value, JsonSerializerOptions options) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	✅ Add runtime checks for hydration Suggestion Impact: The commit introduced private nullable backing fields and updated the BiDi property in both UserContext and BrowsingContext to throw InvalidOperationException when not hydrated, matching the suggested pattern. code diff: + private BiDi? _bidi; + internal UserContext(string id) { Id = id; @@ -35,7 +37,11 @@ internal string Id { get; } [JsonIgnore] - public BiDi BiDi { get; internal set; } + public BiDi BiDi + { + get => _bidi ?? throw new InvalidOperationException($"{nameof(BiDi)} instance has not been hydrated."); + internal set => _bidi = value; + } public Task RemoveAsync() { # File: dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs @@ -41,8 +41,14 @@ internal string Id { get; } - [JsonIgnore] - public BiDi BiDi { get; internal set; } + private BiDi? _bidi; + + [JsonIgnore] + public BiDi BiDi + { + get => _bidi ?? throw new InvalidOperationException("BiDi instance has not been hydrated."); + internal set => _bidi = value; + } To prevent NullReferenceExceptions, add a runtime check to the BiDi property getter on objects using the new hydration pattern. This check should throw an InvalidOperationException if the property is accessed before it has been hydrated. Examples: dotnet/src/webdriver/BiDi/Browser/UserContext.cs [38] public BiDi BiDi { get; internal set; } dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [45] public BiDi BiDi { get; internal set; } Solution Walkthrough: Before: // In dotnet/src/webdriver/BiDi/Browser/UserContext.cs public sealed class UserContext : IAsyncDisposable { // ... constructor ... [JsonIgnore] public BiDi BiDi { get; internal set; } public Task RemoveAsync() { // This will throw NullReferenceException if BiDi is not hydrated return BiDi.Browser.RemoveUserContextAsync(this); } // ... } After: // In dotnet/src/webdriver/BiDi/Browser/UserContext.cs public sealed class UserContext : IAsyncDisposable { private BiDi? _bidi; // ... constructor ... [JsonIgnore] public BiDi BiDi { get => _bidi ?? throw new InvalidOperationException("BiDi instance has not been hydrated."); internal set => _bidi = value; } public Task RemoveAsync() { return BiDi.Browser.RemoveUserContextAsync(this); } // ... } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant design weakness in the new hydration pattern, preventing potential NullReferenceExceptions and improving API robustness with explicit, fail-fast checks.	Medium
General	Improve interface accessibility for testability Change the Hydrate method in the IBiDiHydratable interface from internal to public to allow implementation and mocking outside the assembly. dotnet/src/webdriver/BiDi/IBiDiHydratable.cs [22-25] public interface IBiDiHydratable { - internal void Hydrate(BiDi bidi); + void Hydrate(BiDi bidi); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that an internal method in a public interface limits its utility for consumers, particularly for mocking and testing, and proposes making it public to improve testability.	Low
More

[nvborisenko]

We are duplicating shared json options. Is it good? Or do we want to return back the construction of json context in runtime rather than generated options?

[nvborisenko]

Is it safe to remove runtime options completely? As I remember we implemented it as converter?..

[nvborisenko]

If we choose generated options, then rename -> s_jsonContext

[nvborisenko]

I successfully implemented 2 variants:

1. In general the first approach is new MyJsonSerializerContext(somEOptions) - as soon as we do it we are getting +2MB into assembly size with AOT trimming.
2. Be fully json generated. In this case assembly is slim, fully trimmed unnecessary unused types. But in this case hydration process will look like:

Task<Result> DoSomethingAsync()
{
  var result = await Broker.ExecuteCommandAsync(....);
  result.Hydrate(); // walk through all nested object properties and set `BiDi` instance
  return result;
}

The 2nd variant is technically ideal, but it requires a lot of boilerplate code. It also introduces new rule for us.