fix: uploads, bodiless responses, late responses and attempt timeouts under load

src/Api/AwaitableWebpage.php

@@ -42,6 +42,13 @@ public function __construct(
             'keys',
             'drag',
             'append',
+            // A repeated navigation restarts the page load the previous attempt was waiting
+            // for, so on a slow runner none of them ever finishes within an attempt. Playwright
+            // already waits for the load state itself.
+            'navigate',
+            'refresh',
+            'back',
+            'forward',
         ],
     ) {
         //

src/Drivers/LaravelHttpServer.php

@@ -374,11 +374,15 @@ private function handleRequest(AmpRequest $request): Response
             }
         }
 
-        return new Response(
-            $response->getStatusCode(),
-            $response->headers->all(), // @phpstan-ignore-line
-            $content,
-        );
+        // Headers before the body: the constructor sets the body first and then replaces every
+        // header with the given ones, which drops the Content-Length the body had just set. Without
+        // one, a 204 or 304 is framed chunked and its terminator reaches Chromium as the start of the
+        // next response on the socket.
+        $ampResponse = new Response($response->getStatusCode());
+        $ampResponse->setHeaders($response->headers->all()); // @phpstan-ignore-line
+        $ampResponse->setBody($content);
+
+        return $ampResponse;
     }
 
     /**
@@ -488,7 +492,9 @@ private function parseMultipartBody(string $body, string $boundary): array
         $files = [];
 
         foreach (explode("--{$boundary}", $body) as $part) {
-            $part = mb_ltrim($part, "\r\n");
+            // Byte for byte, not mb_ltrim(): that replaces every byte which is not valid UTF-8, and a
+            // file part is binary.
+            $part = (string) preg_replace('/^[\r\n]+/', '', $part);
             if ($part === '' || str_starts_with($part, '--')) {
                 continue;
             }

src/Execution.php

@@ -126,9 +126,14 @@ public function waitForExpectation(callable $callback): mixed
         $start = microtime(true);
         $end = $start + ($timeout / 1_000);
 
+        // Each attempt bounds the Playwright calls it makes, so a page that loads or paints slowly
+        // gets a longer attempt when the configured timeout is longer, instead of every attempt
+        // failing at the same one second on a busy CI runner.
+        $attempt = max(1_000, intdiv($timeout, 5));
+
         while (microtime(true) < $end) {
             try {
-                return Playwright::usingTimeout(1_000, $callback);
+                return Playwright::usingTimeout($attempt, $callback);
             } catch (ExpectationFailedException) {
                 //
             }

src/Playwright/Client.php

@@ -107,6 +107,15 @@ public function execute(string $guid, string $method, array $params = [], array
             /** @var array{id: string|null, params: array{add: string|null}, error: array{error: array{message: string|null}}} $response */
             $response = json_decode($responseJson, true);
 
