[dotnet] [bidi] Modules as extensions

[nvborisenko]

User description

Contributes to #15329

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)
• Bug fix (backwards compatible)
• New feature (non-breaking change which adds functionality and tests!)
• Breaking change (fix or feature that would cause existing functionality to change)

---

PR Type

Enhancement

---

Description

• Refactor BiDi modules to use extension pattern

• Centralize JSON serialization configuration in BiDi class

• Replace individual module properties with generic AsModule method

• Update all module constructors to parameterless design

---

Diagram Walkthrough

flowchart LR
  A["BiDi Class"] --> B["AsModule<T>() Method"]
  B --> C["ConcurrentDictionary<Type, Module>"]
  C --> D["Module.Create<T>()"]
  D --> E["Individual Modules"]
  A --> F["Centralized JsonSerializerOptions"]
  F --> G["BiDiJsonSerializerContext"]
  G --> E

Loading

File Walkthrough

Relevant files

Enhancement

13 files BiDi.cs Centralize JSON context and implement module extensions    +46/-129 Module.cs Add factory method and JSON context properties                      +22/-2    Broker.cs Update command execution with JSON context parameter          +22/-58  BrowserModule.cs Remove constructor parameter and use JsonContext                  +10/-10  BrowsingContextModule.cs Remove constructor parameter and use JsonContext                  +41/-41  SessionModule.cs Remove constructor parameter and use JsonContext                  +6/-6      NetworkModule.cs Remove constructor parameter and use JsonContext                  +25/-25  ScriptModule.cs Remove constructor parameter and use JsonContext                  +12/-12  LogModule.cs Remove constructor parameter and use JsonContext                  +3/-3      StorageModule.cs Remove constructor parameter and use JsonContext                  +4/-4      InputModule.cs Remove constructor parameter and use JsonContext                  +4/-4      EmulationModule.cs Remove constructor parameter and use JsonContext                  +10/-11  WebExtensionModule.cs Remove constructor parameter and use JsonContext                  +3/-3

---

[qodo-merge-pro]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Unhandled NotImplemented Description: Module initialization method throws NotImplementedException, which can crash consumers at runtime when the module is created via the new factory, creating a denial-of-service for that functionality. EmulationModule.cs [93-96] Referred Code protected internal override void Initialize(Broker broker) { throw new NotImplementedException(); }
	Unhandled NotImplemented Description: Module initialization throws NotImplementedException, potentially causing runtime failures and making the module unusable when created, leading to stability issues. InputModule.cs [49-52] Referred Code protected internal override void Initialize(Broker broker) { throw new System.NotImplementedException(); }
	Unhandled NotImplemented Description: Initialize method throws NotImplementedException which may be hit during module creation via factory, causing unexpected termination at runtime. LogModule.cs [38-41] Referred Code protected internal override void Initialize(Broker broker) { throw new NotImplementedException(); }
	Unhandled NotImplemented Description: Initialize throws NotImplementedException, which could cause runtime failure when the module is instantiated through Module.Create, leading to service disruption. NetworkModule.cs [177-180] Referred Code protected internal override void Initialize(Broker broker) { throw new System.NotImplementedException(); }
	Unhandled NotImplemented Description: Initialize method throws NotImplementedException, potentially breaking functionality at runtime when storage module is created via factory. StorageModule.cs [48-51] Referred Code protected internal override void Initialize(Broker broker) { throw new System.NotImplementedException(); }
	Unhandled NotImplemented Description: Initialize throws NotImplementedException, risking runtime crashes when module factory initializes JSON context or other settings. WebExtensionModule.cs [41-44] Referred Code protected internal override void Initialize(Broker broker) { throw new System.NotImplementedException(); }
	Unsafe deserialization config Description: Broker exposes ConfigureJsonContext allowing arbitrary modification of shared JsonSerializerOptions which may impact deserialization of untrusted WebSocket data across modules, increasing risk of insecure parsing if misconfigured by extensions. Broker.cs [111-119] Referred Code // Add base BiDi generated context resolver; keep options mutable for module contexts _jsonSerializerOptions.TypeInfoResolverChain.Add(BiDiJsonSerializerContext.Default); } public void ConfigureJsonContext(Action<JsonSerializerOptions> action) { // Keep options mutable; do not create a context bound to them (avoids InvalidOperationException) action(_jsonSerializerOptions); }
Ticket Compliance
🟡	🎫 #1234 🔴 Investigate and ensure BiDi/WebDriver click triggers JavaScript in link href in Firefox 42 scenario where 2.48.0/2.48.2 regressed vs 2.47.1. Provide a fix or behavioral change so alerts triggered via href JavaScript execute on click. ⚪ Validate behavior specifically for Firefox environment noted; add tests if applicable.
🟡	🎫 #5678 🔴 Diagnose ChromeDriver "ConnectFailure (Connection refused)" after first instance on Ubuntu 16.04.4 with Selenium 3.9.0, Chrome 65, Chromedriver 2.35. Provide a fix or guidance to prevent repeated instantiation failures. Potentially add robustness/retry or connection handling changes.
🟡	🎫 #15329 🟢 Provide an extension-style mechanism, e.g., driver.AsBiDiAsync().AsPermissions(), to plug in new modules. Allow modules to configure JSON serialization (context/resolvers) needed for their messages and events. Include usage pattern aligning with factory/extension approach. 🔴 Avoid reliance on internal members; ensure APIs used are accessible as intended. ⚪ Expose minimal public API in .NET to allow third parties to implement extendable BiDi modules without relying on internal members.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
⚪	No custom compliance provided Follow the guide to enable custom compliance check.

Compliance status legend

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

[nvborisenko]

@RenderMichael despite on these dummy changes, let's see how we can modify JsonSerializerContext/Options in runtime. I have no ideas, the best what I got: System.InvalidOperationException : This JsonSerializerOptions instance is read-only or has already been used in serialization or deserialization.

If you know please advise.

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Complete the partial module implementations The new abstract Initialize method is not properly implemented in most modules, throwing NotImplementedException. These implementations must be completed to prevent runtime exceptions when modules are created. Examples: dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs [93-96] protected internal override void Initialize(Broker broker) { throw new NotImplementedException(); } dotnet/src/webdriver/BiDi/Input/InputModule.cs [49-52] protected internal override void Initialize(Broker broker) { throw new System.NotImplementedException(); } Solution Walkthrough: Before: // In Module.cs public static TModule Create<TModule>(Broker broker) where TModule : Module, new() { TModule module = new(); module.Broker = broker; module.Initialize(broker); // This will throw for most modules return module; } // In EmulationModule.cs, InputModule.cs, etc. public sealed class EmulationModule : Module { // ... protected internal override void Initialize(Broker broker) { throw new NotImplementedException(); } } After: // In Module.cs public static TModule Create<TModule>(Broker broker) where TModule : Module, new() { TModule module = new(); module.Broker = broker; module.Initialize(broker); // This will no longer throw return module; } // In EmulationModule.cs, InputModule.cs, etc. public sealed class EmulationModule : Module { // ... protected internal override void Initialize(Broker broker) { // Implementation for JSON serialization context, or empty if not needed. // For example: // broker.ConfigureJsonContext(opts => opts.TypeInfoResolverChain.Add(EmulationModuleJsonSerializerContext.Default)); } } Suggestion importance[1-10]: 10 __ Why: This suggestion correctly identifies a critical flaw where most module Initialize methods are unimplemented, which would cause runtime exceptions and break functionality for almost all BiDi modules.	High
Possible issue	✅ Remove exception from unimplemented method Suggestion Impact: The commit removed the Initialize method that threw NotImplementedException, effectively preventing the runtime crash as suggested. code diff: - protected internal override void Initialize(Broker broker) - { - throw new NotImplementedException(); + return await Broker.ExecuteCommandAsync<SetGeolocationOverrideCommand, EmptyResult>(new SetGeolocationOverrideCommand(@params), options, JsonContext).ConfigureAwait(false); } Remove the NotImplementedException from the Initialize method in EmulationModule and other modules. If no initialization is needed, the method body should be empty to prevent runtime crashes. dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs [93-96] protected internal override void Initialize(Broker broker) { - throw new NotImplementedException(); + // No-op for this module. } [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical bug introduced by the PR. The Initialize method will be called upon module creation, and throwing a NotImplementedException will cause a runtime crash, making the modules unusable.	High
General	✅ Register module-specific JSON serialization context Suggestion Impact: The commit wires the module’s JsonContext into all ExecuteCommandAsync and SubscribeAsync calls by passing JsonContext, and it also removes the previously empty Initialize override, implying the module now relies on a configured JSON context. This aligns with the suggestion’s goal of registering/using a module-specific serialization context, though the exact ConfigureJsonContext call is not shown in this diff. code diff: - var createResult = await Broker.ExecuteCommandAsync<CreateCommand, CreateResult>(new CreateCommand(@params), options).ConfigureAwait(false); + var createResult = await Broker.ExecuteCommandAsync<CreateCommand, CreateResult>(new CreateCommand(@params), options, JsonContext).ConfigureAwait(false); return createResult.Context; } @@ -38,221 +38,216 @@ { var @params = new NavigateParameters(context, url, options?.Wait); - return await Broker.ExecuteCommandAsync<NavigateCommand, NavigateResult>(new NavigateCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<NavigateCommand, NavigateResult>(new NavigateCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<EmptyResult> ActivateAsync(BrowsingContext context, ActivateOptions? options = null) { var @params = new ActivateParameters(context); - return await Broker.ExecuteCommandAsync<ActivateCommand, EmptyResult>(new ActivateCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<ActivateCommand, EmptyResult>(new ActivateCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<LocateNodesResult> LocateNodesAsync(BrowsingContext context, Locator locator, LocateNodesOptions? options = null) { var @params = new LocateNodesParameters(context, locator, options?.MaxNodeCount, options?.SerializationOptions, options?.StartNodes); - return await Broker.ExecuteCommandAsync<LocateNodesCommand, LocateNodesResult>(new LocateNodesCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<LocateNodesCommand, LocateNodesResult>(new LocateNodesCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<CaptureScreenshotResult> CaptureScreenshotAsync(BrowsingContext context, CaptureScreenshotOptions? options = null) { var @params = new CaptureScreenshotParameters(context, options?.Origin, options?.Format, options?.Clip); - return await Broker.ExecuteCommandAsync<CaptureScreenshotCommand, CaptureScreenshotResult>(new CaptureScreenshotCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<CaptureScreenshotCommand, CaptureScreenshotResult>(new CaptureScreenshotCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<EmptyResult> CloseAsync(BrowsingContext context, CloseOptions? options = null) { var @params = new CloseParameters(context, options?.PromptUnload); - return await Broker.ExecuteCommandAsync<CloseCommand, EmptyResult>(new CloseCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<CloseCommand, EmptyResult>(new CloseCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<TraverseHistoryResult> TraverseHistoryAsync(BrowsingContext context, int delta, TraverseHistoryOptions? options = null) { var @params = new TraverseHistoryParameters(context, delta); - return await Broker.ExecuteCommandAsync<TraverseHistoryCommand, TraverseHistoryResult>(new TraverseHistoryCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<TraverseHistoryCommand, TraverseHistoryResult>(new TraverseHistoryCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<NavigateResult> ReloadAsync(BrowsingContext context, ReloadOptions? options = null) { var @params = new ReloadParameters(context, options?.IgnoreCache, options?.Wait); - return await Broker.ExecuteCommandAsync<ReloadCommand, NavigateResult>(new ReloadCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<ReloadCommand, NavigateResult>(new ReloadCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<EmptyResult> SetViewportAsync(BrowsingContext context, SetViewportOptions? options = null) { var @params = new SetViewportParameters(context, options?.Viewport, options?.DevicePixelRatio); - return await Broker.ExecuteCommandAsync<SetViewportCommand, EmptyResult>(new SetViewportCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<SetViewportCommand, EmptyResult>(new SetViewportCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<GetTreeResult> GetTreeAsync(GetTreeOptions? options = null) { var @params = new GetTreeParameters(options?.MaxDepth, options?.Root); - return await Broker.ExecuteCommandAsync<GetTreeCommand, GetTreeResult>(new GetTreeCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<GetTreeCommand, GetTreeResult>(new GetTreeCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<PrintResult> PrintAsync(BrowsingContext context, PrintOptions? options = null) { var @params = new PrintParameters(context, options?.Background, options?.Margin, options?.Orientation, options?.Page, options?.PageRanges, options?.Scale, options?.ShrinkToFit); - return await Broker.ExecuteCommandAsync<PrintCommand, PrintResult>(new PrintCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<PrintCommand, PrintResult>(new PrintCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<EmptyResult> HandleUserPromptAsync(BrowsingContext context, HandleUserPromptOptions? options = null) { var @params = new HandleUserPromptParameters(context, options?.Accept, options?.UserText); - return await Broker.ExecuteCommandAsync<HandleUserPromptCommand, EmptyResult>(new HandleUserPromptCommand(@params), options).ConfigureAwait(false); + return await Broker.ExecuteCommandAsync<HandleUserPromptCommand, EmptyResult>(new HandleUserPromptCommand(@params), options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.navigationStarted", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.navigationStarted", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.navigationStarted", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.navigationStarted", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnFragmentNavigatedAsync(Func<NavigationInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnFragmentNavigatedAsync(Action<NavigationInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnHistoryUpdatedAsync(Func<HistoryUpdatedEventArgs, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.historyUpdated", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.historyUpdated", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnHistoryUpdatedAsync(Action<HistoryUpdatedEventArgs> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.historyUpdated", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.historyUpdated", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnDomContentLoadedAsync(Func<NavigationInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.domContentLoaded", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.domContentLoaded", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnDomContentLoadedAsync(Action<NavigationInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.domContentLoaded", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.domContentLoaded", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnLoadAsync(Func<NavigationInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.load", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.load", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnLoadAsync(Action<NavigationInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.load", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.load", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnDownloadWillBeginAsync(Func<DownloadWillBeginEventArgs, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.downloadWillBegin", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.downloadWillBegin", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnDownloadWillBeginAsync(Action<DownloadWillBeginEventArgs> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.downloadWillBegin", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.downloadWillBegin", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnDownloadEndAsync(Func<DownloadEndEventArgs, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.downloadEnd", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.downloadEnd", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnDownloadEndAsync(Action<DownloadEndEventArgs> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.downloadEnd", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.downloadEnd", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnNavigationAbortedAsync(Func<NavigationInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.navigationAborted", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.navigationAborted", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnNavigationAbortedAsync(Action<NavigationInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.navigationAborted", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.navigationAborted", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnNavigationFailedAsync(Func<NavigationInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.navigationFailed", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.navigationFailed", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnNavigationFailedAsync(Action<NavigationInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.navigationFailed", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.navigationFailed", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnNavigationCommittedAsync(Func<NavigationInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.navigationCommitted", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.navigationCommitted", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnNavigationCommittedAsync(Action<NavigationInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.navigationCommitted", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.navigationCommitted", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnContextCreatedAsync(Func<BrowsingContextInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.contextCreated", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.contextCreated", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnContextCreatedAsync(Action<BrowsingContextInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.contextCreated", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.contextCreated", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnContextDestroyedAsync(Func<BrowsingContextInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.contextDestroyed", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.contextDestroyed", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnContextDestroyedAsync(Action<BrowsingContextInfo> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.contextDestroyed", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.contextDestroyed", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnUserPromptOpenedAsync(Func<UserPromptOpenedEventArgs, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.userPromptOpened", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.userPromptOpened", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnUserPromptOpenedAsync(Action<UserPromptOpenedEventArgs> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.userPromptOpened", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.userPromptOpened", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnUserPromptClosedAsync(Func<UserPromptClosedEventArgs, Task> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.userPromptClosed", handler, options).ConfigureAwait(false); + return await Broker.SubscribeAsync("browsingContext.userPromptClosed", handler, options, JsonContext).ConfigureAwait(false); } public async Task<Subscription> OnUserPromptClosedAsync(Action<UserPromptClosedEventArgs> handler, BrowsingContextsSubscriptionOptions? options = null) { - return await Broker.SubscribeAsync("browsingContext.userPromptClosed", handler, options).ConfigureAwait(false); - } - - protected internal override void Initialize(Broker broker) - { - + return await Broker.SubscribeAsync("browsingContext.userPromptClosed", handler, options, JsonContext).ConfigureAwait(false); } Implement the Initialize method in BrowsingContextModule to register a module-specific JsonSerializerContext. This aligns with the pattern in BrowserModule and is necessary for proper JSON serialization. dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs [254-257] protected internal override void Initialize(Broker broker) { - + broker.ConfigureJsonContext(opts => opts.TypeInfoResolverChain.Add(BrowsingContextModuleJsonSerializerContext.Default)); } [Suggestion processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that the PR's refactoring goal is to register module-specific JSON contexts, which is missing for BrowsingContextModule. Implementing this is crucial for completing the refactoring and gaining the performance/AOT benefits of source-generated serialization.	Medium
Learned best practice	Add null check for broker Add a null check for the incoming broker to fail fast with a clear error if it's not provided. dotnet/src/webdriver/BiDi/Module.cs [30-36] public static TModule Create<TModule>(Broker broker) where TModule : Module, new() { + if (broker is null) throw new ArgumentNullException(nameof(broker)); TModule module = new(); module.Broker = broker; module.Initialize(broker); return module; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Validate inputs and states early; guard against null dependencies to prevent NullReferenceExceptions.	Low
More

[nvborisenko]

I don't want to believe that injecting custom TypeInfoResolver is not possible.

[nvborisenko]

Nice! Thank you.

Parameterless ctor for Module looks ugly. But we need something parameterless to be able to new T(). I think we can introduce TModuleFactory, and then new TModuleFactory().Create(any args). It would be better, because we avoid Initialize() method at all, and move this logic into module's ctor.

[RenderMichael]

If we targeted only the latest .NET versions, this would be a great place to have a static abstract member:

interface ICreateModule<TModule> where TModule : ICreateModule<TModule>
{
    static abstract TModule Create(Broker broker);
}

public class MyModule : Module, ICreateModule<MyModule>
{
    private JsonSerializerContext _context;
    private MyModule(Broker broker, JsonSerializerOptions options) : base(broker)
    {
        _context = new MySerializerContext(broker.CreateOptions());
    }

    public static MyModule Create(Broker broker)
    {
        return new MyModule(broker);
    }
}

But alas, we cannot expose it the "best" way on modern .NET but a different way on older TFMs.

---

I think we can introduce TModuleFactory, and then new TModuleFactory().Create(any args). It would be better, because we avoid Initialize() method at all, and move this logic into module's ctor.

I don't see how that would work differently from the Module.Create<TModule>(any args) introduced here. Beyond the static abstract approach, or some reflection-based constructor duck-typing, I do not think there is another option.

[nvborisenko]

I tried to implement different factory approaches, no luck (anyway consumer should provide factory func when calling, meaning this signature is exposed). Current approach looks better:

• Module parameterless ctor
• Module is enforced to provide some initialization (via protected abstract)

I will continue.

[nvborisenko]

Now each module owns JsonSerializerContext, and I see 2 issues:

Performance degradation (memory)

Before:

| Method                         | Mean     | Error   | StdDev  | Gen0     | Gen1     | Gen2     | Allocated |
|------------------------------- |---------:|--------:|--------:|---------:|---------:|---------:|----------:|
| SeleniumCaptureScreenshotAsync | 128.1 ms | 6.83 ms | 4.52 ms | 250.0000 | 250.0000 | 250.0000 |   1.65 MB |

After:

| Method                         | Mean     | Error   | StdDev  | Gen0     | Gen1     | Gen2     | Allocated |
|------------------------------- |---------:|--------:|--------:|---------:|---------:|---------:|----------:|
| SeleniumCaptureScreenshotAsync | 128.5 ms | 9.98 ms | 5.94 ms | 500.0000 | 500.0000 | 500.0000 |   2.31 MB |

Cross-module type referencing

If we define json context per module, then we also have to define all types this module is referencing to. It is nightmare! We should track references somehow, and it is easy to make a mistake.

How to move on?

I think we can use one big shared json context by all modules. External modules will define their own context.

[nvborisenko]

@RenderMichael I am confident with proposed changes, please review.

• BiDi is owner of JsonOptions/Context
• Built-in modules reuse JsonContext
• External module may create own JsonContext and propagate to commands

Edited: Module has access to JsonContext, meaning it can provide specific JsonTypeInfo for serialization process. It is beneficial, I already measured.

[qodo-merge-pro]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Deserialization risk Description: JSON deserialization context is selected dynamically per event method via a runtime map, which could enable deserialization of attacker-controlled event payloads into unexpected types if an event name is spoofed or mis-registered; ensure event names are strictly validated and contexts are not user-influenced. Broker.cs [277-317] Referred Code type = reader.GetString(); break; case "method": method = reader.GetString(); break; case "result": resultReader = reader; // snapshot break; case "params": paramsReader = reader; // snapshot break; case "error": error = reader.GetString(); break; case "message": message = reader.GetString(); ... (clipped 20 lines)
Ticket Compliance
🟡	🎫 #15329 🟢 Expose minimal, extensible APIs so third parties can build modules. 🔴 Provide a possibility to implement a BiDi Permissions module in .NET without relying on internal members. Offer an API surface so users can obtain a BiDi instance and then get a Permissions module via an extension-like pattern (e.g., AsPermissions or similar).
🟡	🎫 #5678 🔴 Investigate and resolve ChromeDriver "ConnectFailure (Connection refused)" errors on subsequent driver instantiations in Ubuntu 16.04 with Selenium 3.9 and Chrome 65/ChromeDriver 2.35.
🟡	🎫 #1234 🔴 Ensure click() triggers JavaScript in link href for Firefox (issue observed in Selenium 2.48 with Firefox 42).
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
⚪	No custom compliance provided Follow the guide to enable custom compliance check.

Compliance status legend

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

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use a thread-safe collection Replace the Dictionary used for _eventTypesMap with a ConcurrentDictionary to prevent a race condition. dotnet/src/webdriver/BiDi/Communication/Broker.cs [42] -private readonly Dictionary<string, (Type EventType, JsonSerializerContext JsonContext)> _eventTypesMap = []; +private readonly ConcurrentDictionary<string, (Type EventType, JsonSerializerContext JsonContext)> _eventTypesMap = []; Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a race condition where the non-thread-safe _eventTypesMap dictionary is modified by concurrent calls to SubscribeAsync, and proposes using ConcurrentDictionary to ensure thread safety and prevent data corruption.	High
Learned best practice	Properly complete event queue on dispose Complete the blocking collection during disposal to stop the consumer loop and prevent potential hangs; signal completion with CompleteAdding and await the emitter task. dotnet/src/webdriver/BiDi/Communication/Broker.cs [234-248] -private readonly BlockingCollection<MessageEvent> _pendingEvents = []; -... private async Task ProcessEventsAwaiterAsync() { foreach (var result in _pendingEvents.GetConsumingEnumerable()) { try { - if (_eventHandlers.TryGetValue(result.Method, out var eventHandlers)) + if (_eventHandlers.TryGetValue(result.Method, out var eventHandlers) && eventHandlers is not null) { - if (eventHandlers is not null) + foreach (var handler in eventHandlers.ToArray()) { - foreach (var handler in eventHandlers.ToArray()) // copy handlers avoiding modified collection while iterating - ... + ... } } } catch (Exception ex) { _logger.Log(LogEventLevel.Error, ex, "Unhandled exception while processing event"); } } } -... + public async ValueTask DisposeAsync() { - ... - // missing completion for _pendingEvents to stop consumer + try + { + _receiveMessagesCancellationTokenSource?.Cancel(); + } + catch { } + + _pendingEvents.CompleteAdding(); + + if (_receivingMessageTask is not null) + { + await Task.WhenAny(_receivingMessageTask, Task.Delay(TimeSpan.FromSeconds(5))).ConfigureAwait(false); + } + if (_eventEmitterTask is not null) + { + await Task.WhenAny(_eventEmitterTask, Task.Delay(TimeSpan.FromSeconds(5))).ConfigureAwait(false); + } + + await _transport.DisposeAsync().ConfigureAwait(false); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Make concurrency and lifecycle code robust by correctly canceling tasks and avoiding blocking collections without proper shutdown signaling.	Low
More

[RenderMichael]

Looks pretty good! A few comments, none of them blocking.