Bug 95181 – [soup] List value response headers are not handled in RespourceResponse

• Component: Platform
• Status: RESOLVED FIXED

[Alexander Shalamov]

When http response contains list-value headers, RespourceResponse handles them as single-value. (ResourceResponseSoup.cpp WebCore::RespourceResponse::updateFromSoupMessage)

[Alexander Shalamov]

Created attachment 160939 [details]
Patch

[Dan Winship]

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]

(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]

(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]

Created attachment 161174 [details]
test

[Alexander Shalamov]

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]

Created attachment 177228 [details]
Patch 3

[Martin Robinson]

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]

Created attachment 177727 [details]
Patch 4

- simplified if else block
- added comment to m_httpHeaderFields.clear()

[Dan Winship]

(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 on attachment 177727 [details]
Patch 4

Clearing flags on attachment: 177727

Committed r136705: <http://trac.webkit.org/changeset/136705>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.