From 9717d13f3a487a4ede16af65663ebb6096125d76 Mon Sep 17 00:00:00 2001 From: DM_ <6091595+x0day@users.noreply.github.com> Date: Sun, 16 Apr 2023 11:24:33 +0800 Subject: [PATCH 1/6] fix: setNetworkInterceptionPatterns race issue --- playwright/_impl/_page.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/playwright/_impl/_page.py b/playwright/_impl/_page.py index 2414df934..c3dc57341 100644 --- a/playwright/_impl/_page.py +++ b/playwright/_impl/_page.py @@ -23,6 +23,7 @@ TYPE_CHECKING, Any, Callable, + Coroutine, Dict, List, Optional, @@ -308,6 +309,24 @@ def _on_video(self, params: Any) -> None: artifact = from_channel(params["artifact"]) cast(Video, self.video)._artifact_ready(artifact) + async def _race_with_page_close(self, future: Coroutine) -> None: + fut = asyncio.create_task(future) + # Rewrite the user's stack to the new task which runs in the background. + setattr( + fut, + "__pw_stack__", + getattr(asyncio.current_task(self._loop), "__pw_stack__", inspect.stack()), + ) + target_closed_future = self._closed_or_crashed_future + await asyncio.wait( + [fut, target_closed_future], + return_when=asyncio.FIRST_COMPLETED, + ) + if fut.done() and fut.exception(): + raise cast(BaseException, fut.exception()) + if target_closed_future.done(): + await asyncio.gather(fut, return_exceptions=True) + @property def context(self) -> "BrowserContext": return self._browser_context @@ -640,8 +659,8 @@ async def route_from_har( async def _update_interception_patterns(self) -> None: patterns = RouteHandler.prepare_interception_patterns(self._routes) - await self._channel.send( - "setNetworkInterceptionPatterns", {"patterns": patterns} + await self._race_with_page_close( + self._channel.send("setNetworkInterceptionPatterns", {"patterns": patterns}) ) async def screenshot( From 2a11fbf92ef860536bb3474c8d20356467896e5b Mon Sep 17 00:00:00 2001 From: DM_ <6091595+x0day@users.noreply.github.com> Date: Mon, 24 Apr 2023 03:46:13 +0000 Subject: [PATCH 2/6] fix: asyncio.create_task race condition --- playwright/_impl/_browser_context.py | 18 +++++------ playwright/_impl/_page.py | 45 +++++++--------------------- 2 files changed, 18 insertions(+), 45 deletions(-) diff --git a/playwright/_impl/_browser_context.py b/playwright/_impl/_browser_context.py index f2787f862..e3b3eb9cc 100644 --- a/playwright/_impl/_browser_context.py +++ b/playwright/_impl/_browser_context.py @@ -120,12 +120,10 @@ def __init__( ) self._channel.on( "route", - lambda params: asyncio.create_task( - self._on_route( - from_channel(params.get("route")), - ) + lambda params: self._on_route( + from_channel(params.get("route")), ), - ) + ), self._channel.on( "backgroundPage", @@ -202,20 +200,18 @@ async def _on_route(self, route: Route) -> None: handled = await route_handler.handle(route) finally: if len(self._routes) == 0: - asyncio.create_task( - self._connection.wrap_api_call( - lambda: self._update_interception_patterns(), True - ) + await self._connection.wrap_api_call( + lambda: self._update_interception_patterns(), True ) if handled: return await route._internal_continue(is_internal=True) - def _on_binding(self, binding_call: BindingCall) -> None: + async def _on_binding(self, binding_call: BindingCall) -> None: func = self._bindings.get(binding_call._initializer["name"]) if func is None: return - asyncio.create_task(binding_call.call(func)) + await binding_call.call(func) def set_default_navigation_timeout(self, timeout: float) -> None: self._timeout_settings.set_navigation_timeout(timeout) diff --git a/playwright/_impl/_page.py b/playwright/_impl/_page.py index c3dc57341..6c653bc6c 100644 --- a/playwright/_impl/_page.py +++ b/playwright/_impl/_page.py @@ -23,7 +23,6 @@ TYPE_CHECKING, Any, Callable, - Coroutine, Dict, List, Optional, @@ -194,9 +193,7 @@ def __init__( ) self._channel.on( "route", - lambda params: asyncio.create_task( - self._on_route(from_channel(params["route"])) - ), + lambda params: self._on_route(from_channel(params["route"])), ) self._channel.on("video", lambda params: self._on_video(params)) self._channel.on( @@ -256,20 +253,18 @@ async def _on_route(self, route: Route) -> None: handled = await route_handler.handle(route) finally: if len(self._routes) == 0: - asyncio.create_task( - self._connection.wrap_api_call( - lambda: self._update_interception_patterns(), True - ) + await self._connection.wrap_api_call( + lambda: self._update_interception_patterns(), True ) if handled: return await self._browser_context._on_route(route) - def _on_binding(self, binding_call: "BindingCall") -> None: + async def _on_binding(self, binding_call: "BindingCall") -> None: func = self._bindings.get(binding_call._initializer["name"]) if func: - asyncio.create_task(binding_call.call(func)) - self._browser_context._on_binding(binding_call) + await binding_call.call(func) + await self._browser_context._on_binding(binding_call) def _on_worker(self, worker: "Worker") -> None: self._workers.append(worker) @@ -287,15 +282,15 @@ def _on_close(self) -> None: def _on_crash(self) -> None: self.emit(Page.Events.Crash, self) - def _on_dialog(self, params: Any) -> None: + async def _on_dialog(self, params: Any) -> None: dialog = cast(Dialog, from_channel(params["dialog"])) if self.listeners(Page.Events.Dialog): self.emit(Page.Events.Dialog, dialog) else: if dialog.type == "beforeunload": - asyncio.create_task(dialog.accept()) + await dialog.accept() else: - asyncio.create_task(dialog.dismiss()) + await dialog.dismiss() def _on_download(self, params: Any) -> None: url = params["url"] @@ -309,24 +304,6 @@ def _on_video(self, params: Any) -> None: artifact = from_channel(params["artifact"]) cast(Video, self.video)._artifact_ready(artifact) - async def _race_with_page_close(self, future: Coroutine) -> None: - fut = asyncio.create_task(future) - # Rewrite the user's stack to the new task which runs in the background. - setattr( - fut, - "__pw_stack__", - getattr(asyncio.current_task(self._loop), "__pw_stack__", inspect.stack()), - ) - target_closed_future = self._closed_or_crashed_future - await asyncio.wait( - [fut, target_closed_future], - return_when=asyncio.FIRST_COMPLETED, - ) - if fut.done() and fut.exception(): - raise cast(BaseException, fut.exception()) - if target_closed_future.done(): - await asyncio.gather(fut, return_exceptions=True) - @property def context(self) -> "BrowserContext": return self._browser_context @@ -659,8 +636,8 @@ async def route_from_har( async def _update_interception_patterns(self) -> None: patterns = RouteHandler.prepare_interception_patterns(self._routes) - await self._race_with_page_close( - self._channel.send("setNetworkInterceptionPatterns", {"patterns": patterns}) + await self._channel.send( + "setNetworkInterceptionPatterns", {"patterns": patterns} ) async def screenshot( From 34a3d6b6c12b6246e87f2cb7cda4774bc06be739 Mon Sep 17 00:00:00 2001 From: DM_ <6091595+x0day@users.noreply.github.com> Date: Tue, 25 Apr 2023 02:58:11 +0000 Subject: [PATCH 3/6] fix: try/catch --- playwright/_impl/_browser_context.py | 8 +++++--- playwright/_impl/_page.py | 17 ++++++++++------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/playwright/_impl/_browser_context.py b/playwright/_impl/_browser_context.py index e3b3eb9cc..1e136374c 100644 --- a/playwright/_impl/_browser_context.py +++ b/playwright/_impl/_browser_context.py @@ -13,6 +13,7 @@ # limitations under the License. import asyncio +import contextlib import json import sys from pathlib import Path @@ -200,9 +201,10 @@ async def _on_route(self, route: Route) -> None: handled = await route_handler.handle(route) finally: if len(self._routes) == 0: - await self._connection.wrap_api_call( - lambda: self._update_interception_patterns(), True - ) + with contextlib.suppress(Exception): + await self._connection.wrap_api_call( + lambda: self._update_interception_patterns(), True + ) if handled: return await route._internal_continue(is_internal=True) diff --git a/playwright/_impl/_page.py b/playwright/_impl/_page.py index 6c653bc6c..b39d916d1 100644 --- a/playwright/_impl/_page.py +++ b/playwright/_impl/_page.py @@ -14,6 +14,7 @@ import asyncio import base64 +import contextlib import inspect import re import sys @@ -253,9 +254,10 @@ async def _on_route(self, route: Route) -> None: handled = await route_handler.handle(route) finally: if len(self._routes) == 0: - await self._connection.wrap_api_call( - lambda: self._update_interception_patterns(), True - ) + with contextlib.suppress(Exception): + await self._connection.wrap_api_call( + lambda: self._update_interception_patterns(), True + ) if handled: return await self._browser_context._on_route(route) @@ -287,10 +289,11 @@ async def _on_dialog(self, params: Any) -> None: if self.listeners(Page.Events.Dialog): self.emit(Page.Events.Dialog, dialog) else: - if dialog.type == "beforeunload": - await dialog.accept() - else: - await dialog.dismiss() + with contextlib.suppress(Exception): + if dialog.type == "beforeunload": + await dialog.accept() + else: + await dialog.dismiss() def _on_download(self, params: Any) -> None: url = params["url"] From 024ef3928d1b4023ea57209448f5beea35bbe708 Mon Sep 17 00:00:00 2001 From: DM_ <6091595+x0day@users.noreply.github.com> Date: Sun, 7 May 2023 05:19:12 +0000 Subject: [PATCH 4/6] fix --- playwright/_impl/_browser_context.py | 18 +++++++++-------- playwright/_impl/_connection.py | 14 ++++++++++++++ playwright/_impl/_page.py | 29 ++++++++++++++-------------- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/playwright/_impl/_browser_context.py b/playwright/_impl/_browser_context.py index 1e136374c..366bbf5fb 100644 --- a/playwright/_impl/_browser_context.py +++ b/playwright/_impl/_browser_context.py @@ -13,7 +13,6 @@ # limitations under the License. import asyncio -import contextlib import json import sys from pathlib import Path @@ -121,10 +120,12 @@ def __init__( ) self._channel.on( "route", - lambda params: self._on_route( - from_channel(params.get("route")), + lambda params: self._emit_sync( + self._on_route( + from_channel(params.get("route")), + ) ), - ), + ) self._channel.on( "backgroundPage", @@ -201,19 +202,20 @@ async def _on_route(self, route: Route) -> None: handled = await route_handler.handle(route) finally: if len(self._routes) == 0: - with contextlib.suppress(Exception): - await self._connection.wrap_api_call( + self._emit_sync( + self._connection.wrap_api_call( lambda: self._update_interception_patterns(), True ) + ) if handled: return await route._internal_continue(is_internal=True) - async def _on_binding(self, binding_call: BindingCall) -> None: + def _on_binding(self, binding_call: BindingCall) -> None: func = self._bindings.get(binding_call._initializer["name"]) if func is None: return - await binding_call.call(func) + self._emit_sync(binding_call.call(func)) def set_default_navigation_timeout(self, timeout: float) -> None: self._timeout_settings.set_navigation_timeout(timeout) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index aa57f2157..f9046bbf6 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -23,6 +23,7 @@ TYPE_CHECKING, Any, Callable, + Coroutine, Dict, List, Mapping, @@ -169,6 +170,19 @@ def _add_event_handler(self, event: str, k: Any, v: Any) -> None: self._update_subscription(event, True) super()._add_event_handler(event, k, v) + def _emit_sync(self, coro: Coroutine, ignore_errors: bool = True) -> None: + fut = asyncio.ensure_future(coro, loop=self._loop) + + def cb(f: asyncio.Task) -> None: + if f.cancelled(): + return + + exc: Optional[BaseException] = f.exception() + if exc and not ignore_errors: + self.emit("error", exc) + + fut.add_done_callback(cb) + def remove_listener(self, event: str, f: Any) -> None: super().remove_listener(event, f) if not self.listeners(event): diff --git a/playwright/_impl/_page.py b/playwright/_impl/_page.py index b39d916d1..97e3cdf0f 100644 --- a/playwright/_impl/_page.py +++ b/playwright/_impl/_page.py @@ -14,7 +14,6 @@ import asyncio import base64 -import contextlib import inspect import re import sys @@ -194,7 +193,9 @@ def __init__( ) self._channel.on( "route", - lambda params: self._on_route(from_channel(params["route"])), + lambda params: self._emit_sync( + self._on_route(from_channel(params["route"])) + ), ) self._channel.on("video", lambda params: self._on_video(params)) self._channel.on( @@ -254,19 +255,20 @@ async def _on_route(self, route: Route) -> None: handled = await route_handler.handle(route) finally: if len(self._routes) == 0: - with contextlib.suppress(Exception): - await self._connection.wrap_api_call( + self._emit_sync( + self._connection.wrap_api_call( lambda: self._update_interception_patterns(), True ) + ) if handled: return await self._browser_context._on_route(route) - async def _on_binding(self, binding_call: "BindingCall") -> None: + def _on_binding(self, binding_call: "BindingCall") -> None: func = self._bindings.get(binding_call._initializer["name"]) if func: - await binding_call.call(func) - await self._browser_context._on_binding(binding_call) + self._emit_sync(binding_call.call(func)) + self._browser_context._on_binding(binding_call) def _on_worker(self, worker: "Worker") -> None: self._workers.append(worker) @@ -284,16 +286,15 @@ def _on_close(self) -> None: def _on_crash(self) -> None: self.emit(Page.Events.Crash, self) - async def _on_dialog(self, params: Any) -> None: + def _on_dialog(self, params: Any) -> None: dialog = cast(Dialog, from_channel(params["dialog"])) if self.listeners(Page.Events.Dialog): self.emit(Page.Events.Dialog, dialog) else: - with contextlib.suppress(Exception): - if dialog.type == "beforeunload": - await dialog.accept() - else: - await dialog.dismiss() + if dialog.type == "beforeunload": + self._emit_sync(dialog.accept()) + else: + self._emit_sync(dialog.dismiss()) def _on_download(self, params: Any) -> None: url = params["url"] @@ -1268,7 +1269,7 @@ async def call(self, func: Callable) -> None: await self._channel.send("resolve", dict(result=serialize_argument(result))) except Exception as e: tb = sys.exc_info()[2] - asyncio.create_task( + self._emit_sync( self._channel.send( "reject", dict(error=dict(error=serialize_error(e, tb))) ) From ac23262582ed67a9f459d3ededadd18978163331 Mon Sep 17 00:00:00 2001 From: DM_ <6091595+x0day@users.noreply.github.com> Date: Sun, 7 May 2023 06:30:36 +0000 Subject: [PATCH 5/6] fix: keep reference to the task --- playwright/_impl/_connection.py | 56 ++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index f9046bbf6..fef0b285c 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -18,6 +18,7 @@ import inspect import sys import traceback +from asyncio import AbstractEventLoop from pathlib import Path from typing import ( TYPE_CHECKING, @@ -28,13 +29,15 @@ List, Mapping, Optional, + Set, + Tuple, Union, cast, ) from greenlet import greenlet from pyee import EventEmitter -from pyee.asyncio import AsyncIOEventEmitter +from pyee.asyncio import AsyncIOEventEmitter as PyeeAsyncIOEventEmitter import playwright from playwright._impl._helper import ParsedMessagePayload, parse_error @@ -51,6 +54,53 @@ from typing_extensions import TypedDict +class AsyncIOEventEmitter(PyeeAsyncIOEventEmitter): + # https://github.com/python/cpython/issues/88831 + # https://stackoverflow.com/questions/71938799/python-asyncio-create-task-really-need-to-keep-a-reference + # https://github.com/jfhbrook/pyee/issues/120 + # https://github.com/microsoft/playwright/issues/12182 + def __init__(self, loop: Optional[AbstractEventLoop] = None): + super(AsyncIOEventEmitter, self).__init__() + self._loop: Optional[AbstractEventLoop] = loop + self._running_tasks: Set[asyncio.Task] = set() + + def _emit_run( + self, + f: Callable, + args: Tuple[Any, ...], + kwargs: Dict[str, Any], + ) -> None: + try: + coro: Any = f(*args, **kwargs) + except Exception as exc: + self.emit("error", exc) + else: + if asyncio.iscoroutine(coro): + if self._loop: + # ensure_future is *extremely* cranky about the types here, + # but this is relatively well-tested and I think the types + # are more strict than they should be + fut: Any = asyncio.ensure_future(cast(Any, coro), loop=self._loop) + else: + fut = asyncio.ensure_future(cast(Any, coro)) + elif isinstance(coro, asyncio.Future): + fut = cast(Any, coro) + else: + return + + def callback(f: asyncio.Task) -> None: + self._running_tasks.remove(f) + if f.cancelled(): + return + + exc: Optional[BaseException] = f.exception() + if exc: + self.emit("error", exc) + + self._running_tasks.add(fut) + fut.add_done_callback(callback) + + class Channel(AsyncIOEventEmitter): def __init__(self, connection: "Connection", guid: str) -> None: super().__init__() @@ -139,6 +189,8 @@ def __init__( self._event_to_subscription_mapping: Dict[str, str] = {} + self._running_tasks: Set[asyncio.Task] = set() + def _dispose(self) -> None: # Clean up from parent and connection. if self._parent: @@ -174,6 +226,7 @@ def _emit_sync(self, coro: Coroutine, ignore_errors: bool = True) -> None: fut = asyncio.ensure_future(coro, loop=self._loop) def cb(f: asyncio.Task) -> None: + self._running_tasks.remove(f) if f.cancelled(): return @@ -181,6 +234,7 @@ def cb(f: asyncio.Task) -> None: if exc and not ignore_errors: self.emit("error", exc) + self._running_tasks.add(fut) fut.add_done_callback(cb) def remove_listener(self, event: str, f: Any) -> None: From 98d50c96a31f36a40a1c88ef19b7348c8a5eb58e Mon Sep 17 00:00:00 2001 From: DM_ <6091595+x0day@users.noreply.github.com> Date: Thu, 1 Jun 2023 13:17:43 +0000 Subject: [PATCH 6/6] fix: remove patch for AsyncIOEventEmitter & merge --- playwright/_impl/_connection.py | 58 ++++----------------------------- playwright/_impl/_page.py | 10 ------ 2 files changed, 6 insertions(+), 62 deletions(-) diff --git a/playwright/_impl/_connection.py b/playwright/_impl/_connection.py index cda9f5401..ff171a1d6 100644 --- a/playwright/_impl/_connection.py +++ b/playwright/_impl/_connection.py @@ -18,7 +18,6 @@ import inspect import sys import traceback -from asyncio import AbstractEventLoop from pathlib import Path from typing import ( TYPE_CHECKING, @@ -30,14 +29,13 @@ Mapping, Optional, Set, - Tuple, Union, cast, ) from greenlet import greenlet from pyee import EventEmitter -from pyee.asyncio import AsyncIOEventEmitter as PyeeAsyncIOEventEmitter +from pyee.asyncio import AsyncIOEventEmitter import playwright from playwright._impl._helper import Error, ParsedMessagePayload, parse_error @@ -54,53 +52,6 @@ from typing_extensions import TypedDict -class AsyncIOEventEmitter(PyeeAsyncIOEventEmitter): - # https://github.com/python/cpython/issues/88831 - # https://stackoverflow.com/questions/71938799/python-asyncio-create-task-really-need-to-keep-a-reference - # https://github.com/jfhbrook/pyee/issues/120 - # https://github.com/microsoft/playwright/issues/12182 - def __init__(self, loop: Optional[AbstractEventLoop] = None): - super(AsyncIOEventEmitter, self).__init__() - self._loop: Optional[AbstractEventLoop] = loop - self._running_tasks: Set[asyncio.Task] = set() - - def _emit_run( - self, - f: Callable, - args: Tuple[Any, ...], - kwargs: Dict[str, Any], - ) -> None: - try: - coro: Any = f(*args, **kwargs) - except Exception as exc: - self.emit("error", exc) - else: - if asyncio.iscoroutine(coro): - if self._loop: - # ensure_future is *extremely* cranky about the types here, - # but this is relatively well-tested and I think the types - # are more strict than they should be - fut: Any = asyncio.ensure_future(cast(Any, coro), loop=self._loop) - else: - fut = asyncio.ensure_future(cast(Any, coro)) - elif isinstance(coro, asyncio.Future): - fut = cast(Any, coro) - else: - return - - def callback(f: asyncio.Task) -> None: - self._running_tasks.remove(f) - if f.cancelled(): - return - - exc: Optional[BaseException] = f.exception() - if exc: - self.emit("error", exc) - - self._running_tasks.add(fut) - fut.add_done_callback(callback) - - class Channel(AsyncIOEventEmitter): def __init__(self, connection: "Connection", guid: str) -> None: super().__init__() @@ -214,8 +165,11 @@ def _set_event_to_subscription_mapping(self, mapping: Dict[str, str]) -> None: def _update_subscription(self, event: str, enabled: bool) -> None: protocol_event = self._event_to_subscription_mapping.get(event) if protocol_event: - self._channel.send_no_reply( - "updateSubscription", {"event": protocol_event, "enabled": enabled} + self._connection.wrap_api_call_sync( + lambda: self._channel.send_no_reply( + "updateSubscription", {"event": protocol_event, "enabled": enabled} + ), + True, ) def _add_event_handler(self, event: str, k: Any, v: Any) -> None: diff --git a/playwright/_impl/_page.py b/playwright/_impl/_page.py index 9b3a4d65e..bfd5dc20f 100644 --- a/playwright/_impl/_page.py +++ b/playwright/_impl/_page.py @@ -280,16 +280,6 @@ def _on_close(self) -> None: def _on_crash(self) -> None: self.emit(Page.Events.Crash, self) - def _on_dialog(self, params: Any) -> None: - dialog = cast(Dialog, from_channel(params["dialog"])) - if self.listeners(Page.Events.Dialog): - self.emit(Page.Events.Dialog, dialog) - else: - if dialog.type == "beforeunload": - self._emit_sync(dialog.accept()) - else: - self._emit_sync(dialog.dismiss()) - def _on_download(self, params: Any) -> None: url = params["url"] suggested_filename = params["suggestedFilename"]