[dotnet] [bidi] Null guard for event handlers by nvborisenko · Pull Request #16967 · SeleniumHQ/selenium

nvborisenko · 2026-01-20T23:34:14Z

User description

💥 What does this PR do?

Fail fast with NullArgumentException if provided event handler is null.

🔧 Implementation Notes

Also translated inline lambda expression to private method for better stacktrace.

🔄 Types of changes

Cleanup (formatting, renaming)

PR Type

Enhancement, Tests

Description

Add null guard checks for event handlers with ArgumentNullException
Extract inline lambda expressions to private named methods for better stacktraces
Refactor Broker.SubscribeAsync to consolidate duplicate logic
Improve code maintainability across BiDi event subscription modules

Diagram Walkthrough

flowchart LR
  A["Event Handler Methods"] -->|"Add null checks"| B["ArgumentNullException"]
  A -->|"Extract lambdas"| C["Private Named Methods"]
  C -->|"Improve"| D["Stack Traces"]
  E["Broker.SubscribeAsync"] -->|"Consolidate"| F["Single Implementation"]

File Walkthrough

Relevant files

Enhancement

Broker.cs `Consolidate SubscribeAsync implementations` dotnet/src/webdriver/BiDi/Broker.cs Refactored SubscribeAsync methods to eliminate code duplication Created single private implementation handling both sync and async handlers Removed redundant event registration logic from public overloads Changed public methods from async to sync, delegating to private async method	+9/-13
BrowsingContext.cs `Extract event handlers to private methods with null guards` dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs Added null guard checks for all event handler parameters Extracted 24 inline lambda expressions to private named methods Improved stack trace clarity for debugging event handler issues Maintained context filtering logic in dedicated handler methods	+259/-139
BrowsingContextInputModule.cs `Add null guards and extract input module handlers` dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextInputModule.cs Added null guard checks for FileDialogOpened event handlers Extracted inline lambdas to HandleFileDialogOpenedAsync and HandleFileDialogOpened methods Improved error reporting with ArgumentNullException	+25/-13
BrowsingContextLogModule.cs `Add null guards and extract log module handlers` dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextLogModule.cs Added null guard checks for EntryAdded event handlers Extracted inline lambdas to HandleEntryAddedAsync and HandleEntryAdded methods Simplified subscription options handling with ContextSubscriptionOptions.WithContext	+25/-13
BrowsingContextNetworkModule.cs `Add null guards and extract network module handlers` dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextNetworkModule.cs Added null guard checks for all 8 network event handler methods Extracted 16 inline lambda expressions to private named handler methods Improved code readability and stack trace quality for network events Maintained context filtering in dedicated handler implementations	+129/-69

Error handling

EventHandler.cs `Add null guards to EventHandler constructors` dotnet/src/webdriver/BiDi/EventHandler.cs Added null guard checks in AsyncEventHandler constructor for func parameter Added null guard checks in SyncEventHandler constructor for action parameter Throws ArgumentNullException with descriptive messages for null handlers	+2/-2

