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 */