Skip to content

Commit

Permalink
Revert "fetch: Set response body to null when status is a null body s…
Browse files Browse the repository at this point in the history
…tatus"

This reverts commit 58a2d91.

Reason for revert: Breaks a significant number of apps that depend on the non-conforming behavior.  See crbug.com/1431885

Original change's description:
> fetch: Set response body to null when status is a null body status
>
> The Fetch standard specifies this behavior[1].
>
> [1] https://fetch.spec.whatwg.org/#main-fetch
>
> Bug: 1297060
> Change-Id: I74cce53c0b36a6356716380d0a10885584a70d0d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4355096
> Reviewed-by: Adam Rice <ricea@chromium.org>
> Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1120397}

(cherry picked from commit ad69498)

Bug: 1297060, 1431885
Change-Id: Id50bb07c7b13bc7f34bcf9af6d03f12335d688c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4508015
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Patrick Meenan <pmeenan@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1139848}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4509653
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Prudhvikumar Bommana <pbommana@google.com>
Commit-Queue: Prudhvikumar Bommana <pbommana@google.com>
Cr-Commit-Position: refs/branch-heads/5672_63@{#7}
Cr-Branched-From: 0e1a447-refs/branch-heads/5672@{#912}
Cr-Branched-From: 5f2a724-refs/heads/main@{#1121455}
  • Loading branch information
pmeenan authored and Chromium LUCI CQ committed May 5, 2023
1 parent 89fbcab commit 135354b
Show file tree
Hide file tree
Showing 10 changed files with 74 additions and 31 deletions.
20 changes: 3 additions & 17 deletions third_party/blink/renderer/core/fetch/fetch_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -418,19 +418,10 @@ void FetchManager::Loader::DidReceiveResponse(
execution_context_->GetSecurityOrigin()));
}

// Step 21 of https://fetch.spec.whatwg.org/#main-fetch
// Set response body to null when method is `HEAD` or `CONNECT`, or status
// is a null body status.
BodyStreamBuffer* buffer = nullptr;
if (!Response::IsNullBodyStatus(response.HttpStatusCode()) &&
fetch_request_data_->Method() != http_names::kHEAD &&
fetch_request_data_->Method() != http_names::kCONNECT) {
place_holder_body_ = MakeGarbageCollected<PlaceHolderBytesConsumer>();
buffer = BodyStreamBuffer::Create(script_state, place_holder_body_, signal_,
cached_metadata_handler_);
}
place_holder_body_ = MakeGarbageCollected<PlaceHolderBytesConsumer>();
FetchResponseData* response_data =
FetchResponseData::CreateWithBuffer(buffer);
FetchResponseData::CreateWithBuffer(BodyStreamBuffer::Create(
script_state, place_holder_body_, signal_, cached_metadata_handler_));
if (!execution_context_ || execution_context_->IsContextDestroyed() ||
response.GetType() == FetchResponseType::kError) {
// BodyStreamBuffer::Create() may run scripts and cancel this request.
Expand Down Expand Up @@ -508,11 +499,6 @@ void FetchManager::Loader::DidReceiveCachedMetadata(mojo_base::BigBuffer data) {
}

void FetchManager::Loader::DidStartLoadingResponseBody(BytesConsumer& body) {
if (!place_holder_body_) {
body.Cancel();
return;
}

if (fetch_request_data_->Integrity().empty() &&
!response_has_no_store_header_) {
// BufferingBytesConsumer reads chunks from |bytes_consumer| as soon as
Expand Down
18 changes: 9 additions & 9 deletions third_party/blink/renderer/core/fetch/response.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,15 @@ FetchResponseData* CreateFetchResponseDataFromFetchAPIResponse(
return response;
}

// Checks whether |status| is a null body status.
// Spec: https://fetch.spec.whatwg.org/#null-body-status
bool IsNullBodyStatus(uint16_t status) {
if (status == 101 || status == 204 || status == 205 || status == 304)
return true;

return false;
}

// Check whether |statusText| is a ByteString and
// matches the Reason-Phrase token production.
// RFC 2616: https://tools.ietf.org/html/rfc2616
Expand All @@ -118,15 +127,6 @@ bool IsValidReasonPhrase(const String& status_text) {

} // namespace

// static
bool Response::IsNullBodyStatus(uint16_t status) {
if (status == 101 || status == 204 || status == 205 || status == 304) {
return true;
}

return false;
}

Response* Response::Create(ScriptState* script_state,
ExceptionState& exception_state) {
return Create(script_state, nullptr, String(), ResponseInit::Create(),
Expand Down
4 changes: 0 additions & 4 deletions third_party/blink/renderer/core/fetch/response.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ class CORE_EXPORT Response final : public ScriptWrappable,
DEFINE_WRAPPERTYPEINFO();

public:
// Checks whether `status` is a null body status.
// Spec: https://fetch.spec.whatwg.org/#null-body-status
static bool IsNullBodyStatus(uint16_t status);

// These "create" function which takes a ScriptState* must be called with
// entering an appropriate V8 context.
// From Response.idl:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
"POST",
"OPTIONS",
"PUT",
"CONNECT",
"Accept",
"Accept-CH",
"Accept-Language",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
This is a testharness.js-based test.
FAIL Response.body is null for responses with status=204 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=204 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=204 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with method=HEAD assert_equals: the body should be null expected null but got object "[object ReadableStream]"
Harness: the test ran to completion.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
This is a testharness.js-based test.
FAIL Response.body is null for responses with status=204 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=204 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=204 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with method=HEAD assert_equals: the body should be null expected null but got object "[object ReadableStream]"
Harness: the test ran to completion.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
This is a testharness.js-based test.
FAIL Response.body is null for responses with status=204 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=204 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=204 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with method=HEAD assert_equals: the body should be null expected null but got object "[object ReadableStream]"
Harness: the test ran to completion.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
This is a testharness.js-based test.
FAIL Response.body is null for responses with status=204 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=204 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=204 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=205 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=GET) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=POST) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with status=304 (method=OPTIONS) assert_equals: the body should be null expected null but got object "[object ReadableStream]"
FAIL Response.body is null for responses with method=HEAD assert_equals: the body should be null expected null but got object "[object ReadableStream]"
Harness: the test ran to completion.

Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ url: http://127.0.0.1:8000/inspector-protocol/resources/data-xfer-resource.php?r
headersSize: 0
receivedDataSize: 0

url: http://127.0.0.1:8000/inspector-protocol/resources/data-xfer-resource.php?cached=1
isChunked: false
isH2: false
redirected: false
headersSize: 101
receivedDataSize: 0
reportedTotalSize: 101

url: http://127.0.0.1:8000/inspector-protocol/resources/data-xfer-resource.php?
isChunked: false
isH2: false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@
testRunner.log('Network agent enabled');
sendRequest('/inspector-protocol/resources/data-xfer-resource.php?' +
'redirect=1');
sendRequest('/inspector-protocol/resources/data-xfer-resource.php?' +
'cached=1');
sendRequest('/inspector-protocol/resources/data-xfer-resource.php?' +
'size=4&' +
'flush_header_with_x_bytes=1&' +
Expand Down

0 comments on commit 135354b

Please sign in to comment.