From c428a49588f7a0ab939d6c6eddffca7a42382ba4 Mon Sep 17 00:00:00 2001 From: Jean-Beru Date: Wed, 30 Sep 2026 17:22:57 +0200 Subject: [PATCH] Release closed browser contexts A long-lived browser kept every context it ever created: Browser::$contexts never dropped closed contexts, and JsonRpcTransport kept the event dispatcher of every context and page for its whole lifetime. Each closed context stayed reachable with its pages, their route handlers and whatever those closures captured. BrowserContext::close() now unregisters itself and its pages from the transport, clears its pages and route handlers, and asks its Browser to forget it. The cleanup also runs when the close command fails, as the server does. Page::close() and the pageClosed event unregister the page, and Page::close() removes it from its context. Popups returned by Page::waitForPopup() are now tracked by their context, so they are released with it. Dispatchers are removed after the close command has been sent; late events for an unregistered objectId are only logged at debug level. JsonRpcTransport::removeEventDispatcher() is not added to TransportInterface, like addEventDispatcher(), and callers guard it with method_exists(). Assisted-by: Claude:claude-opus-5-5 [gh] --- CHANGELOG.md | 3 + src/Browser/Browser.php | 8 + src/Browser/BrowserContext.php | 45 ++++- src/Page/Page.php | 15 +- src/Transport/JsonRpc/JsonRpcTransport.php | 5 + tests/Mocks/EventDispatcherTransport.php | 79 +++++++++ .../Unit/Browser/BrowserContextCloseTest.php | 167 ++++++++++++++++++ .../JsonRpc/JsonRpcTransportTest.php | 34 ++++ 8 files changed, 351 insertions(+), 5 deletions(-) create mode 100644 tests/Mocks/EventDispatcherTransport.php create mode 100644 tests/Unit/Browser/BrowserContextCloseTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 5546de8..3461563 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## [Unreleased] +### Fixed +- Closing a browser context releases it, its pages and their route handlers instead of keeping them for the browser's lifetime + ## [1.5.0] - 2026-09-20 ### Added diff --git a/src/Browser/Browser.php b/src/Browser/Browser.php index 41de6ae..c324f80 100644 --- a/src/Browser/Browser.php +++ b/src/Browser/Browser.php @@ -88,6 +88,14 @@ public function contexts(): array return $this->contexts; } + /** + * @internal called by BrowserContext::close() + */ + public function forgetContext(BrowserContextInterface $context): void + { + $this->contexts = array_values(array_filter($this->contexts, static fn (BrowserContextInterface $c): bool => $c !== $context)); + } + public function browserType(): BrowserType { return $this->browserType; diff --git a/src/Browser/BrowserContext.php b/src/Browser/BrowserContext.php index 5bf917d..0952ebb 100644 --- a/src/Browser/BrowserContext.php +++ b/src/Browser/BrowserContext.php @@ -145,6 +145,7 @@ public function dispatchEvent(string $eventName, array $params): void $pageId = $params['pageId'] ?? null; if (is_string($pageId)) { unset($this->pages[$pageId]); + $this->removeEventDispatcher($pageId); } return; @@ -237,10 +238,46 @@ public function close(): void $this->saveAutoTrace(); } - $this->transport->send([ - 'action' => 'context.close', - 'contextId' => $this->contextId, - ]); + try { + $this->transport->send([ + 'action' => 'context.close', + 'contextId' => $this->contextId, + ]); + } finally { + foreach (array_keys($this->pages) as $pageId) { + $this->removeEventDispatcher($pageId); + } + $this->removeEventDispatcher($this->contextId); + $this->pages = []; + $this->routeHandlers = []; + + if ($this->browser instanceof Browser) { + $this->browser->forgetContext($this); + } + } + } + + /** + * @internal called by Page::waitForPopup() + */ + public function registerPage(PageInterface $page): void + { + $this->pages[$page->getPageIdForTransport()] = $page; + } + + /** + * @internal called by Page::close() + */ + public function forgetPage(PageInterface $page): void + { + unset($this->pages[$page->getPageIdForTransport()]); + } + + private function removeEventDispatcher(string $id): void + { + if (method_exists($this->transport, 'removeEventDispatcher')) { + $this->transport->removeEventDispatcher($id); + } } public function isClosed(): bool diff --git a/src/Page/Page.php b/src/Page/Page.php index 3a35201..f1b36f2 100644 --- a/src/Page/Page.php +++ b/src/Page/Page.php @@ -15,6 +15,7 @@ namespace Playwright\Page; use Playwright\API\APIRequestContextInterface; +use Playwright\Browser\BrowserContext; use Playwright\Browser\BrowserContextInterface; use Playwright\Clock\ClockInterface; use Playwright\Configuration\PlaywrightConfig; @@ -639,6 +640,13 @@ public function close(): void $this->sendCommand('close'); $this->isClosed = true; + + if (method_exists($this->transport, 'removeEventDispatcher')) { + $this->transport->removeEventDispatcher($this->pageId); + } + if ($this->context instanceof BrowserContext) { + $this->context->forgetPage($this); + } } public function isClosed(): bool @@ -1257,7 +1265,12 @@ public function waitForPopup(callable $action, array|WaitForPopupOptions $option throw new TimeoutException('No popup was created within the timeout period'); } - return new self($this->transport, $this->context, $popupPageId, $this->config, $this->logger); + $popup = new self($this->transport, $this->context, $popupPageId, $this->config, $this->logger); + if ($this->context instanceof BrowserContext) { + $this->context->registerPage($popup); + } + + return $popup; } /** diff --git a/src/Transport/JsonRpc/JsonRpcTransport.php b/src/Transport/JsonRpc/JsonRpcTransport.php index 7cd2168..07ec504 100644 --- a/src/Transport/JsonRpc/JsonRpcTransport.php +++ b/src/Transport/JsonRpc/JsonRpcTransport.php @@ -60,6 +60,11 @@ public function addEventDispatcher(string $id, EventDispatcherInterface $dispatc $this->eventDispatchers[$id] = $dispatcher; } + public function removeEventDispatcher(string $id): void + { + unset($this->eventDispatchers[$id]); + } + public function connect(): void { if ($this->connected) { diff --git a/tests/Mocks/EventDispatcherTransport.php b/tests/Mocks/EventDispatcherTransport.php new file mode 100644 index 0000000..48a9333 --- /dev/null +++ b/tests/Mocks/EventDispatcherTransport.php @@ -0,0 +1,79 @@ + */ + private array $eventDispatchers = []; + + public function __construct(private readonly TransportInterface $decorated) + { + } + + public function addEventDispatcher(string $id, EventDispatcherInterface $dispatcher): void + { + $this->eventDispatchers[$id] = $dispatcher; + } + + public function removeEventDispatcher(string $id): void + { + unset($this->eventDispatchers[$id]); + } + + /** + * @return array + */ + public function getEventDispatchers(): array + { + return $this->eventDispatchers; + } + + public function connect(): void + { + $this->decorated->connect(); + } + + public function disconnect(): void + { + $this->decorated->disconnect(); + } + + public function send(array $message): array + { + return $this->decorated->send($message); + } + + public function sendAsync(array $message): void + { + $this->decorated->sendAsync($message); + } + + public function isConnected(): bool + { + return $this->decorated->isConnected(); + } + + public function processEvents(): void + { + $this->decorated->processEvents(); + } +} diff --git a/tests/Unit/Browser/BrowserContextCloseTest.php b/tests/Unit/Browser/BrowserContextCloseTest.php new file mode 100644 index 0000000..354a9c9 --- /dev/null +++ b/tests/Unit/Browser/BrowserContextCloseTest.php @@ -0,0 +1,167 @@ +mockTransport = new MockTransport(); + $this->mockTransport->connect(); + $this->transport = new EventDispatcherTransport($this->mockTransport); + $this->browser = new Browser($this->transport, 'browser_1', 'ctx_default', '1.0', new PlaywrightConfig()); + } + + public function testCloseForgetsTheContextAndReleasesItsDispatchers(): void + { + $context = $this->openContext('ctx_1'); + $this->mockTransport->queueResponse(['pageId' => 'page_1']); + $context->newPage(); + $this->mockTransport->queueResponse([]); + $context->route('**/*', static function (): void {}); + + $this->assertSame([$this->browser->context(), $context], $this->browser->contexts()); + $this->assertSame(['ctx_default', 'ctx_1', 'page_1'], array_keys($this->transport->getEventDispatchers())); + + $this->mockTransport->queueResponse([]); + $context->close(); + + $this->assertSame([$this->browser->context()], $this->browser->contexts()); + $this->assertSame(['ctx_default'], array_keys($this->transport->getEventDispatchers())); + $this->assertSame([], $context->pages()); + $this->assertSame( + ['newContext', 'context.newPage', 'context.route', 'context.close'], + array_column($this->mockTransport->getSentMessages(), 'action'), + ); + } + + public function testClosedContextsAreGarbageCollected(): void + { + $references = []; + for ($i = 0; $i < 3; ++$i) { + $context = $this->openContext('ctx_'.$i); + $this->mockTransport->queueResponse(['pageId' => 'page_'.$i]); + $page = $context->newPage(); + $this->mockTransport->queueResponse([]); + $page->route('**/*', static fn () => $context); + $this->mockTransport->queueResponse([]); + $context->route('**/*', static fn () => $page); + $this->mockTransport->queueResponse(['popupPageId' => 'popup_'.$i]); + $popup = $page->waitForPopup(static fn () => null); + $this->mockTransport->queueResponse([]); + $popup->route('**/*', static fn () => $context); + + $this->mockTransport->queueResponse([]); + $context->close(); + + $references[] = \WeakReference::create($context); + } + unset($context, $page, $popup); + + gc_collect_cycles(); + + foreach ($references as $reference) { + $this->assertNull($reference->get()); + } + } + + public function testCloseReleasesTheContextWhenTheCloseCommandFails(): void + { + $context = $this->openContext('ctx_1'); + $this->mockTransport->queueResponse(['pageId' => 'page_1']); + $context->newPage(); + + $this->mockTransport->queueResponse(new TransportException('context.close failed')); + try { + $context->close(); + $this->fail('Expected the close command to fail.'); + } catch (TransportException) { + } + + $this->assertSame([$this->browser->context()], $this->browser->contexts()); + $this->assertSame(['ctx_default'], array_keys($this->transport->getEventDispatchers())); + $this->assertSame([], $context->pages()); + } + + public function testPopupIsTrackedByItsContext(): void + { + $context = $this->openContext('ctx_1'); + $this->mockTransport->queueResponse(['pageId' => 'page_1']); + $page = $context->newPage(); + $this->mockTransport->queueResponse(['popupPageId' => 'popup_1']); + + $popup = $page->waitForPopup(static fn () => null); + + $this->assertSame([$page, $popup], $context->pages()); + + $this->mockTransport->queueResponse([]); + $context->close(); + + $this->assertSame(['ctx_default'], array_keys($this->transport->getEventDispatchers())); + } + + public function testPageCloseReleasesThePage(): void + { + $context = $this->openContext('ctx_1'); + $this->mockTransport->queueResponse(['pageId' => 'page_1']); + $page = $context->newPage(); + + $this->mockTransport->queueResponse([]); + $page->close(); + + $this->assertSame([], $context->pages()); + $this->assertSame(['ctx_default', 'ctx_1'], array_keys($this->transport->getEventDispatchers())); + $this->assertSame( + ['newContext', 'context.newPage', 'page.close'], + array_column($this->mockTransport->getSentMessages(), 'action'), + ); + } + + public function testPageClosedEventReleasesThePageDispatcher(): void + { + $context = $this->openContext('ctx_1'); + $this->mockTransport->queueResponse(['pageId' => 'page_1']); + $context->newPage(); + + $context->dispatchEvent('pageClosed', ['pageId' => 'page_1']); + + $this->assertSame([], $context->pages()); + $this->assertSame(['ctx_default', 'ctx_1'], array_keys($this->transport->getEventDispatchers())); + } + + private function openContext(string $contextId): BrowserContext + { + $this->mockTransport->queueResponse(['contextId' => $contextId]); + + return $this->browser->newContext(); + } +} diff --git a/tests/Unit/Transport/JsonRpc/JsonRpcTransportTest.php b/tests/Unit/Transport/JsonRpc/JsonRpcTransportTest.php index 944d53e..cbf7e2b 100644 --- a/tests/Unit/Transport/JsonRpc/JsonRpcTransportTest.php +++ b/tests/Unit/Transport/JsonRpc/JsonRpcTransportTest.php @@ -16,6 +16,7 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; +use Playwright\Event\EventDispatcherInterface; use Playwright\Exception\NetworkException; use Playwright\Tests\Mocks\TestLogger; use Playwright\Transport\JsonRpc\JsonRpcClient; @@ -222,6 +223,39 @@ public function testDestructorDisconnects(): void $this->assertFalse($transport->isConnected()); } + public function testEventsAreDispatchedToTheRegisteredDispatcher(): void + { + $transport = new JsonRpcTransport(processLauncher: $this->processLauncher, logger: $this->logger); + + $dispatcher = $this->createMock(EventDispatcherInterface::class); + $dispatcher->expects($this->once())->method('dispatchEvent')->with('console', ['text' => 'hi']); + $transport->addEventDispatcher('page_1', $dispatcher); + + $this->handleEvent($transport, ['objectId' => 'page_1', 'event' => 'console', 'params' => ['text' => 'hi']]); + } + + public function testRemovedDispatcherNoLongerReceivesEvents(): void + { + $transport = new JsonRpcTransport(processLauncher: $this->processLauncher, logger: $this->logger); + + $dispatcher = $this->createMock(EventDispatcherInterface::class); + $dispatcher->expects($this->never())->method('dispatchEvent'); + $transport->addEventDispatcher('page_1', $dispatcher); + $transport->removeEventDispatcher('page_1'); + + $this->handleEvent($transport, ['objectId' => 'page_1', 'event' => 'console', 'params' => []]); + + $this->assertFalse($this->logger->hasWarningRecords()); + } + + /** + * @param array $event + */ + private function handleEvent(JsonRpcTransport $transport, array $event): void + { + (new \ReflectionMethod($transport, 'handleEvent'))->invoke($transport, $event); + } + /** * @param array $config */