Skip to content

Commit 1e5fcaf

Browse files
mxschmittclaude
andcommitted
fix: prevent ProtocolCallback memory leak during tracing (#2977)
Fix memory leak where ProtocolCallback objects accumulated when tracing was enabled. The issue affected both sync and async Playwright, but was more severe in async usage where callbacks persisted permanently. - When tracing is enabled, send_no_reply() creates ProtocolCallback objects - These callbacks have circular references via task cancellation handlers - For no_reply messages, futures are never resolved, preventing GC cleanup - Each page operation during tracing leaked ~10 callbacks permanently - Add no_reply parameter to ProtocolCallback constructor - For no_reply=True: skip cancellation setup and resolve future immediately - For no_reply=False: use full constructor with cancellation handling - Update dispatch() to handle missing callbacks gracefully - Added regression test in test_reference_count_async.py - Test fails before fix (30+ leaked callbacks) and passes after (≤5) - All existing tracing and asyncio tests continue to pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent e43199c commit 1e5fcaf

File tree

2 files changed

+86
-11
lines changed

2 files changed

+86
-11
lines changed

playwright/_impl/_connection.py

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -218,10 +218,16 @@ def remove_listener(self, event: str, f: Any) -> None:
218218

219219

220220
class ProtocolCallback:
221-
def __init__(self, loop: asyncio.AbstractEventLoop) -> None:
221+
def __init__(self, loop: asyncio.AbstractEventLoop, no_reply: bool = False) -> None:
222222
self.stack_trace: traceback.StackSummary
223-
self.no_reply: bool
223+
self.no_reply = no_reply
224224
self.future = loop.create_future()
225+
226+
# Skip cancellation setup for no_reply callbacks to avoid circular references
227+
if no_reply:
228+
self.future.set_result(None)
229+
return
230+
225231
# The outer task can get cancelled by the user, this forwards the cancellation to the inner task.
226232
current_task = asyncio.current_task()
227233

@@ -360,14 +366,7 @@ def _send_message_to_server(
360366
)
361367
self._last_id += 1
362368
id = self._last_id
363-
callback = ProtocolCallback(self._loop)
364-
task = asyncio.current_task(self._loop)
365-
callback.stack_trace = cast(
366-
traceback.StackSummary,
367-
getattr(task, "__pw_stack_trace__", traceback.extract_stack(limit=10)),
368-
)
369-
callback.no_reply = no_reply
370-
self._callbacks[id] = callback
369+
371370
stack_trace_information = cast(ParsedStackTrace, self._api_zone.get())
372371
frames = stack_trace_information.get("frames", [])
373372
location = (
@@ -400,6 +399,18 @@ def _send_message_to_server(
400399
self.local_utils.add_stack_to_tracing_no_reply(id, frames)
401400

402401
self._transport.send(message)
402+
403+
# For no_reply messages, create lightweight callback without cancellation setup
404+
if no_reply:
405+
return ProtocolCallback(self._loop, no_reply=True)
406+
407+
# For messages that expect replies, create full callback with cancellation handling
408+
callback = ProtocolCallback(self._loop, no_reply=False)
409+
task = asyncio.current_task(self._loop)
410+
callback.stack_trace = cast(
411+
traceback.StackSummary,
412+
getattr(task, "__pw_stack_trace__", traceback.extract_stack(limit=10)),
413+
)
403414
self._callbacks[id] = callback
404415

405416
return callback
@@ -409,7 +420,10 @@ def dispatch(self, msg: ParsedMessagePayload) -> None:
409420
return
410421
id = msg.get("id")
411422
if id:
412-
callback = self._callbacks.pop(id)
423+
callback = self._callbacks.pop(id, None)
424+
# If callback doesn't exist, it was likely a no_reply message
425+
if callback is None:
426+
return
413427
if callback.future.cancelled():
414428
return
415429
# No reply messages are used to e.g. waitForEventInfo(after) which returns exceptions on page close.

tests/test_reference_count_async.py

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
# limitations under the License.
1414

1515
import gc
16+
import os
17+
import tempfile
1618
from collections import defaultdict
1719
from typing import Any
1820

@@ -71,3 +73,62 @@ def handle_network_response_received(event: Any) -> None:
7173
assert "Dialog" not in pw_objects
7274
assert "Request" not in pw_objects
7375
assert "Route" not in pw_objects
76+
77+
78+
@pytest.mark.asyncio
79+
async def test_tracing_should_not_leak_protocol_callbacks(browser_name: str) -> None:
80+
"""
81+
Regression test for https://github.com/microsoft/playwright-python/issues/2977
82+
83+
This test ensures that ProtocolCallback objects don't accumulate when tracing is enabled.
84+
The memory leak occurred because no_reply callbacks were created with circular references
85+
but never cleaned up.
86+
"""
87+
88+
def count_protocol_callbacks() -> int:
89+
"""Count ProtocolCallback objects in memory."""
90+
gc.collect()
91+
count = 0
92+
for obj in gc.get_objects():
93+
if (
94+
hasattr(obj, "__class__")
95+
and obj.__class__.__name__ == "ProtocolCallback"
96+
):
97+
count += 1
98+
return count
99+
100+
with tempfile.TemporaryDirectory() as temp_dir:
101+
trace_file = os.path.join(temp_dir, "test_trace.zip")
102+
103+
async with async_playwright() as p:
104+
browser = await p[browser_name].launch()
105+
context = await browser.new_context()
106+
107+
# Start tracing to trigger the creation of no_reply callbacks
108+
await context.tracing.start(screenshots=True, snapshots=True)
109+
110+
initial_count = count_protocol_callbacks()
111+
112+
# Perform operations that would create tracing callbacks
113+
for _ in range(3):
114+
page = await context.new_page()
115+
await page.goto("data:text/html,<h1>Test Page</h1>")
116+
await page.wait_for_load_state("networkidle")
117+
await page.evaluate(
118+
"document.querySelector('h1').textContent = 'Modified'"
119+
)
120+
await page.close()
121+
122+
# Stop tracing
123+
await context.tracing.stop(path=trace_file)
124+
await browser.close()
125+
126+
# Force garbage collection and check callback count
127+
gc.collect()
128+
final_count = count_protocol_callbacks()
129+
130+
# The key assertion: callback count should not have increased significantly
131+
# Allow for a small number of legitimate callbacks but ensure no major leak
132+
assert (
133+
final_count - initial_count <= 5
134+
), f"ProtocolCallback leak detected: initial={initial_count}, final={final_count}, leaked={final_count - initial_count}"

0 commit comments

Comments
 (0)