Implement CrashUploadListWin to enable chrome://crashes on Windows.

Implement CrashUploadListWin to enable chrome://crashes on Windows.

Added new class CrashUploadListWin, subclass of CrashUploadList which
has the Windows platform-specific logic to read crash logs from Windows
Event Log.

Added static CrashUploadList::Create() factory method which returns an
instance of CrashUploadListWin on Windows and an instance of
CrashUploadList on other platforms.

Also implemented the max_count parameter in
CrashUploadList::GetUploadedCrashes(), which was part of
the interface but was ignored by the code.

BUG=73468
TEST=Open chrome://crashes under Windows after one or more
     crash reports were uploaded. See the crash reports listed.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80165

[stuartmorgan, 2011-03-29 21:19:57]

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.cc:53:
InformDelegateOfCompletionAsynchronously();
It would be cleaner for the subclass to make a new method called by
LoadCrashListAsynchronously that calls UploadLog then does the bounce back to
the original thread, so the subclass doesn't need to know anything about that
part of the plumbing.

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.cc:72: count != max_count; ++i) {
Whoops :( Good catch.

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list_win.cc:47: messageString.substr(19,
crashId.length() - 20);
Can you turn the prefix into a static string, and then replace the magic numbers
here with computations of the length of that string?

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list_win.cc:50: crashId.c_str()));
It's odd that the subclass is serializing the crash data into the log file
format just to let the superclass parse it again. Instead, GetUploadedCrashes
should be refactored to call something that returns a vector of CrashInfo
structs, and in the superclass does the parsing. then this subclass can just
overload that second method.

Alternately, the superclass could do the parsing on the file thread and then
both would store structs directly.

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list_win.h (right):

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list_win.h:11: class CrashUploadListWin : public
CrashUploadList {
Needs a class-level comment

[Alexei Svitkine (slow), 2011-03-30 15:36:40]

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.cc:53:
InformDelegateOfCompletionAsynchronously();
On 2011/03/29 21:19:57, stuartmorgan wrote:
> It would be cleaner for the subclass to make a new method called by
> LoadCrashListAsynchronously that calls UploadLog then does the bounce back to
> the original thread, so the subclass doesn't need to know anything about that
> part of the plumbing.

Done.

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list_win.cc:47: messageString.substr(19,
crashId.length() - 20);
On 2011/03/29 21:19:57, stuartmorgan wrote:
> Can you turn the prefix into a static string, and then replace the magic
numbers
> here with computations of the length of that string?

Done. The resulting code is unfortunately uglier, but magic-number free!

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list_win.cc:50: crashId.c_str()));
On 2011/03/29 21:19:57, stuartmorgan wrote:
> It's odd that the subclass is serializing the crash data into the log file
> format just to let the superclass parse it again. Instead, GetUploadedCrashes
> should be refactored to call something that returns a vector of CrashInfo
> structs, and in the superclass does the parsing. then this subclass can just
> overload that second method.
> 
> Alternately, the superclass could do the parsing on the file thread and then
> both would store structs directly.

Good point. I've refactored the code to have the base class store a vector of
CrashInfo entries directly, instead of log messages.

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list_win.h (right):

