win: Add implementation of HTTPTransport based on WinHTTP

win: Add implementation of HTTPTransport based on WinHTTP

(There's also https://codereview.chromium.org/854363006/ based on
WinInet, I'm still a little uncertain which is preferable here.)


R=cpu@chromium.org, mark@chromium.org, rsesek@chromium.org, ananta@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/7c9bd944ae1318f312e19401fb59f7364b6effb7

[scottmg, 2015-01-21 00:03:28]

Patchset #2 (id:20001) has been deleted

[scottmg, 2015-01-21 00:13:57]

scottmg@chromium.org changed reviewers:
+ rsesek@chromium.org
- mark@chromium.org

[scottmg, 2015-01-21 00:17:50]

cpu/ananta, could you take a look at the WinHTTP goop?

rsesek for overall HTTPTransport.

[scottmg, 2015-01-21 01:13:50]

Patchset #10 (id:190001) has been deleted

[cpu_(ooo_6.6-7.5), 2015-01-21 02:26:25]

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
File util/net/http_transport_win.cc (right):

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:33: const int kErrorMessageBufferSize = 256;
nit: remove the constant name since it is not used anywhere.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:73: HINTERNET session =
WinHttpOpen(L"Crashpad/1.0",
magic string is magic. do we have the same string in mac?

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:76: WINHTTP_NO_PROXY_BYPASS,
we'll be fiddling with the proxy setting eventually, for now its fine.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:85: url_components.dwHostNameLength = 1;
sample uses -1. I guess any value other than zero? Don't recall ever cracking
urls, sorry.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:100: 
not using GURL in this project I assume?

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:120: // Add headers to the request.
if the headers are trusted we are fine. Else we have issues.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:140: uint8_t buffer[4 << 10];
rather use the decimal (or hex constant) ? neither 4 or 10 are used in the
for().

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:185: 
closing the handles only happens here below, not in a lot of error related early
returns.

I would not be oppose to a 'goto cleanup' or whatever. Some of the early returns
like in 157 can happen naturally so I fear that not closing will lead to
threadpool accumulations or other resource leaks.

[Mark Mentovai, 2015-01-21 19:23:17]

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
File util/net/http_transport_test.cc (right):

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_test.cc:34: #include <stdlib.h>
This header is available on non-Windows, you don’t need to be OS-specific about
including it.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
File util/net/http_transport_win.cc (right):

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:73: HINTERNET session =
WinHttpOpen(L"Crashpad/1.0",
cpu wrote:
> magic string is magic. do we have the same string in mac?

We don’t currently set the user-agent on Mac.

Rather than hard-coding this here, you can #include "package.h" and use
PACKAGE_NAME and PACKAGE_VERSION.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:145: post_data.insert(post_data.end(), buffer,
buffer + bytes_to_write);
Is there no callback-based way to get winhttp to ask for data? This will load
the entire dump into memory, which at some point may be quite large. That’s why
this was designed to consume a bunch of byte buffers, so that data could be
provided to the transport incrementally without having to buffer it all.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:185: 
cpu wrote:
> closing the handles only happens here below, not in a lot of error related
early
> returns.
> 
> I would not be oppose to a 'goto cleanup' or whatever. Some of the early
returns
> like in 157 can happen naturally so I fear that not closing will lead to
> threadpool accumulations or other resource leaks.

Sounds like a task for a simple ScopedGeneric-based scoper.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:210: 
Blank line at EOF.

[scottmg, 2015-01-21 19:46:40]

Patchset #12 (id:250001) has been deleted

[scottmg, 2015-01-21 19:47:47]

Patchset #12 (id:270001) has been deleted

[scottmg, 2015-01-21 19:47:52]

Patchset #11 (id:230001) has been deleted

[scottmg, 2015-01-21 19:48:27]

Thanks!

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
File util/net/http_transport_test.cc (right):

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_test.cc:34: #include <stdlib.h>
On 2015/01/21 19:23:17, Mark Mentovai wrote:
> This header is available on non-Windows, you don’t need to be OS-specific
about
> including it.

Done.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
File util/net/http_transport_win.cc (right):

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:33: const int kErrorMessageBufferSize = 256;
On 2015/01/21 02:26:25, cpu wrote:
> nit: remove the constant name since it is not used anywhere.

Done.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:73: HINTERNET session =
WinHttpOpen(L"Crashpad/1.0",
On 2015/01/21 19:23:17, Mark Mentovai wrote:
> cpu wrote:
> > magic string is magic. do we have the same string in mac?
> 
> We don’t currently set the user-agent on Mac.
> 
> Rather than hard-coding this here, you can #include "package.h" and use
> PACKAGE_NAME and PACKAGE_VERSION.

Done.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:76: WINHTTP_NO_PROXY_BYPASS,
On 2015/01/21 02:26:24, cpu wrote:
> we'll be fiddling with the proxy setting eventually, for now its fine.

Acknowledged.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:85: url_components.dwHostNameLength = 1;
On 2015/01/21 02:26:24, cpu wrote:
> sample uses -1. I guess any value other than zero? Don't recall ever cracking
> urls, sorry.

Yeah, it's a bit crazy, explained here in "Remarks":
https://msdn.microsoft.com/en-us/library/windows/desktop/aa385420(v=vs.85).aspx 

I tried switching to -1, but then I get

d:\src\crashpad\crashpad\util\net\http_transport_win.cc(89) : warning C4245: '='
: conversion from 'int' to 'DWORD', signed/unsigned mismatch

so seems a bit uglier to cast -1, so just left it as 1.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:100: 
On 2015/01/21 02:26:25, cpu wrote:
> not using GURL in this project I assume?

Not at the moment. Maybe we should?

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:120: // Add headers to the request.
On 2015/01/21 02:26:24, cpu wrote:
> if the headers are trusted we are fine. Else we have issues.

Good point, noted. At the moment it's only used in test code, but we'll have to
be careful in the future.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:140: uint8_t buffer[4 << 10];
On 2015/01/21 02:26:24, cpu wrote:
> rather use the decimal (or hex constant) ? neither 4 or 10 are used in the
> for().

Done.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:145: post_data.insert(post_data.end(), buffer,
buffer + bytes_to_write);
On 2015/01/21 19:23:17, Mark Mentovai wrote:
> Is there no callback-based way to get winhttp to ask for data? This will load
> the entire dump into memory, which at some point may be quite large. That’s
why
> this was designed to consume a bunch of byte buffers, so that data could be
> provided to the transport incrementally without having to buffer it all.

It doesn't need the data up front (it can be written later as a stream with
WinHttpWriteData) but it does need the size for the Content-Length. I didn't see
a way to tell it to use Chunked, but perhaps that's also possible.

I planned to make a subsequent change to the to HttpBody interface to allow
determining the stream length ahead of time to avoid the extra copying.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:185: 
On 2015/01/21 19:23:17, Mark Mentovai wrote:
> cpu wrote:
> > closing the handles only happens here below, not in a lot of error related
> early
> > returns.
> > 
> > I would not be oppose to a 'goto cleanup' or whatever. Some of the early
> returns
> > like in 157 can happen naturally so I fear that not closing will lead to
> > threadpool accumulations or other resource leaks.
> 
> Sounds like a task for a simple ScopedGeneric-based scoper.

Done.

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:210: 
On 2015/01/21 19:23:17, Mark Mentovai wrote:
> Blank line at EOF.

Done.

[scottmg, 2015-01-21 20:44:00]

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
File util/net/http_transport_win.cc (right):

https://codereview.chromium.org/852213004/diff/210001/util/net/http_transport...
util/net/http_transport_win.cc:145: post_data.insert(post_data.end(), buffer,
buffer + bytes_to_write);
On 2015/01/21 19:48:27, scottmg wrote:
> On 2015/01/21 19:23:17, Mark Mentovai wrote:
> > Is there no callback-based way to get winhttp to ask for data? This will
load
> > the entire dump into memory, which at some point may be quite large. That’s
> why
> > this was designed to consume a bunch of byte buffers, so that data could be
> > provided to the transport incrementally without having to buffer it all.
> 
> It doesn't need the data up front (it can be written later as a stream with
> WinHttpWriteData) but it does need the size for the Content-Length. I didn't
see
> a way to tell it to use Chunked, but perhaps that's also possible.
> 
> I planned to make a subsequent change to the to HttpBody interface to allow
> determining the stream length ahead of time to avoid the extra copying.

Hm, actually I have some reading comprehension problems. It's documented in
WinHttpSendRequest, but the built in support seems to be Vista+, so we may still
need to calculate Content-Length up front.

[scottmg, 2015-01-22 00:12:07]

(

I'm also confused by the errors when running the test that connects to
util\net\http_transport_test_server.py.

WinHTTP complains that:

[11892:11528:20150121,155755.015:ERROR
d:\src\crashpad\crashpad\util\net\http_transport_win.cc:46]
WinHttpReceiveResponse: The server returned an invalid or unrecognized response 
(0x2F78)

When I instead POST the same request to httpbin.org/post, WinHTTP seems happy
with the response from that server, so I think it's a problem with the test
server (only on Windows maybe?), rather than the client code.

I put a Wireshark dump of connecting to test server here:
https://dl.dropboxusercontent.com/u/116514/http_transport.pcap

And for comparison of curl connecting to httpbin:
https://dl.dropboxusercontent.com/u/116514/curl-to-httpbin.pcapng

Wireshark doesn't seem to parse the python server's response as a valid 200 OK,
whereas it does for the curl response, so I guess that's a hint that something's
going wrong, but I don't see what.

)

[cpu_(ooo_6.6-7.5), 2015-01-22 00:13:06]

lgtm

I think we are going to end up using GURL but that is something we can add in
another CL, it would remove the crack url insanity.

[scottmg, 2015-01-22 00:20:00]

On 2015/01/22 00:12:07, scottmg wrote:
> (
> 
> I'm also confused by the errors when running the test that connects to
> util\net\http_transport_test_server.py.
> 
> WinHTTP complains that:
> 
> [11892:11528:20150121,155755.015:ERROR
> d:\src\crashpad\crashpad\util\net\http_transport_win.cc:46]
> WinHttpReceiveResponse: The server returned an invalid or unrecognized
response 
> (0x2F78)
> 
> When I instead POST the same request to httpbin.org/post, WinHTTP seems happy
> with the response from that server, so I think it's a problem with the test
> server (only on Windows maybe?), rather than the client code.
> 
> I put a Wireshark dump of connecting to test server here:
> https://dl.dropboxusercontent.com/u/116514/http_transport.pcap
> 
> And for comparison of curl connecting to httpbin:
> https://dl.dropboxusercontent.com/u/116514/curl-to-httpbin.pcapng
> 
> Wireshark doesn't seem to parse the python server's response as a valid 200
OK,
> whereas it does for the curl response, so I guess that's a hint that
something's
> going wrong, but I don't see what.
> 
> )

Continuing my monologue, curl seems relatively happy to post to the test server

C:\Users\Scott>curl --data "param1=value1&param2=value2" --dump-header headers h
ttp://127.0.0.1:15703/upload --verbose
* About to connect() to 127.0.0.1 port 15703 (#0)
*   Trying 127.0.0.1...
* connected
* Connected to 127.0.0.1 (127.0.0.1) port 15703 (#0)
> POST /upload HTTP/1.1
> User-Agent: curl/7.27.0
> Host: 127.0.0.1:15703
> Accept: */*
> Content-Length: 27
> Content-Type: application/x-www-form-urlencoded
>
* upload completely sent off: 27 out of 27 bytes
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Server: BaseHTTP/0.3 Python/2.7.6
< Date: Thu, 22 Jan 2015 00:17:39 GMT
* Closing connection #0

but Wireshark again doesn't like the response too much.
https://dl.dropboxusercontent.com/u/116514/curl-to-test-server.pcap

Um, hmm.

[scottmg, 2015-01-22 01:01:23]

On 2015/01/22 00:20:00, scottmg wrote:
> On 2015/01/22 00:12:07, scottmg wrote:
> > (
> > 
> > I'm also confused by the errors when running the test that connects to
> > util\net\http_transport_test_server.py.
> > 
> > WinHTTP complains that:
> > 
> > [11892:11528:20150121,155755.015:ERROR
> > d:\src\crashpad\crashpad\util\net\http_transport_win.cc:46]
> > WinHttpReceiveResponse: The server returned an invalid or unrecognized
> response 
> > (0x2F78)
> > 
> > When I instead POST the same request to httpbin.org/post, WinHTTP seems
happy
> > with the response from that server, so I think it's a problem with the test
> > server (only on Windows maybe?), rather than the client code.
> > 
> > I put a Wireshark dump of connecting to test server here:
> > https://dl.dropboxusercontent.com/u/116514/http_transport.pcap
> > 
> > And for comparison of curl connecting to httpbin:
> > https://dl.dropboxusercontent.com/u/116514/curl-to-httpbin.pcapng
> > 
> > Wireshark doesn't seem to parse the python server's response as a valid 200
> OK,
> > whereas it does for the curl response, so I guess that's a hint that
> something's
> > going wrong, but I don't see what.
> > 
> > )
> 
> Continuing my monologue, curl seems relatively happy to post to the test
server
> 
> C:\Users\Scott>curl --data "param1=value1&param2=value2" --dump-header headers
h
> ttp://127.0.0.1:15703/upload --verbose
> * About to connect() to 127.0.0.1 port 15703 (#0)
> *   Trying 127.0.0.1...
> * connected
> * Connected to 127.0.0.1 (127.0.0.1) port 15703 (#0)
> > POST /upload HTTP/1.1
> > User-Agent: curl/7.27.0
> > Host: 127.0.0.1:15703
> > Accept: */*
> > Content-Length: 27
> > Content-Type: application/x-www-form-urlencoded
> >
> * upload completely sent off: 27 out of 27 bytes
> * HTTP 1.0, assume close after body
> < HTTP/1.0 200 OK
> < Server: BaseHTTP/0.3 Python/2.7.6
> < Date: Thu, 22 Jan 2015 00:17:39 GMT
> * Closing connection #0
> 
> but Wireshark again doesn't like the response too much.
> https://dl.dropboxusercontent.com/u/116514/curl-to-test-server.pcap
> 
> Um, hmm.

Calling end_headers() after send_response() appears to be enough to make it
happy:

https://codereview.chromium.org/852213004/diff2/290001:310001/util/net/http_t...


which is just "write('\r\n')". That seems to be common usage looking at a few
other examples. Doh!

[scottmg, 2015-01-22 19:42:32]

Patchset #14 (id:350001) has been deleted

[Robert Sesek, 2015-01-26 21:17:09]

LGTM. Sorry, I forgot this was to me after Mark replied.

https://codereview.chromium.org/852213004/diff/370001/util/net/http_transport...
File util/net/http_transport_win.cc (right):

https://codereview.chromium.org/852213004/diff/370001/util/net/http_transport...
util/net/http_transport_win.cc:151: // We need the Content-Length up front, so
buffer in memory. We should modify
This would be helpful on OS X too, and then we can avoid any chunked encoding.

[scottmg, 2015-01-26 21:22:35]

Thanks. Mark, do you want to take another look, or OK to land?

https://codereview.chromium.org/852213004/diff/370001/util/net/http_transport...
File util/net/http_transport_win.cc (right):

https://codereview.chromium.org/852213004/diff/370001/util/net/http_transport...
util/net/http_transport_win.cc:151: // We need the Content-Length up front, so
buffer in memory. We should modify
On 2015/01/26 21:17:09, Robert Sesek wrote:
> This would be helpful on OS X too, and then we can avoid any chunked encoding.

OK, I'll take a look at doing that as a follow up. I think it shouldn't be too
hard for the existing implementations of HTTPBodyStream. FileHTTPBodyStream is
the only non-obvious one, just have to decide how to handle the file contents
changing between length retrieval and data retrieval.

[Mark Mentovai, 2015-01-26 21:27:03]

mark@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2015-01-26 21:27:04]

I was happy with this after your changes. LGTM.

[scottmg, 2015-01-26 21:30:59]

Patchset #15 (id:390001) has been deleted

[scottmg, 2015-01-26 21:31:39]

Committed patchset #14 (id:370001) manually as
7c9bd944ae1318f312e19401fb59f7364b6effb7 (presubmit successful).