Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 6, 2025

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 module instantiation to use factory pattern

  • Add module initialization extension point for JSON serialization

  • Replace direct constructors with Module.Create<T>() method

  • Implement JSON serializer context configuration in modules


Diagram Walkthrough

flowchart LR
  A["BiDi.cs"] --> B["Module.Create<T>()"]
  B --> C["Module.Initialize()"]
  C --> D["Broker.ConfigureJsonContext()"]
  D --> E["JSON Serialization"]
Loading

File Walkthrough

Relevant files
Enhancement
13 files
BiDi.cs
Replace direct module constructors with factory method     
+10/-10 
BrowserModule.cs
Add JSON serializer context and initialization method       
+22/-2   
BrowsingContextModule.cs
Add empty initialization method implementation                     
+6/-1     
Broker.cs
Add JSON context configuration and refactor serialization
+15/-15 
EmulationModule.cs
Add NotImplementedException initialization method               
+6/-1     
InputModule.cs
Add NotImplementedException initialization method               
+6/-1     
LogModule.cs
Add NotImplementedException initialization method               
+6/-1     
Module.cs
Implement factory pattern with initialization hooks           
+12/-2   
NetworkModule.cs
Add NotImplementedException initialization method               
+6/-1     
ScriptModule.cs
Add NotImplementedException initialization method               
+6/-1     
SessionModule.cs
Add NotImplementedException initialization method               
+6/-1     
StorageModule.cs
Add NotImplementedException initialization method               
+6/-1     
WebExtensionModule.cs
Add NotImplementedException initialization method               
+6/-1     

@nvborisenko nvborisenko marked this pull request as draft October 6, 2025 19:41
@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Oct 6, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 6, 2025

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
Copy link
Member Author

@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.

Copy link
Contributor

qodo-merge-pro bot commented Oct 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    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

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.
 }
  • Apply / Chat
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

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));
 }
  • Apply / Chat
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
Copy link
Member Author

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

protected Broker Broker { get; } = broker;
protected Broker Broker { get; private set; }

protected internal abstract void Initialize(Broker broker);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want third-party code to be able to make their own modules? If so, we can make this protected instead of protected internal so others can override.


var data = JsonSerializer.SerializeToUtf8Bytes(command, typeof(TCommand), _jsonSerializerContext);

var data = JsonSerializer.SerializeToUtf8Bytes(command, typeof(TCommand), _jsonSerializerOptions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we are passing in options instead of context, we can make this strongly typed

Suggested change
var data = JsonSerializer.SerializeToUtf8Bytes(command, typeof(TCommand), _jsonSerializerOptions);
var data = JsonSerializer.SerializeToUtf8Bytes(command, _jsonSerializerOptions);


_jsonSerializerContext = new BiDiJsonSerializerContext(jsonSerializerOptions);
// Add base BiDi generated context resolver; keep options mutable for module contexts
_jsonSerializerOptions.TypeInfoResolverChain.Add(BiDiJsonSerializerContext.Default);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should factor out BiDiJsonSerializerContext to instead use the Initialize method instead

@nvborisenko
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants