Add HTTPTransport, a Mac implementation, and an end-to-end test.

Add HTTPTransport, a Mac implementation, and an end-to-end test.

BUG=https://crbug.com/415544
R=mark@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/d88711adfa1c930cb709b7b7f5d52c8534a72c00

[Mark Mentovai, 2014-10-30 21:13:55]

https://codereview.chromium.org/692963002/diff/1/util/net/http_multipart_buil...
File util/net/http_multipart_builder.cc (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_multipart_buil...
util/net/http_multipart_builder.cc:184: return std::make_pair(kContentType,
content_type);
#include <utility>

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.cc
File util/net/http_transport.cc (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.cc#n...
util/net/http_transport.cc:24: body_stream_() {
headers_() too?

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h
File util/net/http_transport.h (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:18: #include <stdint.h>
Unused.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:22: #include "base/compiler_specific.h"
You don’t need this, but you do need base/basictypes.h for
DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:42: //! \param[in] url The request URL.
I’ve been putting a blank //! line between sections like \brief and \param.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:43: void SetURL(const std::string& url);
I’d consider it OK to inline these nonvirtual setters if you’re interested in
doing that.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:45: //! \brief Sets the HTTP method to execute. E.g.,
GET, POST, etc.
Is there a default method if nobody calls this?

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:53: void SetHeader(const std::string& header, const
std::string& value);
Maybe AddHeader()?

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:65: virtual bool ExecuteSynchronously() = 0;
I think we will want a way to get the response header and body in addition to
just having a “bool” return value. It’s OK to save that for a future change,
though. In fact, it’s probably preferable to do it in a separate change.

Rationale: the crash server returns the ID that it stored the report under to
the uploading client in the message body.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac.h
File util/net/http_transport_mac.h (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.h:18: #include <string>
Unused.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac.mm
File util/net/http_transport_mac.mm (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:29: crashpad::HTTPBodyStream* bodyStream_;
// weak

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:49: return YES;
I guess you could return NO if streamStatus_ is NSStreamStatusAtEnd,
NSStreamStatusClosed, or NSStreamStatusError.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:59: streamStatus_ = NSStreamStatusReading;
The documentation for this status says that it’s supposed to be set while the
read is happening. I think that the status should be set to
NSStreamStatusReading before calling GetBytesBuffer(), and then set back to
NSStreamStatusOpen right here.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:69: - (void)scheduleInRunLoop:(NSRunLoop*)runLoop
Can you add a comment about why this is unnecessary? (I believe it’s because the
stream is only used synchronously.)

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:111: NSString* urlNSString =
base::SysUTF8ToNSString(url());
You’re not in the @implementation anymore here: url_ns_string. Also, below,
http_response.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:116: timeoutInterval:15.0];
It’d be reasonable for this to be tunable at the HTTPTransport level.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test.cc
File util/net/http_transport_test.cc (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:61: // The child will write the HTTP server port
number as a packed int to
I’d prefer fixed-size (uint16_t). Fixed-size for response_code_ too.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:63: int port = -1;
No initialization necessary. CheckedReadFD() is guaranteed to set port, or your
money back. No need to ASSERT on it, either.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:68: CheckedWriteFD(WritePipeFD(),
&response_code_, sizeof(response_code_));
This wasn’t clear to me when I started reading through it, because I’m thinking
“the server is supposed to return the response code, what the hell is the client
writing it for?” Now I see that you’re giving the server’s desired return code
to it here, but you should add a comment saying that’s the protocol you’ve come
up with.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:73:
transport->SetURL(base::StringPrintf("http://localhost:%d/upload", port));
Since you listen explicitly on 127.0.0.1 in the Python script, you should use
that address here too (reducing any flake caused by local resolver stupidity).

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:94: scoped_ptr<HTTPTransport> CreateTransport()
{
Instead of doing this here, should we have a static Create() factory function on
HTTPTransport to centralize platform-specific creation?

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:137: ASSERT_GE(content_type.length(),
strlen(kMultipartFormData));
#include <string.h>

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:251: }  // namespace crashpad
Nothing with a file part? Are you just trusting that HTTPMultipartBuilder did
its job?

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
File util/net/http_transport_test_server.py (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test_server.py:2: # Copyright 2014 The Crashpad Authors.
All rights reserved.
Gimme a blank line before this.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test_server.py:21: one HTTP request, writing it out
entirely to stdout, and will then terminate.
Do you want to say up here that the main motivation was to decode
Transfer-Encoding: Chunked?

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test_server.py:70: length =
int(self.headers['Content-Length'])
This field may not be present. If it’s not, you should read the whole self.rfile
to the end and stick that in body.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test_server.py:79:
<http://tools.ietf.org/html/rfc7230#section-4.1>. This returns the result
RFCs are pretty canonical, you don’t need a link, you can just name the RFC and
section like we do elsewhere.

[Robert Sesek, 2014-10-31 14:48:15]

https://codereview.chromium.org/692963002/diff/1/util/net/http_multipart_buil...
File util/net/http_multipart_builder.cc (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_multipart_buil...
util/net/http_multipart_builder.cc:184: return std::make_pair(kContentType,
content_type);
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> #include <utility>

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.cc
File util/net/http_transport.cc (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.cc#n...
util/net/http_transport.cc:24: body_stream_() {
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> headers_() too?

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h
File util/net/http_transport.h (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:18: #include <stdint.h>
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> Unused.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:22: #include "base/compiler_specific.h"
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> You don’t need this, but you do need base/basictypes.h for
> DISALLOW_COPY_AND_ASSIGN.

I get these confused, esp. because this has moved to macros.h in upstream.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:42: //! \param[in] url The request URL.
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> I’ve been putting a blank //! line between sections like \brief and \param.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:43: void SetURL(const std::string& url);
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> I’d consider it OK to inline these nonvirtual setters if you’re interested in
> doing that.

Acknowledged.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:45: //! \brief Sets the HTTP method to execute. E.g.,
GET, POST, etc.
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> Is there a default method if nobody calls this?

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:53: void SetHeader(const std::string& header, const
std::string& value);
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> Maybe AddHeader()?

I chose Set deliberately since this isn't additive (HTTP does allow repeating
the same header).

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport.h#ne...
util/net/http_transport.h:65: virtual bool ExecuteSynchronously() = 0;
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> I think we will want a way to get the response header and body in addition to
> just having a “bool” return value. It’s OK to save that for a future change,
> though. In fact, it’s probably preferable to do it in a separate change.
> 
> Rationale: the crash server returns the ID that it stored the report under to
> the uploading client in the message body.

Acknowledged.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac.h
File util/net/http_transport_mac.h (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.h:18: #include <string>
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> Unused.

Now this entire file is.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac.mm
File util/net/http_transport_mac.mm (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:29: crashpad::HTTPBodyStream* bodyStream_;
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> // weak

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:49: return YES;
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> I guess you could return NO if streamStatus_ is NSStreamStatusAtEnd,
> NSStreamStatusClosed, or NSStreamStatusError.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:59: streamStatus_ = NSStreamStatusReading;
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> The documentation for this status says that it’s supposed to be set while the
> read is happening. I think that the status should be set to
> NSStreamStatusReading before calling GetBytesBuffer(), and then set back to
> NSStreamStatusOpen right here.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:69: - (void)scheduleInRunLoop:(NSRunLoop*)runLoop
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> Can you add a comment about why this is unnecessary? (I believe it’s because
the
> stream is only used synchronously.)

I would if I knew why it were necessary ;). I don't think the synchronous aspect
has anything to do with it, since the upload is done on a background thread and
the calling thread just waits on a semaphore.

My guess is that this is used for streams that have some underlying type that
should be signaled and waited on using the runloop, which isn't the case for our
streams.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:111: NSString* urlNSString =
base::SysUTF8ToNSString(url());
On 2014/10/30 21:13:54, Mark Mentovai wrote:
> You’re not in the @implementation anymore here: url_ns_string. Also, below,
> http_response.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_mac....
util/net/http_transport_mac.mm:116: timeoutInterval:15.0];
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> It’d be reasonable for this to be tunable at the HTTPTransport level.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test.cc
File util/net/http_transport_test.cc (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:61: // The child will write the HTTP server port
number as a packed int to
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> I’d prefer fixed-size (uint16_t). Fixed-size for response_code_ too.

uint16_t is not enough bits. Dynamic ports go all the way to
IPPORT_HILASTAUTO=65535. Used a uint32_t.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:63: int port = -1;
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> No initialization necessary. CheckedReadFD() is guaranteed to set port, or
your
> money back. No need to ASSERT on it, either.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:68: CheckedWriteFD(WritePipeFD(),
&response_code_, sizeof(response_code_));
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> This wasn’t clear to me when I started reading through it, because I’m
thinking
> “the server is supposed to return the response code, what the hell is the
client
> writing it for?”&nbsp;Now I see that you’re giving the server’s desired return
code
> to it here, but you should add a comment saying that’s the protocol you’ve
come
> up with.

I thought the above comment covered this, but I adjusted it.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:73:
transport->SetURL(base::StringPrintf("http://localhost:%d/upload", port));
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> Since you listen explicitly on 127.0.0.1 in the Python script, you should use
> that address here too (reducing any flake caused by local resolver stupidity).

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:94: scoped_ptr<HTTPTransport> CreateTransport()
{
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> Instead of doing this here, should we have a static Create() factory function
on
> HTTPTransport to centralize platform-specific creation?

I went back on forth for this, but since you brought it up, done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:137: ASSERT_GE(content_type.length(),
strlen(kMultipartFormData));
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> #include <string.h>

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:251: }  // namespace crashpad
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> Nothing with a file part? Are you just trusting that HTTPMultipartBuilder did
> its job?

Yes, we should rely on composition of components and trust that they do what
they say.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
File util/net/http_transport_test_server.py (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test_server.py:2: # Copyright 2014 The Crashpad Authors.
All rights reserved.
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> Gimme a blank line before this.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test_server.py:21: one HTTP request, writing it out
entirely to stdout, and will then terminate.
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> Do you want to say up here that the main motivation was to decode
> Transfer-Encoding: Chunked?

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test_server.py:70: length =
int(self.headers['Content-Length'])
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> This field may not be present. If it’s not, you should read the whole
self.rfile
> to the end and stick that in body.

Done.

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test_server.py:79:
<http://tools.ietf.org/html/rfc7230#section-4.1>. This returns the result
On 2014/10/30 21:13:55, Mark Mentovai wrote:
> RFCs are pretty canonical, you don’t need a link, you can just name the RFC
and
> section like we do elsewhere.

Done.

[Robert Sesek, 2014-10-31 15:12:00]

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test.cc
File util/net/http_transport_test.cc (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:61: // The child will write the HTTP server port
number as a packed int to
On 2014/10/31 14:48:15, Robert Sesek wrote:
> On 2014/10/30 21:13:55, Mark Mentovai wrote:
> > I’d prefer fixed-size (uint16_t). Fixed-size for response_code_ too.
> 
> uint16_t is not enough bits. Dynamic ports go all the way to
> IPPORT_HILASTAUTO=65535. Used a uint32_t.

Sorry, I was holding Python's struct wrong. uint16_t works.

[Mark Mentovai, 2014-10-31 15:13:36]

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test.cc
File util/net/http_transport_test.cc (right):

https://codereview.chromium.org/692963002/diff/1/util/net/http_transport_test...
util/net/http_transport_test.cc:61: // The child will write the HTTP server port
number as a packed int to
Robert Sesek wrote:
> On 2014/10/30 21:13:55, Mark Mentovai wrote:
> > I’d prefer fixed-size (uint16_t). Fixed-size for response_code_ too.
> 
> uint16_t is not enough bits. Dynamic ports go all the way to
> IPPORT_HILASTAUTO=65535. Used a uint32_t.

I’m not sure why you think uint16_t isn’t wide enough to store 65535. Its range
is [0, 65535].

TCP (RFC 793) uses 16-bit fields for ports.

https://codereview.chromium.org/692963002/diff/20001/util/net/http_transport_...
File util/net/http_transport_test.cc (right):

https://codereview.chromium.org/692963002/diff/20001/util/net/http_transport_...
util/net/http_transport_test.cc:60: // The child will write the HTTP server port
number as a packed int to
The comment should reflect the changed type.

[Mark Mentovai, 2014-10-31 16:07:55]

LGTM

https://codereview.chromium.org/692963002/diff/60001/util/net/http_transport_...
File util/net/http_transport_mac.mm (right):

https://codereview.chromium.org/692963002/diff/60001/util/net/http_transport_...
util/net/http_transport_mac.mm:112: class HTTPTransportMac final : public
HTTPTransport {
This should go into an inner anonymous namespace now.

https://codereview.chromium.org/692963002/diff/60001/util/net/http_transport_...
File util/net/http_transport_test_server.py (right):

https://codereview.chromium.org/692963002/diff/60001/util/net/http_transport_...
util/net/http_transport_test_server.py:2: # coding=utf-8
Thanks for the §. :)

Match mig.py? “coding: utf-8”

[Robert Sesek, 2014-10-31 16:17:25]

https://codereview.chromium.org/692963002/diff/60001/util/net/http_transport_...
File util/net/http_transport_mac.mm (right):

https://codereview.chromium.org/692963002/diff/60001/util/net/http_transport_...
util/net/http_transport_mac.mm:112: class HTTPTransportMac final : public
HTTPTransport {
On 2014/10/31 16:07:55, Mark Mentovai wrote:
> This should go into an inner anonymous namespace now.

Done.

https://codereview.chromium.org/692963002/diff/60001/util/net/http_transport_...
File util/net/http_transport_test_server.py (right):

https://codereview.chromium.org/692963002/diff/60001/util/net/http_transport_...
util/net/http_transport_test_server.py:2: # coding=utf-8
On 2014/10/31 16:07:55, Mark Mentovai wrote:
> Thanks for the §. :)
> 
> Match mig.py? “coding: utf-8”

Done.

[Robert Sesek, 2014-10-31 16:17:36]

Committed patchset #5 (id:80001) manually as
d88711adfa1c930cb709b7b7f5d52c8534a72c00 (presubmit successful).