qodo-code-review · 2026-01-20T23:34:53Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
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: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation 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: No audit logging: The refactored subscription flow does not add any logging/audit metadata, so it is not verifiable from the diff whether critical actions (if any) are captured with required context (user, timestamp, outcome). Referred Code public Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { var eventHandler = new SyncEventHandler<TEventArgs>(eventName, action); return SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo); } public Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { var eventHandler = new AsyncEventHandler<TEventArgs>(eventName, func); return SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo); } private async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { _eventTypesMap[eventName] = jsonTypeInfo; var handlers = _eventHandlers.GetOrAdd(eventName, (a) => []); ... (clipped 6 lines) 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: Missing input validation: The new `SubscribeAsync` overloads do not validate potentially caller-provided inputs such as `eventName` and `jsonTypeInfo`, so it is unclear from the diff whether invalid/malicious values are prevented upstream. Referred Code public Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { var eventHandler = new SyncEventHandler<TEventArgs>(eventName, action); return SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo); } public Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { var eventHandler = new AsyncEventHandler<TEventArgs>(eventName, func); return SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo); } private async Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, EventHandler eventHandler, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { _eventTypesMap[eventName] = jsonTypeInfo; 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 · 2026-01-20T23:36:33Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Refactor to avoid massive duplication The PR adds many nearly identical private handler methods, causing significant code duplication. Refactor this by using a single generic private method to handle context filtering for all events, which would be more maintainable. Examples: dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [309-467] private async Task HandleNavigationStartedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler) { if (Equals(e.Context)) { await handler(e).ConfigureAwait(false); } } private void HandleNavigationStarted(NavigationInfo e, Action<NavigationInfo> handler) { ... (clipped 149 lines) dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextNetworkModule.cs [183-261] private async Task HandleBeforeRequestSentAsync(BeforeRequestSentEventArgs e, Func<BeforeRequestSentEventArgs, Task> handler) { if (context.Equals(e.Context)) { await handler(e).ConfigureAwait(false); } } private void HandleBeforeRequestSent(BeforeRequestSentEventArgs e, Action<BeforeRequestSentEventArgs> handler) { ... (clipped 69 lines) Solution Walkthrough: Before: // In dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ...) { if (handler is null) throw new ArgumentNullException(nameof(handler)); return BiDi.BrowsingContext.OnNavigationStartedAsync( e => HandleNavigationStarted(e, handler), ...); } private void HandleNavigationStarted(NavigationInfo e, Action<NavigationInfo> handler) { if (Equals(e.Context)) { handler(e); } } // ... dozens of similar handler methods for different events ... private void HandleFragmentNavigated(NavigationInfo e, Action<NavigationInfo> handler) { if (Equals(e.Context)) { handler(e); } } After: // In a new helper class or within each module private static Action<T> CreateContextFilteredHandler<T>( T e, Action<T> handler, BrowsingContext context) where T : IContextEventArgs { return (args) => { if (context.Equals(args.Context)) { handler(args); } }; } // In dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ...) { if (handler is null) throw new ArgumentNullException(nameof(handler)); return BiDi.BrowsingContext.OnNavigationStartedAsync( CreateContextFilteredHandler(handler, this), ...); } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies massive code duplication introduced across multiple files, which contradicts the PR's goal of improving maintainability and is a significant design flaw.	High
Possible issue	Restore special handling for subscription options Restore the special handling for `OnEntryAddedAsync` subscription options, which was removed during refactoring, to avoid unintended context scoping due to a known spec issue. dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextLogModule.cs [28-44] public Task<Subscription> OnEntryAddedAsync(Func<Log.LogEntry, Task> handler, ContextSubscriptionOptions? options = null) { if (handler is null) throw new ArgumentNullException(nameof(handler)); return logModule.OnEntryAddedAsync( e => HandleEntryAddedAsync(e, handler), - ContextSubscriptionOptions.WithContext(options, context)); + new SubscriptionOptions() { Timeout = options?.Timeout }); // special case, don't scope to context, awaiting https://github.com/w3c/webdriver-bidi/issues/1032 } public Task<Subscription> OnEntryAddedAsync(Action<Log.LogEntry> handler, ContextSubscriptionOptions? options = null) { if (handler is null) throw new ArgumentNullException(nameof(handler)); return logModule.OnEntryAddedAsync( e => HandleEntryAdded(e, handler), - ContextSubscriptionOptions.WithContext(options, context)); + new SubscriptionOptions() { Timeout = options?.Timeout }); // special case, don't scope to context, awaiting https://github.com/w3c/webdriver-bidi/issues/1032 } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the refactoring removed special-case logic for `OnEntryAddedAsync` that was intentionally added to work around a spec issue, which could be a significant regression.	Medium
General	Guard against null type info Add a null check for the `jsonTypeInfo` parameter in `SubscribeAsync` to prevent potential exceptions. dotnet/src/webdriver/BiDi/Broker.cs [165] +if (jsonTypeInfo is null) + throw new ArgumentNullException(nameof(jsonTypeInfo), "Event JSON type info cannot be null."); _eventTypesMap[eventName] = jsonTypeInfo; Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out a missing null check for the `jsonTypeInfo` parameter, which improves the method's robustness by preventing potential null reference exceptions.	Medium
Learned best practice	Centralize repeated handler logic Replace the many per-event `HandleXxx` methods with 1–2 generic helpers that apply the context filter and invoke the provided handler to keep behavior single-sourced. dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [309-339] -private async Task HandleNavigationStartedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler) +private Task InvokeIfThisContextAsync<T>(T e, Func<T, Task> handler, Func<T, BrowsingContext> getContext) { - if (Equals(e.Context)) - { - await handler(e).ConfigureAwait(false); - } + return Equals(getContext(e)) ? handler(e) : Task.CompletedTask; } -private void HandleNavigationStarted(NavigationInfo e, Action<NavigationInfo> handler) +private void InvokeIfThisContext<T>(T e, Action<T> handler, Func<T, BrowsingContext> getContext) { - if (Equals(e.Context)) + if (Equals(getContext(e))) { handler(e); } } -private async Task HandleFragmentNavigatedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler) -{ - if (Equals(e.Context)) - { - await handler(e).ConfigureAwait(false); - } -} +private Task HandleNavigationStartedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler) => + InvokeIfThisContextAsync(e, handler, x => x.Context); -private void HandleFragmentNavigated(NavigationInfo e, Action<NavigationInfo> handler) -{ - if (Equals(e.Context)) - { - handler(e); - } -} +private void HandleNavigationStarted(NavigationInfo e, Action<NavigationInfo> handler) => + InvokeIfThisContext(e, handler, x => x.Context); +private Task HandleFragmentNavigatedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler) => + InvokeIfThisContextAsync(e, handler, x => x.Context); + +private void HandleFragmentNavigated(NavigationInfo e, Action<NavigationInfo> handler) => + InvokeIfThisContext(e, handler, x => x.Context); + Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Reduce duplication by centralizing shared behavior (use helpers for repeated context-filter + invoke logic instead of many near-identical methods).	Low
More

