Skip to content

Commit

Permalink
[java][bidi] Close BiDi connection on webdriver quit command
Browse files Browse the repository at this point in the history
  • Loading branch information
@pujagani
pujagani committed Dec 7, 2022
1 parent b529aba commit 7a469e0
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 11 deletions.
27 changes: 24 additions & 3 deletions java/src/org/openqa/selenium/bidi/BUILD.bazel
View file
Original file line number Diff line number Diff line change
@@ -1,24 +1,45 @@
load("@rules_jvm_external//:defs.bzl", "artifact")
load("//java:defs.bzl", "java_library")

AUGMENTER_SRCS = [
"BiDiProvider.java",
]

java_library(
name = "augmenter",
srcs = AUGMENTER_SRCS,
visibility = [
"//java/src/org/openqa/selenium/remote:__pkg__",
],
deps = [
":bidi",
"//java:auto-service",
"//java/src/org/openqa/selenium:core",
"//java/src/org/openqa/selenium/remote:api",
"//java/src/org/openqa/selenium/remote/http",
],
)

java_library(
name = "bidi",
srcs = glob([
"*.java",
"log/*.java",
"browsingcontext/*.java"
]),
],
exclude = AUGMENTER_SRCS,
),
visibility = [
"//java/src/org/openqa/selenium/bidi:__subpackages__",
"//java/src/org/openqa/selenium/firefox:__subpackages__",
"//java/src/org/openqa/selenium/remote:__pkg__",
"//java/test/org/openqa/selenium/bidi:__subpackages__",
"//java/test/org/openqa/selenium/grid:__subpackages__",
],
deps = [
"//java:auto-service",
"//java/src/org/openqa/selenium:core",
"//java/src/org/openqa/selenium/json",
"//java/src/org/openqa/selenium/remote",
"//java/src/org/openqa/selenium/remote/http",
artifact("com.google.guava:guava"),
],
)
8 changes: 0 additions & 8 deletions java/src/org/openqa/selenium/bidi/BiDiProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,6 @@
import com.google.auto.service.AutoService;

import org.openqa.selenium.Capabilities;
import org.openqa.selenium.devtools.CdpEndpointFinder;
import org.openqa.selenium.devtools.CdpInfo;
import org.openqa.selenium.devtools.CdpVersionFinder;
import org.openqa.selenium.devtools.DevTools;
import org.openqa.selenium.devtools.HasDevTools;
import org.openqa.selenium.devtools.SeleniumCdpConnection;
import org.openqa.selenium.devtools.noop.NoOpCdpInfo;
import org.openqa.selenium.remote.AugmenterProvider;
import org.openqa.selenium.remote.ExecuteMethod;
import org.openqa.selenium.remote.http.ClientConfig;
Expand All @@ -36,7 +29,6 @@
import java.net.URISyntaxException;
import java.util.Optional;
import java.util.function.Predicate;
import java.util.logging.Logger;

@AutoService(AugmenterProvider.class)
public class BiDiProvider implements AugmenterProvider<HasBiDi> {
Expand Down
4 changes: 4 additions & 0 deletions java/src/org/openqa/selenium/remote/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ java_export(
exports = [
":api",
"//java/src/org/openqa/selenium:core",
"//java/src/org/openqa/selenium/bidi",
"//java/src/org/openqa/selenium/bidi:augmenter",
"//java/src/org/openqa/selenium/devtools",
"//java/src/org/openqa/selenium/devtools:augmenter",
"//java/src/org/openqa/selenium/io",
Expand All @@ -44,11 +46,13 @@ java_library(
],
visibility = [
"//java/src/org/openqa/selenium/devtools:__pkg__",
"//java/src/org/openqa/selenium/bidi:__pkg__",
],
exports = [
],
deps = [
"//java/src/org/openqa/selenium:core",
"//java/src/org/openqa/selenium/bidi",
"//java/src/org/openqa/selenium/concurrent",
"//java/src/org/openqa/selenium/devtools",
"//java/src/org/openqa/selenium/io",
Expand Down
6 changes: 6 additions & 0 deletions java/src/org/openqa/selenium/remote/RemoteWebDriver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.WebElement;
import org.openqa.selenium.WindowType;
import org.openqa.selenium.bidi.BiDi;
import org.openqa.selenium.bidi.HasBiDi;
import org.openqa.selenium.devtools.DevTools;
import org.openqa.selenium.devtools.HasDevTools;
import org.openqa.selenium.interactions.Interactive;
Expand Down Expand Up @@ -444,6 +446,10 @@ public void quit() {
((HasDevTools) this).maybeGetDevTools().ifPresent(DevTools::close);
}

if (this instanceof HasBiDi) {
((HasBiDi) this).maybeGetBiDi().ifPresent(BiDi::close);
}

execute(DriverCommand.QUIT);
} finally {
sessionId = null;
Expand Down

5 comments on commit 7a469e0

@titusfortner
Copy link
Member

Choose a reason for hiding this comment

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

@diemol / @pujagani what happens if a user calls driver.close and it ends the session. Are they going to face this issue?

@diemol
Copy link
Member

@diemol diemol commented on 7a469e0 Jan 6, 2023

Choose a reason for hiding this comment

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

Which issue? I guess @pujagani has more context?

@pujagani
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to need more context on the issue you are referring to.

@titusfortner
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm talking about closing the browser window — https://w3c.github.io/webdriver/#close-window
If it's the last window it will also end the session, but I don't think we've covered that case as far as also closing the bidi session. Someone reported an error on Slack and it seemed to go away when they switched to using quit instead of close.

@pujagani
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay. I see your point. You are right, we have not covered that case. I will look into ensuring that if is the last window that ends the session, then even the BiDi session is closed. So that the clean-up is done correctly. Thank you for bringing up this use case.

Please sign in to comment.