[dotnet] Fix stopping of network monitoring via DevTools

[nvborisenko]

🔗 Related Issues

Fixes #17350

💥 What does this PR do?

This pull request makes improvements to network monitoring cleanup and enhances test coverage for network interception in the WebDriver codebase. The main changes ensure that network and fetch domains are properly disabled when monitoring stops and that tests verify network state resets after interception.

Network monitoring cleanup:

• NetworkManager.cs: Added calls to explicitly disable the network and fetch domains in StopMonitoring() to ensure all network interception hooks are removed when monitoring ends.

Test improvements:

• NetworkInterceptionTests.cs: Enhanced the TestCanInterceptNetworkCalls test to refresh the page and verify that network interception no longer affects subsequent requests, ensuring proper cleanup.

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Fix network monitoring cleanup in .NET WebDriver

🐞 Bug fix

Walkthroughs

Description

• Explicitly disable network and fetch domains in StopMonitoring()
• Verify network interception cleanup after monitoring stops
• Ensure proper removal of network interception hooks

File Changes

1. dotnet/src/webdriver/NetworkManager.cs 🐞 Bug fix +2/-0

Add explicit network domain disabling on stop

• Added explicit calls to DisableNetwork() and DisableFetch() in StopMonitoring() method
• Ensures all network interception hooks are properly removed when monitoring ends
• Complements existing network caching disablement

dotnet/src/webdriver/NetworkManager.cs

---

2. dotnet/test/webdriver/NetworkInterceptionTests.cs 🧪 Tests +3/-0

Verify network interception cleanup in tests

• Enhanced TestCanInterceptNetworkCalls test with page refresh after monitoring stops
• Added verification that network interception no longer affects subsequent requests
• Validates proper cleanup of network state between test phases

dotnet/test/webdriver/NetworkInterceptionTests.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2)   📘 Rule violations (0)   📎 Requirement gaps (0)

🐞\ ≡ Correctness (1) ☼ Reliability (1)

1. Misindented text assertion lines ☑ 📘 ⚙

Description

The newly added lines in TestCanInterceptNetworkCalls have inconsistent indentation, which can
cause dotnet format/lint checks to fail and introduce formatting churn. This should be formatted
to match surrounding code style.

Code

dotnet/test/webdriver/NetworkInterceptionTests.cs[R60-61]

+             text = driver.FindElement(By.CssSelector("h1")).Text;
+             Assert.That(text, Is.EqualTo("Heading"));

Evidence

The compliance checklist requires changed code to remain format/lint clean. The added lines show
inconsistent leading whitespace compared to adjacent statements in the same block.

AGENTS.md
dotnet/test/webdriver/NetworkInterceptionTests.cs[59-61]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Newly added lines in `TestCanInterceptNetworkCalls` are misindented relative to surrounding code, which may fail the repo's formatting/lint checks.

## Issue Context
The repo includes automated formatting/lint workflows; changed code should match existing indentation/style.

## Fix Focus Areas
- dotnet/test/webdriver/NetworkInterceptionTests.cs[59-61]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. StopMonitoring not idempotent 🐞 ≡

Description

StopMonitoring() always forces Lazy<DevToolsSession>.Value creation and now issues
DisableNetwork/DisableFetch, so calling StopMonitoring without a prior StartMonitoring will create a
DevTools session and change CDP domain state instead of being a safe no-op.
This can also surface unexpected exceptions in defensive cleanup code paths.

Code

dotnet/src/webdriver/NetworkManager.cs[R95-96]

+        await this.session.Value.Domains.Network.DisableNetwork().ConfigureAwait(false);
+        await this.session.Value.Domains.Network.DisableFetch().ConfigureAwait(false);

Evidence

INetwork.StopMonitoring() does not specify a precondition that StartMonitoring must have been called
first. NetworkManager stores the session as Lazy<DevToolsSession> and dereferences session.Value
inside StopMonitoring, which triggers session creation; with this PR, StopMonitoring additionally
disables CDP Network/Fetch domains, introducing observable side effects even when monitoring was
never started.

dotnet/src/webdriver/INetwork.cs[75-90]
dotnet/src/webdriver/NetworkManager.cs[41-54]
dotnet/src/webdriver/NetworkManager.cs[89-97]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`NetworkManager.StopMonitoring()` is not idempotent: it always dereferences `Lazy<DevToolsSession>.Value` and now sends `DisableNetwork()`/`DisableFetch()` commands even if `StartMonitoring()` was never called. This means `StopMonitoring()` can unexpectedly create a DevTools session and mutate CDP state (or throw) when used in defensive cleanup paths.

