Fix /wp-admin blank page when response crosses Comlink worker boundary#3301
Fix /wp-admin blank page when response crosses Comlink worker boundary#3301JanJakes merged 2 commits intoWordPress:trunkfrom
Conversation
StreamedPHPResponse.fromPHPResponse() created an empty headersStream and relied on setting parsedHeaders directly. This worked on the same thread but failed when the response crossed a Comlink worker boundary, since the transfer handler only serializes the raw headersStream — the parsedHeaders property was lost. Encode the headers into the headersStream using the JSON format that parseHeadersStream expects, so headers survive Comlink serialization. The parsedHeaders fast-path is kept for same-thread access.
There was a problem hiding this comment.
Pull request overview
Fixes a regression where /wp-admin (no trailing slash) could render blank after crossing a Comlink worker boundary by ensuring response headers survive serialization.
Changes:
- Serialize headers into
headersStreamusing the JSON format expected byparseHeadersStream. - Keep
parsedHeadersas a same-thread fast path to avoid re-parsing. - Add tests verifying header encoding (including multi-value headers) and cross-thread reconstruction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/php-wasm/universal/src/lib/php-response.ts | Writes headers into headersStream so Comlink transfer preserves redirects/status/headers. |
| packages/php-wasm/universal/src/lib/php-response.spec.ts | Adds regression tests covering cross-thread header serialization and multi-value headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Set pre-parsed headers as a fast-path for same-thread | ||
| // access (avoids re-parsing the stream we just created) | ||
| streamed.parsedHeaders = Promise.resolve({ | ||
| headers: response.headers, | ||
| httpStatusCode: response.httpStatusCode, |
There was a problem hiding this comment.
This introduces two sources of truth for headers (the JSON encoded into headersStream and the parsedHeaders fast-path). To prevent future divergence, consider generating both from a single shared representation/helper (e.g., one function that returns { headersJson, parsedHeaders }, or deriving parsedHeaders from the same normalized structure used to build headerLines).
There was a problem hiding this comment.
Same as above — parsedHeaders is a cache, not a second source of truth. The stream carries the canonical data; the cached promise is a same-thread optimization set in the same call site.
There was a problem hiding this comment.
I would still remove it @epeicher . This introduces a second data flow that compute the same result. This means that, from now on, we need to always keep the two code paths in sync. I guarantee that one day someone will update one code path without updating the other one. Re-parsing is very fast, probably under a millisecond. It's not a performance problem at all. Let's re-parse and have less places in code where we can make a mistake.
There was a problem hiding this comment.
Ok, let me work on a quick follow-up to avoid that 👍
There was a problem hiding this comment.
@adamziel @epeicher Btw., this is not directly related, but I noticed there is already a difference between same thread and Comlink usage. It seems that in parseHeadersStream, we lowercase the header name, while on a single thread, it remains untouched. Should we unify that too?
ashfame
requested review from
a team,
JanJakes and
fellyph
and removed request for
a team
JanJakes
left a comment
JanJakes
left a comment
There was a problem hiding this comment.
Looks good, thanks for fixing this! I have just small question.
| headersStream, | ||
| stdout, | ||
| emptyStream(), | ||
| emptyStream, |
There was a problem hiding this comment.
When at it, could we make fromPHPResponse also preserve stderr? I know there is no current usage of that (none of the instantiated responses have errors in them), but it would make the method more complete and universal.
There was a problem hiding this comment.
That makes total sense, thanks for the suggestion! I've added this commit 9783ad6 that encodes response.errors into the stderr stream instead of using an empty stream, and updated the existing tests to assert stderr is preserved through fromPHPResponse() and the round-trip.
Encode response.errors into the stderr stream instead of using an empty stream, so stderr data survives Comlink worker boundary transfer.
Motivation for the change, related issues
In Playground 3.1.2, navigating to
/wp-admin(without trailing slash) in an incognito window shows a blank page instead of redirecting to the login page. This regression was introduced by the streaming response refactor (PR #3222).Implementation details
StreamedPHPResponse.fromPHPResponse()created an emptyheadersStreamand relied on setting theparsedHeadersproperty directly. This worked on the same thread, but when the response crossed a Comlink worker boundary, the transfer handler only serialized the rawheadersStream— theparsedHeadersproperty was lost.On the receiving thread, the deserialized response had an empty
headersStream. When headers were accessed,parseHeadersStreamread the empty stream,JSON.parse('')failed, and the fallback returned{ headers: {}, httpStatusCode: 200 }. The 301 redirect became a 200 OK with no headers and an empty body.The fix encodes headers into the
headersStreamusing the JSON format thatparseHeadersStreamexpects ({ status, headers: ["Name: value", ...] }), so headers survive Comlink serialization. TheparsedHeadersfast-path is kept for same-thread access.This affected all responses created via
fromPHPResponse()orforHttpCode()that cross a worker boundary: directory trailing-slash 301 redirects, 404 responses, 502 responses, and static file responses.Testing Instructions (or ideally a Blueprint)
npx nx test php-wasm-universal --testFile=php-response.spec.tsnpx nx test php-wasm-universal --testFile=php-request-handler.spec.tsManual test
npx nx build php-wasm-universalnpx nx dev playground-cli server --wp=6.8 --php=8.4 --loginhttp://127.0.0.1:9400/wp-admin(please ensure that there isn't a trailing slash/at the end)wp-adminsuccessfullytrunkand check that you see a blank page in trunk