From 3dcc41193d5fc7130c4fbab710643a3f91529165 Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Tue, 20 Apr 2021 17:08:36 +0200
Subject: [PATCH 01/11] feat: added BrowserType.connect

---
 playwright/_impl/_browser.py             |  15 ++-
 playwright/_impl/_browser_type.py        |  25 +++-
 playwright/_impl/_connection.py          |  16 ++-
 playwright/_impl/_transport.py           | 106 ++++++++++++++--
 playwright/async_api/_context_manager.py |   3 +-
 playwright/async_api/_generated.py       |  32 +++++
 playwright/sync_api/_context_manager.py  |   5 +-
 playwright/sync_api/_generated.py        |  32 +++++
 scripts/expected_api_mismatch.txt        |   3 -
 tests/async/test_browsertype_connect.py  | 150 +++++++++++++++++++++++
 tests/conftest.py                        |  35 +++++-
 11 files changed, 396 insertions(+), 26 deletions(-)
 create mode 100644 tests/async/test_browsertype_connect.py

diff --git a/playwright/_impl/_browser.py b/playwright/_impl/_browser.py
index a72de5c67..11eb24200 100644
--- a/playwright/_impl/_browser.py
+++ b/playwright/_impl/_browser.py
@@ -50,6 +50,7 @@ def __init__(
         self._is_connected = True
         self._is_closed_or_closing = False
         self._is_remote = False
+        self._is_connected_over_websocket = False
 
         self._contexts: List[BrowserContext] = []
         self._channel.on("close", lambda _: self._on_close())
@@ -59,7 +60,7 @@ def __repr__(self) -> str:
 
     def _on_close(self) -> None:
         self._is_connected = False
-        self.emit(Browser.Events.Disconnected)
+        self.emit(Browser.Events.Disconnected, self)
         self._is_closed_or_closing = True
 
     @property
@@ -145,6 +146,10 @@ async def new_page(
         return page
 
     async def close(self) -> None:
+        if self._is_connected_over_websocket:
+            await self._connection.stop_async()
+            self._notify_remote_closed()
+            return
         if self._is_closed_or_closing:
             return
         self._is_closed_or_closing = True
@@ -154,6 +159,14 @@ async def close(self) -> None:
             if not is_safe_close_error(e):
                 raise e
 
+    def _notify_remote_closed(self) -> None:
+        # Emulate all pages, contexts and the browser closing upon disconnect.
+        for context in self.contexts:
+            for page in context.pages:
+                page._on_close()
+            context._on_close()
+        self._on_close()
+
     @property
     def version(self) -> str:
         return self._initializer["version"]
diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py
index bb42e4aae..cfe94dcf1 100644
--- a/playwright/_impl/_browser_type.py
+++ b/playwright/_impl/_browser_type.py
@@ -23,11 +23,7 @@
 )
 from playwright._impl._browser import Browser, normalize_context_params
 from playwright._impl._browser_context import BrowserContext
-from playwright._impl._connection import (
-    ChannelOwner,
-    from_channel,
-    from_nullable_channel,
-)
+from playwright._impl._connection import ChannelOwner, Connection, from_channel
 from playwright._impl._helper import (
     BrowserChannel,
     ColorScheme,
@@ -35,6 +31,7 @@
     locals_to_params,
     not_installed_error,
 )
+from playwright._impl._transport import WebSocketTransport
 
 
 class BrowserType(ChannelOwner):
@@ -157,6 +154,24 @@ async def connect_over_cdp(
         if default_context:
             browser._contexts.append(default_context)
             default_context._browser = browser
+        return browser
+
+    async def connect(
+        self, ws_endpoint: str, timeout: float = None, slow_mo: float = None
+    ) -> Browser:
+        transport = WebSocketTransport(ws_endpoint, timeout)
+
+        connection = Connection(None, self._connection._object_factory, transport)
+        connection._loop = self._connection._loop
+        connection._loop.create_task(connection.run())
+        playwright = await connection.wait_for_object_with_known_name("Playwright")
+        pre_launched_browser = playwright._initializer.get("preLaunchedBrowser")
+        assert pre_launched_browser
+        browser = cast(Browser, from_channel(pre_launched_browser))
+        browser._is_remote = True
+        browser._is_connected_over_websocket = True
+
+        transport.on("close", browser._on_close)
 
         return browser
 
diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py
index fac8281de..48af2c7fb 100644
--- a/playwright/_impl/_connection.py
+++ b/playwright/_impl/_connection.py
@@ -44,7 +44,12 @@ async def inner_send(
         if params is None:
             params = {}
         callback = self._connection._send_message_to_server(self._guid, method, params)
-        result = await callback.future
+
+        done, pending = await asyncio.wait(
+            {callback.future, self._connection._transport.on_error_future},
+            return_when=asyncio.FIRST_COMPLETED,
+        )
+        result = next(iter(done)).result()
         # Protocol now has named return values, assume result is one level deeper unless
         # there is explicit ambiguity.
         if not result:
@@ -143,10 +148,13 @@ def __init__(self, connection: "Connection") -> None:
 
 class Connection:
     def __init__(
-        self, dispatcher_fiber: Any, object_factory: Any, driver_executable: Path
+        self,
+        dispatcher_fiber: Any,
+        object_factory: Callable[[ChannelOwner, str, str, Dict], Any],
+        transport: Transport,
     ) -> None:
-        self._dispatcher_fiber: Any = dispatcher_fiber
-        self._transport = Transport(driver_executable)
+        self._dispatcher_fiber = dispatcher_fiber
+        self._transport = transport
         self._transport.on_message = lambda msg: self._dispatch(msg)
         self._waiting_for_object: Dict[str, Any] = {}
         self._last_id = 0
diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py
index 4dd37337d..83a7c9c97 100644
--- a/playwright/_impl/_transport.py
+++ b/playwright/_impl/_transport.py
@@ -17,8 +17,14 @@
 import json
 import os
 import sys
+from abc import ABC, abstractmethod
 from pathlib import Path
-from typing import Dict, Optional
+from typing import Any, Dict, Optional
+
+import websockets
+from pyee import AsyncIOEventEmitter
+
+from playwright._impl._api_types import Error
 
 
 # Sourced from: https://github.com/pytest-dev/pytest/blob/da01ee0a4bb0af780167ecd228ab3ad249511302/src/_pytest/faulthandler.py#L69-L77
@@ -34,10 +40,44 @@ def _get_stderr_fileno() -> Optional[int]:
         return sys.__stderr__.fileno()
 
 
-class Transport:
+class Transport(ABC):
+    def __init__(self) -> None:
+        self.on_message = lambda _: None
+        self.on_error_future: asyncio.Future = asyncio.Future()
+
+    @abstractmethod
+    def stop(self) -> None:
+        pass
+
+    @abstractmethod
+    async def wait_until_stopped(self) -> None:
+        pass
+
+    @abstractmethod
+    async def run(self) -> None:
+        pass
+
+    @abstractmethod
+    def send(self, message: Dict) -> None:
+        pass
+
+    def serialize_message(self, message: Dict) -> Any:
+        msg = json.dumps(message)
+        if "DEBUGP" in os.environ:  # pragma: no cover
+            print("\x1b[32mSEND>\x1b[0m", json.dumps(message, indent=2))
+        return msg.encode()
+
+    def deserialize_message(self, data: bytes) -> Any:
+        obj = json.loads(data)
+
+        if "DEBUGP" in os.environ:  # pragma: no cover
+            print("\x1b[33mRECV>\x1b[0m", json.dumps(obj, indent=2))
+        return obj
+
+
+class PipeTransport(Transport):
     def __init__(self, driver_executable: Path) -> None:
         super().__init__()
-        self.on_message = lambda _: None
         self._stopped = False
         self._driver_executable = driver_executable
         self._loop: asyncio.AbstractEventLoop
@@ -79,10 +119,8 @@ async def run(self) -> None:
                         buffer = buffer + data
                     else:
                         buffer = data
-                obj = json.loads(buffer)
 
-                if "DEBUGP" in os.environ:  # pragma: no cover
-                    print("\x1b[33mRECV>\x1b[0m", json.dumps(obj, indent=2))
+                obj = self.deserialize_message(buffer)
                 self.on_message(obj)
             except asyncio.IncompleteReadError:
                 break
@@ -90,10 +128,58 @@ async def run(self) -> None:
         self._stopped_future.set_result(None)
 
     def send(self, message: Dict) -> None:
-        msg = json.dumps(message)
-        if "DEBUGP" in os.environ:  # pragma: no cover
-            print("\x1b[32mSEND>\x1b[0m", json.dumps(message, indent=2))
-        data = msg.encode()
+        data = self.serialize_message(message)
         self._output.write(
             len(data).to_bytes(4, byteorder="little", signed=False) + data
         )
+
+
+class WebSocketTransport(AsyncIOEventEmitter, Transport):
+    def __init__(self, ws_endpoint: str, timeout: float) -> None:
+        super().__init__()
+        Transport.__init__(self)
+
+        self._stopped = False
+        self.ws_endpoint = ws_endpoint
+        self.timeout = timeout
+        self._loop: asyncio.AbstractEventLoop
+
+    def stop(self) -> None:
+        self._stopped = True
+        self._loop.create_task(self._connection.close())
+
+    async def wait_until_stopped(self) -> None:
+        await self._connection.wait_closed()
+
+    async def run(self) -> None:
+        self._loop = asyncio.get_running_loop()
+
+        options = {}
+        if self.timeout is not None:
+            options["close_timeout"] = self.timeout / 1000
+            options["ping_timeout"] = self.timeout / 1000
+        self._connection = await websockets.connect(self.ws_endpoint, **options)
+
+        while not self._stopped:
+            try:
+                message = await self._connection.recv()
+                obj = self.deserialize_message(message)
+                self.on_message(obj)
+            except websockets.exceptions.ConnectionClosed:
+                self.emit("close")
+                self.on_error_future.set_exception(
+                    Error("Playwright connection closed")
+                )
+                break
+            except Exception as exc:
+                print(f"Received unhandled exception: {exc}")
+                self.on_error_future.set_exception(exc)
+
+    def send(self, message: Dict) -> None:
+        self.test_if_closed()
+        data = self.serialize_message(message)
+        self._loop.create_task(self._connection.send(data))
+
+    def test_if_closed(self):
+        if self._stopped or self._connection.closed:
+            raise Error("Playwright connection closed")
diff --git a/playwright/async_api/_context_manager.py b/playwright/async_api/_context_manager.py
index 2b64e20ae..51800dd48 100644
--- a/playwright/async_api/_context_manager.py
+++ b/playwright/async_api/_context_manager.py
@@ -18,6 +18,7 @@
 from playwright._impl._connection import Connection
 from playwright._impl._driver import compute_driver_executable
 from playwright._impl._object_factory import create_remote_object
+from playwright._impl._transport import PipeTransport
 from playwright.async_api._generated import Playwright as AsyncPlaywright
 
 
@@ -27,7 +28,7 @@ def __init__(self) -> None:
 
     async def __aenter__(self) -> AsyncPlaywright:
         self._connection = Connection(
-            None, create_remote_object, compute_driver_executable()
+            None, create_remote_object, PipeTransport(compute_driver_executable())
         )
         loop = asyncio.get_running_loop()
         self._connection._loop = loop
diff --git a/playwright/async_api/_generated.py b/playwright/async_api/_generated.py
index 304d89ed8..0e573e32d 100644
--- a/playwright/async_api/_generated.py
+++ b/playwright/async_api/_generated.py
@@ -9603,6 +9603,38 @@ async def connect_over_cdp(
             )
         )
 
+    async def connect(
+        self, ws_endpoint: str, *, timeout: float = None, slow_mo: float = None
+    ) -> "Browser":
+        """BrowserType.connect
+
+        This methods attaches Playwright to an existing browser instance.
+
+        Parameters
+        ----------
+        ws_endpoint : str
+            A browser websocket endpoint to connect to.
+        timeout : Union[float, NoneType]
+            Maximum time in milliseconds to wait for the connection to be established. Defaults to `30000` (30 seconds). Pass `0` to
+            disable timeout.
+        slow_mo : Union[float, NoneType]
+            Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on.
+            Defaults to 0.
+
+        Returns
+        -------
+        Browser
+        """
+
+        return mapping.from_impl(
+            await self._async(
+                "browser_type.connect",
+                self._impl_obj.connect(
+                    ws_endpoint=ws_endpoint, timeout=timeout, slow_mo=slow_mo
+                ),
+            )
+        )
+
 
 mapping.register(BrowserTypeImpl, BrowserType)
 
diff --git a/playwright/sync_api/_context_manager.py b/playwright/sync_api/_context_manager.py
index 43fb80040..df12d3af8 100644
--- a/playwright/sync_api/_context_manager.py
+++ b/playwright/sync_api/_context_manager.py
@@ -22,6 +22,7 @@
 from playwright._impl._driver import compute_driver_executable
 from playwright._impl._object_factory import create_remote_object
 from playwright._impl._playwright import Playwright
+from playwright._impl._transport import PipeTransport
 from playwright.sync_api._generated import Playwright as SyncPlaywright
 
 
@@ -53,7 +54,9 @@ def greenlet_main() -> None:
 
         dispatcher_fiber = greenlet(greenlet_main)
         self._connection = Connection(
-            dispatcher_fiber, create_remote_object, compute_driver_executable()
+            dispatcher_fiber,
+            create_remote_object,
+            PipeTransport(compute_driver_executable()),
         )
 
         g_self = greenlet.getcurrent()
diff --git a/playwright/sync_api/_generated.py b/playwright/sync_api/_generated.py
index a24a990e4..48647b948 100644
--- a/playwright/sync_api/_generated.py
+++ b/playwright/sync_api/_generated.py
@@ -9549,6 +9549,38 @@ def connect_over_cdp(
             )
         )
 
+    def connect(
+        self, ws_endpoint: str, *, timeout: float = None, slow_mo: float = None
+    ) -> "Browser":
+        """BrowserType.connect
+
+        This methods attaches Playwright to an existing browser instance.
+
+        Parameters
+        ----------
+        ws_endpoint : str
+            A browser websocket endpoint to connect to.
+        timeout : Union[float, NoneType]
+            Maximum time in milliseconds to wait for the connection to be established. Defaults to `30000` (30 seconds). Pass `0` to
+            disable timeout.
+        slow_mo : Union[float, NoneType]
+            Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on.
+            Defaults to 0.
+
+        Returns
+        -------
+        Browser
+        """
+
+        return mapping.from_impl(
+            self._sync(
+                "browser_type.connect",
+                self._impl_obj.connect(
+                    ws_endpoint=ws_endpoint, timeout=timeout, slow_mo=slow_mo
+                ),
+            )
+        )
+
 
 mapping.register(BrowserTypeImpl, BrowserType)
 
diff --git a/scripts/expected_api_mismatch.txt b/scripts/expected_api_mismatch.txt
index 545c390b7..bd2be231c 100644
--- a/scripts/expected_api_mismatch.txt
+++ b/scripts/expected_api_mismatch.txt
@@ -16,6 +16,3 @@ Parameter type mismatch in Page.unroute(handler=): documented as Union[Callable[
 # Get vs set cookies
 Parameter type mismatch in BrowserContext.storage_state(return=): documented as {cookies: List[{name: str, value: str, domain: str, path: str, expires: float, httpOnly: bool, secure: bool, sameSite: Union["Lax", "None", "Strict"]}], origins: List[{origin: str, localStorage: List[{name: str, value: str}]}]}, code has {cookies: Union[List[{name: str, value: str, url: Union[str, NoneType], domain: Union[str, NoneType], path: Union[str, NoneType], expires: Union[float, NoneType], httpOnly: Union[bool, NoneType], secure: Union[bool, NoneType], sameSite: Union["Lax", "None", "Strict", NoneType]}], NoneType], origins: Union[List[{origin: str, localStorage: List[{name: str, value: str}]}], NoneType]}
 Parameter type mismatch in BrowserContext.cookies(return=): documented as List[{name: str, value: str, domain: str, path: str, expires: float, httpOnly: bool, secure: bool, sameSite: Union["Lax", "None", "Strict"]}], code has List[{name: str, value: str, url: Union[str, NoneType], domain: Union[str, NoneType], path: Union[str, NoneType], expires: Union[float, NoneType], httpOnly: Union[bool, NoneType], secure: Union[bool, NoneType], sameSite: Union["Lax", "None", "Strict", NoneType]}]
-
-# TODO: gets added soon
-Method not implemented: BrowserType.connect
diff --git a/tests/async/test_browsertype_connect.py b/tests/async/test_browsertype_connect.py
new file mode 100644
index 000000000..85cd16580
--- /dev/null
+++ b/tests/async/test_browsertype_connect.py
@@ -0,0 +1,150 @@
+# Copyright (c) Microsoft Corporation.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import asyncio
+
+import pytest
+
+from playwright.async_api import BrowserType, Error
+from tests.server import Server
+
+
+async def test_browser_type_connect_should_be_able_to_reconnect_to_a_browser(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = await browser_type.connect(remote_server.ws_endpoint)
+    browser_context = await browser.new_context()
+    assert len(browser_context.pages) == 0
+    page = await browser_context.new_page()
+    assert await page.evaluate("11 * 11") == 121
+    await page.goto(server.EMPTY_PAGE)
+    await browser.close()
+
+    browser = await browser_type.connect(remote_server.ws_endpoint)
+    browser_context = await browser.new_context()
+    page = await browser_context.new_page()
+    await page.goto(server.EMPTY_PAGE)
+    await browser.close()
+
+
+async def test_browser_type_connect_should_be_able_to_connect_two_browsers_at_the_same_time(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser1 = await browser_type.connect(remote_server.ws_endpoint)
+    assert len(browser1.contexts) == 0
+    await browser1.new_context()
+    assert len(browser1.contexts) == 1
+
+    browser2 = await browser_type.connect(remote_server.ws_endpoint)
+    assert len(browser2.contexts) == 0
+    await browser2.new_context()
+    assert len(browser2.contexts) == 1
+    assert len(browser1.contexts) == 1
+
+    await browser1.close()
+    page2 = await browser2.new_page()
+    # original browser should still work
+    assert await page2.evaluate("7 * 6") == 42
+
+    await browser2.close()
+
+
+async def test_browser_type_connect_disconnected_event_should_be_emitted_when_browser_is_closed_or_server_is_closed(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    # Launch another server to not affect other tests.
+    remote = launch_server()
+
+    browser1 = await browser_type.connect(remote.ws_endpoint)
+    browser2 = await browser_type.connect(remote.ws_endpoint)
+
+    disconnected1 = []
+    disconnected2_event_future: asyncio.Future[bool] = asyncio.Future()
+    browser1.on("disconnected", lambda: disconnected1.append(True))
+    browser2.on("disconnected", lambda: disconnected2_event_future.set_result(True))
+
+    page2 = await browser2.new_page()
+
+    await browser1.close()
+    assert len(disconnected1) == 1
+    assert disconnected2_event_future.done() is False
+
+    remote.kill()
+    assert len(disconnected1) == 1
+
+    with pytest.raises(Error):
+        # Tickle connection so that it gets a chance to dispatch disconnect event.
+        await page2.title()
+    await disconnected2_event_future
+
+
+async def test_browser_type_disconnected_event_should_have_browser_as_argument(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = await browser_type.connect(remote_server.ws_endpoint)
+    event_payloads = []
+    browser.on("disconnected", lambda b: event_payloads.append(b))
+    await browser.close()
+    assert event_payloads[0] == browser
+
+
+async def test_browser_type_connect_set_browser_connected_state(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = await browser_type.connect(remote_server.ws_endpoint)
+    assert browser.is_connected()
+    await browser.close()
+    assert browser.is_connected() is False
+
+
+async def test_browser_type_connect_should_throw_when_used_after_is_connected_returns_false(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = await browser_type.connect(remote_server.ws_endpoint)
+    page = await browser.new_page()
+
+    disconnected_event_future: asyncio.Future[bool] = asyncio.Future()
+    browser.on("disconnected", lambda: disconnected_event_future.set_result(True))
+
+    remote_server.kill()
+
+    # Wait until the process and its WS connection is closed
+    await disconnected_event_future
+
+    with pytest.raises(Error) as exc_info:
+        await page.evaluate("1 + 1")
+    assert "Protocol error (Runtime.evaluate): Target closed." in exc_info.value.message
+    on_disconnected: asyncio.Future[None] = asyncio.Future()
+    browser.on("disconnected", lambda: on_disconnected.set_result(None))
+    await on_disconnected
+    assert browser.is_connected() is False
+
+
+async def test_browser_type_connect_should_reject_navigation_when_browser_closes(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = await browser_type.connect(remote_server.ws_endpoint)
+    page = await browser.new_page()
+    server.set_route("/one-style.css", lambda: None)
+    page.on("request", lambda: asyncio.create_task(browser.close()))
+
+    with pytest.raises(Error) as exc_info:
+        await page.goto(server.PREFIX + "/one-style.html")
+    assert "Playwright connection closed" in exc_info.value.message
diff --git a/tests/conftest.py b/tests/conftest.py
index 2c43f2133..cfaaa148b 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -14,6 +14,9 @@
 
 import asyncio
 import io
+import os
+import signal
+import subprocess
 import sys
 
 import pytest
@@ -21,6 +24,7 @@
 from pixelmatch import pixelmatch
 from pixelmatch.contrib.PIL import from_PIL_to_raw_data
 
+from playwright._impl._driver import compute_driver_executable
 from playwright._impl._path_utils import get_file_dirname
 
 from .server import test_server
@@ -48,7 +52,6 @@ def assetdir():
 
 @pytest.fixture(scope="session")
 def launch_arguments(pytestconfig):
-    print(pytestconfig.getoption("--browser-channel"))
     return {
         "headless": not pytestconfig.getoption("--headful"),
         "channel": pytestconfig.getoption("--browser-channel"),
@@ -201,3 +204,33 @@ def compare(received_raw: bytes, golden_name: str):
         assert diff_pixels == 0
 
     return compare
+
+
+class RemoteServer:
+    def __init__(self, browser_name: str) -> None:
+        self.process = subprocess.Popen(
+            [compute_driver_executable(), "launch-server", browser_name],
+            stdout=subprocess.PIPE,
+            preexec_fn=os.setsid,
+        )
+        assert self.process.stdout
+        self.ws_endpoint = self.process.stdout.readline().decode().strip()
+
+    def kill(self):
+        # Send the signal to all the process groups
+        os.killpg(os.getpgid(self.process.pid), signal.SIGTERM)
+
+
+@pytest.fixture
+def launch_server(browser_name: str):
+    remotes = []
+
+    def _launch_server():
+        remote = RemoteServer(browser_name)
+        remotes.append(remote)
+        return remote
+
+    yield _launch_server
+
+    for remote in remotes:
+        remote.kill()

From be149ed7639512ec7e2cbb4dde05773c216043b9 Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Wed, 21 Apr 2021 15:02:35 +0200
Subject: [PATCH 02/11] fixed tests

---
 local-requirements.txt                  |   3 +-
 playwright/_impl/_browser_type.py       |  17 ++-
 playwright/_impl/_connection.py         |   7 ++
 playwright/_impl/_transport.py          |  19 +++-
 setup.py                                |   1 +
 tests/async/test_browsertype_connect.py |  32 ++----
 tests/conftest.py                       |  54 +++++++--
 tests/sync/test_browsertype_connect.py  | 141 ++++++++++++++++++++++++
 8 files changed, 233 insertions(+), 41 deletions(-)
 create mode 100644 tests/sync/test_browsertype_connect.py

diff --git a/local-requirements.txt b/local-requirements.txt
index 433e4db24..5fcaa62e9 100644
--- a/local-requirements.txt
+++ b/local-requirements.txt
@@ -11,8 +11,9 @@ pixelmatch==0.2.3
 pre-commit==2.10.1
 pyOpenSSL==20.0.1
 pytest==6.2.2
-pytest-asyncio==0.14.0
+pytest-asyncio==0.15.0
 pytest-cov==2.11.1
+pytest-repeat==0.9.1
 pytest-sugar==0.9.4
 pytest-timeout==1.4.2
 pytest-xdist==2.2.1
diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py
index cfe94dcf1..c66af3989 100644
--- a/playwright/_impl/_browser_type.py
+++ b/playwright/_impl/_browser_type.py
@@ -161,9 +161,14 @@ async def connect(
     ) -> Browser:
         transport = WebSocketTransport(ws_endpoint, timeout)
 
-        connection = Connection(None, self._connection._object_factory, transport)
+        connection = Connection(
+            self._connection._dispatcher_fiber,
+            self._connection._object_factory,
+            transport,
+        )
         connection._loop = self._connection._loop
         connection._loop.create_task(connection.run())
+        self._connection._child_ws_connections.append(connection)
         playwright = await connection.wait_for_object_with_known_name("Playwright")
         pre_launched_browser = playwright._initializer.get("preLaunchedBrowser")
         assert pre_launched_browser
@@ -171,7 +176,15 @@ async def connect(
         browser._is_remote = True
         browser._is_connected_over_websocket = True
 
-        transport.on("close", browser._on_close)
+        def handle_transport_close() -> None:
+            browser._on_close()
+
+        transport.on("close", handle_transport_close)
+
+        browser.once(
+            Browser.Events.Disconnected,
+            lambda b: transport.remove_listener("close", handle_transport_close),
+        )
 
         return browser
 
diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py
index 48af2c7fb..1539794ae 100644
--- a/playwright/_impl/_connection.py
+++ b/playwright/_impl/_connection.py
@@ -163,6 +163,7 @@ def __init__(
         self._object_factory = object_factory
         self._is_sync = False
         self._api_name = ""
+        self._child_ws_connections: List["Connection"] = []
 
     async def run_as_sync(self) -> None:
         self._is_sync = True
@@ -176,10 +177,16 @@ async def run(self) -> None:
     def stop_sync(self) -> None:
         self._transport.stop()
         self._dispatcher_fiber.switch()
+        self.cleanup()
 
     async def stop_async(self) -> None:
         self._transport.stop()
         await self._transport.wait_until_stopped()
+        self.cleanup()
+
+    def cleanup(self) -> None:
+        for ws_connection in self._child_ws_connections:
+            ws_connection._transport.cleanup()
 
     async def wait_for_object_with_known_name(self, guid: str) -> Any:
         if guid in self._objects:
diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py
index 83a7c9c97..a0c1a8432 100644
--- a/playwright/_impl/_transport.py
+++ b/playwright/_impl/_transport.py
@@ -49,13 +49,16 @@ def __init__(self) -> None:
     def stop(self) -> None:
         pass
 
+    def cleanup(self) -> None:
+        pass
+
     @abstractmethod
     async def wait_until_stopped(self) -> None:
         pass
 
-    @abstractmethod
     async def run(self) -> None:
-        pass
+        self._loop = asyncio.get_running_loop()
+        self.on_error_future: asyncio.Future[bool] = asyncio.Future()
 
     @abstractmethod
     def send(self, message: Dict) -> None:
@@ -91,7 +94,7 @@ async def wait_until_stopped(self) -> None:
         await self._proc.wait()
 
     async def run(self) -> None:
-        self._loop = asyncio.get_running_loop()
+        await super().run()
         self._stopped_future: asyncio.Future = asyncio.Future()
 
         self._proc = proc = await asyncio.create_subprocess_exec(
@@ -148,11 +151,14 @@ def stop(self) -> None:
         self._stopped = True
         self._loop.create_task(self._connection.close())
 
+    def cleanup(self) -> None:
+        self.on_error_future.cancel()
+
     async def wait_until_stopped(self) -> None:
         await self._connection.wait_closed()
 
     async def run(self) -> None:
-        self._loop = asyncio.get_running_loop()
+        await super().run()
 
         options = {}
         if self.timeout is not None:
@@ -166,7 +172,8 @@ async def run(self) -> None:
                 obj = self.deserialize_message(message)
                 self.on_message(obj)
             except websockets.exceptions.ConnectionClosed:
-                self.emit("close")
+                if not self._stopped:
+                    self.emit("close")
                 self.on_error_future.set_exception(
                     Error("Playwright connection closed")
                 )
@@ -180,6 +187,6 @@ def send(self, message: Dict) -> None:
         data = self.serialize_message(message)
         self._loop.create_task(self._connection.send(data))
 
-    def test_if_closed(self):
+    def test_if_closed(self) -> None:
         if self._stopped or self._connection.closed:
             raise Error("Playwright connection closed")
diff --git a/setup.py b/setup.py
index f88249d40..7e00ab59b 100644
--- a/setup.py
+++ b/setup.py
@@ -142,6 +142,7 @@ def run(self) -> None:
     packages=["playwright"],
     include_package_data=True,
     install_requires=[
+        "websockets>=8.1",
         "greenlet>=0.4",
         "pyee>=8.0.1",
         "typing-extensions;python_version<='3.8'",
diff --git a/tests/async/test_browsertype_connect.py b/tests/async/test_browsertype_connect.py
index 85cd16580..c852e8869 100644
--- a/tests/async/test_browsertype_connect.py
+++ b/tests/async/test_browsertype_connect.py
@@ -28,6 +28,7 @@ async def test_browser_type_connect_should_be_able_to_reconnect_to_a_browser(
     browser_context = await browser.new_context()
     assert len(browser_context.pages) == 0
     page = await browser_context.new_page()
+    assert len(browser_context.pages) == 1
     assert await page.evaluate("11 * 11") == 121
     await page.goto(server.EMPTY_PAGE)
     await browser.close()
@@ -40,7 +41,7 @@ async def test_browser_type_connect_should_be_able_to_reconnect_to_a_browser(
 
 
 async def test_browser_type_connect_should_be_able_to_connect_two_browsers_at_the_same_time(
-    server: Server, browser_type: BrowserType, launch_server
+    browser_type: BrowserType, launch_server
 ):
     remote_server = launch_server()
     browser1 = await browser_type.connect(remote_server.ws_endpoint)
@@ -63,7 +64,7 @@ async def test_browser_type_connect_should_be_able_to_connect_two_browsers_at_th
 
 
 async def test_browser_type_connect_disconnected_event_should_be_emitted_when_browser_is_closed_or_server_is_closed(
-    server: Server, browser_type: BrowserType, launch_server
+    browser_type: BrowserType, launch_server
 ):
     # Launch another server to not affect other tests.
     remote = launch_server()
@@ -72,15 +73,15 @@ async def test_browser_type_connect_disconnected_event_should_be_emitted_when_br
     browser2 = await browser_type.connect(remote.ws_endpoint)
 
     disconnected1 = []
-    disconnected2_event_future: asyncio.Future[bool] = asyncio.Future()
+    disconnected2 = []
     browser1.on("disconnected", lambda: disconnected1.append(True))
-    browser2.on("disconnected", lambda: disconnected2_event_future.set_result(True))
+    browser2.on("disconnected", lambda: disconnected2.append(True))
 
     page2 = await browser2.new_page()
 
     await browser1.close()
     assert len(disconnected1) == 1
-    assert disconnected2_event_future.done() is False
+    assert len(disconnected2) == 0
 
     remote.kill()
     assert len(disconnected1) == 1
@@ -88,11 +89,11 @@ async def test_browser_type_connect_disconnected_event_should_be_emitted_when_br
     with pytest.raises(Error):
         # Tickle connection so that it gets a chance to dispatch disconnect event.
         await page2.title()
-    await disconnected2_event_future
+    assert len(disconnected2) == 1
 
 
 async def test_browser_type_disconnected_event_should_have_browser_as_argument(
-    server: Server, browser_type: BrowserType, launch_server
+    browser_type: BrowserType, launch_server
 ):
     remote_server = launch_server()
     browser = await browser_type.connect(remote_server.ws_endpoint)
@@ -103,7 +104,7 @@ async def test_browser_type_disconnected_event_should_have_browser_as_argument(
 
 
 async def test_browser_type_connect_set_browser_connected_state(
-    server: Server, browser_type: BrowserType, launch_server
+    browser_type: BrowserType, launch_server
 ):
     remote_server = launch_server()
     browser = await browser_type.connect(remote_server.ws_endpoint)
@@ -113,26 +114,17 @@ async def test_browser_type_connect_set_browser_connected_state(
 
 
 async def test_browser_type_connect_should_throw_when_used_after_is_connected_returns_false(
-    server: Server, browser_type: BrowserType, launch_server
+    browser_type: BrowserType, launch_server
 ):
     remote_server = launch_server()
     browser = await browser_type.connect(remote_server.ws_endpoint)
     page = await browser.new_page()
 
-    disconnected_event_future: asyncio.Future[bool] = asyncio.Future()
-    browser.on("disconnected", lambda: disconnected_event_future.set_result(True))
-
     remote_server.kill()
 
-    # Wait until the process and its WS connection is closed
-    await disconnected_event_future
-
     with pytest.raises(Error) as exc_info:
         await page.evaluate("1 + 1")
-    assert "Protocol error (Runtime.evaluate): Target closed." in exc_info.value.message
-    on_disconnected: asyncio.Future[None] = asyncio.Future()
-    browser.on("disconnected", lambda: on_disconnected.set_result(None))
-    await on_disconnected
+    assert "Playwright connection closed" == exc_info.value.message
     assert browser.is_connected() is False
 
 
@@ -142,7 +134,7 @@ async def test_browser_type_connect_should_reject_navigation_when_browser_closes
     remote_server = launch_server()
     browser = await browser_type.connect(remote_server.ws_endpoint)
     page = await browser.new_page()
-    server.set_route("/one-style.css", lambda: None)
+    server.set_route("/one-style.css", lambda r: None)
     page.on("request", lambda: asyncio.create_task(browser.close()))
 
     with pytest.raises(Error) as exc_info:
diff --git a/tests/conftest.py b/tests/conftest.py
index cfaaa148b..52c50d7e0 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -13,18 +13,21 @@
 # limitations under the License.
 
 import asyncio
+import inspect
 import io
+import json
 import os
-import signal
 import subprocess
 import sys
+from pathlib import Path
+from typing import Dict, List
 
 import pytest
 from PIL import Image
 from pixelmatch import pixelmatch
 from pixelmatch.contrib.PIL import from_PIL_to_raw_data
 
-from playwright._impl._driver import compute_driver_executable
+import playwright
 from playwright._impl._path_utils import get_file_dirname
 
 from .server import test_server
@@ -207,26 +210,53 @@ def compare(received_raw: bytes, golden_name: str):
 
 
 class RemoteServer:
-    def __init__(self, browser_name: str) -> None:
-        self.process = subprocess.Popen(
-            [compute_driver_executable(), "launch-server", browser_name],
+    def __init__(
+        self, browser_name: str, launch_server_options: Dict, tmpfile: Path
+    ) -> None:
+        driver_dir = Path(inspect.getfile(playwright)).parent / "driver"
+        if sys.platform == "win32":
+            node_executable = driver_dir / "node.exe"
+        else:
+            node_executable = driver_dir / "node"
+        cli_js = driver_dir / "package" / "lib" / "cli" / "cli.js"
+        tmpfile.write_text(json.dumps(launch_server_options))
+        self.process = subprocess.Popen(  # type: ignore
+            [
+                str(node_executable),
+                str(cli_js),
+                "launch-server",
+                browser_name,
+                str(tmpfile),
+            ],
             stdout=subprocess.PIPE,
-            preexec_fn=os.setsid,
+            stderr=sys.stderr,
+            cwd=driver_dir,
         )
         assert self.process.stdout
         self.ws_endpoint = self.process.stdout.readline().decode().strip()
 
     def kill(self):
         # Send the signal to all the process groups
-        os.killpg(os.getpgid(self.process.pid), signal.SIGTERM)
+        if sys.platform == "win32":
+            os.system(f"taskkill /F /PID {self.process.pid}")
+        else:
+            self.process.kill()
+        self.process.wait()
 
 
 @pytest.fixture
-def launch_server(browser_name: str):
-    remotes = []
-
-    def _launch_server():
-        remote = RemoteServer(browser_name)
+def launch_server(browser_name: str, launch_arguments: Dict, tmp_path: Path):
+    remotes: List[RemoteServer] = []
+
+    def _launch_server(override_launch_options: Dict = {}):
+        remote = RemoteServer(
+            browser_name,
+            {
+                **launch_arguments,
+                **override_launch_options,
+            },
+            tmp_path / f"settings-{len(remotes)}.json",
+        )
         remotes.append(remote)
         return remote
 
diff --git a/tests/sync/test_browsertype_connect.py b/tests/sync/test_browsertype_connect.py
new file mode 100644
index 000000000..4ca30b2e7
--- /dev/null
+++ b/tests/sync/test_browsertype_connect.py
@@ -0,0 +1,141 @@
+# Copyright (c) Microsoft Corporation.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pytest
+
+from playwright.sync_api import BrowserType, Error
+from tests.server import Server
+
+
+def test_browser_type_connect_should_be_able_to_reconnect_to_a_browser(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = browser_type.connect(remote_server.ws_endpoint)
+    browser_context = browser.new_context()
+    assert len(browser_context.pages) == 0
+    page = browser_context.new_page()
+    assert len(browser_context.pages) == 1
+    assert page.evaluate("11 * 11") == 121
+    page.goto(server.EMPTY_PAGE)
+    browser.close()
+
+    browser = browser_type.connect(remote_server.ws_endpoint)
+    browser_context = browser.new_context()
+    page = browser_context.new_page()
+    page.goto(server.EMPTY_PAGE)
+    browser.close()
+
+
+def test_browser_type_connect_should_be_able_to_connect_two_browsers_at_the_same_time(
+    browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser1 = browser_type.connect(remote_server.ws_endpoint)
+    assert len(browser1.contexts) == 0
+    browser1.new_context()
+    assert len(browser1.contexts) == 1
+
+    browser2 = browser_type.connect(remote_server.ws_endpoint)
+    assert len(browser2.contexts) == 0
+    browser2.new_context()
+    assert len(browser2.contexts) == 1
+    assert len(browser1.contexts) == 1
+
+    browser1.close()
+    page2 = browser2.new_page()
+    # original browser should still work
+    assert page2.evaluate("7 * 6") == 42
+
+    browser2.close()
+
+
+def test_browser_type_connect_disconnected_event_should_be_emitted_when_browser_is_closed_or_server_is_closed(
+    browser_type: BrowserType, launch_server
+):
+    # Launch another server to not affect other tests.
+    remote = launch_server()
+
+    browser1 = browser_type.connect(remote.ws_endpoint)
+    browser2 = browser_type.connect(remote.ws_endpoint)
+
+    disconnected1 = []
+    disconnected2 = []
+    browser1.on("disconnected", lambda: disconnected1.append(True))
+    browser2.on("disconnected", lambda: disconnected2.append(True))
+
+    page2 = browser2.new_page()
+
+    browser1.close()
+    assert len(disconnected1) == 1
+    assert len(disconnected2) == 0
+
+    remote.kill()
+    assert len(disconnected1) == 1
+
+    with pytest.raises(Error):
+        # Tickle connection so that it gets a chance to dispatch disconnect event.
+        page2.title()
+    assert len(disconnected2) == 1
+
+
+def test_browser_type_disconnected_event_should_have_browser_as_argument(
+    browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = browser_type.connect(remote_server.ws_endpoint)
+    event_payloads = []
+    browser.on("disconnected", lambda b: event_payloads.append(b))
+    browser.close()
+    assert event_payloads[0] == browser
+
+
+def test_browser_type_connect_set_browser_connected_state(
+    browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = browser_type.connect(remote_server.ws_endpoint)
+    assert browser.is_connected()
+    browser.close()
+    assert browser.is_connected() is False
+
+
+def test_browser_type_connect_should_throw_when_used_after_is_connected_returns_false(
+    browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = browser_type.connect(remote_server.ws_endpoint)
+    page = browser.new_page()
+
+    remote_server.kill()
+
+    with pytest.raises(Error) as exc_info:
+        page.evaluate("1 + 1")
+    assert "Playwright connection closed" == exc_info.value.message
+    assert browser.is_connected() is False
+
+
+@pytest.mark.skip
+def test_browser_type_connect_should_reject_navigation_when_browser_closes(
+    server: Server, browser_type: BrowserType, launch_server
+):
+    remote_server = launch_server()
+    browser = browser_type.connect(remote_server.ws_endpoint)
+    page = browser.new_page()
+    server.set_route("/one-style.css", lambda r: None)
+    page.on("request", lambda: browser.close())
+
+    with pytest.raises(Error) as exc_info:
+        page.goto(server.PREFIX + "/one-style.html")
+    assert "Playwright connection closed" in exc_info.value.message

From b132745fff37bfc18c8582c21d1cf67a2935520a Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Wed, 21 Apr 2021 21:22:05 +0200
Subject: [PATCH 03/11] debug

---
 .github/workflows/ci.yml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index c97117bfe..5382731e2 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -82,6 +82,8 @@ jobs:
         python -m playwright install-deps
     - name: Install browsers
       run: python -m playwright install
+    - name: Setup tmate session
+      uses: mxschmitt/action-tmate@v3
     - name: Common Tests
       run: pytest tests/common --browser=${{ matrix.browser }} --timeout 90
     - name: Test Reference count

From 54271ccbdecbd22d72ed0d49c4c8f7b6b23a60c5 Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Thu, 22 Apr 2021 11:38:20 +0200
Subject: [PATCH 04/11] fix sync test

---
 .github/workflows/ci.yml               | 2 --
 playwright/_impl/_browser_type.py      | 1 +
 playwright/_impl/_transport.py         | 9 +++------
 setup.cfg                              | 2 +-
 tests/conftest.py                      | 5 +++--
 tests/sync/test_browsertype_connect.py | 1 -
 6 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 5382731e2..c97117bfe 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -82,8 +82,6 @@ jobs:
         python -m playwright install-deps
     - name: Install browsers
       run: python -m playwright install
-    - name: Setup tmate session
-      uses: mxschmitt/action-tmate@v3
     - name: Common Tests
       run: pytest tests/common --browser=${{ matrix.browser }} --timeout 90
     - name: Test Reference count
diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py
index c66af3989..c56cfedc7 100644
--- a/playwright/_impl/_browser_type.py
+++ b/playwright/_impl/_browser_type.py
@@ -166,6 +166,7 @@ async def connect(
             self._connection._object_factory,
             transport,
         )
+        connection._is_sync = True
         connection._loop = self._connection._loop
         connection._loop.create_task(connection.run())
         self._connection._child_ws_connections.append(connection)
diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py
index a0c1a8432..9bab46abf 100644
--- a/playwright/_impl/_transport.py
+++ b/playwright/_impl/_transport.py
@@ -64,7 +64,7 @@ async def run(self) -> None:
     def send(self, message: Dict) -> None:
         pass
 
-    def serialize_message(self, message: Dict) -> Any:
+    def serialize_message(self, message: Dict) -> bytes:
         msg = json.dumps(message)
         if "DEBUGP" in os.environ:  # pragma: no cover
             print("\x1b[32mSEND>\x1b[0m", json.dumps(message, indent=2))
@@ -183,10 +183,7 @@ async def run(self) -> None:
                 self.on_error_future.set_exception(exc)
 
     def send(self, message: Dict) -> None:
-        self.test_if_closed()
-        data = self.serialize_message(message)
-        self._loop.create_task(self._connection.send(data))
-
-    def test_if_closed(self) -> None:
         if self._stopped or self._connection.closed:
             raise Error("Playwright connection closed")
+        data = self.serialize_message(message)
+        self._loop.create_task(self._connection.send(data))
diff --git a/setup.cfg b/setup.cfg
index 0329788c0..50018e225 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,5 +1,5 @@
 [tool:pytest]
-addopts = -rsx -vv
+addopts = -rsx -vv -s
 markers =
     skip_browser
     only_browser
diff --git a/tests/conftest.py b/tests/conftest.py
index 52c50d7e0..366ec2939 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -16,7 +16,6 @@
 import inspect
 import io
 import json
-import os
 import subprocess
 import sys
 from pathlib import Path
@@ -237,8 +236,10 @@ def __init__(
 
     def kill(self):
         # Send the signal to all the process groups
+        if self.process.poll() is not None:
+            return
         if sys.platform == "win32":
-            os.system(f"taskkill /F /PID {self.process.pid}")
+            subprocess.check_call(["taskkill", "/F", "/PID", str(self.process.pid)])
         else:
             self.process.kill()
         self.process.wait()
diff --git a/tests/sync/test_browsertype_connect.py b/tests/sync/test_browsertype_connect.py
index 4ca30b2e7..9c7a526e2 100644
--- a/tests/sync/test_browsertype_connect.py
+++ b/tests/sync/test_browsertype_connect.py
@@ -126,7 +126,6 @@ def test_browser_type_connect_should_throw_when_used_after_is_connected_returns_
     assert browser.is_connected() is False
 
 
-@pytest.mark.skip
 def test_browser_type_connect_should_reject_navigation_when_browser_closes(
     server: Server, browser_type: BrowserType, launch_server
 ):

From 9071e430a4dc8836a7d168661dfb8595795d0012 Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Thu, 22 Apr 2021 17:57:08 +0200
Subject: [PATCH 05/11] debug flaky tests

---
 .github/workflows/ci.yml          | 3 +++
 playwright/_impl/_browser_type.py | 2 +-
 playwright/_impl/_connection.py   | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index c97117bfe..b4a00570b 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -107,6 +107,9 @@ jobs:
       with:
         name: ${{ matrix.browser }}-${{ matrix.os }}-${{ matrix.python-version }}
         path: pw-log.txt
+    - if: ${{ failure() }}
+      uses: mxschmitt/action-tmate@v3
+      timeout-minutes: 45
 
   test-stable:
     name: Stable
diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py
index c56cfedc7..2c97e1dea 100644
--- a/playwright/_impl/_browser_type.py
+++ b/playwright/_impl/_browser_type.py
@@ -166,7 +166,7 @@ async def connect(
             self._connection._object_factory,
             transport,
         )
-        connection._is_sync = True
+        connection._is_sync = self._connection._is_sync
         connection._loop = self._connection._loop
         connection._loop.create_task(connection.run())
         self._connection._child_ws_connections.append(connection)
diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py
index 1539794ae..c261d6384 100644
--- a/playwright/_impl/_connection.py
+++ b/playwright/_impl/_connection.py
@@ -46,7 +46,7 @@ async def inner_send(
         callback = self._connection._send_message_to_server(self._guid, method, params)
 
         done, pending = await asyncio.wait(
-            {callback.future, self._connection._transport.on_error_future},
+            {self._connection._transport.on_error_future, callback.future},
             return_when=asyncio.FIRST_COMPLETED,
         )
         result = next(iter(done)).result()

From f9ddbdb8f2e9946b80d5eb9f3f11da055039c796 Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Thu, 22 Apr 2021 18:04:34 +0200
Subject: [PATCH 06/11] idea

---
 playwright/_impl/_connection.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py
index c261d6384..fe8e16b63 100644
--- a/playwright/_impl/_connection.py
+++ b/playwright/_impl/_connection.py
@@ -49,6 +49,8 @@ async def inner_send(
             {self._connection._transport.on_error_future, callback.future},
             return_when=asyncio.FIRST_COMPLETED,
         )
+        if not callback.future.done():
+            callback.future.cancel()
         result = next(iter(done)).result()
         # Protocol now has named return values, assume result is one level deeper unless
         # there is explicit ambiguity.

From 89ffa16e457243cb2cf5ce8e1987e8f09347f098 Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Thu, 22 Apr 2021 20:25:14 +0200
Subject: [PATCH 07/11] fix flaky test

---
 .github/workflows/ci.yml       | 3 ---
 playwright/_impl/_transport.py | 5 +++++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index b4a00570b..c97117bfe 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -107,9 +107,6 @@ jobs:
       with:
         name: ${{ matrix.browser }}-${{ matrix.os }}-${{ matrix.python-version }}
         path: pw-log.txt
-    - if: ${{ failure() }}
-      uses: mxschmitt/action-tmate@v3
-      timeout-minutes: 45
 
   test-stable:
     name: Stable
diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py
index 9bab46abf..8d83c4619 100644
--- a/playwright/_impl/_transport.py
+++ b/playwright/_impl/_transport.py
@@ -169,6 +169,11 @@ async def run(self) -> None:
         while not self._stopped:
             try:
                 message = await self._connection.recv()
+                if self._stopped:
+                    self.on_error_future.set_exception(
+                        Error("Playwright connection closed")
+                    )
+                    break
                 obj = self.deserialize_message(message)
                 self.on_message(obj)
             except websockets.exceptions.ConnectionClosed:

From 7fbce4b80deeaa06b4ea0d698d9a63db23753ea2 Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Thu, 22 Apr 2021 22:43:12 +0200
Subject: [PATCH 08/11] nits

---
 playwright/_impl/_browser.py      |  6 +++---
 playwright/_impl/_browser_type.py |  7 +------
 playwright/_impl/_connection.py   |  6 +++---
 playwright/_impl/_transport.py    | 12 ++++++------
 4 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/playwright/_impl/_browser.py b/playwright/_impl/_browser.py
index 11eb24200..53bd13ab3 100644
--- a/playwright/_impl/_browser.py
+++ b/playwright/_impl/_browser.py
@@ -146,13 +146,13 @@ async def new_page(
         return page
 
     async def close(self) -> None:
+        if self._is_closed_or_closing:
+            return
+        self._is_closed_or_closing = True
         if self._is_connected_over_websocket:
             await self._connection.stop_async()
             self._notify_remote_closed()
             return
-        if self._is_closed_or_closing:
-            return
-        self._is_closed_or_closing = True
         try:
             await self._channel.send("close")
         except Exception as e:
diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py
index 2c97e1dea..cdebe310f 100644
--- a/playwright/_impl/_browser_type.py
+++ b/playwright/_impl/_browser_type.py
@@ -180,12 +180,7 @@ async def connect(
         def handle_transport_close() -> None:
             browser._on_close()
 
-        transport.on("close", handle_transport_close)
-
-        browser.once(
-            Browser.Events.Disconnected,
-            lambda b: transport.remove_listener("close", handle_transport_close),
-        )
+        transport.once("close", handle_transport_close)
 
         return browser
 
diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py
index fe8e16b63..fc4a8bd42 100644
--- a/playwright/_impl/_connection.py
+++ b/playwright/_impl/_connection.py
@@ -177,18 +177,18 @@ async def run(self) -> None:
         await self._transport.run()
 
     def stop_sync(self) -> None:
-        self._transport.stop()
+        self._transport.request_stop()
         self._dispatcher_fiber.switch()
         self.cleanup()
 
     async def stop_async(self) -> None:
-        self._transport.stop()
+        self._transport.request_stop()
         await self._transport.wait_until_stopped()
         self.cleanup()
 
     def cleanup(self) -> None:
         for ws_connection in self._child_ws_connections:
-            ws_connection._transport.cleanup()
+            ws_connection._transport.dispose()
 
     async def wait_for_object_with_known_name(self, guid: str) -> Any:
         if guid in self._objects:
diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py
index 8d83c4619..8a003ea3e 100644
--- a/playwright/_impl/_transport.py
+++ b/playwright/_impl/_transport.py
@@ -46,10 +46,10 @@ def __init__(self) -> None:
         self.on_error_future: asyncio.Future = asyncio.Future()
 
     @abstractmethod
-    def stop(self) -> None:
+    def request_stop(self) -> None:
         pass
 
-    def cleanup(self) -> None:
+    def dispose(self) -> None:
         pass
 
     @abstractmethod
@@ -58,7 +58,7 @@ async def wait_until_stopped(self) -> None:
 
     async def run(self) -> None:
         self._loop = asyncio.get_running_loop()
-        self.on_error_future: asyncio.Future[bool] = asyncio.Future()
+        self.on_error_future: asyncio.Future = asyncio.Future()
 
     @abstractmethod
     def send(self, message: Dict) -> None:
@@ -85,7 +85,7 @@ def __init__(self, driver_executable: Path) -> None:
         self._driver_executable = driver_executable
         self._loop: asyncio.AbstractEventLoop
 
-    def stop(self) -> None:
+    def request_stop(self) -> None:
         self._stopped = True
         self._output.close()
 
@@ -147,11 +147,11 @@ def __init__(self, ws_endpoint: str, timeout: float) -> None:
         self.timeout = timeout
         self._loop: asyncio.AbstractEventLoop
 
-    def stop(self) -> None:
+    def request_stop(self) -> None:
         self._stopped = True
         self._loop.create_task(self._connection.close())
 
-    def cleanup(self) -> None:
+    def dispose(self) -> None:
         self.on_error_future.cancel()
 
     async def wait_until_stopped(self) -> None:

From ae908a3f1bee011452721b59827dab679b60ac79 Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Fri, 23 Apr 2021 14:54:52 +0200
Subject: [PATCH 09/11] reworked closing logic

---
 playwright/_impl/_browser.py            | 14 ++-----------
 playwright/_impl/_browser_type.py       | 12 ++++++-----
 playwright/_impl/_transport.py          |  3 +--
 tests/async/test_browsertype_connect.py |  9 +++++----
 tests/sync/test_browsertype_connect.py  | 27 ++++++++++++++++---------
 5 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/playwright/_impl/_browser.py b/playwright/_impl/_browser.py
index 53bd13ab3..add366a2c 100644
--- a/playwright/_impl/_browser.py
+++ b/playwright/_impl/_browser.py
@@ -149,23 +149,13 @@ async def close(self) -> None:
         if self._is_closed_or_closing:
             return
         self._is_closed_or_closing = True
-        if self._is_connected_over_websocket:
-            await self._connection.stop_async()
-            self._notify_remote_closed()
-            return
         try:
             await self._channel.send("close")
         except Exception as e:
             if not is_safe_close_error(e):
                 raise e
-
-    def _notify_remote_closed(self) -> None:
-        # Emulate all pages, contexts and the browser closing upon disconnect.
-        for context in self.contexts:
-            for page in context.pages:
-                page._on_close()
-            context._on_close()
-        self._on_close()
+        if self._is_connected_over_websocket:
+            await self._connection.stop_async()
 
     @property
     def version(self) -> str:
diff --git a/playwright/_impl/_browser_type.py b/playwright/_impl/_browser_type.py
index cdebe310f..b7b8f0ad8 100644
--- a/playwright/_impl/_browser_type.py
+++ b/playwright/_impl/_browser_type.py
@@ -23,7 +23,12 @@
 )
 from playwright._impl._browser import Browser, normalize_context_params
 from playwright._impl._browser_context import BrowserContext
-from playwright._impl._connection import ChannelOwner, Connection, from_channel
+from playwright._impl._connection import (
+    ChannelOwner,
+    Connection,
+    from_channel,
+    from_nullable_channel,
+)
 from playwright._impl._helper import (
     BrowserChannel,
     ColorScheme,
@@ -177,10 +182,7 @@ async def connect(
         browser._is_remote = True
         browser._is_connected_over_websocket = True
 
-        def handle_transport_close() -> None:
-            browser._on_close()
-
-        transport.once("close", handle_transport_close)
+        transport.once("close", browser._on_close)
 
         return browser
 
diff --git a/playwright/_impl/_transport.py b/playwright/_impl/_transport.py
index 8a003ea3e..fb20f3392 100644
--- a/playwright/_impl/_transport.py
+++ b/playwright/_impl/_transport.py
@@ -43,7 +43,6 @@ def _get_stderr_fileno() -> Optional[int]:
 class Transport(ABC):
     def __init__(self) -> None:
         self.on_message = lambda _: None
-        self.on_error_future: asyncio.Future = asyncio.Future()
 
     @abstractmethod
     def request_stop(self) -> None:
@@ -138,7 +137,7 @@ def send(self, message: Dict) -> None:
 
 
 class WebSocketTransport(AsyncIOEventEmitter, Transport):
-    def __init__(self, ws_endpoint: str, timeout: float) -> None:
+    def __init__(self, ws_endpoint: str, timeout: float = None) -> None:
         super().__init__()
         Transport.__init__(self)
 
diff --git a/tests/async/test_browsertype_connect.py b/tests/async/test_browsertype_connect.py
index c852e8869..9d5ac495b 100644
--- a/tests/async/test_browsertype_connect.py
+++ b/tests/async/test_browsertype_connect.py
@@ -12,8 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import asyncio
-
 import pytest
 
 from playwright.async_api import BrowserType, Error
@@ -134,8 +132,11 @@ async def test_browser_type_connect_should_reject_navigation_when_browser_closes
     remote_server = launch_server()
     browser = await browser_type.connect(remote_server.ws_endpoint)
     page = await browser.new_page()
-    server.set_route("/one-style.css", lambda r: None)
-    page.on("request", lambda: asyncio.create_task(browser.close()))
+    await browser.close()
+
+    with pytest.raises(Error) as exc_info:
+        await page.goto(server.PREFIX + "/one-style.html")
+    assert "Playwright connection closed" in exc_info.value.message
 
     with pytest.raises(Error) as exc_info:
         await page.goto(server.PREFIX + "/one-style.html")
diff --git a/tests/sync/test_browsertype_connect.py b/tests/sync/test_browsertype_connect.py
index 9c7a526e2..d698d8da1 100644
--- a/tests/sync/test_browsertype_connect.py
+++ b/tests/sync/test_browsertype_connect.py
@@ -126,15 +126,22 @@ def test_browser_type_connect_should_throw_when_used_after_is_connected_returns_
     assert browser.is_connected() is False
 
 
-def test_browser_type_connect_should_reject_navigation_when_browser_closes(
-    server: Server, browser_type: BrowserType, launch_server
+def test_browser_type_connect_should_forward_close_events_to_pages(
+    browser_type: BrowserType, launch_server
 ):
-    remote_server = launch_server()
-    browser = browser_type.connect(remote_server.ws_endpoint)
-    page = browser.new_page()
-    server.set_route("/one-style.css", lambda r: None)
-    page.on("request", lambda: browser.close())
+    # Launch another server to not affect other tests.
+    remote = launch_server()
 
-    with pytest.raises(Error) as exc_info:
-        page.goto(server.PREFIX + "/one-style.html")
-    assert "Playwright connection closed" in exc_info.value.message
+    browser = browser_type.connect(remote.ws_endpoint)
+    context = browser.new_context()
+    page = context.new_page()
+
+    events = []
+    browser.on("disconnected", lambda: events.append("browser::disconnected"))
+    context.on("close", lambda: events.append("context::close"))
+    page.on("close", lambda: events.append("page::close"))
+
+    browser.close()
+    assert events == ["page::close", "context::close", "browser::disconnected"]
+    remote.kill()
+    assert events == ["page::close", "context::close", "browser::disconnected"]

From 3349e4d972742a9a60c5ee2062b2d1b945a1c85e Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Fri, 23 Apr 2021 17:45:23 +0200
Subject: [PATCH 10/11] added artifacts/video remote handling

---
 playwright/_impl/_page.py               |  5 ++-
 playwright/_impl/_video.py              |  9 +++++
 tests/async/test_browsertype_connect.py | 47 +++++++++++++++++++++++--
 tests/conftest.py                       |  4 +--
 4 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/playwright/_impl/_page.py b/playwright/_impl/_page.py
index 7db7c50be..527b2ec1e 100644
--- a/playwright/_impl/_page.py
+++ b/playwright/_impl/_page.py
@@ -29,6 +29,7 @@
     ViewportSize,
 )
 from playwright._impl._api_types import Error
+from playwright._impl._artifact import Artifact
 from playwright._impl._connection import (
     ChannelOwner,
     from_channel,
@@ -285,7 +286,9 @@ def _on_dialog(self, params: Any) -> None:
     def _on_download(self, params: Any) -> None:
         url = params["url"]
         suggested_filename = params["suggestedFilename"]
-        artifact = from_channel(params["artifact"])
+        artifact = cast(Artifact, from_channel(params["artifact"]))
+        if self._browser_context._browser:
+            artifact._is_remote = self._browser_context._browser._is_remote
         self.emit(
             Page.Events.Download, Download(self, url, suggested_filename, artifact)
         )
diff --git a/playwright/_impl/_video.py b/playwright/_impl/_video.py
index 171852506..d9227284f 100644
--- a/playwright/_impl/_video.py
+++ b/playwright/_impl/_video.py
@@ -28,6 +28,10 @@ def __init__(self, page: "Page") -> None:
         self._dispatcher_fiber = page._dispatcher_fiber
         self._page = page
         self._artifact_future = page._loop.create_future()
+        if page.context.browser:
+            self._is_remote = page.context.browser._is_remote
+        else:
+            self._is_remote = False
         if page.is_closed():
             self._page_closed()
         else:
@@ -42,9 +46,14 @@ def _page_closed(self) -> None:
 
     def _artifact_ready(self, artifact: Artifact) -> None:
         if not self._artifact_future.done():
+            artifact._is_remote = self._is_remote
             self._artifact_future.set_result(artifact)
 
     async def path(self) -> pathlib.Path:
+        if self._is_remote:
+            raise Error(
+                "Path is not available when using browserType.connect(). Use save_as() to save a local copy."
+            )
         artifact = await self._artifact_future
         if not artifact:
             raise Error("Page did not produce any video frames")
diff --git a/tests/async/test_browsertype_connect.py b/tests/async/test_browsertype_connect.py
index 9d5ac495b..842b6c1ee 100644
--- a/tests/async/test_browsertype_connect.py
+++ b/tests/async/test_browsertype_connect.py
@@ -138,6 +138,47 @@ async def test_browser_type_connect_should_reject_navigation_when_browser_closes
         await page.goto(server.PREFIX + "/one-style.html")
     assert "Playwright connection closed" in exc_info.value.message
 
-    with pytest.raises(Error) as exc_info:
-        await page.goto(server.PREFIX + "/one-style.html")
-    assert "Playwright connection closed" in exc_info.value.message
+
+async def test_should_not_allow_getting_the_path(
+    browser_type: BrowserType, launch_server, server: Server
+):
+    def handle_download(request):
+        request.setHeader("Content-Type", "application/octet-stream")
+        request.setHeader("Content-Disposition", "attachment")
+        request.write(b"Hello world")
+        request.finish()
+
+    server.set_route("/download", handle_download)
+
+    remote_server = launch_server()
+    browser = await browser_type.connect(remote_server.ws_endpoint)
+    page = await browser.new_page(accept_downloads=True)
+    await page.set_content(f'<a href="{server.PREFIX}/download">download</a>')
+    async with page.expect_download() as download_info:
+        await page.click("a")
+    download = await download_info.value
+    with pytest.raises(Error) as exc:
+        await download.path()
+    assert (
+        exc.value.message
+        == "Path is not available when using browser_type.connect(). Use save_as() to save a local copy."
+    )
+    remote_server.kill()
+
+
+async def test_prevent_getting_video_path(
+    browser_type: BrowserType, launch_server, tmpdir, server
+):
+    remote_server = launch_server()
+    browser = await browser_type.connect(remote_server.ws_endpoint)
+    page = await browser.new_page(record_video_dir=tmpdir)
+    await page.goto(server.PREFIX + "/grid.html")
+    await browser.close()
+    assert page.video
+    with pytest.raises(Error) as exc:
+        await page.video.path()
+    assert (
+        exc.value.message
+        == "Path is not available when using browserType.connect(). Use save_as() to save a local copy."
+    )
+    remote_server.kill()
diff --git a/tests/conftest.py b/tests/conftest.py
index 366ec2939..ad1b622e9 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -249,12 +249,12 @@ def kill(self):
 def launch_server(browser_name: str, launch_arguments: Dict, tmp_path: Path):
     remotes: List[RemoteServer] = []
 
-    def _launch_server(override_launch_options: Dict = {}):
+    def _launch_server(**kwargs: Dict):
         remote = RemoteServer(
             browser_name,
             {
                 **launch_arguments,
-                **override_launch_options,
+                **kwargs,
             },
             tmp_path / f"settings-{len(remotes)}.json",
         )

From 140ad2eb4802c65a7a91c8a640ea2a9bb3df385d Mon Sep 17 00:00:00 2001
From: Max Schmitt <max@schmitt.mx>
Date: Sat, 24 Apr 2021 13:05:42 +0200
Subject: [PATCH 11/11] fix tests

---
 playwright/_impl/_page.py  | 1 +
 playwright/_impl/_video.py | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/playwright/_impl/_page.py b/playwright/_impl/_page.py
index 527b2ec1e..69f8a2248 100644
--- a/playwright/_impl/_page.py
+++ b/playwright/_impl/_page.py
@@ -111,6 +111,7 @@ def __init__(
         self, parent: ChannelOwner, type: str, guid: str, initializer: Dict
     ) -> None:
         super().__init__(parent, type, guid, initializer)
+        self._browser_context: BrowserContext = None  # type: ignore
         self.accessibility = Accessibility(self._channel)
         self.keyboard = Keyboard(self._channel)
         self.mouse = Mouse(self._channel)
diff --git a/playwright/_impl/_video.py b/playwright/_impl/_video.py
index d9227284f..8b4d8e1bc 100644
--- a/playwright/_impl/_video.py
+++ b/playwright/_impl/_video.py
@@ -28,8 +28,8 @@ def __init__(self, page: "Page") -> None:
         self._dispatcher_fiber = page._dispatcher_fiber
         self._page = page
         self._artifact_future = page._loop.create_future()
-        if page.context.browser:
-            self._is_remote = page.context.browser._is_remote
+        if page._browser_context and page._browser_context._browser:
+            self._is_remote = page._browser_context._browser._is_remote
         else:
             self._is_remote = False
         if page.is_closed():