Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions java/src/org/openqa/selenium/bidi/BiDi.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

import java.io.Closeable;
import java.time.Duration;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand All @@ -34,6 +36,7 @@ public class BiDi implements Closeable {

private final Duration timeout;
private final Connection connection;
private final Map<Event<?>, List<Long>> contextListenerIds = new HashMap<>();

/**
* @deprecated Use constructor with timeout parameter: {@link #BiDi(Connection, Duration)}
Expand Down Expand Up @@ -104,7 +107,7 @@ public <X> long addListener(String browsingContextId, Event<X> event, Consumer<X
}

public <X> long addListener(Set<String> browsingContextIds, Event<X> event, Consumer<X> handler) {
Require.nonNull("List of browsing context ids", browsingContextIds);
Require.nonEmpty("List of browsing context ids", browsingContextIds);
Require.nonNull("Event to listen for", event);
Require.nonNull("Handler to call", handler);

Expand All @@ -113,7 +116,9 @@ public <X> long addListener(Set<String> browsingContextIds, Event<X> event, Cons
"session.subscribe",
Map.of("contexts", browsingContextIds, "events", List.of(event.getMethod()))));

return connection.addListener(event, handler);
long id = connection.addListener(event, handler);
contextListenerIds.computeIfAbsent(event, k -> new ArrayList<>()).add(id);
return id;
}

public <X> void clearListener(Event<X> event) {
Expand All @@ -127,6 +132,32 @@ public <X> void clearListener(Event<X> event) {
}
}

public <X> void clearListener(Set<String> browsingContextIds, Event<X> event) {
Require.nonEmpty("List of browsing context ids", browsingContextIds);
Require.nonNull("Event to listen for", event);

// The browser throws an error if we try to unsubscribe an event that was not subscribed in the
// first place
if (!connection.isEventSubscribed(event)) {
return;
}

List<Long> ids = contextListenerIds.remove(event);
if (ids != null && !ids.isEmpty()) {
// Subscription was context-specific: unsubscribe only for those contexts.
send(
new Command<>(
"session.unsubscribe",
Map.of("contexts", browsingContextIds, "events", List.of(event.getMethod()))));
ids.forEach(connection::removeListener);
Comment on lines +135 to +152
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

1. contextlistenerids map not synchronized 📘 Rule violation ☼ Reliability

BiDi introduces shared mutable state (contextListenerIds) that is mutated from public APIs
without any synchronization, even though listener callbacks/subscriptions are handled concurrently
in Connection. This can lead to lost updates, internal map/list corruption, or inconsistent
listener cleanup when multiple threads add or clear listeners at the same time.
Agent Prompt
## Issue description
`BiDi.contextListenerIds` is a shared `HashMap<Event<?>, List<Long>>` updated and removed from public APIs (e.g., `addListener(Set, ...)` and `clearListener(Set, ...)`) without synchronization. Because BiDi listener callbacks can run concurrently and `Connection` already uses locks to protect its callback registry, these unsynchronized `HashMap`/`ArrayList` operations can race, causing lost updates, internal map/list corruption, inconsistent listener cleanup/removal, and potential runtime failures.

## Issue Context
`Connection` explicitly guards its callback/subscription state with locking (e.g., a `ReadWriteLock`/`callbacksLock` around callback maps), indicating multi-threaded access is expected in this codepath. The newly introduced bookkeeping in `BiDi` should adopt a consistent thread-safety strategy (either thread-safe data structures or a dedicated lock) so that listener ID tracking remains coherent under concurrent client usage.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[34-40]
- java/src/org/openqa/selenium/bidi/BiDi.java[39-39]
- java/src/org/openqa/selenium/bidi/BiDi.java[109-121]
- java/src/org/openqa/selenium/bidi/BiDi.java[119-121]
- java/src/org/openqa/selenium/bidi/BiDi.java[135-153]

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good suggestion

} else {
// Subscription was global: context-specific unsubscription is invalid per the BiDi protocol,
// so fall back to a global unsubscription.
send(new Command<>("session.unsubscribe", Map.of("events", List.of(event.getMethod()))));
connection.clearListener(event);
}
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
Comment thread
Delta456 marked this conversation as resolved.
Comment on lines +145 to +158
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

2. String context clears globally 🐞 Bug ≡ Correctness

BiDi.addListener(String, ...) performs a context-scoped session.subscribe but does not record
the listener ID in contextListenerIds, so clearListener(Set, ...) will treat it as a global
subscription and send a global session.unsubscribe, potentially removing subscriptions beyond the
intended contexts. This can also clear all local callbacks for the event even though the caller
asked for context-scoped cleanup.
Agent Prompt
### Issue description
`BiDi.addListener(String browsingContextId, Event<X> event, Consumer<X> handler)` subscribes with `contexts: [browsingContextId]` but does not add the returned listener id to the context-specific bookkeeping used by `clearListener(Set<String>, Event<X>)`. As a result, `clearListener(Set, event)` falls into the "global" branch and performs a global `session.unsubscribe` + `connection.clearListener(event)`, which is broader than intended.

### Issue Context
- Context-specific subscription (String overload) exists and is public API.
- New context-specific clear relies on `contextListenerIds` presence to decide whether it can do context-scoped unsubscribe.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[96-121]
- java/src/org/openqa/selenium/bidi/BiDi.java[135-159]

### Suggested fix
- Record listener ids created via `addListener(String, ...)` into the same context-specific tracking used by the Set overload (or, preferably, refactor tracking to include context IDs, e.g., `Map<Event<?>, Map<String, List<Long>>>`).
- Update `clearListener(Set, event)` to use that tracking to decide whether to perform context-scoped unsubscribe vs global unsubscribe.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fair

}

public void removeListener(long id) {
connection.removeListener(id);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.io.StringReader;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Consumer;
Expand Down Expand Up @@ -240,6 +241,27 @@ private void addNavigationEventListener(String name, Consumer<NavigationInfo> co
}
}

public void clearListener(String browsingContextId) {
Require.nonNull("Browsing context id", browsingContextId);
clearListeners(Collections.singleton(browsingContextId));
}

public void clearListeners(Set<String> browsingContextIds) {
Require.nonNull("Browsing context id list", browsingContextIds);

List.of(
browsingContextCreated,
browsingContextDestroyed,
userPromptOpened,
userPromptClosed,
historyUpdated,
downloadWillBeginEvent,
downloadEndEvent)
.forEach(event -> this.bidi.clearListener(browsingContextIds, event));

navigationEventSet.forEach(event -> this.bidi.clearListener(browsingContextIds, event));
}

@Override
public void close() {
this.bidi.clearListener(browsingContextCreated);
Expand Down
12 changes: 11 additions & 1 deletion java/src/org/openqa/selenium/bidi/module/LogInspector.java
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,18 @@ private long addLogEntryAddedListener(Consumer<LogEntry> consumer) {
}
}

public void clearListener(String browsingContextId) {
Require.nonNull("Browsing context id", browsingContextId);
clearListeners(Collections.singleton(browsingContextId));
}

public void clearListeners(Set<String> browsingContextIds) {
Require.nonNull("Browsing context id list", browsingContextIds);
this.bidi.clearListener(browsingContextIds, this.logEntryAddedEvent);
}

@Override
public void close() {
this.bidi.clearListener(Log.entryAdded());
this.bidi.clearListener(this.logEntryAddedEvent);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,18 @@ public void removeListener(long subscriptionId) {
this.bidi.removeListener(subscriptionId);
}

public void clearListener(String browsingContextId) {
Require.nonNull("Browsing context id", browsingContextId);
clearListeners(Collections.singleton(browsingContextId));
}

public void clearListeners(Set<String> browsingContextIds) {
Require.nonNull("Browsing context id list", browsingContextIds);
this.bidi.clearListener(browsingContextIds, this.prefetchStatusUpdatedEvent);
}

@Override
public void close() {
this.bidi.clearListener(Speculation.prefetchStatusUpdated());
this.bidi.clearListener(this.prefetchStatusUpdatedEvent);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,15 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.openqa.selenium.testing.drivers.Browser.FIREFOX;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
Expand Down Expand Up @@ -318,6 +322,71 @@ void canListenToNavigationFailedEvent()
}
}

@Test
@NeedsFreshDriver
void canClearListenersForBrowsingContext()
throws ExecutionException, InterruptedException, TimeoutException {
try (BrowsingContextInspector inspector = new BrowsingContextInspector(driver)) {
CompletableFuture<NavigationInfo> future = new CompletableFuture<>();
inspector.onDomContentLoaded(future::complete);

BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
context.navigate(appServer.whereIs("/bidi/logEntryAdded.html"), ReadinessState.COMPLETE);

NavigationInfo navigationInfo = future.get(5, TimeUnit.SECONDS);
assertThat(navigationInfo.getBrowsingContextId()).isEqualTo(context.getId());

// Clear listeners for this browsing context
inspector.clearListener(context.getId());

// Re-subscribe and verify events still work after re-subscribing
CompletableFuture<NavigationInfo> newFuture = new CompletableFuture<>();
inspector.onDomContentLoaded(newFuture::complete);

context.navigate(appServer.whereIs("/simpleTest.html"), ReadinessState.COMPLETE);

NavigationInfo newNavigationInfo = newFuture.get(5, TimeUnit.SECONDS);
assertThat(newNavigationInfo.getBrowsingContextId()).isEqualTo(context.getId());
assertThat(newNavigationInfo.getUrl()).contains("/simpleTest.html");
}
}

@Test
@NeedsFreshDriver
void canClearListenersForMultipleBrowsingContexts()
throws ExecutionException, InterruptedException, TimeoutException {
Set<String> browsingContextIds = new HashSet<>();
browsingContextIds.add(driver.getWindowHandle());

try (BrowsingContextInspector inspector = new BrowsingContextInspector(driver)) {
List<NavigationInfo> receivedEvents = new ArrayList<>();
CountDownLatch latch = new CountDownLatch(1);
inspector.onNavigationStarted(
info -> {
receivedEvents.add(info);
latch.countDown();
});

BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
context.navigate(appServer.whereIs("/bidi/logEntryAdded.html"), ReadinessState.COMPLETE);

latch.await(5, TimeUnit.SECONDS);
assertThat(receivedEvents).hasSizeGreaterThanOrEqualTo(1);

// Clear listeners for the set of browsing context ids
inspector.clearListeners(browsingContextIds);

// Re-subscribe with a new listener after clearing
CompletableFuture<NavigationInfo> newFuture = new CompletableFuture<>();
inspector.onNavigationStarted(newFuture::complete);

context.navigate(appServer.whereIs("/simpleTest.html"), ReadinessState.COMPLETE);

NavigationInfo newNavigationInfo = newFuture.get(5, TimeUnit.SECONDS);
assertThat(newNavigationInfo.getBrowsingContextId()).isEqualTo(context.getId());
}
}

@Test
@NeedsFreshDriver
void canListenToHistoryUpdatedEvent()
Expand Down
70 changes: 70 additions & 0 deletions java/test/org/openqa/selenium/bidi/log/LogInspectorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CountDownLatch;
Expand Down Expand Up @@ -430,6 +432,74 @@ void canListenToAnyTypeOfLogForMultipleBrowsingContexts() throws InterruptedExce
}
}

@Test
@NeedsFreshDriver
void canClearListenersForBrowsingContext()
throws ExecutionException, InterruptedException, TimeoutException {
String browsingContextId = driver.getWindowHandle();
try (LogInspector logInspector = new LogInspector(driver)) {
CompletableFuture<ConsoleLogEntry> future = new CompletableFuture<>();
logInspector.onConsoleEntry(future::complete);

page = appServer.whereIs("/bidi/logEntryAdded.html");
driver.get(page);
driver.findElement(By.id("consoleLog")).click();

ConsoleLogEntry logEntry = future.get(5, TimeUnit.SECONDS);
assertThat(logEntry.getText()).isEqualTo("Hello, world!");

// Clear listeners for this browsing context
logInspector.clearListener(browsingContextId);

// Re-subscribe and verify events still work after re-subscribing
CompletableFuture<ConsoleLogEntry> newFuture = new CompletableFuture<>();
logInspector.onConsoleEntry(newFuture::complete);

driver.findElement(By.id("consoleLog")).click();

ConsoleLogEntry newLogEntry = newFuture.get(5, TimeUnit.SECONDS);
assertThat(newLogEntry.getText()).isEqualTo("Hello, world!");
}
}

@Test
@NeedsFreshDriver
void canClearListenersForMultipleBrowsingContexts()
throws ExecutionException, InterruptedException, TimeoutException {
Set<String> browsingContextIds = new HashSet<>();
browsingContextIds.add(driver.getWindowHandle());

try (LogInspector logInspector = new LogInspector(driver)) {
List<ConsoleLogEntry> receivedEntries = new ArrayList<>();
CountDownLatch latch = new CountDownLatch(1);
logInspector.onConsoleEntry(
entry -> {
receivedEntries.add(entry);
latch.countDown();
});

page = appServer.whereIs("/bidi/logEntryAdded.html");
driver.get(page);
driver.findElement(By.id("consoleLog")).click();

latch.await(5, TimeUnit.SECONDS);
assertThat(receivedEntries).hasSize(1);
assertThat(receivedEntries.get(0).getText()).isEqualTo("Hello, world!");

// Clear listeners for the set of browsing context ids
logInspector.clearListeners(browsingContextIds);

// Re-subscribe with a new listener after clearing
CompletableFuture<ConsoleLogEntry> newFuture = new CompletableFuture<>();
logInspector.onConsoleEntry(newFuture::complete);

driver.findElement(By.id("consoleLog")).click();

ConsoleLogEntry newLogEntry = newFuture.get(5, TimeUnit.SECONDS);
assertThat(newLogEntry.getText()).isEqualTo("Hello, world!");
}
}

@Test
@NeedsFreshDriver
void canListenToLogsWithMultipleConsumers()
Expand Down
Loading
Loading