Repository navigation
fix(chromium): allow explicit response.body() to re-fetch any evicted GET - #42863
Closed
Evan Rusackas (rusackas) wants to merge 1 commit into
Closed
Evan Rusackas (rusackas) wants to merge 1 commit into
Evan Rusackas (rusackas) wants to merge 1 commit into
Conversation
… GET microsoft#42124/microsoft#42201 restricted the evicted-body re-fetch in createResponseBodyCallback to static-subresource GETs and prefetches, to stop harTracer's automatic per-response HAR capture from silently re-issuing requests with server-side side effects (microsoft#42002). That same restriction also applies when a test explicitly awaits response.body() once for a response it already knows about, which is a different situation: harTracer's capture is implicit and blanket, an explicit body() call is a one-off the caller asked for by name. The gate collaterally broke response.body() for any ordinary fetch()/XHR GET whose body the page has already read (e.g. .blob() before building a download), which Chromium evicts the same way it evicts image bodies - "fetch" and "xhr" are not in kRefetchSafeResourceTypes and carry no Sec-Purpose: prefetch header. Thread an isExplicit flag from Response.body() (the client-facing, dispatcher-driven path) down through internalBody() to createResponseBodyCallback(), defaulting to false. harTracer.ts's existing internalBody() call is untouched and keeps today's restrictive behavior. The method check (never re-fetch non-GET) is unconditional either way: an explicit call to body() is not an invitation to resubmit a POST. GetResponseBodyCallback gained a parameter; firefox/webkit/bidi's zero-argument callbacks stay unchanged and still satisfy the type (fewer declared params than the type expects is a valid assignment), so only Chromium's implementation needed behavior changes. Bisected against a real-world break: apache/superset's Playwright e2e suite started failing exactly this way on the 1.62.1 -> 1.63.0 bump, asserting on an exported file's contents via page.waitForResponse() after the app's own code read it into a Blob for download. Fixes microsoft#42862 Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
Thanks in advance for any CI approval and review comments... happy to help here wherever I'm able. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
response.body()for a page-levelResponsecan return an empty buffer once the page's own JS has already consumed the samefetch()response (e.g. via.blob()), on any request whose CDP resource type isn't inkRefetchSafeResourceTypes(i.e. anything but static subresources / prefetches) - most notably plainfetch()/XHR GETs. Filed as #42862, with a minimal, version-bisected repro (works on 1.62.1, broken on 1.63.0).Root cause
#42124 / #42201 restricted the evicted-body recovery path (
Network.loadNetworkResourcere-fetch) to static-subresource GETs and prefetch requests, to stopharTracer's automatic per-response HAR/trace capture from silently re-issuing requests with server-side side effects (#42002 - a tracking pixel getting hit twice).That fix is correct for the case it targets, but the same restriction also gates the case where a test explicitly awaits
response.body()once, for one response it already asked for by name (page.waitForResponse(...)thenawait response.body()). That's a fundamentally different risk:harTracercallsinternalBody()on every response with no way to opt out, while an explicitresponse.body()call is a one-off the caller deliberately wrote. Gating both the same way means any app that reads afetch()/XHR response in-page (e.g.const blob = await response.blob()before turning it into a download - a completely ordinary pattern) makes that response's body permanently unrecoverable for any laterresponse.body()call, once Chromium evicts it.Fix
Thread an
isExplicitflag fromResponse.body()(the client-facing, dispatcher-driven entrypoint) down throughinternalBody()tocreateResponseBodyCallback(), defaulting tofalse:harTracer.ts's existingresponse.internalBody()call is untouched, so the automatic HAR/trace capture path keeps exactly today's restrictive behavior and [Bug]: Tracing silently re-downloads resources via Network.loadNetworkResource, duplicating requests with side effects (Chromium only) #42002 stays fixed.Response.body()now callsinternalBody(true), so an explicit caller can recover an evicted body for any GET, not just the static-subresource/prefetch allowlist.method !== 'GET'check is unconditional either way: an explicitbody()call is not an invitation to resubmit a POST.GetResponseBodyCallbackgained a parameter; Firefox/WebKit/BiDi's existing zero-argument callbacks are unchanged and still satisfy the (now one-parameter) type, since a function with fewer declared parameters is assignable to a type expecting more. Only Chromium's implementation needed a behavior change.Testing
should return body for a fetch()ed resource with evicted body when the page already read itnext to the existingshould return body for image with evicted body(fix(chromium): only re-fetch replay-safe requests when reading response body #42124's own regression test) intests/page/page-network-response.spec.ts.Expected: "hello from resource.bin",Received: "") and passes with it.tests/page/page-network-response.spec.ts: 29 passed.tests/library/har.spec.ts+tests/library/browsercontext-har.spec.ts: 97 passed, 1 skipped (unrelated) - confirms theharTracerpath (and [Bug]: Tracing silently re-downloads resources via Network.loadNetworkResource, duplicating requests with side effects (Chromium only) #42002's fix) is unaffected.tests/library/tracing.spec.ts: 40 passed, 1 skipped (unrelated).npm run lint(eslint, tsc, doc, check-deps, lint-tests, test-types, lint-packages): clean, only one pre-existing unrelated warning intrace-viewer/workbench.tsx.Fixes #42862