+            // A response to another request is the late answer of one whose consumer stopped
+            // reading before it arrived: a navigation once it reached its waitUntil state, a
+            // querySelector once the handle was created. Its result and its error belong to that
+            // request. Taken as this one's, the error fails an assertion that passed, and the
+            // result reaches whichever call reads the next value, an evaluate() included.
+            if (isset($response['id']) && $response['id'] !== $requestId) {
+                continue;
+            }
+
             if (isset($response['error']['error']['message'])) {
                 $message = $response['error']['error']['message'];
 

tests/Browser/Visit/BodilessResponseTest.php

@@ -0,0 +1,69 @@
+<?php
+
+declare(strict_types=1);
+
+use Amp\CancelledException;
+use Amp\TimeoutCancellation;
+use Illuminate\Http\Response;
+use Illuminate\Support\Facades\Route;
+use Pest\Browser\Drivers\LaravelHttpServer;
+use Pest\Browser\ServerManager;
+
+use function Amp\Socket\connect;
+
+/**
+ * Everything the server writes for one request, read raw because a browser assertion cannot see it:
+ * Chromium discards a stray chunk terminator that has already arrived, and fails the next request on
+ * the socket when it has not, so the same bug passes or fails with timing.
+ */
+function bodilessResponseWireTo(string $path): string
+{
+    $http = ServerManager::instance()->http();
+    assert($http instanceof LaravelHttpServer);
+
+    $socket = connect("127.0.0.1:{$http->port}");
+    $socket->write("GET {$path} HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: keep-alive\r\n\r\n");
+
+    $raw = '';
+
+    try {
+        while (($chunk = $socket->read(new TimeoutCancellation(0.4))) !== null) {
+            $raw .= $chunk;
+        }
+    } catch (CancelledException) {
+        // Silence is the end of the response: the connection stays open on keep-alive.
+    }
+
+    $socket->close();
+
+    return $raw;
+}
+
+it('writes nothing after the headers of a response that has no body', function (int $status): void {
+    Route::get('/', fn (): string => 'home');
+    Route::get('/bodiless', fn (): Response => new Response('', $status));
+
+    // Starts the in-process server; the response under test is then read off the wire.
+    visit('/')->assertSee('home');
+
+    $raw = bodilessResponseWireTo('/bodiless');
+
+    expect($raw)->toStartWith("HTTP/1.1 {$status} ")
+        ->and($raw)->toEndWith("\r\n\r\n")
+        // The bytes Chromium reads as the start of the next response on the socket.
+        ->and($raw)->not->toContain("0\r\n\r\n")
+        ->and(mb_strtolower($raw))->not->toContain('transfer-encoding: chunked');
+})->with([204, 304]);
+
+it('writes a response that has a body with its length', function (): void {
+    Route::get('/', fn (): string => 'home');
+    Route::get('/body', fn (): string => 'hello');
+
+    visit('/')->assertSee('home');
+
+    $raw = bodilessResponseWireTo('/body');
+
+    expect($raw)->toStartWith('HTTP/1.1 200 ')
+        ->and(mb_strtolower($raw))->toContain("content-length: 5\r\n")
+        ->and($raw)->toEndWith("\r\n\r\nhello");
+});

tests/Browser/Webpage/UploadTest.php

@@ -39,3 +39,27 @@
 
     unlink($tempFile);
 });
