Skip to content

Conversation

@hbenl
Copy link
Collaborator

@hbenl hbenl commented Nov 6, 2025

Fixes the following tests in Firefox:

  • tests/library/har.spec.ts:
    • "should include postData"
    • "should include binary postData"
    • "should include form params"
  • tests/page/network-post-data.spec.ts:
    • "should return correct postData buffer for utf-8 body"
    • "should return post data w/o content-type"
    • "should throw on invalid JSON in post data"
    • "should return post data for PUT requests"
    • "should get post data for file/blob"
    • "should get post data for navigator.sendBeacon api calls"
  • tests/page/page-network-request.spec.ts:
    • "should parse the data if content-type is application/x-www-form-urlencoded"
    • "should parse the data if content-type is application/x-www-form-urlencoded; charset=UTF-8"
    • "should parse the json post data"
    • "should return multipart/form-data"
    • "should return postData"
    • "should work with binary post data"
    • "should work with binary post data and interception"
  • tests/page/page-request-intercept.spec.ts:
    • "request.postData is not null when fetching FormData with a Blob"
  • tests/page/page-route.spec.ts:
    • "should intercept when postData is more than 1MB"

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@dgozman dgozman requested a review from yury-s November 6, 2025 22:34
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2025

Test results for "MCP"

2588 passed, 116 skipped


Merge workflow run.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2025

Test results for "tests 1"

1 failed
❌ [playwright-test] › runner.spec.ts:118 › should ignore subprocess creation error because of SIGINT @macos-latest-node18-2

4 flaky ⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/page-goto.spec.ts:83 › should work with Cross-Origin-Opener-Policy `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104 › should work with strict CSP policy `@firefox-ubuntu-22.04-node18`
⚠️ [playwright-test] › ui-mode-trace.spec.ts:719 › should partition action tree state by test `@macos-latest-node18-2`

40317 passed, 789 skipped


Merge workflow run.

}
}
const request = new BidiRequest(frame, redirectedFrom, param, route);
const requestData = await getNetworkData(this._session, param.request, bidi.Network.DataType.Request).catch(() => null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot do asynchronous staff in an event handler, the dispatcher is not waiting for the listener to complete and will fire subsequent events. With the current implementation the request may not be in the map when _onResponseStarted arrives and we'll miss the response.

Ideally we'd have an asynchronous request.body() and not have to wait here at all, but all the postData* methods in playwright API today are synchronous (which is a regret).

Alternatively, we could buffer all network events for a given request. However, to preserve their relative order we’d effectively need to buffer all network events, which complicates things. It also means unrelated events could interleave differently. E.g. a console message might appear before a network request. If we don't buffer up all the requests and wait for the request bodies independently, users may see sub-resource requests before the main request.

Considering all the above, we should add an async getter for the request body and have Bidi implement only that. Let's postpone this change until that is done. Filed #38150 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants