[🐛 Bug]: Calling start_devtools() in 2 different drivers doesn't work

[victorvianna]

Description

Repro

git am the patch below and run the new test with bazel test py:common-chrome
https://gist.github.com/victorvianna/d3930c0962b8fe322d6341b3f29f1fe5

Expected: works fine

Actual: "local variable 'ws_url' referenced before assignment"

We have concrete cases in Chromium benchmarks where this is a problem.

Root cause

devtools is a global and thus outlives individual WebDrivers. When start_devtools() is called the second time, the if not devtools block containing ws_url initialization is skipped

Discussion

Having the devtools auto-generated module be a global seems conceptually wrong. For one thing, this value depends on the browser version (the protocol changes over time). The least invasive change is probably to turn it into a WebDriver member like _websocket_connection.

On a separate note, I wonder:

1. Why was a global used in the first place? Would it be costly to include the module again on every call? Because bidi_connection() is doing exactly that ATM (notice how there's no global devtools line in the function)

2. _websocket_connection a single central websocket, shared with other methods. But why limit to a single socket, why not return a new one on every call? Does the browser not support it? One advantage I could imagine is guaranteeing that WebDriver calls close() when its done, but that's not done right now AFAICT

Reproducible Code

See above

Debugging Logs

See above

[selenium-ci]

@victorvianna, thank you for creating this issue. We will troubleshoot it as soon as we can.

Selenium Triage Team: remember to follow the Triage Guide

[victorvianna]

@p0deje

[victorvianna]

@titusfortner

[victorvianna]

_websocket_connection a single central websocket, shared with other methods. But why limit to a single socket

Also, it seems like the socket URL can even depend on the method? Some methods use webSocketUrl capability, whereas others use se:cdp

[cgoldberg]

@victorvianna this seems like a bug. New drivers should be able to be created without leftover state from a previous instance.

the if not devtools block containing ws_url initialization is skipped

We should at least set it back to None when the driver quits, but ideally not use a global at all.

Feel free to submit a PR.

[p0deje]

While implementing, I followed the initial CDP approach which used globals, but if there is a way to refactor to avoid it (I'm not a Python expert), I'm all for it.