Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860863003:
Rework GetStabilityFileForProcess for use by Crashpad integration (Closed)
|
Created:
3 years, 7 months ago by manzagop (departed) Modified:
3 years, 7 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRework GetStabilityFileForProcess for use by Crashpad integration
The stability file naming code is needed both to know which file to
record to and to know which file to collect on crash (note that for the
postmortem case we simply glob files).
Note: changing the file naming is fine wrt live code as the only
current consumption of stability files is via glob on the postmortem
path.
BUG=718437
Review-Url: https://codereview.chromium.org/2860863003
Cr-Commit-Position: refs/heads/master@{#469733}
Committed: https://chromium.googlesource.com/chromium/src/+/531ed20b52b93b78c30a73e37ccf968238c25d84
Patch Set 1 #
Total comments: 18
Patch Set 2 : address comments #Patch Set 3 : Merge #
Total comments: 6
Patch Set 4 : Address Mark's second round #
Messages
Total messages: 31 (20 generated)
Description was changed from ========== Rework GetStabilityFileForProcess for use by Crashpad integration The stability file naming code is needed both to know which file to record to and to know which file to collect on crash (note that for the postmortem case we simply glob files). BUG=620813 ========== to ========== Rework GetStabilityFileForProcess for use by Crashpad integration The stability file naming code is needed both to know which file to record to and to know which file to collect on crash (note that for the postmortem case we simply glob files). BUG=718437 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
manzagop@chromium.org changed reviewers: + mark@chromium.org
Hi Mark, This prep work for Crashpad integration. Could you have a look? Thanks! Pierre-Antoine
Description was changed from ========== Rework GetStabilityFileForProcess for use by Crashpad integration The stability file naming code is needed both to know which file to record to and to know which file to collect on crash (note that for the postmortem case we simply glob files). BUG=718437 ========== to ========== Rework GetStabilityFileForProcess for use by Crashpad integration The stability file naming code is needed both to know which file to record to and to know which file to collect on crash (note that for the postmortem case we simply glob files). Note: changing the file naming is fine wrt live code as the only current consumption of stability files is via glob on the postmortem path. BUG=718437 ==========
Description was changed from ========== Rework GetStabilityFileForProcess for use by Crashpad integration The stability file naming code is needed both to know which file to record to and to know which file to collect on crash (note that for the postmortem case we simply glob files). Note: changing the file naming is fine wrt live code as the only current consumption of stability files is via glob on the postmortem path. BUG=718437 ========== to ========== Rework GetStabilityFileForProcess for use by Crashpad integration The stability file naming code is needed both to know which file to record to and to know which file to collect on crash (note that for the postmortem case we simply glob files). Note: changing the file naming is fine wrt live code as the only current consumption of stability files is via glob on the postmortem path. BUG=718437 ==========
https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/BUILD.gn (right): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/BUILD.gn:97: "//third_party/crashpad/crashpad/compat", Did you really need this? crashpad/util didn’t give you what you needed? https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/stability_paths.cc (right): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:22: #include "third_party/crashpad/crashpad/util/win/time.h" You’ll probably need to update a DEPS file to be able to do this too. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:27: const uint64_t kMicrosecondsPerSecond = static_cast<uint64_t>(1E6); This is only used in one spot, you can move it to that spot. Also, it can be constexpr, not just const. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:30: DCHECK(creation_time); It’s OK to not do this and let GetProcessTimes() crash. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:33: FILETIME ignore2 = {}; Is it OK to use the same “ignore” each time? We do that in a Crashpad test, and it seems fine there. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:34: FILETIME ignore3 = {}; And since you’re just gonna ignore it, there’s no sense in initializing it with = {}. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/stability_paths.h (left): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.h:13: #if defined(OS_WIN) You’re getting rid of these guards here in the header, but the implementation’s still guarded as Windows-only. What’s up? https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/stability_paths.h (right): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.h:11: #if defined(OS_WIN) You should #include "build/build_config.h" to successfully use this macro.
Thanks! Another look? https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/BUILD.gn (right): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/BUILD.gn:97: "//third_party/crashpad/crashpad/compat", On 2017/05/04 16:47:10, Mark Mentovai wrote: > Did you really need this? crashpad/util didn’t give you what you needed? Yup, otherwise I get "C1083: Cannot open include file: 'sys/time.h'". I notice I can remove it if I switch compat/BUILD.gn from using public_configs to using all_dependent_configs. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/stability_paths.cc (right): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:22: #include "third_party/crashpad/crashpad/util/win/time.h" On 2017/05/04 16:47:10, Mark Mentovai wrote: > You’ll probably need to update a DEPS file to be able to do this too. components/browser_watcher/DEPS already has "+third_party/crashpad/crashpad" because it registers postmortem dumps to a crashpad database. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:27: const uint64_t kMicrosecondsPerSecond = static_cast<uint64_t>(1E6); On 2017/05/04 16:47:10, Mark Mentovai wrote: > This is only used in one spot, you can move it to that spot. > > Also, it can be constexpr, not just const. Done. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:30: DCHECK(creation_time); On 2017/05/04 16:47:10, Mark Mentovai wrote: > It’s OK to not do this and let GetProcessTimes() crash. Done. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:33: FILETIME ignore2 = {}; On 2017/05/04 16:47:10, Mark Mentovai wrote: > Is it OK to use the same “ignore” each time? We do that in a Crashpad test, and > it seems fine there. Looks like exit_code_watcher_win.cc does this too. Done. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.cc:34: FILETIME ignore3 = {}; On 2017/05/04 16:47:10, Mark Mentovai wrote: > And since you’re just gonna ignore it, there’s no sense in initializing it with > = {}. Done. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/stability_paths.h (left): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.h:13: #if defined(OS_WIN) On 2017/05/04 16:47:10, Mark Mentovai wrote: > You’re getting rid of these guards here in the header, but the implementation’s > still guarded as Windows-only. What’s up? Yeah. The intent is for these to be cross-platform but I've only got windows implementations for now. I'll put back the guards on the declarations. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/stability_paths.h (right): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/stability_paths.h:11: #if defined(OS_WIN) On 2017/05/04 16:47:10, Mark Mentovai wrote: > You should #include "build/build_config.h" to successfully use this macro. Done.
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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I think making stability_client win only is the right way to go to fix the bots (which I just did, in the wrong client!). Gotta run, will send that tomorrow.
LGTM. I’m happy with these fixes, but you’ll need to sneak this past the try bots. https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/BUILD.gn (right): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/BUILD.gn:97: "//third_party/crashpad/crashpad/compat", On 2017/05/04 19:52:41, manzagop wrote: > On 2017/05/04 16:47:10, Mark Mentovai wrote: > > Did you really need this? crashpad/util didn’t give you what you needed? > > Yup, otherwise I get "C1083: Cannot open include file: 'sys/time.h'". > > I notice I can remove it if I switch compat/BUILD.gn from using public_configs > to using all_dependent_configs. That’s never really the right solution (which is probably why you didn’t do it!) What about making compat a public_dep of util instead of a dep? https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... File components/browser_watcher/BUILD.gn (right): https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... components/browser_watcher/BUILD.gn:98: "//third_party/crashpad/crashpad/util", According to try server results, you’ll probably want to do this on Windows. You can also do it on Windows and Mac, since this target should at least work on Mac, even if it’s not necessary here yet. But you definitely shouldn’t depend on it on any other platform. https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... File components/browser_watcher/stability_paths.cc (right): https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... components/browser_watcher/stability_paths.cc:48: std::string file_name = base::StringPrintf("%u-%llu", pid, creation_time_us); Since creation_time_us is int64_t, it seems like %lld is a better fit. https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... File components/browser_watcher/stability_paths.h (right): https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... components/browser_watcher/stability_paths.h:24: // to ensure unicity in the face of pid recycling. uniqueness > uniquity > unicity, although they’re all fine.
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...
Thanks Mark! https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... File components/browser_watcher/BUILD.gn (right): https://codereview.chromium.org/2860863003/diff/40001/components/browser_watc... components/browser_watcher/BUILD.gn:97: "//third_party/crashpad/crashpad/compat", On 2017/05/04 21:27:37, Mark Mentovai wrote: > On 2017/05/04 19:52:41, manzagop wrote: > > On 2017/05/04 16:47:10, Mark Mentovai wrote: > > > Did you really need this? crashpad/util didn’t give you what you needed? > > > > Yup, otherwise I get "C1083: Cannot open include file: 'sys/time.h'". > > > > I notice I can remove it if I switch compat/BUILD.gn from using public_configs > > to using all_dependent_configs. > > That’s never really the right solution (which is probably why you didn’t do it!) > > What about making compat a public_dep of util instead of a dep? Ah nice! Removed here, below and in components/crash/content/app:unit_tests. Done. https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... File components/browser_watcher/BUILD.gn (right): https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... components/browser_watcher/BUILD.gn:98: "//third_party/crashpad/crashpad/util", On 2017/05/04 21:27:37, Mark Mentovai wrote: > According to try server results, you’ll probably want to do this on Windows. > > You can also do it on Windows and Mac, since this target should at least work on > Mac, even if it’s not necessary here yet. But you definitely shouldn’t depend on > it on any other platform. I folded the target inside the is_win. Done. https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... File components/browser_watcher/stability_paths.cc (right): https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... components/browser_watcher/stability_paths.cc:48: std::string file_name = base::StringPrintf("%u-%llu", pid, creation_time_us); On 2017/05/04 21:27:37, Mark Mentovai wrote: > Since creation_time_us is int64_t, it seems like %lld is a better fit. Done. https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... File components/browser_watcher/stability_paths.h (right): https://codereview.chromium.org/2860863003/diff/80001/components/browser_watc... components/browser_watcher/stability_paths.h:24: // to ensure unicity in the face of pid recycling. On 2017/05/04 21:27:37, Mark Mentovai wrote: > uniqueness > uniquity > unicity, although they’re all fine. Done.
manzagop@chromium.org changed reviewers: + holte@chromium.org
Hi Steven, Could you have an OWNERS' look for components\metrics? Thanks! Pierre
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks for the review!
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2860863003/#ps100001 (title: "Address Mark's second round")
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": 100001, "attempt_start_ts": 1494008789986070,
"parent_rev": "717be5e78519e6531071e4491b951adc02f2fe38", "commit_rev":
"531ed20b52b93b78c30a73e37ccf968238c25d84"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494008789986070,
"parent_rev": "717be5e78519e6531071e4491b951adc02f2fe38", "commit_rev":
"531ed20b52b93b78c30a73e37ccf968238c25d84"}
Message was sent while issue was closed.
Description was changed from ========== Rework GetStabilityFileForProcess for use by Crashpad integration The stability file naming code is needed both to know which file to record to and to know which file to collect on crash (note that for the postmortem case we simply glob files). Note: changing the file naming is fine wrt live code as the only current consumption of stability files is via glob on the postmortem path. BUG=718437 ========== to ========== Rework GetStabilityFileForProcess for use by Crashpad integration The stability file naming code is needed both to know which file to record to and to know which file to collect on crash (note that for the postmortem case we simply glob files). Note: changing the file naming is fine wrt live code as the only current consumption of stability files is via glob on the postmortem path. BUG=718437 Review-Url: https://codereview.chromium.org/2860863003 Cr-Commit-Position: refs/heads/master@{#469733} Committed: https://chromium.googlesource.com/chromium/src/+/531ed20b52b93b78c30a73e37ccf... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/531ed20b52b93b78c30a73e37ccf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
