[java][bidi] Clear fetchError listener in Network.close() - #17903
Open
Mochxd wants to merge 1 commit into
Open
Conversation
|
|
selenium-ci
added
C-java
Contributor
PR Summary by QodoClear BiDi fetchError listener when Network is closed
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
|
Code review by qodo was updated up to the latest commit abc7ffa |
Mochxd
force-pushed
the
fix-bidi-network-close-fetch-error
branch
from
August 11, 2026 20:42
abc7ffa to
0a615a0
Compare
| @Override | ||
| public void close() { | ||
| this.bidi.clearListener(beforeRequestSentEvent); | ||
| this.bidi.clearListener(fetchErrorEvent); |
Contributor
There was a problem hiding this comment.
1. Close clears shared fetcherror 🐞 Bug ≡ Correctness
Network.close() now calls bidi.clearListener(fetchErrorEvent), which unsubscribes the 'network.fetchError' event by event name and removes all callbacks for that event from the shared BiDi connection. Closing one Network instance can therefore disable other fetchError listeners still in use on the same connection.
Agent Prompt
### Issue description
`Network.close()` uses `bidi.clearListener(fetchErrorEvent)`, which unsubscribes by event name and clears *all* callbacks registered for `network.fetchError` on the shared `BiDi` connection. This can break other components/tests that still have active `network.fetchError` listeners when any `Network` instance is closed.
### Issue Context
- `Network.onFetchError(...)` calls `bidi.addListener(...)` but does not retain the returned subscription id, so `close()` cannot currently unsubscribe only the listener(s) created by this `Network` instance.
- `BiDi.clearListener(Event)` performs `session.unsubscribe` by `events:[eventMethod]` and then `connection.clearListener(event)` drops the entire callback map for that event.
### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/module/Network.java[157-204]
### Suggested fix approach
1. In `Network`, store the subscription id(s) returned by `bidi.addListener(...)` when registering `fetchErrorEvent` (and ideally the other events too for consistency).
2. In `close()`, call `bidi.removeListener(subscriptionId)` for the stored id(s) instead of `bidi.clearListener(fetchErrorEvent)`.
3. (Optional but recommended) Add/adjust a regression test for the multi-listener case: register `fetchError` on one `Network`, create and close a second `Network`, then verify the first still receives a `fetchError` event (skip browsers where `network.fetchError` is not delivered).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 0a615a0 |
Mochxd
force-pushed
the
fix-bidi-network-close-fetch-error
branch
from
August 11, 2026 20:56
0a615a0 to
9d55e05
Compare
| @Override | ||
| public void close() { | ||
| this.bidi.clearListener(beforeRequestSentEvent); | ||
| this.bidi.clearListener(fetchErrorEvent); |
Contributor
There was a problem hiding this comment.
1. Cross-binding network.close() mismatch 📘 Rule violation ≡ Correctness
Java Network.close() now clears fetchErrorEvent, but the JavaScript BiDi Network.close() still unsubscribes only four network events and does not include network.fetchError. This risks user-visible divergence across bindings unless aligned or explicitly documented as intentional.
Agent Prompt
## Issue description
The Java BiDi `Network.close()` behavior changed to clear the `network.fetchError` listener, but the JavaScript binding’s `Network.close()` does not unsubscribe from `network.fetchError`, creating a cross-language behavior mismatch.
## Issue Context
PR Compliance requires comparing other bindings for user-visible behavior changes and either keeping behavior consistent or documenting intentional divergence.
## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/module/Network.java[198-203]
- java/test/org/openqa/selenium/bidi/network/NetworkEventsTest.java[179-196]
- javascript/selenium-webdriver/bidi/network.js[401-422]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 9d55e05 |
Mochxd
force-pushed
the
fix-bidi-network-close-fetch-error
branch
from
August 11, 2026 21:06
9d55e05 to
d67b0db
Compare
Contributor
|
Code review by qodo was updated up to the latest commit d67b0db |
Mochxd
force-pushed
the
fix-bidi-network-close-fetch-error
branch
from
August 11, 2026 21:12
d67b0db to
a2b984f
Compare
| @Override | ||
| public void close() { | ||
| this.bidi.clearListener(beforeRequestSentEvent); | ||
| this.bidi.clearListener(fetchErrorEvent); |
Contributor
There was a problem hiding this comment.
1. network.close() missing javadoc 📘 Rule violation ✧ Quality
Network.close() is a public method that was modified in this change, but it has no Javadoc comment. This violates the requirement that all changed public API methods in non-test code include complete Javadoc.
Agent Prompt
## Issue description
`Network.close()` is a changed public API method in non-test code but has no Javadoc block comment, which violates the requirement for complete Javadoc on public API methods.
## Issue Context
The method is `public` and was modified in this PR (added `clearListener(fetchErrorEvent)`), so it falls under the “changed public method” scope.
## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/module/Network.java[197-204]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit a2b984f |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
None. I noticed this while reading through the BiDi
Networkmodule.💥 What does this PR do?
onFetchError()subscribes tonetwork.fetchError, butclose()never cleared it. The other four network events all get cleared, so this one looks like it was just missed. The handler stays alive after theNetworkis closed and can still fire on a later navigation.Adds the missing
clearListenercall, plus a test that closes theNetworkand checks that no event arrives.🔧 Implementation Notes
Kept it in line with the existing clear calls, so there's no API change.
BiDi.clearListeneralready checksisEventSubscribedfirst, so the extra call does nothing ifonFetchErrorwas never used.The test is
@Ignored on Chrome and Edge because they don't delivernetwork.fetchErrorat all, which is also whycanListenToFetchErroris annotated for them.@NotYetImplementedwould be wrong here, since the harness treats those as expected to fail and this test passes. Firefox is where it actually runs.🤖 AI assistance
💡 Additional Considerations
The JS binding has the same gap:
bidi/network.jsexposesfetchError()but itsclose()unsubscribes only the same four events. JavaScript.close()also missesrealmCreated/realmDestroyed. I left both out to keep this PR to one thing, happy to send follow-ups for either.🔄 Types of changes