Use net's response header parser for parsing multipart headers.

Use net's response header parser for parsing multipart headers.

BUG=493379
Committed: https://crrev.com/8e7ed88c4187c496961b87fdcd1a2c307c39fe1b
Cr-Commit-Position: refs/heads/master@{#333909}

[haavardm, 2015-06-04 11:00:54]

haavardm@opera.com changed reviewers:
+ jochen@chromium.org, mmenke@chromium.org

[haavardm, 2015-06-04 11:00:55]

jochen: please review
Mmemke: FYI

[jochen (gone - plz use gerrit), 2015-06-05 12:10:55]

Matt, is this ok?

happy to rubberstamp when you lgtm

[mmenke, 2015-06-05 12:15:59]

On 2015/06/05 12:10:55, jochen wrote:
> Matt, is this ok?
> 
> happy to rubberstamp when you lgtm

Great!  I'll review later today.

[mmenke, 2015-06-05 16:01:42]

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
File content/child/multipart_response_delegate.cc (right):

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:217: i++;
This isn't correct (If we have a bunch of LFs or CRLFs at the start, we'll skip
over them all).

My suggestion:  Add a method to net::HttpUtil to handle this case. 
LocateEndOfAdditionalHeaders?  Internally, it and LocateEndOfHeaders use the
same function from an anonymous namespace - difference is an additional argument
which sets was_lf to true (And perhaps rename it?  can_be_last_line?).  We'll
need an additional unit test or two for the method, but they should be trivial.

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:234:
scoped_refptr<net::HttpResponseHeaders> response_headers =
nit:  include base/ref_counted.h

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:236:
net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.size()));
Again, suggest a method for HttpUtil that does this, too.  Note that this method
may also be useful if we add support for trailers.

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:251: }
I don't feel like I can adequately review the implications of switching from
setting to an empty string to not setting these when we don't have the
corresponding headers.

[haavardm, 2015-06-09 15:12:35]

mmemke : PTAL

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
File content/child/multipart_response_delegate.cc (right):

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:217: i++;
On 2015/06/05 16:01:42, mmenke wrote:
> This isn't correct (If we have a bunch of LFs or CRLFs at the start, we'll
skip
> over them all).
> 

agh, of course..

> My suggestion:  Add a method to net::HttpUtil to handle this case. 
> LocateEndOfAdditionalHeaders?  Internally, it and LocateEndOfHeaders use the
> same function from an anonymous namespace - difference is an additional
argument
> which sets was_lf to true (And perhaps rename it?  can_be_last_line?).  We'll
> need an additional unit test or two for the method, but they should be
trivial.

Done.

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:234:
scoped_refptr<net::HttpResponseHeaders> response_headers =
On 2015/06/05 16:01:42, mmenke wrote:
> nit:  include base/ref_counted.h

Done.

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:236:
net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.size()));
On 2015/06/05 16:01:42, mmenke wrote:
> Again, suggest a method for HttpUtil that does this, too.  Note that this
method
> may also be useful if we add support for trailers.

I'm a bit unsure about what you suggest to add a method for. The header copying
below is very specific for multiparts, where only some headers are replaced and
some are not copied.  I believe that would be different for header trailers,
since those headers would be additional headers only.

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:251: }
On 2015/06/05 16:01:42, mmenke wrote:
> I don't feel like I can adequately review the implications of switching from
> setting to an empty string to not setting these when we don't have the
> corresponding headers.

I think it should handle not setting the mime or text encoding if they are not
present, since there are other such methods that are not called. However, I've
changed it to setting empty strings to preserve the current behavior.

[mmenke, 2015-06-09 20:23:37]

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
File content/child/multipart_response_delegate.cc (right):

https://codereview.chromium.org/1166953002/diff/20001/content/child/multipart...
content/child/multipart_response_delegate.cc:236:
net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.size()));
On 2015/06/09 15:12:34, haavardm wrote:
> On 2015/06/05 16:01:42, mmenke wrote:
> > Again, suggest a method for HttpUtil that does this, too.  Note that this
> method
> > may also be useful if we add support for trailers.
> 
> I'm a bit unsure about what you suggest to add a method for. The header
copying
> below is very specific for multiparts, where only some headers are replaced
and
> some are not copied.  I believe that would be different for header trailers,
> since those headers would be additional headers only.

I was thinking of making a method that does this:

scoped_refptr<net::HttpResponseHeaders> Blarg(char* data, int len) {
  std::string headers("HTTP/1.1 200 OK\r\n");
  headers.append(data, 0, len);
  return scoped_refptr(new net::HttpResponseHeaders(
      net::HttpUtil::AssembleRawHeaders(data, len));
}

Anyhow, doesn't save us a whole lot, and if you prefer things as-is, that's
fine.

https://codereview.chromium.org/1166953002/diff/40001/content/child/multipart...
File content/child/multipart_response_delegate.cc (right):

https://codereview.chromium.org/1166953002/diff/40001/content/child/multipart...
content/child/multipart_response_delegate.cc:254: }
This doesn't work, for two reasons:  Coalescing headers, and duplicate headers. 
Note that the old code was actually broken in the latter case.  For consistency,
should do what WebURLLoaderImpl::PopulateURLResponse does:

Switch the if to a while, replace the NULL with a void*, and use
addHTTPHeaderField on all but the first instance of each header name.

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util.h#ne...
net/http/http_util.h:150: // as end-of-headers (just like Mozilla).
Maybe add:  "The first line of |buf| is considered the status line, even if
empty."  Just to try and make it clearer how it's different from
LocateEndOfAdditionalHeaders.

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util.h#ne...
net/http/http_util.h:155: // an empty header list if |buf| starts with a single
[CR]LF.
Suggest focusing on what it's used for, rather than what it does.  Maybe
something like:

