Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2657693002:
Postmortem dumper: dump user data (Closed)
|
Created:
3 years, 10 months ago by manzagop (departed) Modified:
3 years, 10 months ago Reviewers:
bcwhite CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPostmortem dumper: dump user data
BUG=620813
Review-Url: https://codereview.chromium.org/2657693002
Cr-Commit-Position: refs/heads/master@{#446789}
Committed: https://chromium.googlesource.com/chromium/src/+/1973beba8eb5419f66f3ec9e685c7c257781ff36
Patch Set 1 #Patch Set 2 : Fix format specifier #
Total comments: 16
Patch Set 3 : Address bcwhite's comments #
Total comments: 4
Patch Set 4 : bcwhite's second round #Messages
Total messages: 29 (20 generated)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
manzagop@chromium.org changed reviewers: + bcwhite@chromium.org
Fleshing out the dumper!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... File components/browser_watcher/dump_postmortem_minidump_main_win.cc (right): https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:36: void Indent(FILE* out, size_t indent_level) { "int" for indent_level is fine and will eliminate the "0U" in favor of just "0" below. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:58: fprintf(out, "%02X", bytes_value.at(i)); perhaps "%02X " (separate bytes with a space?) https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:63: fprintf(out, "bytes reference (address: %llu, size: %lld)\n", Should size be "%llu" as well? https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:68: fprintf(out, "%s\n", value.string_value().c_str()); "\"%s\"\n" in case there are spaces at the head or tail. Also clearly indicates that the value is a string so there is no confusion should the string be hexadecimal digits. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:76: fprintf(out, "%s\n", value.char_value().c_str()); "'%s'\n" https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:79: fprintf(out, "%d\n", value.bool_value()); "%s", value.bool_value() ? "true" : "false" https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:122: fprintf(out, "thread_id: %lld\n", activity.thread_id()); Should you display these in hex?
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Please have another look. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... File components/browser_watcher/dump_postmortem_minidump_main_win.cc (right): https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:36: void Indent(FILE* out, size_t indent_level) { On 2017/01/27 15:16:57, bcwhite wrote: > "int" for indent_level is fine and will eliminate the "0U" in favor of just "0" > below. Done. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:58: fprintf(out, "%02X", bytes_value.at(i)); On 2017/01/27 15:16:57, bcwhite wrote: > perhaps "%02X " (separate bytes with a space?) Done. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:63: fprintf(out, "bytes reference (address: %llu, size: %lld)\n", On 2017/01/27 15:16:57, bcwhite wrote: > Should size be "%llu" as well? Hm. The size is stored and analyzed as unsigned, but looks like I actually created the proto's field as signed. Is this something we need to address? Note we also have a signed / unsigned mismatch for the CodeModule proto. This is a preexisting proto (breakpad/server side) that we reused. It stores module address and size as signed numbers. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:68: fprintf(out, "%s\n", value.string_value().c_str()); On 2017/01/27 15:16:57, bcwhite wrote: > "\"%s\"\n" > in case there are spaces at the head or tail. Also clearly indicates that the > value is a string so there is no confusion should the string be hexadecimal > digits. Done. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:76: fprintf(out, "%s\n", value.char_value().c_str()); On 2017/01/27 15:16:57, bcwhite wrote: > "'%s'\n" Done. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:79: fprintf(out, "%d\n", value.bool_value()); On 2017/01/27 15:16:57, bcwhite wrote: > "%s", value.bool_value() ? "true" : "false" Done. https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:122: fprintf(out, "thread_id: %lld\n", activity.thread_id()); On 2017/01/27 15:16:57, bcwhite wrote: > Should you display these in hex? I believe they're most commonly seen in decimal.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... File components/browser_watcher/dump_postmortem_minidump_main_win.cc (right): https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:63: fprintf(out, "bytes reference (address: %llu, size: %lld)\n", On 2017/01/27 19:16:42, manzagop wrote: > On 2017/01/27 15:16:57, bcwhite wrote: > > Should size be "%llu" as well? > > Hm. The size is stored and analyzed as unsigned, but looks like I actually > created the proto's field as signed. > > Is this something we need to address? > > Note we also have a signed / unsigned mismatch for the CodeModule proto. This is > a preexisting proto (breakpad/server side) that we reused. It stores module > address and size as signed numbers. The address could be a problem, given that a randomized address space has a 50% chance of this being negative, but the size shouldn't be. It's probably worth fixing the proto to be consistent. https://codereview.chromium.org/2657693002/diff/40001/components/browser_watc... File components/browser_watcher/dump_postmortem_minidump_main_win.cc (right): https://codereview.chromium.org/2657693002/diff/40001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:63: fprintf(out, "bytes reference (address: %llx, size: %llx)\n", Elsewhere, is capital X. https://codereview.chromium.org/2657693002/diff/40001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:71: fprintf(out, "string reference (address: %llx, size: %llx)\n", capital X?
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Another look? https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... File components/browser_watcher/dump_postmortem_minidump_main_win.cc (right): https://codereview.chromium.org/2657693002/diff/20001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:63: fprintf(out, "bytes reference (address: %llu, size: %lld)\n", On 2017/01/27 20:03:24, bcwhite wrote: > On 2017/01/27 19:16:42, manzagop wrote: > > On 2017/01/27 15:16:57, bcwhite wrote: > > > Should size be "%llu" as well? > > > > Hm. The size is stored and analyzed as unsigned, but looks like I actually > > created the proto's field as signed. > > > > Is this something we need to address? > > > > Note we also have a signed / unsigned mismatch for the CodeModule proto. This > is > > a preexisting proto (breakpad/server side) that we reused. It stores module > > address and size as signed numbers. > > The address could be a problem, given that a randomized address space has a 50% > chance of this being negative, but the size shouldn't be. It's probably worth > fixing the proto to be consistent. I created https://crbug.com/686187 https://codereview.chromium.org/2657693002/diff/40001/components/browser_watc... File components/browser_watcher/dump_postmortem_minidump_main_win.cc (right): https://codereview.chromium.org/2657693002/diff/40001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:63: fprintf(out, "bytes reference (address: %llx, size: %llx)\n", On 2017/01/27 20:03:24, bcwhite wrote: > Elsewhere, is capital X. Done. https://codereview.chromium.org/2657693002/diff/40001/components/browser_watc... components/browser_watcher/dump_postmortem_minidump_main_win.cc:71: fprintf(out, "string reference (address: %llx, size: %llx)\n", On 2017/01/27 20:03:24, bcwhite wrote: > capital X? Done throughout.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review!
The CQ bit was checked by manzagop@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485554027274380,
"parent_rev": "5b207820938548653d3b8f0b539affa1bb14436f", "commit_rev":
"1973beba8eb5419f66f3ec9e685c7c257781ff36"}
Message was sent while issue was closed.
Description was changed from ========== Postmortem dumper: dump user data BUG=620813 ========== to ========== Postmortem dumper: dump user data BUG=620813 Review-Url: https://codereview.chromium.org/2657693002 Cr-Commit-Position: refs/heads/master@{#446789} Committed: https://chromium.googlesource.com/chromium/src/+/1973beba8eb5419f66f3ec9e685c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1973beba8eb5419f66f3ec9e685c... |
