[remote] Ensure that asyncio event loop outlives webdriver connection

Depends on D122577

When using the BiDiSession class we need the event loop to live at
least as long as a specific connection, but not necessarily as long as
the BiDISession object itself. Therefore we move the loop parameter
into the start() method, to associate a loop with a specific connection.

In addition, in the tests we make the event loop a global so that it
lives as long as a session; the fixture is updated to reuse the
existing loop where possible. This has the tradeoff that we're more
likely to share a loop between tests, so there is some additional
hidden shared state.

Differential Revision: https://phabricator.services.mozilla.com/D124834

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1694143
gecko-commit: 908c3e4788588a371b10b01e2e079df2cfb28b70
gecko-reviewers: webdriver-reviewers, whimboo

tools/webdriver/webdriver/bidi/client.py

@@ -64,8 +64,7 @@ def __init__(self,
                  websocket_url: str,
                  session_id: Optional[str] = None,
                  capabilities: Optional[Mapping[str, Any]] = None,
-                 requested_capabilities: Optional[Mapping[str, Any]] = None,
-                 loop: Optional[asyncio.AbstractEventLoop] = None):
+                 requested_capabilities: Optional[Mapping[str, Any]] = None):
         self.transport: Optional[Transport] = None
 
         # The full URL for a websocket looks like
@@ -103,15 +102,10 @@ def __init__(self,
         # For each module, have a property representing that module
         self.session = Session(self)
 
-        if loop is None:
-            loop = get_running_loop()
-        self.loop = loop
-
     @classmethod
     def from_http(cls,
                   session_id: str,
-                  capabilities: Mapping[str, Any],
-                  loop: Optional[asyncio.AbstractEventLoop] = None) -> "BidiSession":
+                  capabilities: Mapping[str, Any]) -> "BidiSession":
         """Create a BiDi session from an existing HTTP session
 
         :param session_id: String id of the session
@@ -121,18 +115,17 @@ def from_http(cls,
             raise ValueError("No webSocketUrl found in capabilities")
         if not isinstance(websocket_url, str):
             raise ValueError("webSocketUrl is not a string")
-        return cls(websocket_url, session_id=session_id, capabilities=capabilities, loop=loop)
+        return cls(websocket_url, session_id=session_id, capabilities=capabilities)
 
     @classmethod
     def bidi_only(cls,
                   websocket_url: str,
-                  requested_capabilities: Optional[Mapping[str, Any]],
-                  loop: Optional[asyncio.AbstractEventLoop] = None) -> "BidiSession":
+                  requested_capabilities: Optional[Mapping[str, Any]]) -> "BidiSession":
         """Create a BiDi session where there is no existing HTTP session
 
         :param webdocket_url: URL to the WebSocket server listening for BiDi connections
         :param requested_capabilities: Capabilities request for establishing the session."""
-        return cls(websocket_url, requested_capabilities=requested_capabilities, loop=loop)
+        return cls(websocket_url, requested_capabilities=requested_capabilities)
 
     async def __aenter__(self) -> "BidiSession":
         await self.start()
@@ -141,9 +134,13 @@ async def __aenter__(self) -> "BidiSession":
     async def __aexit__(self, *args: Any) -> None:
         await self.end()
 
-    async def start(self) -> None:
+    async def start(self,
+                    loop: Optional[asyncio.AbstractEventLoop] = None) -> None:
         """Connect to the WebDriver BiDi remote via WebSockets"""
-        self.transport = Transport(self.websocket_url, self.on_message, loop=self.loop)
+
+        if loop is None:
+            loop = get_running_loop()
+        self.transport = Transport(self.websocket_url, self.on_message, loop=loop)
 
         if self.session_id is None:
             self.session_id, self.capabilities = await self.session.new(self.requested_capabilities)
@@ -162,8 +159,8 @@ async def send_command(self, method: str, params: Mapping[str, Any]) -> Awaitabl
             "params": params
         }
         assert command_id not in self.pending_commands
-        self.pending_commands[command_id] = self.loop.create_future()
         assert self.transport is not None
+        self.pending_commands[command_id] = self.transport.loop.create_future()
         await self.transport.send(body)
 
         return self.pending_commands[command_id]
@@ -201,6 +198,7 @@ async def end(self) -> None:
         """Close websocket connection."""
         assert self.transport is not None
         await self.transport.end()
+        self.transport = None
 
     def add_event_listener(self,
                            name: Optional[str],

webdriver/tests/support/fixtures.py

@@ -14,7 +14,12 @@
 from tests.support.http_request import HTTPRequest
 
 
+# The webdriver session can outlive a pytest session
 _current_session = None
+
+# The event loop needs to outlive the webdriver session
+_event_loop = None
+
 _custom_session = False
 
 
@@ -39,15 +44,14 @@ def pytest_generate_tests(metafunc):
             metafunc.parametrize("capabilities", marker.args, ids=None)
 
 
-# Ensure that the event loop is restarted once per session rather than the default
-# of once per test. If we don't do this, tests will try to reuse a closed event
-# loop and fail with an error that the "future belongs to a different loop".
 @pytest.fixture(scope="session")
 def event_loop():
-    """Change event_loop fixture to session level."""
-    loop = asyncio.get_event_loop_policy().new_event_loop()
-    yield loop
-    loop.close()
+    """Change event_loop fixture to global."""
+    global _event_loop
+
+    if _event_loop is None:
+        _event_loop = asyncio.get_event_loop_policy().new_event_loop()
+    return _event_loop
 
 
 @pytest.fixture
@@ -90,7 +94,7 @@ async def reset_current_session_if_necessary(caps):
 
 
 @pytest.fixture(scope="function")
-async def session(capabilities, configuration, request):
+async def session(capabilities, configuration):
     """Create and start a session for a test that does not itself test session creation.
 
     By default the session will stay open after each test, but we always try to start a
@@ -127,7 +131,7 @@ async def session(capabilities, configuration, request):
 
 
 @pytest.fixture(scope="function")
-async def bidi_session(capabilities, configuration, request):
+async def bidi_session(capabilities, configuration):
     """Create and start a bidi session.
 
     Can be used for a test that does not itself test bidi session creation.
@@ -159,8 +163,9 @@ async def bidi_session(capabilities, configuration, request):
     await _current_session.bidi_session.start()
 
     # Enforce a fixed default window size and position
-    _current_session.window.size = defaults.WINDOW_SIZE
-    _current_session.window.position = defaults.WINDOW_POSITION
+    if _current_session.capabilities.get("setWindowRect"):
+        _current_session.window.size = defaults.WINDOW_SIZE
+        _current_session.window.position = defaults.WINDOW_POSITION
 
     yield _current_session.bidi_session