"Same as |LocateEndOfHeaders|, but does not expect a status line, so can be used
on multi-part responses or HTTP/1.x trailers.  As a result, if |buf| starts with
a single [CR]LF,  it is considered an empty header list, as opposed to an empty
status line above a header list."

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util_unit...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util_unit...
net/http/http_util_unittest.cc:270: {"\r", -1},
Suggest tossing in \r\n\r\n or \n\n.

[haavardm, 2015-06-10 11:54:02]

Matt, PTAL

https://codereview.chromium.org/1166953002/diff/40001/content/child/multipart...
File content/child/multipart_response_delegate.cc (right):

https://codereview.chromium.org/1166953002/diff/40001/content/child/multipart...
content/child/multipart_response_delegate.cc:254: }
On 2015/06/09 20:23:37, mmenke wrote:
> This doesn't work, for two reasons:  Coalescing headers, and duplicate
headers. 
> Note that the old code was actually broken in the latter case.  For
consistency,
> should do what WebURLLoaderImpl::PopulateURLResponse does:
> 
> Switch the if to a while, replace the NULL with a void*, and use
> addHTTPHeaderField on all but the first instance of each header name.

I don't think we need to have a special case for the first instance though,
since addHTTPHeaderField will create the header if it doesn't already exist.
Since the header copier is filtering out KReplacesHeaders, we don't need to make
sure the first instance overwrite the original header either.  I also switched
from to latin1 like is done in PopulateURLResponse, as that will copy the header
value as-is instead of assuming utf8.

I don't see that neither setHTTPHeaderField nor setHTTPHeaderField will properly
handle duplicate headers. That is, such headers will be combined into a single
header (which might be fine in many cases, but not all). This problem is also
stated in  WebKit/Source/platform/network/HTTPHeaderMap.h which is ultimately
used to store the headers for the response object.

Finally I am a bit confused why set-cookie and range from the multiparts are
handled, as the RFCs describing multiparts seems to say that all headers but
those starting with "content-" should be ignored.

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util.h#ne...
net/http/http_util.h:150: // as end-of-headers (just like Mozilla).
On 2015/06/09 20:23:37, mmenke wrote:
> Maybe add:  "The first line of |buf| is considered the status line, even if
> empty."  Just to try and make it clearer how it's different from
> LocateEndOfAdditionalHeaders.

Done.

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util.h#ne...
net/http/http_util.h:155: // an empty header list if |buf| starts with a single
[CR]LF.
On 2015/06/09 20:23:37, mmenke wrote:
> Suggest focusing on what it's used for, rather than what it does.  Maybe
> something like:
> 
> "Same as |LocateEndOfHeaders|, but does not expect a status line, so can be
used
> on multi-part responses or HTTP/1.x trailers.  As a result, if |buf| starts
with
> a single [CR]LF,  it is considered an empty header list, as opposed to an
empty
> status line above a header list."

Agree. I didn't take the http status into the equation here, but it makes sense
to do so.

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util_unit...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1166953002/diff/40001/net/http/http_util_unit...
net/http/http_util_unittest.cc:270: {"\r", -1},
On 2015/06/09 20:23:37, mmenke wrote:
> Suggest tossing in \r\n\r\n or \n\n.

Done.

[mmenke, 2015-06-10 17:55:55]

LGTM

https://codereview.chromium.org/1166953002/diff/40001/content/child/multipart...
File content/child/multipart_response_delegate.cc (right):

https://codereview.chromium.org/1166953002/diff/40001/content/child/multipart...
content/child/multipart_response_delegate.cc:254: }
On 2015/06/10 11:54:01, haavardm wrote:
> On 2015/06/09 20:23:37, mmenke wrote:
> > This doesn't work, for two reasons:  Coalescing headers, and duplicate
> headers. 
> > Note that the old code was actually broken in the latter case.  For
> consistency,
> > should do what WebURLLoaderImpl::PopulateURLResponse does:
> > 
> > Switch the if to a while, replace the NULL with a void*, and use
> > addHTTPHeaderField on all but the first instance of each header name.
> 
> I don't think we need to have a special case for the first instance though,
> since addHTTPHeaderField will create the header if it doesn't already exist.
> Since the header copier is filtering out KReplacesHeaders, we don't need to
make
> sure the first instance overwrite the original header either.  I also switched
> from to latin1 like is done in PopulateURLResponse, as that will copy the
header
> value as-is instead of assuming utf8.
> 
> I don't see that neither setHTTPHeaderField nor setHTTPHeaderField will
properly
> handle duplicate headers. That is, such headers will be combined into a single
> header (which might be fine in many cases, but not all). This problem is also
> stated in  WebKit/Source/platform/network/HTTPHeaderMap.h which is ultimately
> used to store the headers for the response object.
> 
> Finally I am a bit confused why set-cookie and range from the multiparts are
> handled, as the RFCs describing multiparts seems to say that all headers but
> those starting with "content-" should be ignored.

I have no idea, either.

[haavardm, 2015-06-11 07:18:15]

The CQ bit was checked by haavardm@opera.com

[haavardm, 2015-06-11 07:18:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1166953002/#ps60001
(title: "Address more comments")

[commit-bot: I haz the power, 2015-06-11 07:18:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1166953002/60001

[commit-bot: I haz the power, 2015-06-11 08:20:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-11 08:21:07]

Patchset 4 (id:??) landed as
https://crrev.com/8e7ed88c4187c496961b87fdcd1a2c307c39fe1b
Cr-Commit-Position: refs/heads/master@{#333909}