### Issue Context
`INetwork.StopMonitoring()` does not document a required call order. The session is stored as `Lazy<DevToolsSession>`.

### Fix Focus Areas
- dotnet/src/webdriver/NetworkManager.cs[41-54]
- dotnet/src/webdriver/NetworkManager.cs[73-97]
- dotnet/src/webdriver/INetwork.cs[75-90]

### Suggested change
Make `StopMonitoring()` a safe no-op when monitoring was never started, e.g.:
- Add a private `bool isMonitoring` set to `true` at the end of `StartMonitoring()` and set to `false` at the end of `StopMonitoring()`.
- In `StopMonitoring()`, first check `if (!isMonitoring) return;`.
- Optionally also guard with `if (!this.session.IsValueCreated) return;` to avoid forcing DevTools session creation.

This keeps cleanup safe and prevents unintended CDP commands when the feature was never enabled.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Fetch disabled after unhook 🐞 ☼

Description

NetworkManager.StopMonitoring() detaches RequestPaused/AuthRequired/ResponsePaused handlers before
disabling the Fetch domain; during StopMonitoring execution, any request paused by Fetch will not be
continued by NetworkManager and can block navigation until Fetch is disabled.
This is a race between detaching OnRequestPaused and the later DisableFetch() call.

Code

dotnet/src/webdriver/NetworkManager.cs[R92-96]

        this.session.Value.Domains.Network.AuthRequired -= OnAuthRequired;
        this.session.Value.Domains.Network.RequestPaused -= OnRequestPaused;
        await this.session.Value.Domains.Network.EnableNetworkCaching().ConfigureAwait(false);
+        await this.session.Value.Domains.Network.DisableNetwork().ConfigureAwait(false);
+        await this.session.Value.Domains.Network.DisableFetch().ConfigureAwait(false);

Evidence

StartMonitoring enables Fetch interception for all URL patterns (request+response stages) and relies
on OnRequestPaused to always call a ContinueRequest* API to release paused requests. StopMonitoring
currently removes the OnRequestPaused subscription before calling DisableFetch, so in the
interleaving where a request is paused after unsubscription but before DisableFetch takes effect,
there is no continuation path in this component.

dotnet/src/webdriver/NetworkManager.cs[73-81]
dotnet/src/webdriver/NetworkManager.cs[89-97]
dotnet/src/webdriver/NetworkManager.cs[221-244]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`NetworkManager.StopMonitoring()` unsubscribes from `RequestPaused`/`AuthRequired`/`ResponsePaused` before disabling the CDP Fetch domain. While Fetch is still enabled, requests can still be paused, but with handlers detached there is no code path that calls `ContinueRequest*`, which can block in-flight or concurrent navigations during shutdown.

### Issue Context
Fetch interception is enabled in `StartMonitoring()` and `OnRequestPaused()` is responsible for releasing paused requests via `ContinueRequest*`.

### Fix Focus Areas
- dotnet/src/webdriver/NetworkManager.cs[89-97]
- dotnet/src/webdriver/NetworkManager.cs[73-81]
- dotnet/src/webdriver/NetworkManager.cs[221-244]

### Suggested change
Reorder `StopMonitoring()` so Fetch/Network domains are disabled **before** removing event handlers (or at least keep `OnRequestPaused` subscribed until after `DisableFetch()` completes), e.g.:
- Call `DisableFetch()` first (while handlers are still attached)
- Optionally call `DisableNetwork()` next
- Restore caching state as needed
- Then detach event handlers (potentially in a `finally` to ensure cleanup)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[Copilot]

Pull request overview

Fixes cleanup of DevTools-based network monitoring so that interception doesn’t leave subsequent requests stuck pending, and extends a regression test to confirm monitoring teardown restores normal network behavior.

Changes:

• Explicitly disables the CDP Network and Fetch domains when stopping monitoring.
• Extends the network interception test to refresh and verify the original page content is restored after monitoring stops.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
dotnet/src/webdriver/NetworkManager.cs	Ensures monitoring teardown disables the relevant CDP domains, not just event unsubscription/caching reset.
dotnet/test/webdriver/NetworkInterceptionTests.cs	Adds a post-StopMonitoring refresh/assertion to validate interception no longer affects subsequent loads.

[nvborisenko]

Finally StopMonitoring behaves by design! Thanks!