[dotnet] [bidi] Fix context aware event handlers by nvborisenko · Pull Request #16787 · SeleniumHQ/selenium

nvborisenko · 2025-12-23T21:26:24Z

User description

Recently we went away from concept of ContextEventArgs, meaning handler should be aware itself.

Continuation of #16694

🔧 Implementation Notes

I use == operator here, is it risky?

🔄 Types of changes

Bug fix (backwards compatible)

PR Type

Bug fix

Description

Wrap event handlers with context filtering logic
Check if event context matches current instance before invoking
Support both async and sync handler patterns consistently
Ensure proper async/await handling with ConfigureAwait

Diagram Walkthrough

flowchart LR
  A["Event Handler Methods"] -->|"Wrap with context check"| B["Filter by e.Context == this"]
  B -->|"If match"| C["Invoke User Handler"]
  B -->|"If no match"| D["Skip Handler"]
  C -->|"Async handlers"| E["await with ConfigureAwait"]
  C -->|"Sync handlers"| F["Direct invocation"]

File Walkthrough

Relevant files

Bug fix

BrowsingContext.cs `Implement context-aware event handler filtering` dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs Wrapped all 16 event handler methods with context filtering logic Added `if (e.Context == this)` checks before invoking user handlers Maintained support for both `Action` and `Func` handler patterns Applied `ConfigureAwait(false)` to async handler invocations for proper async context	+140/-20

