DCHECK in FetchManager::Loader::DidReceiveResponse |
|||||
Issue description
I can hit the DCHECK here:
DCHECK(!(NetworkUtils::IsRedirectResponseCode(response_http_status_code_) &&
response_data->HeaderList()->Has(HTTPNames::Location) &&
request_->Redirect() != network::mojom::FetchRedirectMode::kManual));
The test case is something like this in wpt/service-workers/service-worker/ directory, where the service worker is fetch-rewrite-worker.js.
let url = 'test.txt';
// Add '?url' and tell the service worker to request a URL that returns
// a redirect response to a cross-origin URL.
target = host_info.HTTPS_REMOTE_ORIGIN + 'test.txt';
url += '?url=' + 'redirect.py?Redirect=' + encodeURIComponent(target);
return controlledFrame.contentWindow.fetch(url)
.then(result => {
assert_equals(result, 'error event');
});
I'll try to make a reduced test case later.
,
Feb 9 2018
Actually this repros without the service worker. Just have an HTML page that does:
fetch('resources/redirect-empty-location.py');
and you crash.
This looks like a bug in Fetch or Loader.
Seems the problem is we never get the content::ThrottlingURLLoader::OnReceiveRedirect message. Someone doesn't consider this response a redirect, but then FetchManager.cpp is surprised that it got a redirect response while in kFollow mode.
,
Feb 9 2018
-sw, +fetch
,
Feb 9 2018
,
Feb 9 2018
yhirano recommended looking into checking for empty string in the DCHECK. I'll take this I guess. This DCHECK looks added by https://codereview.chromium.org/2785123002, +horo/
,
Feb 9 2018
On closer examination, it looks like //net considers a Location with empty value to be the same as not having a Location header, so it considers the response not a redirect:
In net::HttpResponseHeaders::IsRedirect:
// If we lack a Location header, then we can't treat this as a redirect.
// We assume that the first non-empty location value is the target URL that
// we want to follow. TODO(darin): Is this consistent with other browsers?
size_t i = std::string::npos;
do {
i = FindHeader(++i, "location");
if (i == std::string::npos)
return false;
// If the location value is empty, then it doesn't count.
} while (parsed_[i].value_begin == parsed_[i].value_end);
https://cs.chromium.org/chromium/src/net/http/http_response_headers.cc?l=885&rcl=827e5bcf4903a0129a93953a76ec5117ea22eb70
Meanwhile, Blink seems to assume that a Location header indicates the response is a redirect, regardless of empty.
In blink::FetchManager::Loader::DidReceiveResponse:
DCHECK(!(NetworkUtils::IsRedirectResponseCode(response_http_status_code_) &&
response_data->HeaderList()->Has(HTTPNames::Location) &&
request_->Redirect() != network::mojom::FetchRedirectMode::kManual));
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/FetchManager.cpp?l=453&rcl=827e5bcf4903a0129a93953a76ec5117ea22eb70
,
Feb 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da2cad242070f973962254885cbda03a5145cd6a commit da2cad242070f973962254885cbda03a5145cd6a Author: Matt Falkenhagen <falken@chromium.org> Date: Wed Feb 14 04:38:54 2018 Fetch API: Don't DCHECK on a 302 response with a Location header with empty value. Blink was failing a DCHECK when it expected a redirect to be taken because the request mode was 'follow' and the response had a redirect status code and Location header. However, Chrome's //net considers a Location header with an empty value to be the same as no Location header at all. See the comments on bug 810288 about net::HttpResponseHeaders::IsRedirect(). WPT tests are added. The spec is unclear (https://github.com/whatwg/url/issues/373) but seems to either say to fail with network error or enter a loop of self-redirects that will eventually hit the redirect limit and fail with network error. Chrome currently fails this test because it treats the Location as as non-existent, i.e., not a redirect response. Bug: 810288 ,707185 Change-Id: Iaca93976aaa697380b1542ab45d63c318f5050b0 Reviewed-on: https://chromium-review.googlesource.com/910753 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#536644} [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location-expected.txt [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location-worker-expected.txt [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location-worker.html [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location.html [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-empty-location.js [add] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/LayoutTests/external/wpt/fetch/api/resources/redirect-empty-location.py [modify] https://crrev.com/da2cad242070f973962254885cbda03a5145cd6a/third_party/WebKit/Source/core/fetch/FetchManager.cpp
,
Feb 14 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by falken@chromium.org
, Feb 9 2018This gets hit when the server returned a Location of empty string. Repro by putting these in wpt/service-workers/service-worker: service-workers/service-worker/redirect-crash.https.html service-workers/service-worker/resources/redirect-empty-location.py service-workers/service-worker/resources/redirect-crash-worker.js The key part is the service worker and the server: service worker: self.onfetch = (e => { if (e.request.mode == 'navigation') return; e.respondWith(fetch(new Request('redirect-empty-location.py'))); }); server: def main(request, response): headers = [("Location", "")] return 302, headers, ""