-
-
Notifications
You must be signed in to change notification settings - Fork 87
fix: Prevent silent crashes on WebSocket failure and add cookie persistence #197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.x
Are you sure you want to change the base?
Conversation
…stence
Problem:
- Navigation operations (visit, goto, navigate) would cause PHP to crash silently
- WebSocket connection failures resulted in infinite loops with no error output
- Cookies were lost between visit() calls due to context isolation
Root Cause:
- Client::fetch() returned empty string when WebSocket died
- json_decode('') returns null, causing null access in loop
- No timeout mechanism to detect hung operations
- Each visit() created a new browser context, losing all cookies
Fixes:
1. Client.php:
- Add timeout mechanism using hrtime()
- Detect null/empty WebSocket responses
- Validate JSON decode results
- Throw clear RuntimeException instead of hanging
2. Context.php:
- Add storageState() to export cookies/localStorage
- Add addCookies() to import cookies
- Add cookies() to read current cookies
- Add clearCookies() to remove cookies
3. PendingAwaitablePage.php:
- Add withStorageState() for cookie persistence
- Add withStorageStateFromFile() for file-based state
These changes ensure:
- Tests fail with clear error messages instead of crashing silently
- Cookies can be persisted across visit() calls
- Proper timeout handling prevents infinite loops
Fixes pestphp/pest#1605
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a critical bug where WebSocket connection failures during navigation cause PHP to crash silently due to an infinite loop in Client::execute(). It also adds cookie persistence features to enable storage and restoration of authentication state between browser contexts.
Changes:
- Added timeout mechanism and null handling in
Client::execute()to prevent infinite loops when WebSocket connections are lost - Implemented cookie management methods in
Contextclass (storageState, addCookies, cookies, clearCookies) - Added storage state support in
PendingAwaitablePageto save/restore cookies and localStorage
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/Playwright/Client.php | Adds timeout checks, null response handling, and JSON validation to prevent infinite loops on WebSocket failure |
| src/Playwright/Context.php | Implements cookie management and storage state methods for persisting browser state |
| src/Api/PendingAwaitablePage.php | Adds methods to load storage state from arrays or files when creating new page contexts |
| composer.json | Updates version to 4.2.99 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check for timeout to prevent infinite loops | ||
| $elapsed = hrtime(true) - $startTime; | ||
| if ($elapsed > $maxWaitTimeNs) { | ||
| throw new RuntimeException( | ||
| "Playwright operation '$method' timed out after ".round($elapsed / 1_000_000_000, 2).' seconds' | ||
| ); | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client-side timeout mechanism may cause issues with legitimate long-running operations. The timeout check on line 96 uses the operation timeout (which could be quite long for operations like page navigation), but applies it to the WebSocket message polling loop. This means if a legitimate operation takes longer than expected but the WebSocket is still connected and functioning, it will throw a RuntimeException even though the operation might eventually succeed. Consider using a separate, smaller timeout for detecting WebSocket connection issues versus operation timeouts, or relying on the server-side timeout handling.
| if ($storageState === null) { | ||
| throw new InvalidArgumentException("Invalid storage state JSON in: $path"); |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The withStorageStateFromFile method is missing validation for the JSON decode error case. While it checks if json_decode returns null, this could be due to either invalid JSON or valid JSON containing null. Consider using json_last_error() to differentiate between these cases and provide a more accurate error message.
| if ($storageState === null) { | |
| throw new InvalidArgumentException("Invalid storage state JSON in: $path"); | |
| if (json_last_error() !== JSON_ERROR_NONE) { | |
| throw new InvalidArgumentException( | |
| "Invalid storage state JSON in: $path. Error: ".json_last_error_msg() | |
| ); | |
| } | |
| if ($storageState === null) { | |
| throw new InvalidArgumentException("Storage state JSON must not be null in: $path"); |
| public function withStorageState(array $storageState): self | ||
| { | ||
| return new self($this->browserType, $this->device, $this->url, [ | ||
| 'storageState' => $storageState, | ||
| ...$this->options, | ||
| ]); | ||
| } | ||
|
|
||
| /** | ||
| * Loads storage state from a file path. | ||
| * | ||
| * The file should contain JSON with cookies and localStorage data | ||
| * from a previous Context::storageState() call. | ||
| */ | ||
| public function withStorageStateFromFile(string $path): self | ||
| { | ||
| if (! file_exists($path)) { | ||
| throw new InvalidArgumentException("Storage state file not found: $path"); | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| throw new InvalidArgumentException("Could not read storage state file: $path"); | ||
| } | ||
|
|
||
| /** @var array{cookies?: array, origins?: array}|null $storageState */ | ||
| $storageState = json_decode($contents, true); | ||
|
|
||
| if ($storageState === null) { | ||
| throw new InvalidArgumentException("Invalid storage state JSON in: $path"); | ||
| } | ||
|
|
||
| return $this->withStorageState($storageState); | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new storage state methods (withStorageState and withStorageStateFromFile) lack test coverage. Similar features in the codebase like geolocation have dedicated browser tests. Consider adding tests that verify the storage state can be saved, loaded from a file, and correctly applied to new contexts.
| public function storageState(): array | ||
| { | ||
| $response = $this->sendMessage('storageState'); | ||
|
|
||
| /** @var array{result: array{cookies: array, origins: array}} $message */ | ||
| foreach ($response as $message) { | ||
| if (isset($message['result'])) { | ||
| return $message['result']; | ||
| } | ||
| } | ||
|
|
||
| return ['cookies' => [], 'origins' => []]; | ||
| } | ||
|
|
||
| /** | ||
| * Adds cookies into this browser context. | ||
| * | ||
| * @param array<array{name: string, value: string, domain?: string, path?: string, expires?: float, httpOnly?: bool, secure?: bool, sameSite?: string}> $cookies | ||
| */ | ||
| public function addCookies(array $cookies): self | ||
| { | ||
| $response = $this->sendMessage('addCookies', ['cookies' => $cookies]); | ||
| $this->processVoidResponse($response); | ||
|
|
||
| return $this; | ||
| } | ||
|
|
||
| /** | ||
| * Gets all cookies in this browser context. | ||
| * | ||
| * @param array<string> $urls Optional URLs to filter cookies | ||
| * @return array<array{name: string, value: string, domain: string, path: string, expires: float, httpOnly: bool, secure: bool, sameSite: string}> | ||
| */ | ||
| public function cookies(array $urls = []): array | ||
| { | ||
| $response = $this->sendMessage('cookies', $urls !== [] ? ['urls' => $urls] : []); | ||
|
|
||
| /** @var array{result: array{cookies: array}} $message */ | ||
| foreach ($response as $message) { | ||
| if (isset($message['result']['cookies'])) { | ||
| return $message['result']['cookies']; | ||
| } | ||
| } | ||
|
|
||
| return []; | ||
| } | ||
|
|
||
| /** | ||
| * Clears all cookies from this browser context. | ||
| */ | ||
| public function clearCookies(): self | ||
| { | ||
| $response = $this->sendMessage('clearCookies'); | ||
| $this->processVoidResponse($response); | ||
|
|
||
| return $this; | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new cookie management methods (storageState, addCookies, cookies, clearCookies) lack test coverage. The repository has browser tests for other features, so these methods should have tests to verify they correctly interact with the Playwright server and handle edge cases like empty cookie arrays or invalid URLs.
| if ($response === null) { | ||
| throw new RuntimeException( | ||
| 'Invalid JSON response from Playwright server: '.substr($responseJson, 0, 100) | ||
| ); | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSON decode validation is incomplete. When json_decode() is called with an empty string, it returns null, which is indistinguishable from actual invalid JSON. Additionally, json_last_error() should be checked to differentiate between valid JSON null and decode errors. Consider checking json_last_error() after the decode operation to ensure the failure is actually a JSON parsing error and not a valid null response.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
This PR fixes a critical bug where any navigation crashes PHP silently. The root cause is an infinite loop in
Client::execute()when the WebSocket connection is lost.Changes:
Client.php - Timeout and null handling
hrtime()to prevent infinite loopsRuntimeExceptioninstead of hanging indefinitelyContext.php - Cookie management methods
storageState()- Export cookies and localStorageaddCookies()- Import cookies into contextcookies()- Read current cookiesclearCookies()- Clear all cookiesPendingAwaitablePage.php - Storage state support
withStorageState()- Set storage state for new contextwithStorageStateFromFile()- Load storage state from JSON fileRoot Cause
When WebSocket connection dies during navigation:
fetch()returnsnull(connection closed)json_decode(null)returnsnull$response['id']on nullTest Plan
Fixes pestphp/pest#1605
🤖 Generated with Claude Code