http://codereview.chromium.org/6771021/diff/4001/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list_win.h:11: class CrashUploadListWin : public
CrashUploadList {
On 2011/03/29 21:19:57, stuartmorgan wrote:
> Needs a class-level comment

Done.

[stuartmorgan, 2011-03-30 16:28:10]

Sorry, a few more small things.

(The comments about splitting out helper functions are just suggestions, if
you'd prefer not to that's fine. I've recently re-read some books espousing the
lots-of-small-functions approach, so I see long functions everywhere at the
moment.)

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.cc:51: void CrashUploadList::LoadCrashList() {
This function is doing a couple of things now; you might want to split one or
more single-purposes helpers out (e.g., loading the file contents and returning
a string, or converting a line of the file into a CrashInfo struct).

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list.h (right):

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.h:59: // Reads the upload log and stores the
lines in log_entries_.
s/log_entries_/crashes_/

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.h:62: // Returns a reference to the list of log
entries.
s/log entries/crashes/

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.h:69: Delegate* delegate_;
Variables should be after functions.

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.h:72: // asynchronously.
Too much implementation detail for the header; how about something like "Manages
the background thread work for LoadCrashListAsynchronously"?

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:14: void
CrashUploadListWin::LoadCrashList() {
As with the other LoadCrashList function, this would benefit from having one or
more (private, they don't have to match the superclass's) helpers split out.

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:33: if (success) {
For example, everything in here could easily be split out, so the specific
record handling and the overall "read stuff from the event log" logic are
separate.

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:43: record->NumStrings >= 1) {
Another possibility: everything from "LPWSTR provider_name" looks like it's
conceptually something like "bool CouldBeOmahaCrashRecord(EVENTLOGRECORD*
record)"

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:53: }
This whole block is another possibility for splitting out.

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/ui/webui/cra...
File chrome/browser/ui/webui/crashes_ui.cc (right):

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/ui/webui/cra...
chrome/browser/ui/webui/crashes_ui.cc:188: return true;
This needs to not be part of the CL.

[stuartmorgan, 2011-03-30 16:31:09]

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list_win.h (right):

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.h:19: void LoadCrashList();
Oh, and prompted by the current thread on chromium-dev, this should have
OVERRIDE.

[Alexei Svitkine (slow), 2011-03-30 18:54:37]

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list.h (right):

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.h:59: // Reads the upload log and stores the
lines in log_entries_.
On 2011/03/30 16:28:10, stuartmorgan wrote:
> s/log_entries_/crashes_/

Done.

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.h:62: // Returns a reference to the list of log
entries.
On 2011/03/30 16:28:10, stuartmorgan wrote:
> s/log entries/crashes/

Done.

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.h:69: Delegate* delegate_;
On 2011/03/30 16:28:10, stuartmorgan wrote:
> Variables should be after functions.

Done.

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.h:72: // asynchronously.
On 2011/03/30 16:28:10, stuartmorgan wrote:
> Too much implementation detail for the header; how about something like
"Manages
> the background thread work for LoadCrashListAsynchronously"?

Done.

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:14: void
CrashUploadListWin::LoadCrashList() {
On 2011/03/30 16:28:10, stuartmorgan wrote:
> As with the other LoadCrashList function, this would benefit from having one
or
> more (private, they don't have to match the superclass's) helpers split out.

I've refactored this into a couple of helpers. My original reluctance to do this
was because I wanted to avoid polluting the header file with Windows-specific
types (in parameters).

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/ui/webui/cra...
File chrome/browser/ui/webui/crashes_ui.cc (right):

http://codereview.chromium.org/6771021/diff/12001/chrome/browser/ui/webui/cra...
chrome/browser/ui/webui/crashes_ui.cc:188: return true;
On 2011/03/30 16:28:10, stuartmorgan wrote:
> This needs to not be part of the CL.

Forgot to revert that! Done now..

[stuartmorgan, 2011-03-30 20:29:51]

LGTM with tiny tweaks, assuming mad signs off on the Windows-y stuff. Thanks for
doing all the refactoring here!

http://codereview.chromium.org/6771021/diff/9009/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6771021/diff/9009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.cc:60: std::vector<std::string> log_entries;
Move down a line, so that the declarations and uses are paired.

http://codereview.chromium.org/6771021/diff/9009/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list_win.h (right):

http://codereview.chromium.org/6771021/diff/9009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list_win.h:23: bool
IsPossibleCrashLogRecord(EVENTLOGRECORD* record) const;
These methods need comments.

[Alexei Svitkine (slow), 2011-03-30 20:52:43]

http://codereview.chromium.org/6771021/diff/9009/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6771021/diff/9009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list.cc:60: std::vector<std::string> log_entries;
On 2011/03/30 20:29:51, stuartmorgan wrote:
> Move down a line, so that the declarations and uses are paired.

Done.

http://codereview.chromium.org/6771021/diff/9009/chrome/browser/crash_upload_...
File chrome/browser/crash_upload_list_win.h (right):

http://codereview.chromium.org/6771021/diff/9009/chrome/browser/crash_upload_...
chrome/browser/crash_upload_list_win.h:23: bool
IsPossibleCrashLogRecord(EVENTLOGRECORD* record) const;
On 2011/03/30 20:29:51, stuartmorgan wrote:
> These methods need comments.

Done.

[MAD, 2011-03-31 14:07:58]

Cool... Very well done...

I have a few comments on the Windows implementation... The rest looks good
indeed...

BYE
MAD

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:32: EVENTLOGRECORD* record =
(EVENTLOGRECORD*)&buffer[record_offset];
I'm not sure this works... What if the beginning of the record fits within the
bytes_read, but it gets truncated in the middle?

To make sure this doesn't happen, we should make sure that the size of the read
buffer is a multiple of the size of a record, right?

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:42: // of ERROR_HANDLE_EOF.
I think we should DCHECK that we actually got ERROR_HANDLE_EOF, so that we see
when other errors occur... Or at least log them...

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:63: pattern_prefix.size() +
pattern_suffix.size();
Are you sure about this? I think you need to find the distance between where
these two are found in the string instead of the sum of their sizes, right?

[Alexei Svitkine (slow), 2011-03-31 14:36:46]

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:32: EVENTLOGRECORD* record =
(EVENTLOGRECORD*)&buffer[record_offset];
On 2011/03/31 14:07:58, MAD wrote:
> I'm not sure this works... What if the beginning of the record fits within the
> bytes_read, but it gets truncated in the middle?
> 
> To make sure this doesn't happen, we should make sure that the size of the
read
> buffer is a multiple of the size of a record, right?

It's not explicitly clarified in the API docs:

http://msdn.microsoft.com/en-us/library/aa363674(v=vs.85).aspx

However, we can't do the sizeof() comparison checks because records are variable
length - they have trailing data past the struct header, and each record's
length is specified by record->Length.

I would assume that the API would not return partial records - but again, it is
not explicitly stated in the docs. If we want to be paranoid, we can check that
the size left is enough to fit sizeof(EVENTLOGRECORD), and if that's the case,
do a second check using record->Length.

What do you think?

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:42: // of ERROR_HANDLE_EOF.
On 2011/03/31 14:07:58, MAD wrote:
> I think we should DCHECK that we actually got ERROR_HANDLE_EOF, so that we see
> when other errors occur... Or at least log them...

Good idea! Done.

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:63: pattern_prefix.size() +
pattern_suffix.size();
On 2011/03/31 14:07:58, MAD wrote:
> Are you sure about this? I think you need to find the distance between where
> these two are found in the string instead of the sum of their sizes, right?

Good catch! I think I got this wrong after refactoring from the earlier version.

Fixed by passing message.size() - pattern_length to substr().

[MAD, 2011-03-31 14:42:51]

I didn't get a chance to look at the recent changes (going to do an interview
now), but I wanted to send this comment now so that you can implement your
suggestion...

More later...

BYE
MAD

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/12002/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:32: EVENTLOGRECORD* record =
(EVENTLOGRECORD*)&buffer[record_offset];
On 2011/03/31 14:36:47, asvitkine_ wrote:
> On 2011/03/31 14:07:58, MAD wrote:
> > I'm not sure this works... What if the beginning of the record fits within
the
> > bytes_read, but it gets truncated in the middle?
> > 
> > To make sure this doesn't happen, we should make sure that the size of the
> read
> > buffer is a multiple of the size of a record, right?
> 
> It's not explicitly clarified in the API docs:
> 
> http://msdn.microsoft.com/en-us/library/aa363674%28v=vs.85%29.aspx
> 
> However, we can't do the sizeof() comparison checks because records are
variable
> length - they have trailing data past the struct header, and each record's
> length is specified by record->Length.
> 
> I would assume that the API would not return partial records - but again, it
is
> not explicitly stated in the docs. If we want to be paranoid, we can check
that
> the size left is enough to fit sizeof(EVENTLOGRECORD), and if that's the case,
> do a second check using record->Length.
> 
> What do you think?

Good point, I forgot about the dynamic size allowing contained data to be of any
size...

Yes, please add the check that we have at least enough room for the struct, and
then use the size field to confirm we also got all the dymanic data...

Thanks!

[Alexei Svitkine (slow), 2011-03-31 15:00:01]

Done.

[Alexei Svitkine (slow), 2011-03-31 18:09:01]

I've changed the message string parsing to just look for "Id=*." per Sorin's
suggestion in the other thread.

[MAD, 2011-04-01 14:28:19]

Sorry about the delay...

Almost there... Still a few more comments....

BYE
MAD

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.cc:79: CrashInfo info(components[1],
This used to fit in 80 columns, why did you split it?

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:36: break;
OK, now we won't read beyond bytes_read, but if a record is split across reads,
we will read from the middle of the next one in the next while loop turn, so
this is no good...

Maybe we should simply DCHECK here and hope M$FT does things properly (even
though it's not documented)... We could try with a small stand alone test with a
smaller buffer than two records and see what happens...

If I properly understand this line from the documentation "Remarks"
>>>
When this function returns successfully, the read position in the event log is
adjusted by the number of records read.
<<<

I think it should always end on a record boundary, so we should be OK with
simply putting DCHECKs there... And a comment to explain why too... :-)

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:70: EndsWith(message, pattern_suffix,
false)) {
You are still assuming that the id=XXX. will be at the end of the message, which
might not always be the case as Sorin said, so I think we should find the
end_index with message.find(pattern_suffix, start_index) instead...

[Alexei Svitkine (slow), 2011-04-01 15:01:23]

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.cc:79: CrashInfo info(components[1],
On 2011/04/01 14:28:20, MAD wrote:
> This used to fit in 80 columns, why did you split it?

This got refactored a few times where at one point it was in a deeper nesting
level, hence the need for that change. But now that it's back at the existing
nesting level, I've changed it back.

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list_win.cc (right):

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:36: break;
On 2011/04/01 14:28:20, MAD wrote:
> OK, now we won't read beyond bytes_read, but if a record is split across
reads,
> we will read from the middle of the next one in the next while loop turn, so
> this is no good...
> 
> Maybe we should simply DCHECK here and hope M$FT does things properly (even
> though it's not documented)... We could try with a small stand alone test with
a
> smaller buffer than two records and see what happens...
> 
> If I properly understand this line from the documentation "Remarks"
> >>>
> When this function returns successfully, the read position in the event log is
> adjusted by the number of records read.
> <<<
> 
> I think it should always end on a record boundary, so we should be OK with
> simply putting DCHECKs there... And a comment to explain why too... :-)

Done.

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
chrome/browser/crash_upload_list_win.cc:70: EndsWith(message, pattern_suffix,
false)) {
On 2011/04/01 14:28:20, MAD wrote:
> You are still assuming that the id=XXX. will be at the end of the message,
which
> might not always be the case as Sorin said, so I think we should find the
> end_index with message.find(pattern_suffix, start_index) instead...

Done.

[MAD, 2011-04-01 15:06:27]

Excellent!

Thanks!

LGTM!

BYE!
MAD!
:-)!
!

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
File chrome/browser/crash_upload_list.cc (right):

http://codereview.chromium.org/6771021/diff/19008/chrome/browser/crash_upload...
chrome/browser/crash_upload_list.cc:79: CrashInfo info(components[1],
On 2011/04/01 15:01:23, asvitkine_ wrote:
> On 2011/04/01 14:28:20, MAD wrote:
> > This used to fit in 80 columns, why did you split it?
> 
> This got refactored a few times where at one point it was in a deeper nesting
> level, hence the need for that change. But now that it's back at the existing
> nesting level, I've changed it back.

That's what I thought... Thanks! :-)

[commit-bot: I haz the power, 2011-04-01 15:58:54]

Change committed as 80165