RESOLVED FIXED 205462
Fetch: handle empty Location value 
https://bugs.webkit.org/show_bug.cgi?id=205462
Summary Fetch: handle empty Location value
Rob Buis
Reported 2019-12-19 11:39:57 PST
Handle empty Location value.
Attachments
Patch (5.52 KB, patch)
2019-12-19 11:44 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (5.90 KB, patch)
2019-12-20 04:21 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Rob Buis
Comment 1 2019-12-19 11:44:58 PST
Created attachment 386126 [details] Patch
youenn fablet
Comment 2 2019-12-20 03:29:01 PST
Comment on attachment 386126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386126&action=review > Source/WebCore/loader/SubresourceLoader.cpp:405 > + if (options().redirect == FetchOptions::Redirect::Follow && response.isRedirection()) { response.isRedirection() is not needed here. > Source/WebCore/loader/SubresourceLoader.cpp:406 > + if (response.httpHeaderFields().contains(HTTPHeaderName::Location) && response.httpHeaderField(HTTPHeaderName::Location).isEmpty()) { Can we just have one check with something like !isNull() && isEmpty(). or impl() && impl()->isEmpty() maybe? > Source/WebCore/loader/SubresourceLoader.cpp:410 > + } Would be nice if we could do something like: if (options().redirect == FetchOptions::Redirect::Follow && isLocationURLFailure(response)) { ... return; }
Rob Buis
Comment 3 2019-12-20 04:21:46 PST
Created attachment 386200 [details] Patch
Rob Buis
Comment 4 2019-12-20 04:50:20 PST
Comment on attachment 386126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386126&action=review >> Source/WebCore/loader/SubresourceLoader.cpp:405 >> + if (options().redirect == FetchOptions::Redirect::Follow && response.isRedirection()) { > > response.isRedirection() is not needed here. Ouch! Should have seen that, fixed. >> Source/WebCore/loader/SubresourceLoader.cpp:406 >> + if (response.httpHeaderFields().contains(HTTPHeaderName::Location) && response.httpHeaderField(HTTPHeaderName::Location).isEmpty()) { > > Can we just have one check with something like !isNull() && isEmpty(). or impl() && impl()->isEmpty() maybe? Yes, the former, I think it just uses the latter, but best to stick with the public API (even though StringImpl is not likely to change). >> Source/WebCore/loader/SubresourceLoader.cpp:410 >> + } > > Would be nice if we could do something like: > if (options().redirect == FetchOptions::Redirect::Follow && isLocationURLFailure(response)) { > ... > return; > } Done.
WebKit Commit Bot
Comment 5 2019-12-20 05:35:26 PST
Comment on attachment 386200 [details] Patch Clearing flags on attachment: 386200 Committed r253814: <https://trac.webkit.org/changeset/253814>
WebKit Commit Bot
Comment 6 2019-12-20 05:35:28 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 7 2019-12-20 05:36:18 PST
<rdar://problem/58111194>
Note You need to log in before you can comment on or make changes to this bug.