diff --git a/bin/lib/core.js b/bin/lib/core.js index ae77ed7..0d09a4b 100644 --- a/bin/lib/core.js +++ b/bin/lib/core.js @@ -77,6 +77,7 @@ class ErrorHandler { return { requestId, error: message, + errorName: error.name, stack: process.env.PLAYWRIGHT_DEBUG === 'true' ? error.stack : undefined, command: command?.action, }; diff --git a/bin/lib/handlers.js b/bin/lib/handlers.js index 6210335..d7208f3 100644 --- a/bin/lib/handlers.js +++ b/bin/lib/handlers.js @@ -358,7 +358,7 @@ class ContextHandler extends BaseHandler { requestId, error: error.message }); - return { popupPageId: null }; + throw error; } } } @@ -726,7 +726,7 @@ class PageHandler extends BaseHandler { requestId, error: error.message }); - return { popupPageId: null }; + throw error; } } } diff --git a/bin/lib/popup-coordinator.js b/bin/lib/popup-coordinator.js index 07b086e..c1439d2 100644 --- a/bin/lib/popup-coordinator.js +++ b/bin/lib/popup-coordinator.js @@ -26,6 +26,10 @@ class PopupCoordinator { ? src.waitForEvent('popup', { timeout }) : src.waitForEvent('page', { timeout }); + // PHP may still be running the action when this rejects. Observe + // it now; awaiting the original promise below still throws it. + popupPromise.catch(() => {}); + return { popupPromise, listenerId: generateId(isPage ? 'popup_listener' : 'context_popup_listener'), @@ -65,7 +69,7 @@ class PopupCoordinator { return { popupPageId, popup }; } catch (error) { logger.error(`${isPage ? 'Popup' : 'Context popup'} wait failed`, { requestId, error: error.message }); - return { popupPageId: null }; + throw error; } }, waitForCallback: false diff --git a/bin/playwright-server.js b/bin/playwright-server.js index 05fde22..732bf25 100644 --- a/bin/playwright-server.js +++ b/bin/playwright-server.js @@ -345,7 +345,7 @@ class PlaywrightServer extends BaseHandler { } catch (error) { logger.error('Callback continuation failed', { requestId, error: error.message }); - return { error: error.message }; + throw error; } } diff --git a/src/Transport/JsonRpc/JsonRpcTransport.php b/src/Transport/JsonRpc/JsonRpcTransport.php index 7cd2168..81091a3 100644 --- a/src/Transport/JsonRpc/JsonRpcTransport.php +++ b/src/Transport/JsonRpc/JsonRpcTransport.php @@ -16,6 +16,7 @@ use Playwright\Event\EventDispatcherInterface; use Playwright\Exception\NetworkException; +use Playwright\Transport\ErrorMapper; use Playwright\Transport\TransportInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; @@ -376,34 +377,46 @@ private function handleCallbackCommand(array $message, ?int $timeoutMs): array throw new NetworkException('JSON-RPC client not available'); } $client = $this->client; - $response = $client->sendRaw($message, $timeoutMs); - - $this->logger->debug('Callback command response received', [ - 'requestId' => $requestId, - 'response' => $response, - ]); + try { + $response = $client->sendRaw($message, $timeoutMs); - if (isset($response['type']) && 'callback' === $response['type']) { - $this->logger->info('Server requested callback', [ + $this->logger->debug('Callback command response received', [ 'requestId' => $requestId, - 'callbackType' => $response['callbackType'] ?? 'unknown', + 'response' => $response, ]); - $this->executeCallback($response); + if (isset($response['type']) && 'callback' === $response['type']) { + $this->logger->info('Server requested callback', [ + 'requestId' => $requestId, + 'callbackType' => $response['callbackType'] ?? 'unknown', + ]); - $continueMessage = [ - 'action' => 'callback.continue', - 'requestId' => $requestId, - 'callbackResult' => ['executed' => true], - ]; + $this->executeCallback($response); - $finalResponse = $client->sendRaw($continueMessage, $timeoutMs); - unset($this->pendingCallbacks[$requestId]); + $continueMessage = [ + 'action' => 'callback.continue', + 'requestId' => $requestId, + 'callbackResult' => ['executed' => true], + ]; - return $finalResponse; - } + $response = $client->sendRaw($continueMessage, $timeoutMs); + } - return $response; + if (isset($response['error'])) { + $error = $response['error']; + $errorData = [ + 'name' => is_array($error) ? ($error['name'] ?? null) : ($response['errorName'] ?? null), + 'message' => is_array($error) ? ($error['message'] ?? null) : $error, + 'stack' => is_array($error) ? ($error['stack'] ?? null) : ($response['stack'] ?? null), + 'code' => is_array($error) ? ($error['code'] ?? null) : null, + ]; + throw ErrorMapper::toException($errorData, is_string($message['action'] ?? null) ? $message['action'] : null, $message, $timeoutMs); + } + + return $response; + } finally { + unset($this->pendingCallbacks[$requestId]); + } } /** diff --git a/tests/Integration/Popup/PopupFailureTest.php b/tests/Integration/Popup/PopupFailureTest.php new file mode 100644 index 0000000..f759e38 --- /dev/null +++ b/tests/Integration/Popup/PopupFailureTest.php @@ -0,0 +1,94 @@ +setUpPlaywright(); + } + + protected function tearDown(): void + { + $this->closeSharedPlaywright(); + } + + /** @return iterable */ + public static function popupOwners(): iterable + { + yield 'page' => [false]; + yield 'context' => [true]; + } + + #[DataProvider('popupOwners')] + public function testClosingTheOwnerPreservesTheFailure(bool $useContext): void + { + $context = $this->browser->newContext(); + $owner = $useContext ? $context : $context->newPage(); + + try { + $owner->waitForPopup(static function () use ($owner): void { + $owner->close(); + // The wait rejects before PHP sends the callback continuation. + usleep(50000); + }, ['timeout' => 1000]); + $this->fail('Closing the popup owner must fail the wait.'); + } catch (PlaywrightException $e) { + $this->assertNotInstanceOf(TimeoutException::class, $e); + $this->assertStringContainsString('closed', $e->getMessage()); + } + + $this->assertSame('about:blank', $this->page->url()); + } + + #[DataProvider('popupOwners')] + public function testTimeoutPreservesTheNativeDetails(bool $useContext): void + { + $owner = $useContext ? $this->context : $this->page; + + $this->expectException(TimeoutException::class); + $this->expectExceptionMessage('Timeout 100ms exceeded'); + + $owner->waitForPopup(static function (): void {}, ['timeout' => 100]); + } + + #[DataProvider('popupOwners')] + public function testTimeoutBeforeCallbackReturnsDoesNotCrashTheBridge(bool $useContext): void + { + $owner = $useContext ? $this->context : $this->page; + + try { + $owner->waitForPopup(static function (): void { + usleep(200000); + }, ['timeout' => 50]); + $this->fail('The popup wait must time out.'); + } catch (TimeoutException $e) { + $this->assertStringContainsString('Timeout 50ms exceeded', $e->getMessage()); + } + + $this->assertSame('about:blank', $this->page->url()); + } +} diff --git a/tests/Unit/Transport/JsonRpc/JsonRpcTransportTest.php b/tests/Unit/Transport/JsonRpc/JsonRpcTransportTest.php index 944d53e..6621b38 100644 --- a/tests/Unit/Transport/JsonRpc/JsonRpcTransportTest.php +++ b/tests/Unit/Transport/JsonRpc/JsonRpcTransportTest.php @@ -16,7 +16,9 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; +use Playwright\Exception\DisconnectedException; use Playwright\Exception\NetworkException; +use Playwright\Exception\TimeoutException; use Playwright\Tests\Mocks\TestLogger; use Playwright\Transport\JsonRpc\JsonRpcClient; use Playwright\Transport\JsonRpc\JsonRpcTransport; @@ -248,6 +250,71 @@ private function injectClient(JsonRpcTransport $transport, JsonRpcClient $client (new \ReflectionProperty($transport, 'client'))->setValue($transport, $client); } + public function testCallbackSetupFailurePreservesTheErrorAndReleasesTheCallback(): void + { + $transport = $this->createConnectedTransport(['command' => ['node', 'server.js']]); + $transport->storePendingCallback('popup', fn () => $this->fail('The action must not run after setup fails.')); + $client = $this->createMock(JsonRpcClient::class); + $client->expects($this->once())->method('sendRaw')->willReturn([ + 'error' => ['name' => 'TargetClosedError', 'message' => 'The popup owner has closed'], + ]); + $this->injectClient($transport, $client); + + try { + $transport->send(['action' => 'page.waitForPopup', 'requestId' => 'popup']); + $this->fail('Expected the setup error.'); + } catch (DisconnectedException $e) { + $this->assertSame('The popup owner has closed', $e->getMessage()); + $this->assertSame('page.waitForPopup', $e->getContext()['method']); + } + + $this->assertSame([], (new \ReflectionProperty($transport, 'pendingCallbacks'))->getValue($transport)); + } + + public function testCallbackContinuationPreservesTheTimeoutAndReleasesTheCallback(): void + { + $transport = $this->createConnectedTransport(['command' => ['node', 'server.js']]); + $executed = false; + $transport->storePendingCallback('popup', static function () use (&$executed): void { $executed = true; }); + $client = $this->createMock(JsonRpcClient::class); + $client->expects($this->exactly(2))->method('sendRaw')->willReturnOnConsecutiveCalls( + ['type' => 'callback', 'callbackType' => 'readyForAction', 'requestId' => 'popup'], + ['error' => 'Timeout 100ms exceeded while waiting for popup', 'errorName' => 'TimeoutError'], + ); + $this->injectClient($transport, $client); + + try { + $transport->send(['action' => 'context.waitForPopup', 'requestId' => 'popup']); + $this->fail('Expected the native timeout.'); + } catch (TimeoutException $e) { + $this->assertSame('Timeout 100ms exceeded while waiting for popup', $e->getMessage()); + } + + $this->assertTrue($executed); + $this->assertSame([], (new \ReflectionProperty($transport, 'pendingCallbacks'))->getValue($transport)); + } + + public function testThrowingCallbackIsReleasedAndItsExceptionIsPreserved(): void + { + $transport = $this->createConnectedTransport(['command' => ['node', 'server.js']]); + $failure = new \RuntimeException('Action failed'); + $transport->storePendingCallback('popup', static fn () => throw $failure); + $client = $this->createMock(JsonRpcClient::class); + $client->expects($this->once())->method('sendRaw')->willReturn([ + 'type' => 'callback', 'callbackType' => 'readyForAction', 'requestId' => 'popup', + ]); + $this->injectClient($transport, $client); + + try { + $transport->send(['action' => 'page.waitForPopup', 'requestId' => 'popup']); + $this->fail('Expected the action failure.'); + } catch (\RuntimeException $e) { + $this->assertSame($failure, $e); + } + + $this->assertSame([], (new \ReflectionProperty($transport, 'pendingCallbacks'))->getValue($transport)); + } + public function testSendPassesTheConfiguredTimeoutPerRequest(): void { $transport = $this->createConnectedTransport(['command' => ['node', 'server.js'], 'timeout' => 45]);