+
+it('keeps the bytes of an uploaded file that are not valid UTF-8', function (): void {
+    Route::get('/', fn (): string => '
+        <form method="post" action="/upload" enctype="multipart/form-data">
+            <input type="file" id="avatar" name="avatar">
+            <button type="submit">Upload</button>
+        </form>
+    ');
+    Route::post('/upload', fn (Request $request): string => 'received '.bin2hex(
+        (string) file_get_contents($request->file('avatar')->getPathname()),
+    ));
+
+    // The first bytes of a PNG, which no UTF-8 decoder accepts.
+    $bytes = "\x89PNG\r\n\x1a\n\x00\x00\x00\x0dIHDR\xff\xfe\x80";
+    $tempFile = tempnam(sys_get_temp_dir(), 'test');
+    file_put_contents($tempFile, $bytes);
+
+    visit('/')
+        ->attach('#avatar', $tempFile)
+        ->click('Upload')
+        ->assertSee('received '.bin2hex($bytes));
+
+    unlink($tempFile);
+});

tests/Unit/Playwright/ClientTest.php

@@ -0,0 +1,195 @@
+<?php
+
+declare(strict_types=1);
+
+use Amp\ByteStream\ReadableStream;
+use Amp\Cancellation;
+use Amp\Http\Client\Response;
+use Amp\Socket\SocketAddress;
+use Amp\Socket\TlsInfo;
+use Amp\Websocket\Client\WebsocketConnection;
+use Amp\Websocket\WebsocketCloseCode;
+use Amp\Websocket\WebsocketCloseInfo;
+use Amp\Websocket\WebsocketCount;
+use Amp\Websocket\WebsocketMessage;
+use Amp\Websocket\WebsocketTimestamp;
+use Pest\Browser\Playwright\Client;
+use PHPUnit\Framework\ExpectationFailedException;
+
+/**
+ * A client whose connection answers the next request with the given messages, in order. The
+ * placeholder "{id}" in a message stands for the id of that request.
+ *
+ * @param  array<int, array<string, mixed>>  $messages
+ */
+function clientAnsweringWith(array $messages): Client
+{
+    $connection = new class($messages) implements IteratorAggregate, WebsocketConnection
+    {
+        private string $requestId = '';
+
+        /**
+         * @param  array<int, array<string, mixed>>  $messages
+         */
+        public function __construct(private array $messages)
+        {
+            //
+        }
+
+        public function sendText(string $data): void
+        {
+            /** @var array{id: string} $request */
+            $request = json_decode($data, true);
+
+            $this->requestId = $request['id'];
+        }
+
+        public function receive(?Cancellation $cancellation = null): WebsocketMessage
+        {
+            $message = array_shift($this->messages);
+
+            if ($message === null) {
+                throw new LogicException('The request was answered, yet the client keeps reading.');
+            }
+
+            return WebsocketMessage::fromText(str_replace('{id}', $this->requestId, (string) json_encode($message)));
+        }
+
+        public function getIterator(): Iterator
+        {
+            yield from [];
+        }
+
+        public function getHandshakeResponse(): Response
+        {
+            throw new LogicException('Not part of the scripted exchange.');
+        }
+
+        public function getId(): int
+        {
+            return 0;
+        }
+
+        public function getLocalAddress(): SocketAddress
+        {
+            throw new LogicException('Not part of the scripted exchange.');
+        }
+
+        public function getRemoteAddress(): SocketAddress
+        {
+            throw new LogicException('Not part of the scripted exchange.');
+        }
+
+        public function getTlsInfo(): ?TlsInfo
+        {
+            return null;
+        }
+
+        public function getCloseInfo(): WebsocketCloseInfo
+        {
+            throw new LogicException('Not part of the scripted exchange.');
+        }
+
+        public function isCompressionEnabled(): bool
+        {
+            return false;
+        }
+
+        public function sendBinary(string $data): void
+        {
+            //
+        }
+
+        public function streamText(ReadableStream $stream): void
+        {
+            //
+        }
+
+        public function streamBinary(ReadableStream $stream): void
+        {
+            //
+        }
+
+        public function ping(): void
+        {
+            //
+        }
+
+        public function getCount(WebsocketCount $type): int
+        {
+            return 0;
+        }
+
+        public function getTimestamp(WebsocketTimestamp $type): float
+        {
+            return 0.0;
+        }
+
+        public function isClosed(): bool
+        {
+            return false;
+        }
+
+        public function close(int $code = WebsocketCloseCode::NORMAL_CLOSE, string $reason = ''): void
+        {
+            //
+        }
+
+        public function onClose(Closure $onClose): void
+        {
+            //
+        }
+    };
+
+    $client = new Client;
+
+    new ReflectionProperty(Client::class, 'websocketConnection')->setValue($client, $connection);
+
+    return $client;
+}
+
+it('passes over the late error of an earlier request', function (): void {
+    $client = clientAnsweringWith([
+        ['id' => 'earlier', 'error' => ['error' => ['message' => 'Timeout 1000ms exceeded.']]],
+        ['id' => '{id}', 'result' => ['value' => ['b' => true]]],
+    ]);
+
+    $messages = iterator_to_array($client->execute('frame@1', 'isVisible', ['selector' => '#x']), false);
+
+    expect($messages)->toHaveCount(1)
+        ->and($messages[0]['result']['value'])->toBe(['b' => true]);
+});
+
+it('passes over the late result of an earlier request', function (): void {
+    $client = clientAnsweringWith([
+        ['id' => 'earlier', 'result' => ['value' => ['b' => false]]],
+        ['id' => '{id}', 'result' => ['value' => ['a' => []]]],
+    ]);
+
+    $messages = iterator_to_array($client->execute('frame@1', 'evaluateExpression', ['expression' => 'window.errors']), false);
+
+    expect($messages)->toHaveCount(1)
+        ->and($messages[0]['result']['value'])->toBe(['a' => []]);
+});
+
+it('throws the error of its own request', function (): void {
+    $client = clientAnsweringWith([
+        ['id' => '{id}', 'error' => ['error' => ['message' => 'Timeout 1000ms exceeded.']]],
+    ]);
+
+    expect(fn (): array => iterator_to_array($client->execute('frame@1', 'isVisible', ['selector' => '#x']), false))
+        ->toThrow(ExpectationFailedException::class, 'Timeout 1000ms exceeded.');
+});
+
+it('hands on the events that arrive before its response', function (): void {
+    $client = clientAnsweringWith([
+        ['method' => '__create__', 'params' => ['type' => 'ElementHandle', 'guid' => 'handle@1']],
+        ['id' => '{id}', 'result' => ['element' => ['guid' => 'handle@1']]],
+    ]);
+
+    $messages = iterator_to_array($client->execute('frame@1', 'querySelector', ['selector' => '#x']), false);
+
+    expect($messages)->toHaveCount(2)
+        ->and($messages[0]['method'])->toBe('__create__')
+        ->and($messages[1]['result']['element']['guid'])->toBe('handle@1');
+});