[dotnet] Replace stateful BiDi converters with thread-static BiDi instance

[RenderMichael]

User description

Proposal to replace stateful converters with a thread-static BiDi instance

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)

---

PR Type

Enhancement

---

Description

• Replace stateful converter instances with thread-static BiDi reference

• Remove BiDi constructor parameters from all JSON converters

• Add [JsonConverter] attributes to domain model classes

• Set thread-static BiDi during deserialization in Broker

---

Diagram Walkthrough

flowchart LR
  A["BiDi.cs<br/>Remove converter instances"] --> B["Broker.cs<br/>Add ThreadStaticBiDi"]
  B --> C["Converters<br/>Remove BiDi parameter"]
  C --> D["Domain Models<br/>Add JsonConverter attributes"]
  B -.->|Set during deserialization| C

Loading

File Walkthrough

	Relevant files
Refactoring	19 files BiDi.cs Remove stateful converter instances from JSON options        +0/-9      UserContext.cs Add JsonConverter attribute to UserContext class                  +3/-0      BrowsingContext.cs Add JsonConverter attribute to BrowsingContext class          +2/-0      BrowserUserContextConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      BrowsingContextConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      CollectorConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      HandleConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      InterceptConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      InternalIdConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      PreloadScriptConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      RealmConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      WebExtensionConverter.cs Remove BiDi parameter, use thread-static reference              +2/-8      Collector.cs Add JsonConverter attribute to Collector class                      +3/-0      Intercept.cs Add JsonConverter attribute to Intercept class                      +3/-0      Handle.cs Add JsonConverter attribute to Handle class                            +4/-0      InternalId.cs Add JsonConverter attribute to InternalId class                    +4/-0      PreloadScript.cs Add JsonConverter attribute to PreloadScript class              +3/-0      Realm.cs Add JsonConverter attribute to Realm class                              +4/-0      Extension.cs Add JsonConverter attribute to Extension class                      +3/-0
Enhancement	1 files Broker.cs Add thread-static BiDi instance and setter logic                  +8/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Thread-static state leak Description: Introduction of a thread-static mutable field _threadStaticBiDi can leak state across tasks within the same thread, leading to unintended cross-request data access or race conditions if thread reuse occurs in pooled thread environments. Broker.cs [54-58] Referred Code internal static BiDi? ThreadStaticBiDi => _threadStaticBiDi; [ThreadStatic] private static BiDi? _threadStaticBiDi;
	Stale context on exception Description: Setting _threadStaticBiDi before deserialization and nulling it after can leave the field set if deserialization throws before the nulling line, potentially causing stale BiDi references to be used in later deserializations on the same thread. Broker.cs [285-289] Referred Code _threadStaticBiDi = _bidi; var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo) ?? throw new JsonException("Remote end returned null command result in the 'result' property."); _threadStaticBiDi = null;
Ticket Compliance
🟡	🎫 #1234 🔴 Investigate and fix Firefox BiDi/WebDriver behavior where click() on a link with JavaScript in href does not trigger the JS in Selenium 2.48.x but works in 2.47.1. Provide changes ensuring alerts or JS execution triggered via href are fired correctly in affected Firefox versions. ⚪ Validate behavior specifically on Firefox 42.0 (32-bit) on a 64-bit machine.
🟡	🎫 #5678 🔴 Address intermittent "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu 16.04.4 with Selenium 3.9.0, Chrome 65, ChromeDriver 2.35. Ensure subsequent ChromeDriver instantiations do not log ConnectFailure errors after the first successful instance. Provide reproducible fix or guidance specific to the described environment.
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: Audit logging: The new deserialization flow using thread-static BiDi does not add or reference any logging of critical actions or outcomes, so it is unclear whether audit trails are captured. Referred Code { case "success": if (id is null) throw new JsonException("The remote end responded with 'success' message type, but missed required 'id' property."); if (_pendingCommands.TryGetValue(id.Value, out var command)) { try { _threadStaticBiDi = _bidi; var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo) ?? throw new JsonException("Remote end returned null command result in the 'result' property."); _threadStaticBiDi = null; 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: Null handling: Converters assume a non-null string ID and throw InvalidOperationException if no BiDi is set, but do not handle null/invalid IDs or provide contextual error messages. Referred Code internal class BrowsingContextConverter : JsonConverter<BrowsingContext.BrowsingContext> { public override BrowsingContext.BrowsingContext? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { var id = reader.GetString(); var bidi = Broker.ThreadStaticBiDi ?? throw new InvalidOperationException("No BiDi set"); return new BrowsingContext.BrowsingContext(bidi, id!); } 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: ThreadStatic safety: The use of a thread-static BiDi during deserialization may introduce race or context leakage across logical operations without explicit validation of the deserialization context. Referred Code internal static BiDi? ThreadStaticBiDi => _threadStaticBiDi; [ThreadStatic] private static BiDi? _threadStaticBiDi; 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
Possible issue	Prevent resource leak with finally block Move the logic that nullifies the _threadStaticBiDi field into a finally block to ensure it is always executed, even when exceptions occur, thus preventing potential resource leaks. dotnet/src/webdriver/BiDi/Broker.cs [283-295] try { _threadStaticBiDi = _bidi; var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo) ?? throw new JsonException("Remote end returned null command result in the 'result' property."); - _threadStaticBiDi = null; command.TaskCompletionSource.SetResult((EmptyResult)commandResult); } catch (Exception ex) { command.TaskCompletionSource.SetException(ex); } +finally +{ + _threadStaticBiDi = null; +} [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential state corruption issue where _threadStaticBiDi is not cleared on exception, and proposes the correct fix using a finally block to ensure resource cleanup.	Medium
More

[nvborisenko]

I think we already resolved it.