qodo-code-review · 2025-12-23T21:26:46Z

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: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	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: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities 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: Context equality risk: The new event wrappers gate invocation on `e.Context == this`, which may perform reference equality and incorrectly skip handlers if `e.Context` represents the same logical context via a different instance (e.g., equality-by-`Id`). Referred Code return BiDi.BrowsingContext.OnNavigationStartedAsync(async e => { if (e.Context == this) { await handler(e).ConfigureAwait(false); } }, options.WithContext(this)); } public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ContextSubscriptionOptions? options = null) { return BiDi.BrowsingContext.OnNavigationStartedAsync(e => { if (e.Context == this) { handler(e); } }, options.WithContext(this)); } public Task<Subscription> OnFragmentNavigatedAsync(Func<NavigationInfo, Task> handler, ContextSubscriptionOptions? options = null) ... (clipped 196 lines) 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 · 2025-12-23T21:28:06Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Refactor duplicated logic into helpers To reduce code duplication, create two private helper methods—one for synchronous `Action` handlers and one for asynchronous `Func<T, Task>` handlers—to encapsulate the repeated context-filtering logic. Examples: dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [136-142] return BiDi.BrowsingContext.OnNavigationStartedAsync(async e => { if (e.Context == this) { await handler(e).ConfigureAwait(false); } }, options.WithContext(this)); dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [147-153] return BiDi.BrowsingContext.OnNavigationStartedAsync(e => { if (e.Context == this) { handler(e); } }, options.WithContext(this)); Solution Walkthrough: Before: public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ...) { return BiDi.BrowsingContext.OnNavigationStartedAsync(async e => { if (e.Context == this) { await handler(e).ConfigureAwait(false); } }, ...); } public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ...) { return BiDi.BrowsingContext.OnNavigationStartedAsync(e => { if (e.Context == this) { handler(e); } }, ...); } // ... this pattern is repeated for many other event handlers. After: private Action<T> WrapHandler<T>(Action<T> handler) where T : IHasContext { return e => { if (e.Context == this) handler(e); }; } private Func<T, Task> WrapHandler<T>(Func<T, Task> handler) where T : IHasContext { return async e => { if (e.Context == this) await handler(e).ConfigureAwait(false); }; } public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ...) { return BiDi.BrowsingContext.OnNavigationStartedAsync(WrapHandler(handler), ...); } public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ...) { return BiDi.BrowsingContext.OnNavigationStartedAsync(WrapHandler(handler), ...); } // ... all other handlers are simplified similarly. Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies significant code duplication across all modified methods and proposes an excellent refactoring that would centralize the logic, greatly improving code quality and maintainability.	High
Possible issue	Add exception handling to handlers Add `try-catch` blocks around the invocation of user-provided `handler` delegates in all event subscription methods. This prevents unhandled exceptions within user code from crashing the application. dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [134-154] public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ContextSubscriptionOptions? options = null) { return BiDi.BrowsingContext.OnNavigationStartedAsync(async e => { if (e.Context == this) { - await handler(e).ConfigureAwait(false); + try + { + await handler(e).ConfigureAwait(false); + } + catch + { + // Ignore exceptions from user-provided handlers. + } } }, options.WithContext(this)); } public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ContextSubscriptionOptions? options = null) { return BiDi.BrowsingContext.OnNavigationStartedAsync(e => { if (e.Context == this) { - handler(e); + try + { + handler(e); + } + catch + { + // Ignore exceptions from user-provided handlers. + } } }, options.WithContext(this)); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: This suggestion correctly identifies that unhandled exceptions in user-provided event handlers can crash the application. Wrapping the handler calls in a `try-catch` block is a valid strategy to improve the library's robustness.	Medium
Possible issue	validate handler parameter Add a null check for the `handler` parameter at the beginning of the method and throw an `ArgumentNullException` if it is `null`. dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [134-135] public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ContextSubscriptionOptions? options = null) { + if (handler is null) + { + throw new ArgumentNullException(nameof(handler)); + } Apply / Chat Suggestion importance[1-10]: 6 __ Why: This is a good practice for public APIs to validate arguments and fail early. Adding a `null` check for the `handler` parameter improves code robustness and provides clearer error messages.	Low
Learned best practice	Extract shared context-filter helper The PR repeats the same context-filtering wrapper pattern across many event methods; extract a small private helper to centralize the filter + invocation and call it from each handler to reduce duplication and future drift. dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [134-154] +private bool IsForThisContext(BrowsingContext? context) => Equals(context, this); + +private Action<T> FilterForThisContext<T>(Action<T> handler, Func<T, BrowsingContext?> getContext) => + e => { if (IsForThisContext(getContext(e))) handler(e); }; + +private Func<T, Task> FilterForThisContext<T>(Func<T, Task> handler, Func<T, BrowsingContext?> getContext) => + async e => { if (IsForThisContext(getContext(e))) await handler(e).ConfigureAwait(false); }; + public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ContextSubscriptionOptions? options = null) { - return BiDi.BrowsingContext.OnNavigationStartedAsync(async e => - { - if (e.Context == this) - { - await handler(e).ConfigureAwait(false); - } - }, options.WithContext(this)); + return BiDi.BrowsingContext.OnNavigationStartedAsync( + FilterForThisContext(handler, e => e.Context), + options.WithContext(this)); } public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ContextSubscriptionOptions? options = null) { - return BiDi.BrowsingContext.OnNavigationStartedAsync(e => - { - if (e.Context == this) - { - handler(e); - } - }, options.WithContext(this)); + return BiDi.BrowsingContext.OnNavigationStartedAsync( + FilterForThisContext(handler, e => e.Context), + options.WithContext(this)); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Replace ad-hoc duplication with shared helpers/utilities to reduce repetition and centralize logic.	Low
Update

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs

nvborisenko added 2 commits December 24, 2025 00:19

Sample

7d4c8fb

Context aware

76077be

selenium-ci added the C-dotnet .NET Bindings label Dec 23, 2025

qodo-code-review bot added the Review effort 2/5 label Dec 23, 2025

nvborisenko requested a review from RenderMichael December 23, 2025 21:34

nvborisenko commented Dec 23, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs Outdated Show resolved Hide resolved

nvborisenko changed the title ~~[dotnet] [fix] Fix context aware event handlers~~ [dotnet] [bidi] Fix context aware event handlers Dec 23, 2025

Use Equals(e.Context)

c3db1a8

nvborisenko merged commit f8b54ce into SeleniumHQ:trunk Dec 24, 2025
10 checks passed

nvborisenko deleted the bidi-context-aware-events branch December 24, 2025 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[dotnet] [bidi] Fix context aware event handlers#16787

[dotnet] [bidi] Fix context aware event handlers#16787
nvborisenko merged 3 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-context-aware-events

nvborisenko commented Dec 23, 2025 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Dec 23, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Dec 23, 2025 •

edited

Loading

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

User description

🔧 Implementation Notes

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nvborisenko commented Dec 23, 2025 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Dec 23, 2025 •

edited

Loading

qodo-code-review bot commented Dec 23, 2025 •

edited

Loading