handler: Add crash report upload. Almost

handler: Add crash report upload. Almost.

Upload isn’t actually hooked up yet, but this establishes the upload
thread and provides all of the plumbing to process pending reports. For
the time being, SkipReportUpload() is called for all pending reports.

R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/4b6d54b2e1c9f5d507ea760d09688edd108c175a

[Mark Mentovai, 2015-02-11 20:50:16]

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

[Robert Sesek, 2015-02-12 01:23:38]

Right now there isn't a lot to test, but won't the UploadThread be responsible
for the retry logic, upload enable/disable handling, etc? What's the plan to
test that?

https://codereview.chromium.org/918743002/diff/1/handler/mac/crash_report_exc...
File handler/mac/crash_report_exception_handler.h (right):

https://codereview.chromium.org/918743002/diff/1/handler/mac/crash_report_exc...
handler/mac/crash_report_exception_handler.h:34: //! \param[in] report_pending
The upload thread to notify when a new crash
report_pending -> upload_thread

https://codereview.chromium.org/918743002/diff/1/handler/mac/crash_report_upl...
File handler/mac/crash_report_upload_thread.h (right):

https://codereview.chromium.org/918743002/diff/1/handler/mac/crash_report_upl...
handler/mac/crash_report_upload_thread.h:101: class Semaphore semaphore_;  //
TODO(mark): Use a condition variable instead?
Is |Semaphore| reserved, so you need to use |class| ?

[Mark Mentovai, 2015-02-12 19:38:01]

When all is said and done, I’d like to make CrashReportUploadThread a little bit
less platform-specific and test it with a mock CrashReportDatabase and
HTTPTransport.

In the end, I’m hoping that CrashReportUploadThread can move up a level from
handler/mac into handler.

https://codereview.chromium.org/918743002/diff/1/handler/mac/crash_report_upl...
File handler/mac/crash_report_upload_thread.h (right):

https://codereview.chromium.org/918743002/diff/1/handler/mac/crash_report_upl...
handler/mac/crash_report_upload_thread.h:101: class Semaphore semaphore_;  //
TODO(mark): Use a condition variable instead?
Robert Sesek wrote:
> Is |Semaphore| reserved, so you need to use |class| ?

No, fixed.

Previously, there was a method in this class named Semaphore() that returned the
semaphore, so the class keyword was needed to disambiguate. And the semaphore
was held in CrashReportExceptionHandler as report_pending_, which explains why
you found that name still hanging on in the documentation comment.

[Robert Sesek, 2015-02-12 19:56:13]

LGTM

On 2015/02/12 19:38:01, Mark Mentovai wrote:
> When all is said and done, I’d like to make CrashReportUploadThread a little
bit
> less platform-specific and test it with a mock CrashReportDatabase and
> HTTPTransport.
> 
> In the end, I’m hoping that CrashReportUploadThread can move up a level from
> handler/mac into handler.

Good, I was thinking the same thing. Since both the CrashReportDatabase and
HTTPTransport are virtual interfaces, it should be easy to make this
cross-platform (assuming some sort of Thread abstraction) and testable.

[Mark Mentovai, 2015-02-12 20:02:55]

Right on with a Thread abstraction.

The other thing I identified that will improve testability will be to add a
FileReaderInterface, and to make the database pass out
FileWriterInterface/FileReaderInterface instead of FilePaths or FileHandles. It
will still need a string to track the filename. The uploader could then get data
from a StringFileReader so we could do the whole test without the filesystem. I
think that will also tighten up the design of the whole system.

[Mark Mentovai, 2015-02-12 20:04:03]

Committed patchset #2 (id:20001) manually as
4b6d54b2e1c9f5d507ea760d09688edd108c175a (presubmit successful).