Write MINIDUMP_HANDLE_DATA_STREAM to minidump

Write MINIDUMP_HANDLE_DATA_STREAM to minidump

R=mark@chromium.org
BUG=crashpad:21, crashpad:52
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/3261edd997a4309123951dc6f9be60a9066bec24

[scottmg, 2015-10-20 23:15:06]

Description was changed from

==========
Write MINIDUMP_HANDLE_DATA_STREAM to minidump

BUG=crashpad:21, crashpad:52
==========

to

==========
Write MINIDUMP_HANDLE_DATA_STREAM to minidump

R=mark@chromium.org
BUG=crashpad:21, crashpad:52
==========

[scottmg, 2015-10-20 23:15:06]

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

[Mark Mentovai, 2015-10-21 00:04:21]

Oh, so this is why you’ve been so quiet…

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
File minidump/minidump_handle_writer.cc (right):

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:49: // and never resize the
handle_descriptors_ vector.
Doesn’t this comment belong on the handle_descriptors_.resize() call?

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:51:
strings_.back()->SetUTF16(handle_snapshot.type_name);
There’s probably a lot of duplication in these strings. They should be pooled
(or there should be a TODO to pool them).

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:69: handle_data_stream_base_.SizeOfHeader =
sizeof(MINIDUMP_HANDLE_DATA_STREAM);
Use the member.

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:71: sizeof(MINIDUMP_HANDLE_DESCRIPTOR);
Here too, handle_descriptors_[0]

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
File minidump/minidump_handle_writer.h (right):

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.h:36: //! MINIDUMP_HANDLE_DESCRIPTOR_2.
Another difference between this and most of the other minidump writers is that
this isn’t split into MinidumpHandleWriter and MinidumpHandleListWriter. That
would be more familiar, but if you’re going to keep this structure, call it out
in the comment.

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_strin...
File minidump/minidump_string_writer.h (right):

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_strin...
minidump/minidump_string_writer.h:105: void SetUTF16(const base::string16&
string_utf16);
Precedent for making this inline, see line 125.

https://codereview.chromium.org/1419623003/diff/80001/snapshot/win/end_to_end...
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1419623003/diff/80001/snapshot/win/end_to_end...
snapshot/win/end_to_end_test.py:209: protocol = 'https' if
platform.win32_ver()[0] != 'XP' else 'http'
Oh no, poor XP!

[scottmg, 2015-10-21 00:14:09]

On 2015/10/21 00:04:21, Mark Mentovai wrote:
> Oh, so this is why you’ve been so quiet…

Yeah, sorry for not finishing this off last week. Haven't been feeling that
great.

>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
> File minidump/minidump_handle_writer.cc (right):
> 
>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
> minidump/minidump_handle_writer.cc:49: // and never resize the
> handle_descriptors_ vector.
> Doesn’t this comment belong on the handle_descriptors_.resize() call?
> 
>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
> minidump/minidump_handle_writer.cc:51:
> strings_.back()->SetUTF16(handle_snapshot.type_name);
> There’s probably a lot of duplication in these strings. They should be pooled
> (or there should be a TODO to pool them).
> 
>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
> minidump/minidump_handle_writer.cc:69: handle_data_stream_base_.SizeOfHeader =
> sizeof(MINIDUMP_HANDLE_DATA_STREAM);
> Use the member.
> 
>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
> minidump/minidump_handle_writer.cc:71: sizeof(MINIDUMP_HANDLE_DESCRIPTOR);
> Here too, handle_descriptors_[0]
> 
>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
> File minidump/minidump_handle_writer.h (right):
> 
>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
> minidump/minidump_handle_writer.h:36: //! MINIDUMP_HANDLE_DESCRIPTOR_2.
> Another difference between this and most of the other minidump writers is that
> this isn’t split into MinidumpHandleWriter and MinidumpHandleListWriter. That
> would be more familiar, but if you’re going to keep this structure, call it
out
> in the comment.
> 
>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_strin...
> File minidump/minidump_string_writer.h (right):
> 
>
https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_strin...
> minidump/minidump_string_writer.h:105: void SetUTF16(const base::string16&
> string_utf16);
> Precedent for making this inline, see line 125.
> 
>
https://codereview.chromium.org/1419623003/diff/80001/snapshot/win/end_to_end...
> File snapshot/win/end_to_end_test.py (right):
> 
>
https://codereview.chromium.org/1419623003/diff/80001/snapshot/win/end_to_end...
> snapshot/win/end_to_end_test.py:209: protocol = 'https' if
> platform.win32_ver()[0] != 'XP' else 'http'
> Oh no, poor XP!

[scottmg, 2015-10-21 00:14:15]

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
File minidump/minidump_handle_writer.cc (right):

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:49: // and never resize the
handle_descriptors_ vector.
On 2015/10/21 00:04:21, Mark Mentovai wrote:
> Doesn’t this comment belong on the handle_descriptors_.resize() call?

Done.

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:51:
strings_.back()->SetUTF16(handle_snapshot.type_name);
On 2015/10/21 00:04:21, Mark Mentovai wrote:
> There’s probably a lot of duplication in these strings. They should be pooled
> (or there should be a TODO to pool them).

Yeah, you're right. I'll do that in a follow up.

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:69: handle_data_stream_base_.SizeOfHeader =
sizeof(MINIDUMP_HANDLE_DATA_STREAM);
On 2015/10/21 00:04:21, Mark Mentovai wrote:
> Use the member.

Done.

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.cc:71: sizeof(MINIDUMP_HANDLE_DESCRIPTOR);
On 2015/10/21 00:04:21, Mark Mentovai wrote:
> Here too, handle_descriptors_[0]

Done.

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
File minidump/minidump_handle_writer.h (right):

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_handl...
minidump/minidump_handle_writer.h:36: //! MINIDUMP_HANDLE_DESCRIPTOR_2.
On 2015/10/21 00:04:21, Mark Mentovai wrote:
> Another difference between this and most of the other minidump writers is that
> this isn’t split into MinidumpHandleWriter and MinidumpHandleListWriter. That
> would be more familiar, but if you’re going to keep this structure, call it
out
> in the comment.

Yeah, I noticed that but there didn't seem to be a lot of utility to separating
them.

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_strin...
File minidump/minidump_string_writer.h (right):

https://codereview.chromium.org/1419623003/diff/80001/minidump/minidump_strin...
minidump/minidump_string_writer.h:105: void SetUTF16(const base::string16&
string_utf16);
On 2015/10/21 00:04:21, Mark Mentovai wrote:
> Precedent for making this inline, see line 125.

Done.

https://codereview.chromium.org/1419623003/diff/80001/snapshot/win/end_to_end...
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1419623003/diff/80001/snapshot/win/end_to_end...
snapshot/win/end_to_end_test.py:209: protocol = 'https' if
platform.win32_ver()[0] != 'XP' else 'http'
On 2015/10/21 00:04:21, Mark Mentovai wrote:
> Oh no, poor XP!

Yeah, took me a while to realize what was going on when it couldn't get symbols
on the XP box.

[Mark Mentovai, 2015-10-21 01:30:15]

LGTM

https://codereview.chromium.org/1419623003/diff/100001/minidump/minidump_hand...
File minidump/minidump_handle_writer.h (right):

https://codereview.chromium.org/1419623003/diff/100001/minidump/minidump_hand...
minidump/minidump_handle_writer.h:46: //! \brief Adds an
MINIDUMP_HANDLE_DESCRIPTOR for each handle in \a
an → a

https://codereview.chromium.org/1419623003/diff/100001/minidump/minidump_stri...
File minidump/minidump_string_writer.h (right):

https://codereview.chromium.org/1419623003/diff/100001/minidump/minidump_stri...
minidump/minidump_string_writer.h:105: void SetUTF16(const base::string16&
string_utf16) {
Ought to have at least basic test coverage.

[scottmg, 2015-10-21 16:45:59]

https://codereview.chromium.org/1419623003/diff/100001/minidump/minidump_hand...
File minidump/minidump_handle_writer.h (right):

https://codereview.chromium.org/1419623003/diff/100001/minidump/minidump_hand...
minidump/minidump_handle_writer.h:46: //! \brief Adds an
MINIDUMP_HANDLE_DESCRIPTOR for each handle in \a
On 2015/10/21 01:30:15, Mark Mentovai wrote:
> an → a

Done.

https://codereview.chromium.org/1419623003/diff/100001/minidump/minidump_stri...
File minidump/minidump_string_writer.h (right):

https://codereview.chromium.org/1419623003/diff/100001/minidump/minidump_stri...
minidump/minidump_string_writer.h:105: void SetUTF16(const base::string16&
string_utf16) {
On 2015/10/21 01:30:15, Mark Mentovai wrote:
> Ought to have at least basic test coverage.

Done.

[Mark Mentovai, 2015-10-21 16:53:51]

LGTM

https://codereview.chromium.org/1419623003/diff/120001/minidump/minidump_stri...
File minidump/minidump_string_writer_test.cc (right):

https://codereview.chromium.org/1419623003/diff/120001/minidump/minidump_stri...
minidump/minidump_string_writer_test.cc:60: const base::string16
kString(L"oóöőo");
This was inspired by all of the different kinds of O they have in Hungarian.
True story.

[scottmg, 2015-10-21 17:43:49]

Committed patchset #7 (id:120001) manually as
3261edd997a4309123951dc6f9be60a9066bec24 (presubmit successful).