nvborisenko added 7 commits January 21, 2026 00:56

Example for LogModule

2f4138a

Input

edea604

Network

701b747

Unify

934940f

End up with private methods

820a283

Update BrowsingContext.cs

2ee96cf

Null guard

ebe2299

selenium-ci added the C-dotnet .NET Bindings label Jan 20, 2026

qodo-code-review bot added the Review effort 3/5 label Jan 20, 2026

nvborisenko merged commit 1e1a407 into SeleniumHQ:trunk Jan 20, 2026
17 checks passed

nvborisenko deleted the bidi-events-local-named-handlers branch January 20, 2026 23:57

This was referenced Mar 8, 2026

chore: Bump Selenium.Support and Selenium.WebDriver CyberGutta/AkademiTrack#36

Merged

chore: Bump Selenium.WebDriver from 4.40.0 to 4.41.0 CyberGutta/AkademiTrack#37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[dotnet] [bidi] Null guard for event handlers#16967

[dotnet] [bidi] Null guard for event handlers#16967
nvborisenko merged 7 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-events-local-named-handlers

nvborisenko commented Jan 20, 2026 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Jan 20, 2026

Uh oh!

qodo-code-review bot commented Jan 20, 2026

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

nvborisenko commented Jan 20, 2026 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

💥 What does this PR do?

🔧 Implementation Notes

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Jan 20, 2026

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Jan 20, 2026

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nvborisenko commented Jan 20, 2026 •

edited by qodo-code-review bot

Loading