WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
95181
[soup] List value response headers are not handled in RespourceResponse
https://bugs.webkit.org/show_bug.cgi?id=95181
Summary
[soup] List value response headers are not handled in RespourceResponse
Alexander Shalamov
Reported
2012-08-28 02:26:00 PDT
When http response contains list-value headers, RespourceResponse handles them as single-value. (ResourceResponseSoup.cpp WebCore::RespourceResponse::updateFromSoupMessage)
Attachments
Patch
(5.63 KB, patch)
2012-08-28 02:35 PDT
,
Alexander Shalamov
no flags
Details
Formatted Diff
Diff
test
(2.97 KB, application/octet-stream)
2012-08-29 03:45 PDT
,
Alexander Shalamov
no flags
Details
Patch 2
(6.88 KB, patch)
2012-12-03 04:02 PST
,
Alexander Shalamov
no flags
Details
Formatted Diff
Diff
Patch 3
(6.77 KB, patch)
2012-12-03 04:04 PST
,
Alexander Shalamov
no flags
Details
Formatted Diff
Diff
Patch 4
(6.76 KB, patch)
2012-12-05 04:38 PST
,
Alexander Shalamov
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Alexander Shalamov
Comment 1
2012-08-28 02:35:33 PDT
Created
attachment 160939
[details]
Patch
Dan Winship
Comment 2
2012-08-28 06:01:09 PDT
Comment on
attachment 160939
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=160939&action=review
> Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:74 > + while (soup_message_headers_iter_next(&headersIter, &headerName, &headerValue)) { > + headerValue = soup_message_headers_get_list(soupMessage->response_headers, headerName); > m_httpHeaderFields.set(String::fromUTF8(headerName), > String::fromUTF8WithLatin1Fallback(headerValue, strlen(headerValue)));
so, this is mildly inefficient, in that for headers that appear twice, it will end up setting them twice. eg, in the newly-added test, it will end up doing m_httpHeaderFields.set("Content-Type", "text/plain"); m_httpHeaderFields.set("X-Custom-Header-Single", "single"); m_httpHeaderFields.set("X-Custom-Header-Empty", ""); m_httpHeaderFields.set("X-Custom-Header-List", "one, two"); m_httpHeaderFields.set("X-Custom-Header-List", "one, two"); /* redundant */ however, it still gets the right answer (since the redundant set() just overwrites the first one), and making it *not* do this would probably be more hassle than it's worth. What happens if there are multiple Content-Type or Content-Length headers? In that case, you're supposed to ignore the extras, and I wonder how other backends would end up behaving in this case...
Alexander Shalamov
Comment 3
2012-08-28 06:28:08 PDT
(In reply to
comment #2
)
> (From update of
attachment 160939
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=160939&action=review
> > > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:74 > > + while (soup_message_headers_iter_next(&headersIter, &headerName, &headerValue)) { > > + headerValue = soup_message_headers_get_list(soupMessage->response_headers, headerName); > > m_httpHeaderFields.set(String::fromUTF8(headerName), > > String::fromUTF8WithLatin1Fallback(headerValue, strlen(headerValue))); > > so, this is mildly inefficient, in that for headers that appear twice, it will end up setting them twice. eg, in the newly-added test, it will end up doing > > m_httpHeaderFields.set("Content-Type", "text/plain"); > m_httpHeaderFields.set("X-Custom-Header-Single", "single"); > m_httpHeaderFields.set("X-Custom-Header-Empty", ""); > m_httpHeaderFields.set("X-Custom-Header-List", "one, two"); > m_httpHeaderFields.set("X-Custom-Header-List", "one, two"); /* redundant */ > > however, it still gets the right answer (since the redundant set() just overwrites the first one), and making it *not* do this would probably be more hassle than it's worth.
I could check if map already contains key + skip first iteration.
> What happens if there are multiple Content-Type or Content-Length headers? In that case, you're supposed to ignore the extras, and I wonder how other backends would end up behaving in this case...
I will check how chromium / qt backends handle that. Thanks for the hint.
Alexander Shalamov
Comment 4
2012-08-29 03:30:08 PDT
(In reply to
comment #2
)
> What happens if there are multiple Content-Type or Content-Length headers? In that case, you're supposed to ignore the extras, and I wonder how other backends would end up behaving in this case...
Dan, I tried to send duplicate Content-Type, Content-Length and custom headers, here are the results: Headers sent from python server: send_header("Content-type", "text/plain") send_header("Content-type", "text/html") send_header("Content-Length", 889) send_header("Content-length", 889) send_header("x-custom-header", 1) send_header("x-custom-header", 2) Mozilla: CONTENT-TYPE: text/plain CONTENT-LENGTH: 889 X-CUSTOM-HEADER: 1, 2 Chromium: CONTENT-TYPE: plain/html, text/html CONTENT-LENGTH: 889, 889 X-CUSTOM-HEADER: 1, 2 Opera: CONTENT-TYPE: plain/html, text/html CONTENT-LENGTH: 889, 889 X-CUSTOM-HEADER: 1, 2 GTK / EFL WebKit (with my patch): CONTENT-TYPE: plain/html, text/html CONTENT-LENGTH: 889, 889 X-CUSTOM-HEADER: 1, 2 If I send multiple Content-Length header with different values, Chromium and Mozilla complain about duplicated headers / corrupted content and refuse to load page. send_header("Content-length", 889) send_header("Content-length", 123) send_header("Content-length", 889) Opera loads page and concatenates values, it uses first Content-Length and Content-Type header values: CONTENT-TYPE: plain/html, text/html CONTENT-LENGTH: 889, 123, 889 X-CUSTOM-HEADER: 1, 2 EFL/GTK WebKit (with my patch) - uses last header values for Content-Type and Content-Length. const char* officialType = soup_message_headers_get_one(soupMessage->response_headers, "Content-Type"); setExpectedContentLength(soup_message_headers_get_content_length(soupMessage->response_headers));
Alexander Shalamov
Comment 5
2012-08-29 03:45:16 PDT
Created
attachment 161174
[details]
test
Alexander Shalamov
Comment 6
2012-12-03 04:02:09 PST
Created
attachment 177227
[details]
Patch 2 - clear old headers before updating - concatenate list-value header values whenever key is already in the map
Alexander Shalamov
Comment 7
2012-12-03 04:04:47 PST
Created
attachment 177228
[details]
Patch 3
Martin Robinson
Comment 8
2012-12-03 10:55:50 PST
Comment on
attachment 177228
[details]
Patch 3 View in context:
https://bugs.webkit.org/attachment.cgi?id=177228&action=review
This looks good to me, but I'd also like a quick review by either Dan Winship or Sergio Villar Senin before I give an r+.
> Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:70 > + m_httpHeaderFields.clear();
Why is it important to clear the headers here? I don't doubt that it is, but maybe a comment would help.
> Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:74 > + String headerNameStr = String::fromUTF8WithLatin1Fallback(headerName, strlen(headerName));
We don't typically abbreviate variable names (other than the most common -- for instance it and i for iterators) in WebKit, so maybe headerNameString instead of Str.
> Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:87 > + else { > + if (!it->value.isEmpty()) { > + StringBuilder builder; > + builder.append(it->value); > + builder.appendLiteral(", "); > + builder.append(String::fromUTF8WithLatin1Fallback(headerValue, strlen(headerValue))); > + m_httpHeaderFields.set(headerNameStr, builder.toString()); > + } else > + m_httpHeaderFields.set(headerNameStr, String::fromUTF8WithLatin1Fallback(headerValue, strlen(headerValue))); > + }
You could flatten this out actually: else if (!it->value.isEmpty()) { ... } else { ... } That might make it slightly easier to read because of the fewer levels of nesting.
Alexander Shalamov
Comment 9
2012-12-05 04:38:40 PST
Created
attachment 177727
[details]
Patch 4 - simplified if else block - added comment to m_httpHeaderFields.clear()
Dan Winship
Comment 10
2012-12-05 04:56:52 PST
(In reply to
comment #8
)
> This looks good to me, but I'd also like a quick review by either Dan Winship or Sergio Villar Senin before I give an r+.
yeah, it works... though I'm thinking the "redundant" version from the original patch is probably cleaner
WebKit Review Bot
Comment 11
2012-12-05 09:45:36 PST
Comment on
attachment 177727
[details]
Patch 4 Clearing flags on attachment: 177727 Committed
r136705
: <
http://trac.webkit.org/changeset/136705
>
WebKit Review Bot
Comment 12
2012-12-05 09:45:40 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug