Skip to content

fix: replace FirefoxBinary with Selenium 4 equivalents (fixes #222) #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FernandoCelmer wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: replace FirefoxBinary with Selenium 4 equivalents (fixes #222) #223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 13 additions & 7 deletions tbselenium/tbbinary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,22 @@
from selenium.webdriver.firefox.firefox_binary import FirefoxBinary
import subprocess

Copy link
Copy Markdown
Author

@FernandoCelmer FernandoCelmer Mar 25, 2026

Choose a reason for hiding this comment

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

[Suggestion]

Problem — import subprocess is added at the top of the file but is never used anywhere in the module. The new TBBinary class has no launch_browser() or similar method that calls subprocess.Popen. This dead import will trigger flake8 F401 lint errors and misleads readers into thinking TBBinary is responsible for spawning the browser process.

Failure scenario — Any CI pipeline running flake8 will flag this and fail. A developer reading the file will naturally look for where subprocess is used and waste time concluding it was an incomplete implementation.

Fix — Remove import subprocess. If a launch_browser() method is added in a follow-up, the import can be restored then.


class TBBinary(FirefoxBinary):
class TBBinary:
'''
Extend FirefoxBinary to better handle terminated browser processes.
Manage the Tor Browser (Firefox) binary process.

FirefoxBinary was removed in Selenium 4. This class replaces the previous
TBBinary(FirefoxBinary) subclass and provides only the process-management
functionality that tbselenium relies on internally.
'''

def kill(self):
"""Kill the browser.
def __init__(self, firefox_path=None, log_file=None):
self.firefox_path = firefox_path
self.log_file = log_file
self.process = None
Copy link
Copy Markdown
Author

@FernandoCelmer FernandoCelmer Mar 25, 2026

Choose a reason for hiding this comment

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

[Suggestion]

Problem — self.log_file = log_file is stored in __init__ but is never read by any method in the class. The original FirefoxBinary used the log_file handle to redirect browser stdout/stderr to a file when launching the process. Since TBBinary has no process-launching code, the file handle is silently discarded.

Failure scenario — A caller uses driver.get_tb_binary(logfile='/var/log/tb.log') expecting to capture browser output for debugging. No output is written to the file, no error is raised, and the open file handle is leaked (never closed).

Fix — If log redirection is out of scope for this PR, remove the log_file parameter from __init__ and update get_tb_binary() in tbdriver.py accordingly. Alternatively, add a comment like # log_file is not yet used; process launch not implemented so callers are not misled by the parameter's presence.


This is useful when the browser is stuck.
"""
def kill(self):
"""Kill the browser process if it is still running."""
Copy link
Copy Markdown
Author

@FernandoCelmer FernandoCelmer Mar 25, 2026

Choose a reason for hiding this comment

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

[Blocking]

Problem — kill() will always be a no-op because self.process is initialized to None in __init__ and is never assigned anywhere in the codebase. The original FirefoxBinary base class populated self.process via its own launch_browser() mechanism, which Selenium 3 invoked internally when starting the browser. That hook no longer exists in Selenium 4, and the new standalone TBBinary class has no equivalent — no launch_browser(), no start(), nothing that sets self.process to a live subprocess.Popen handle.

The presence of import subprocess at the top of this file suggests a launch_browser() method was intended but never implemented, leaving kill() as permanently unreachable logic.

Failure scenario — A user calls driver.get_tb_binary().kill() expecting it to terminate a stuck browser process. Instead, the guard if self.process and self.process.poll() is None evaluates to False immediately (since self.process is None) and the method returns silently. The browser is never killed.

Fix — Either implement a launch_browser() / start() method using subprocess.Popen that sets self.process, or explicitly document that the caller is responsible for assigning self.process before calling kill(). If there is no valid use case for this class in the current codebase, consider removing TBBinary and get_tb_binary() entirely to avoid a public API that silently does nothing.

if self.process and self.process.poll() is None:
self.process.kill()
self.process.wait()
18 changes: 8 additions & 10 deletions tbselenium/tbdriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,22 +79,20 @@ def __init__(self,
if use_custom_profile:
print(f'Using custom profile: {self.tbb_profile_path}')
tbb_service = Service(
executable_path=executable_path,
log_path=tbb_logfile_path, # TODO: deprecated, use log_output
executable=executable_path,
log_output=tbb_logfile_path,
service_args=["--marionette-port", "2828"],
port=geckodriver_port
)
else:
tbb_service = Service(
executable_path=executable_path,
log_path=tbb_logfile_path,
executable=executable_path,
log_output=tbb_logfile_path,
port=geckodriver_port
)
# options.binary is path to the Firefox binary and it can be a string
# or a FirefoxBinary object. If it's a string, it will be converted to
# a FirefoxBinary object.
# https://github.com/SeleniumHQ/selenium/blob/7cfd137085fcde932cd71af78642a15fd56fe1f1/py/selenium/webdriver/firefox/options.py#L54
self.options.binary = self.tbb_fx_binary_path
# Use binary_location (Selenium 4 API) to point to the TBB Firefox
# binary. FirefoxBinary and options.binary were removed in Selenium 4.
self.options.binary_location = self.tbb_fx_binary_path
self.options.add_argument('--class')
self.options.add_argument('"Tor Browser"')
if headless:
Expand Down Expand Up @@ -309,7 +307,7 @@ def export_env_vars(self):
prepend_to_env_var("PATH", self.tbb_browser_dir)

def get_tb_binary(self, logfile=None):
"""Return FirefoxBinary pointing to the TBB's firefox binary."""
"""Return a TBBinary instance pointing to the TBB's firefox binary."""
tbb_logfile = open(logfile, 'a+') if logfile else None
return TBBinary(firefox_path=self.tbb_fx_binary_path,
log_file=tbb_logfile)
Expand Down
19 changes: 5 additions & 14 deletions tbselenium/test/test_browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,9 @@ def tearDownClass(cls):

def test_correct_firefox_binary(self):
"""Make sure we use the Firefox binary from the TBB directory."""
try:
# driver.binary was removed in selenium-4.10.0
# https://github.com/SeleniumHQ/selenium/pull/12030/files#diff-89ba579445647535b74423c7bf4b8be79ef1ce33847a2768e623c3083a33545dL127
tbbinary = self.driver.options.binary
except AttributeError:
tbbinary = self.driver.binary
self.assertTrue(tbbinary.which('firefox').startswith(TBB_PATH))
# binary_location is the Selenium 4 API (a plain string path)
tbbinary_path = self.driver.options.binary_location
self.assertTrue(tbbinary_path.startswith(TBB_PATH))

# TODO: log output is always empty
@pytest.mark.xfail
Expand All @@ -54,13 +50,8 @@ def test_should_load_tbb_firefox_libs(self):
driver = self.driver
geckodriver_pid = driver.service.process.pid
process = psutil.Process(geckodriver_pid)
try:
# driver.binary was removed in selenium-4.10.0
# https://github.com/SeleniumHQ/selenium/pull/12030/files#diff-89ba579445647535b74423c7bf4b8be79ef1ce33847a2768e623c3083a33545dL127
tbbinary = self.driver.options.binary
except AttributeError:
tbbinary = self.driver.binary
tbbinary_path = tbbinary.which('firefox') + FF_BINARY_SUFFIX
# binary_location is the Selenium 4 API (a plain string path)
tbbinary_path = self.driver.options.binary_location + FF_BINARY_SUFFIX
for child in process.children():
if tbbinary_path == child.exe():
tb_pid = child.pid
Expand Down