Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867063002:
Stability instrumentation Crashpad integration (Closed)
|
Created:
3 years, 7 months ago by manzagop (departed) Modified:
3 years, 7 months ago Reviewers:
rkaplow, Sigurður Ásgeirsson, scottmg, Mark Mentovai, robertshield, alex clarke (OOO till 29th), grt (UTC plus 2) CC:
chromium-reviews, sadrul, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, jam, darin-cc_chromium.org, kalyank Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionStability instrumentation Crashpad integration
The stability instrumentation is about Chrome recording internal
state to a memory mapped file as it is running. It is currently used
experimentally on Windows only.
This CL makes use of Crashpad's extensibility mechanism to collect
information from the stability file on crash, and include it within
the crash report as an additional minidump user stream.
DETAILS:
- The browser process now passes the user data dir to the crash
handler's command line.
- The handler process creates the Crashpad extension that will search
for a stability file on any crash. If the file is found (currently only
possible for instrumented browser processes), it is collected and the
information is added to the crash report. On successful collection the
stability file is deleted.
TESTING
- launch chrome with the stability instrumentation:
chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging
- validate the presence of the stability file in <data-dir>/Stability
- trigger a crash (visit chrome://inducebrowsercrashforrealz)
- validate the deletion of the the stability file in <data-dir>/Stability
- validate the presence of a crash report in
<data-dir>/Crashpad/Reports and ensure it contains the additional
stream, either with dumpchk.exe or the dump_stability build target.
BUG=718437
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2867063002
Cr-Commit-Position: refs/heads/master@{#474759}
Committed: https://chromium.googlesource.com/chromium/src/+/fecdf607e706474b65a4d26e554f0504aead3f3e
Patch Set 1 #Patch Set 2 : Mac user data dir plumbing #Patch Set 3 : mac compile issues (aka trybot development) #Patch Set 4 : second try: user data dir only known to embedder #Patch Set 5 : some loose ends #Patch Set 6 : Moar comments and fixups #
Total comments: 18
Patch Set 7 : Address Siggi's first round #
Total comments: 6
Patch Set 8 : Merge / Siggi second round #
Total comments: 20
Patch Set 9 : Siggi round 3 #
Total comments: 26
Patch Set 10 : siggi + grt round #Patch Set 11 : Explicit destructor for PrimaryInstallDetails (style checker) #Patch Set 12 : fix setup target compilation #Patch Set 13 : merge #Patch Set 14 : MakeProductDetails checks CurrentProcessNeedsProfileDir #
Total comments: 18
Patch Set 15 : fully switch to InstallDetails userdatadir via helper #Patch Set 16 : process type: auto init + crashpad handler type #
Total comments: 12
Patch Set 17 : grt+scottmg comments #
Total comments: 2
Patch Set 18 : merge #Patch Set 19 : Fix some comments #Patch Set 20 : clang compile #
Total comments: 10
Patch Set 21 : Drop IsProcessTypeInitialized #Patch Set 22 : Remove leftover IsProcessTypeInitialized in test #Patch Set 23 : merge #Messages
Total messages: 158 (111 generated)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
manzagop@chromium.org changed reviewers: + mark@chromium.org
Hi Mark, This is the CP integration, still rough. Does the high level look ok, eg plumbing user-data-dir to crashpad? If so, I'd send the crashpad CL to do it. Thanks, Pierre-Antoine
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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
Hi Mark, I've switched to the ptype approach. Please have a look. Thanks, Pierre-Antoine
Description was changed from ========== WIP - Stability instrumentation Crashpad integration BUG=718437 ========== to ========== Stability instrumentation Crashpad integration Make use of Crashpad's extensibility mechanism to collect the stability file on crash. The file is deleted if it is successfully collected. BUG=718437 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
manzagop@chromium.org changed reviewers: + siggi@chromium.org
Siggi, Do you think you could have a quick look? Thanks, Pierre-Antoine
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Didn't review in great detail yet, but looks overall sane to my eye. https://codereview.chromium.org/2867063002/diff/160001/build/secondary/third_... File build/secondary/third_party/crashpad/crashpad/minidump/BUILD.gn (right): https://codereview.chromium.org/2867063002/diff/160001/build/secondary/third_... build/secondary/third_party/crashpad/crashpad/minidump/BUILD.gn:70: "test/minidump_user_extension_stream_util.cc", no - test code is not for shipping. https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:28: bool GetStabilityFileName(crashpad::ProcessSnapshot* process_snapshot, I'd suggest you grab the user data dir from the user data dir argument, and pass into your instance at creation. https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:61: StabilityReport report; make sure this report is deallocated as soon as you're done with it. As-is, you'll have three copies of this data in memory at once. https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:74: base::File file(stability_file, base::File::FLAG_OPEN | You probably want to capture a metric for how often this fails (cause it's going to fail). https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:78: return base::WrapUnique(new crashpad::test::BufferExtensionStreamDataSource( just re-implement the damn thing, and make sure you get rid of this copying. This code is going to be running under VERY dire memory restrictions on OOM crashes and duplicating the stream here the wrong thing to do. https://codereview.chromium.org/2867063002/diff/160001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:243: /* ??? https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: user_data_dir); Is this appropriately escaped already?
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...
Addressed comments. Another look? https://codereview.chromium.org/2867063002/diff/160001/build/secondary/third_... File build/secondary/third_party/crashpad/crashpad/minidump/BUILD.gn (right): https://codereview.chromium.org/2867063002/diff/160001/build/secondary/third_... build/secondary/third_party/crashpad/crashpad/minidump/BUILD.gn:70: "test/minidump_user_extension_stream_util.cc", On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > no - test code is not for shipping. (Yup, I had a DO NOT SUBMIT at inclusion.) Done. https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:28: bool GetStabilityFileName(crashpad::ProcessSnapshot* process_snapshot, On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > I'd suggest you grab the user data dir from the user data dir argument, and pass > into your instance at creation. Done. https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:61: StabilityReport report; On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > make sure this report is deallocated as soon as you're done with it. > > As-is, you'll have three copies of this data in memory at once. Done. https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:74: base::File file(stability_file, base::File::FLAG_OPEN | On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > You probably want to capture a metric for how often this fails (cause it's going > to fail). My initial thought was we can't do metrics, but I guess they might end up in the Crashpad metrics file? I'll check with Scott and Brian. That said, how would I check this? The file will only be deleted after the crashing process gets cleaned up? I think the way to go here is to update the file's persitent allocator's state - the TODO above. https://codereview.chromium.org/2867063002/diff/160001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:78: return base::WrapUnique(new crashpad::test::BufferExtensionStreamDataSource( On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > just re-implement the damn thing, and make sure you get rid of this copying. > This code is going to be running under VERY dire memory restrictions on OOM > crashes and duplicating the stream here the wrong thing to do. Done. https://codereview.chromium.org/2867063002/diff/160001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:243: /* On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > ??? Yeah, this trips on TOT. MAD was also hitting it. Spent a bit of time on it but it's non obvious. https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: user_data_dir); On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > Is this appropriately escaped already? It comes directly from install_static::GetUserDataDirectory and doesn't look like it. https://cs.chromium.org/chromium/src/chrome/install_static/user_data_dir.h?dr... What can I use? I see EscapePath but it looks like it escapes \. https://cs.chromium.org/chromium/src/net/base/escape.cc?type=cs&l=362
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Another quick set of comments, still didn't review in great detail. https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: user_data_dir); On 2017/05/10 23:06:11, manzagop wrote: > On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > > Is this appropriately escaped already? > > It comes directly from install_static::GetUserDataDirectory and doesn't look > like it. > https://cs.chromium.org/chromium/src/chrome/install_static/user_data_dir.h?dr... > > What can I use? I see EscapePath but it looks like it escapes \. > https://cs.chromium.org/chromium/src/net/base/escape.cc?type=cs&l=362 > Looks like Crashpad uses this: https://chromium.googlesource.com/crashpad/crashpad/+/master/util/win/command... on Windows. Maybe good enough for your purposes? https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:65: std::unique_ptr<std::string> data); why not construct this with the StabilityReport proto, and have it stash the data into a straight-up string member? https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:115: std::unique_ptr<std::string> report = CollectReport(stability_file); yups, just return the extension stream thingy from CollectReport?
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! PTAL. https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: user_data_dir); On 2017/05/11 13:48:32, Sigurður Ásgeirsson wrote: > On 2017/05/10 23:06:11, manzagop wrote: > > On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > > > Is this appropriately escaped already? > > > > It comes directly from install_static::GetUserDataDirectory and doesn't look > > like it. > > > https://cs.chromium.org/chromium/src/chrome/install_static/user_data_dir.h?dr... > > > > What can I use? I see EscapePath but it looks like it escapes \. > > https://cs.chromium.org/chromium/src/net/base/escape.cc?type=cs&l=362 > > > > Looks like Crashpad uses this: > https://chromium.googlesource.com/crashpad/crashpad/+/master/util/win/command... > on Windows. Maybe good enough for your purposes? Looks like this is already applied to all arguments. https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/client/cra... https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:65: std::unique_ptr<std::string> data); On 2017/05/11 13:48:32, Sigurður Ásgeirsson wrote: > why not construct this with the StabilityReport proto, and have it stash the > data into a straight-up string member? Done. Note the string might be empty if the serialization fails, but I'll still return a datasource. https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:115: std::unique_ptr<std::string> report = CollectReport(stability_file); On 2017/05/11 13:48:32, Sigurður Ásgeirsson wrote: > yups, just return the extension stream thingy from CollectReport? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
nice - another batch of nits. https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: user_data_dir); On 2017/05/11 14:54:45, manzagop wrote: > On 2017/05/11 13:48:32, Sigurður Ásgeirsson wrote: > > On 2017/05/10 23:06:11, manzagop wrote: > > > On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > > > > Is this appropriately escaped already? > > > > > > It comes directly from install_static::GetUserDataDirectory and doesn't look > > > like it. > > > > > > https://cs.chromium.org/chromium/src/chrome/install_static/user_data_dir.h?dr... > > > > > > What can I use? I see EscapePath but it looks like it escapes \. > > > https://cs.chromium.org/chromium/src/net/base/escape.cc?type=cs&l=362 > > > > > > > Looks like Crashpad uses this: > > > https://chromium.googlesource.com/crashpad/crashpad/+/master/util/win/command... > > on Windows. Maybe good enough for your purposes? > > Looks like this is already applied to all arguments. > https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/client/cra... Ah - figures. There's no escaping the escaping, alas. https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:65: std::unique_ptr<std::string> data); On 2017/05/11 14:54:45, manzagop wrote: > On 2017/05/11 13:48:32, Sigurður Ásgeirsson wrote: > > why not construct this with the StabilityReport proto, and have it stash the > > data into a straight-up string member? > > Done. Note the string might be empty if the serialization fails, but I'll still > return a datasource. Interesting distinction, is this helpful or harmful? You could construct, test the length and return a nullptr if it's zero - assuming the distinction is helpful. https://codereview.chromium.org/2867063002/diff/200001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/200001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:17: #include <vector> was missing? https://codereview.chromium.org/2867063002/diff/200001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:245: // DO NOT SUBMIT: validate use of CHECK. Ask grt. It feels wonky to CHECK before crash reporting has been set up, though. If this fails in the field, there's no safety net that'd inform us. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:37: return true; return void as this can't fail (at present). https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:44: const StabilityReport& report); you could also use a "bool Init()" method to initialize the data_ member if you want to detect serialization failure. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:75: CollectionStatus status = Extract(path, &report); You can use histograms here to make sure there's a record of failures. The Crashpad handler metrics hookup will provide. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:84: base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ | also leave a trace of failure here? https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:108: // Either this is not an instrumented process, or the stability file cannot is this going to be true for e.g. renderers and !browser processes? If so maybe a quick comment is in order? https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.h (right): https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.h:21: class StabilityReportUserStreamDataSource nit: quick comment on the purpose of this class? https://codereview.chromium.org/2867063002/diff/200001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2867063002/diff/200001/components/crash/conte... components/crash/content/app/crashpad.cc:199: embedded_handler); seems strange to use an overload to pad this argument in. all the callers should be here, why not update them? https://codereview.chromium.org/2867063002/diff/200001/components/crash/conte... File components/crash/content/app/run_as_crashpad_handler_win.cc (right): https://codereview.chromium.org/2867063002/diff/200001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.cc:77: // Note: this stream data source will attempt to collect a stability file Nits: drop Note: make this a statement. "collect a stability file information" wonky verbiage :)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Thanks! Another look? https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/160001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: user_data_dir); On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > On 2017/05/11 14:54:45, manzagop wrote: > > On 2017/05/11 13:48:32, Sigurður Ásgeirsson wrote: > > > On 2017/05/10 23:06:11, manzagop wrote: > > > > On 2017/05/10 17:46:23, Sigurður Ásgeirsson wrote: > > > > > Is this appropriately escaped already? > > > > > > > > It comes directly from install_static::GetUserDataDirectory and doesn't > look > > > > like it. > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/install_static/user_data_dir.h?dr... > > > > > > > > What can I use? I see EscapePath but it looks like it escapes \. > > > > https://cs.chromium.org/chromium/src/net/base/escape.cc?type=cs&l=362 > > > > > > > > > > Looks like Crashpad uses this: > > > > > > https://chromium.googlesource.com/crashpad/crashpad/+/master/util/win/command... > > > on Windows. Maybe good enough for your purposes? > > > > Looks like this is already applied to all arguments. > > > https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/client/cra... > > Ah - figures. There's no escaping the escaping, alas. Acknowledged. https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/180001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:65: std::unique_ptr<std::string> data); On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > On 2017/05/11 14:54:45, manzagop wrote: > > On 2017/05/11 13:48:32, Sigurður Ásgeirsson wrote: > > > why not construct this with the StabilityReport proto, and have it stash the > > > data into a straight-up string member? > > > > Done. Note the string might be empty if the serialization fails, but I'll > still > > return a datasource. > > Interesting distinction, is this helpful or harmful? You could construct, test > the length and return a nullptr if it's zero - assuming the distinction is > helpful. Went with early pruning. Done. https://codereview.chromium.org/2867063002/diff/200001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/200001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:17: #include <vector> On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > was missing? Yup. Ack. https://codereview.chromium.org/2867063002/diff/200001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:245: // DO NOT SUBMIT: validate use of CHECK. On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > Ask grt. It feels wonky to CHECK before crash reporting has been set up, though. > If this fails in the field, there's no safety net that'd inform us. Will do. We actually have 2 calls to this function. This one in the crash client, and another in the handler. In the handler, I've made a change to interpret an empty value as missing, in which case we don't attempt to create an crumbs extension. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:37: return true; On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > return void as this can't fail (at present). Done. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:44: const StabilityReport& report); On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > you could also use a "bool Init()" method to initialize the data_ member if you > want to detect serialization failure. Done. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:75: CollectionStatus status = Extract(path, &report); On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > You can use histograms here to make sure there's a record of failures. The > Crashpad handler metrics hookup will provide. Done. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:84: base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ | On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > also leave a trace of failure here? Done. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:108: // Either this is not an instrumented process, or the stability file cannot On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > is this going to be true for e.g. renderers and !browser processes? If so maybe > a quick comment is in order? Done. https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.h (right): https://codereview.chromium.org/2867063002/diff/200001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.h:21: class StabilityReportUserStreamDataSource On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > nit: quick comment on the purpose of this class? Done. https://codereview.chromium.org/2867063002/diff/200001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2867063002/diff/200001/components/crash/conte... components/crash/content/app/crashpad.cc:199: embedded_handler); On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > seems strange to use an overload to pad this argument in. all the callers should > be here, why not update them? Done. https://codereview.chromium.org/2867063002/diff/200001/components/crash/conte... File components/crash/content/app/run_as_crashpad_handler_win.cc (right): https://codereview.chromium.org/2867063002/diff/200001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.cc:77: // Note: this stream data source will attempt to collect a stability file On 2017/05/11 15:33:40, Sigurður Ásgeirsson wrote: > Nits: > drop Note: make this a statement. > "collect a stability file information" wonky verbiage :) Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
manzagop@chromium.org changed reviewers: + grt@chromium.org
Hi Greg, Could you comment on the use of install_static::GetUserDataDirectory and the chances of it failing? I'm using it in two places: 1) chrome_crash_reporter_client_win.cc: from the browser process, to plumb it down to the crashpad handler's command line 2) chrome_exe_main_win.cc: from the handler process, to locate the stability file Additionnally, you may want to look at the changes in chrome\installer. Thanks! Pierre-Antoine
Last round of nits, promise! https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:26: void GetStabilityFileName(const base::string16& user_data_dir, Actually, might as well return the file path? https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:46: bool Init() { return is_init_; } Hmmm, this seems contrived and/or not super well named. I'd have gone with an Init(const StabilityReport& report) method, which can return failure. If you want to assert on a precondition, you can test !data_.empty(), as presumably that will never happen on successful serialization. https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:122: // Either this is not an instrumented process (currenlty only browser nit: speling [sic]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CL description seems pretty thin. Could you provide more detail about what this Cl does and why? https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:246: CHECK(install_static::GetUserDataDirectory(&user_data_dir, nullptr)); i think this is wrong. the one true user data dir is cached in chrome_elf, so you should to thunk your way to it like https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?type=.... that's a bit clumsy. an alternative would be to add the user data dir to InstallDetails::Payload and InstallDetails and compute it in install_static::MakeProductDetails. callers in chrome.exe and chrome.dll could then do: install_static::InstallDetails::Get().user_data_dir(); and get the one true user data dir cached by chrome_elf. https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:26: void GetStabilityFileName(const base::string16& user_data_dir, On 2017/05/11 19:28:29, Sigurður Ásgeirsson wrote: > Actually, might as well return the file path? yes, please. we treat FilePath as a value type -- copy elision and RVO make it efficient. https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:32: timeval creation_time{}; nit: chromium prefers assignment when possible (https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...): timeval creation_time = {}; https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:40: class BufferExtensionStreamDataSource final i see precious few classes marked "final" in chromium and don't recall seeing a discussion about it. would you mind removing this and starting a thread on cxx@chromium.org about it? https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: start_arguments.push_back(std::string("--user-data-dir=") + "user-data-dir" is a chromium-ish thing, whereas crashpad is more general purpose, no? should this have a different name? what does the other side of this look like? https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... File components/crash/content/app/run_as_crashpad_handler_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.cc:83: user_stream_data_sources.push_back(base::WrapUnique( base::MakeUnique<browser_watcher::StabilityReportUserStreamDataSource>(user_data_dir) https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... File components/crash/content/app/run_as_crashpad_handler_win.h (right): https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.h:25: const base::string16& user_data_dir, const base::FilePath& here on down
Description was changed from ========== Stability instrumentation Crashpad integration Make use of Crashpad's extensibility mechanism to collect the stability file on crash. The file is deleted if it is successfully collected. BUG=718437 ========== to ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. Details: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 ==========
Description was changed from ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. Details: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 ========== to ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. DETAILS: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 ==========
Description was changed from ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. DETAILS: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 ========== to ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. DETAILS: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Thanks! Addressed comments, but I have some questions. PTAL. https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:246: CHECK(install_static::GetUserDataDirectory(&user_data_dir, nullptr)); On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > i think this is wrong. the one true user data dir is cached in chrome_elf, so > you should to thunk your way to it like > https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?type=.... > > that's a bit clumsy. > > an alternative would be to add the user data dir to InstallDetails::Payload and > InstallDetails and compute it in install_static::MakeProductDetails. callers in > chrome.exe and chrome.dll could then do: > > install_static::InstallDetails::Get().user_data_dir(); > > and get the one true user data dir cached by chrome_elf. I tried the second approach. - I needed to make the mode a parameter to install_static::GetUserDataDirectory to avoid the recursion in InstallDetails. - I put a few "DO NO SUBMIT" in places that still need to be revised: 1) ChromeCrashReporterClient::GetCrashMetricsLocation is called outside elf and calls GetUserDataDirectory. 2) Same with GetCrashDumpLocation. - I wanted your input on what to do if the call fails before changing those other places. https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:26: void GetStabilityFileName(const base::string16& user_data_dir, On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > On 2017/05/11 19:28:29, Sigurður Ásgeirsson wrote: > > Actually, might as well return the file path? > > yes, please. we treat FilePath as a value type -- copy elision and RVO make it > efficient. Done. https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:32: timeval creation_time{}; On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > nit: chromium prefers assignment when possible > (https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...): > timeval creation_time = {}; Done. https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:40: class BufferExtensionStreamDataSource final On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > i see precious few classes marked "final" in chromium and don't recall seeing a > discussion about it. would you mind removing this and starting a thread on > mailto:cxx@chromium.org about it? Validating before sending the email. It seems to be be on the chromium c++ 11 list. https://chromium-cpp.appspot.com/ The thread includes classes but discussion mostly focuses on functions. Shall I double check with an email? https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:46: bool Init() { return is_init_; } On 2017/05/11 19:28:29, Sigurður Ásgeirsson wrote: > Hmmm, this seems contrived and/or not super well named. > I'd have gone with an Init(const StabilityReport& report) method, which can > return failure. > > If you want to assert on a precondition, you can test !data_.empty(), as > presumably that will never happen on successful serialization. Done. https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:122: // Either this is not an instrumented process (currenlty only browser On 2017/05/11 19:28:29, Sigurður Ásgeirsson wrote: > nit: speling [sic] Done. https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: start_arguments.push_back(std::string("--user-data-dir=") + On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > "user-data-dir" is a chromium-ish thing, whereas crashpad is more general > purpose, no? should this have a different name? what does the other side of this > look like? The crash handler on Windows is embedded inside chrome.exe. Within that Chrome wrapper we take note of that user-data-dir argument, use it to create the Crashpad extensibility, then start Crashpad with the extensibility and removing the user-data-dir argument. https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... File components/crash/content/app/run_as_crashpad_handler_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.cc:83: user_stream_data_sources.push_back(base::WrapUnique( On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > base::MakeUnique<browser_watcher::StabilityReportUserStreamDataSource>(user_data_dir) Done. https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... File components/crash/content/app/run_as_crashpad_handler_win.h (right): https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.h:25: const base::string16& user_data_dir, On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > const base::FilePath& here on down Done.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:246: CHECK(install_static::GetUserDataDirectory(&user_data_dir, nullptr)); On 2017/05/12 19:27:32, manzagop wrote: > On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > > i think this is wrong. the one true user data dir is cached in chrome_elf, so > > you should to thunk your way to it like > > > https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?type=.... > > > > that's a bit clumsy. > > > > an alternative would be to add the user data dir to InstallDetails::Payload > and > > InstallDetails and compute it in install_static::MakeProductDetails. callers > in > > chrome.exe and chrome.dll could then do: > > > > install_static::InstallDetails::Get().user_data_dir(); > > > > and get the one true user data dir cached by chrome_elf. > > I tried the second approach. > - I needed to make the mode a parameter to install_static::GetUserDataDirectory > to avoid the recursion in InstallDetails. > - I put a few "DO NO SUBMIT" in places that still need to be revised: 1) > ChromeCrashReporterClient::GetCrashMetricsLocation is called outside elf and > calls GetUserDataDirectory. > 2) Same with GetCrashDumpLocation. > - I wanted your input on what to do if the call fails before changing those > other places. Ah, I see now that the user data dir is only computed/needed in the browser process. Since MakeProductDetails is run for all process types (something I would like to change one day), this maybe isn't the right thing after all. Otherwise, there would be one field in InstallDetails that is only initialized/valid in the browser process, which seems a little icky too. I fear I sent you down the garden path. The thunk may be the best for now. I'm not sure why the thunk is resolved manually at runtime rather than using the linker -- note how GetInstallDetailsPayload is exported from chrome_elf and used in other modules via an explicit dllimport in initialize_from_primary_module.cc. You could simplify the use of the thunk like this by adding a function to //chrome/install_static:secondary_module that is used by chrome.dll to get the paths. the user data dir getter should be in //chrome/install_static:primary_module so that it isn't at risk of being called directly from other modules. apologies for leading you down the garden path with that other idea. https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:40: class BufferExtensionStreamDataSource final On 2017/05/12 19:27:32, manzagop wrote: > On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > > i see precious few classes marked "final" in chromium and don't recall seeing > a > > discussion about it. would you mind removing this and starting a thread on > > mailto:cxx@chromium.org about it? > > Validating before sending the email. It seems to be be on the chromium c++ 11 > list. > https://chromium-cpp.appspot.com/ > The thread includes classes but discussion mostly focuses on functions. > > Shall I double check with an email? Yes. I read through some of the discussions, and everyone seemed to be talking exclusively about final methods rather than final classes. Thanks. https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: start_arguments.push_back(std::string("--user-data-dir=") + On 2017/05/12 19:27:32, manzagop wrote: > On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > > "user-data-dir" is a chromium-ish thing, whereas crashpad is more general > > purpose, no? should this have a different name? what does the other side of > this > > look like? > > The crash handler on Windows is embedded inside chrome.exe. Within that Chrome > wrapper we take note of that user-data-dir argument, use it to create the > Crashpad extensibility, then start Crashpad with the extensibility and removing > the user-data-dir argument. Ah, okay, so this is used only in Chromium's crashpad integration code? SGTM if so.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #11 (id:260001) has been deleted
Replying with a Q about the user-data-dir approach. https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:246: CHECK(install_static::GetUserDataDirectory(&user_data_dir, nullptr)); On 2017/05/15 13:03:18, grt (UTC plus 2) wrote: > On 2017/05/12 19:27:32, manzagop wrote: > > On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > > > i think this is wrong. the one true user data dir is cached in chrome_elf, > so > > > you should to thunk your way to it like > > > > > > https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?type=.... > > > > > > that's a bit clumsy. > > > > > > an alternative would be to add the user data dir to InstallDetails::Payload > > and > > > InstallDetails and compute it in install_static::MakeProductDetails. callers > > in > > > chrome.exe and chrome.dll could then do: > > > > > > install_static::InstallDetails::Get().user_data_dir(); > > > > > > and get the one true user data dir cached by chrome_elf. > > > > I tried the second approach. > > - I needed to make the mode a parameter to > install_static::GetUserDataDirectory > > to avoid the recursion in InstallDetails. > > - I put a few "DO NO SUBMIT" in places that still need to be revised: 1) > > ChromeCrashReporterClient::GetCrashMetricsLocation is called outside elf and > > calls GetUserDataDirectory. > > 2) Same with GetCrashDumpLocation. > > - I wanted your input on what to do if the call fails before changing those > > other places. > > Ah, I see now that the user data dir is only computed/needed in the browser > process. Since MakeProductDetails is run for all process types (something I > would like to change one day), this maybe isn't the right thing after all. > Otherwise, there would be one field in InstallDetails that is only > initialized/valid in the browser process, which seems a little icky too. I fear > I sent you down the garden path. The thunk may be the best for now. > > I'm not sure why the thunk is resolved manually at runtime rather than using the > linker -- note how GetInstallDetailsPayload is exported from chrome_elf and used > in other modules via an explicit dllimport in initialize_from_primary_module.cc. > You could simplify the use of the thunk like this by adding a function to > //chrome/install_static:secondary_module that is used by chrome.dll to get the > paths. the user data dir getter should be in > //chrome/install_static:primary_module so that it isn't at risk of being called > directly from other modules. > > apologies for leading you down the garden path with that other idea. Why do you say the user-data-dir would be not init / invalid in other processes? Do you mean instead that it's useless to compute it in other processes? https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... File components/browser_watcher/stability_report_user_stream_data_source.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/browser_wat... components/browser_watcher/stability_report_user_stream_data_source.cc:40: class BufferExtensionStreamDataSource final On 2017/05/15 13:03:18, grt (UTC plus 2) wrote: > On 2017/05/12 19:27:32, manzagop wrote: > > On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > > > i see precious few classes marked "final" in chromium and don't recall > seeing > > a > > > discussion about it. would you mind removing this and starting a thread on > > > mailto:cxx@chromium.org about it? > > > > Validating before sending the email. It seems to be be on the chromium c++ 11 > > list. > > https://chromium-cpp.appspot.com/ > > The thread includes classes but discussion mostly focuses on functions. > > > > Shall I double check with an email? > > Yes. I read through some of the discussions, and everyone seemed to be talking > exclusively about final methods rather than final classes. Thanks. Sent the email. Done. https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/components/crash/conte... components/crash/content/app/crashpad_win.cc:113: start_arguments.push_back(std::string("--user-data-dir=") + On 2017/05/15 13:03:18, grt (UTC plus 2) wrote: > On 2017/05/12 19:27:32, manzagop wrote: > > On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > > > "user-data-dir" is a chromium-ish thing, whereas crashpad is more general > > > purpose, no? should this have a different name? what does the other side of > > this > > > look like? > > > > The crash handler on Windows is embedded inside chrome.exe. Within that Chrome > > wrapper we take note of that user-data-dir argument, use it to create the > > Crashpad extensibility, then start Crashpad with the extensibility and > removing > > the user-data-dir argument. > > Ah, okay, so this is used only in Chromium's crashpad integration code? SGTM if > so. That is correct.
manzagop@chromium.org changed reviewers: + scottmg@chromium.org
Hi Scott, Greg mentioned you added the GetUserDataDirectory thunking [1] and might be able to offer insight about retrieving the user-data-dir. - Greg initially suggested the user data dir could be accessed via InstallDetails - we're now wondering whether this a good idea after all. Part of the answer is whether it'd be used only in the browser process or all processes. - I get the impression we call GetUserDataDirectory in all processes as InitializeUserDataDir [2] is called in all processes during ChromeMainDelegate::PreSandboxStartup and that we could use install details instead. Do you have thoughts on this? Thanks! Pierre [1] https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?l=391 [2] https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?l=808 https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/220001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:246: CHECK(install_static::GetUserDataDirectory(&user_data_dir, nullptr)); On 2017/05/15 13:43:45, manzagop wrote: > On 2017/05/15 13:03:18, grt (UTC plus 2) wrote: > > On 2017/05/12 19:27:32, manzagop wrote: > > > On 2017/05/11 21:08:16, grt (UTC plus 2) wrote: > > > > i think this is wrong. the one true user data dir is cached in chrome_elf, > > so > > > > you should to thunk your way to it like > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?type=.... > > > > > > > > that's a bit clumsy. > > > > > > > > an alternative would be to add the user data dir to > InstallDetails::Payload > > > and > > > > InstallDetails and compute it in install_static::MakeProductDetails. > callers > > > in > > > > chrome.exe and chrome.dll could then do: > > > > > > > > install_static::InstallDetails::Get().user_data_dir(); > > > > > > > > and get the one true user data dir cached by chrome_elf. > > > > > > I tried the second approach. > > > - I needed to make the mode a parameter to > > install_static::GetUserDataDirectory > > > to avoid the recursion in InstallDetails. > > > - I put a few "DO NO SUBMIT" in places that still need to be revised: 1) > > > ChromeCrashReporterClient::GetCrashMetricsLocation is called outside elf and > > > calls GetUserDataDirectory. > > > 2) Same with GetCrashDumpLocation. > > > - I wanted your input on what to do if the call fails before changing those > > > other places. > > > > Ah, I see now that the user data dir is only computed/needed in the browser > > process. Since MakeProductDetails is run for all process types (something I > > would like to change one day), this maybe isn't the right thing after all. > > Otherwise, there would be one field in InstallDetails that is only > > initialized/valid in the browser process, which seems a little icky too. I > fear > > I sent you down the garden path. The thunk may be the best for now. > > > > I'm not sure why the thunk is resolved manually at runtime rather than using > the > > linker -- note how GetInstallDetailsPayload is exported from chrome_elf and > used > > in other modules via an explicit dllimport in > initialize_from_primary_module.cc. > > You could simplify the use of the thunk like this by adding a function to > > //chrome/install_static:secondary_module that is used by chrome.dll to get the > > paths. the user data dir getter should be in > > //chrome/install_static:primary_module so that it isn't at risk of being > called > > directly from other modules. > > > > apologies for leading you down the garden path with that other idea. > > Why do you say the user-data-dir would be not init / invalid in other processes? > Do you mean instead that it's useless to compute it in other processes? As per offline discussion, the concern is wasted work or work that shouldn't / can't be done (e.g. because of sandboxing).
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/05/15 14:21:35, manzagop wrote: > Hi Scott, > > Greg mentioned you added the GetUserDataDirectory thunking [1] and might be able > to offer insight about retrieving the user-data-dir. > > - Greg initially suggested the user data dir could be > accessed via InstallDetails > - we're now wondering whether this a good idea after all. > Part of the answer is whether it'd be used only > in the browser process or all processes. > - I get the impression we call GetUserDataDirectory in > all processes as InitializeUserDataDir [2] is called in all processes during > ChromeMainDelegate::PreSandboxStartup and that we could > use install details instead. I just cracked the code! User data dir is found for the browser and for cloud-print-service procs only: https://cs.chromium.org/chromium/src/chrome/common/chrome_paths_win.cc?sq=pac... Since we already have this "only for X process types" logic, it seems okay to me to move that into install_static so that MakeProductDetails checks the process type and only initializes the user data dir for the proper types. install_static already has a concept of the process type, so this could be extended as needed. InstallDetails::user_data_dir() (or what have you) could DCHECK the process type. As for the implementation, I think you should ensure that no Chrome code directly calls GetUserDataDirectory -- it should be used solely by install_static during MakeProductDetails. WDYT, Scott?
On 2017/05/16 07:18:25, grt (UTC plus 2) wrote: > On 2017/05/15 14:21:35, manzagop wrote: > > Hi Scott, > > > > Greg mentioned you added the GetUserDataDirectory thunking [1] and might be > able > > to offer insight about retrieving the user-data-dir. > > > > - Greg initially suggested the user data dir could be > > accessed via InstallDetails > > - we're now wondering whether this a good idea after all. > > Part of the answer is whether it'd be used only > > in the browser process or all processes. > > - I get the impression we call GetUserDataDirectory in > > all processes as InitializeUserDataDir [2] is called in all processes during > > ChromeMainDelegate::PreSandboxStartup and that we could > > use install details instead. > > I just cracked the code! User data dir is found for the browser and for > cloud-print-service procs only: > https://cs.chromium.org/chromium/src/chrome/common/chrome_paths_win.cc?sq=pac... > > Since we already have this "only for X process types" logic, it seems okay to me > to move that into install_static so that MakeProductDetails checks the process > type and only initializes the user data dir for the proper types. install_static > already has a concept of the process type, so this could be extended as needed. > InstallDetails::user_data_dir() (or what have you) could DCHECK the process > type. As for the implementation, I think you should ensure that no Chrome code > directly calls GetUserDataDirectory -- it should be used solely by > install_static during MakeProductDetails. > > WDYT, Scott? I think that sounds OK. The only thing to be careful of is that on other platforms all ptypes retrieve it, I believe.
On 2017/05/16 15:42:15, scottmg wrote: > On 2017/05/16 07:18:25, grt (UTC plus 2) wrote: > > On 2017/05/15 14:21:35, manzagop wrote: > > > Hi Scott, > > > > > > Greg mentioned you added the GetUserDataDirectory thunking [1] and might be > > able > > > to offer insight about retrieving the user-data-dir. > > > > > > - Greg initially suggested the user data dir could be > > > accessed via InstallDetails > > > - we're now wondering whether this a good idea after all. > > > Part of the answer is whether it'd be used only > > > in the browser process or all processes. > > > - I get the impression we call GetUserDataDirectory in > > > all processes as InitializeUserDataDir [2] is called in all processes > during > > > ChromeMainDelegate::PreSandboxStartup and that we could > > > use install details instead. > > > > I just cracked the code! User data dir is found for the browser and for > > cloud-print-service procs only: > > > https://cs.chromium.org/chromium/src/chrome/common/chrome_paths_win.cc?sq=pac... > > > > Since we already have this "only for X process types" logic, it seems okay to > me > > to move that into install_static so that MakeProductDetails checks the process > > type and only initializes the user data dir for the proper types. > install_static > > already has a concept of the process type, so this could be extended as > needed. > > InstallDetails::user_data_dir() (or what have you) could DCHECK the process > > type. As for the implementation, I think you should ensure that no Chrome code > > directly calls GetUserDataDirectory -- it should be used solely by > > install_static during MakeProductDetails. > > > > WDYT, Scott? > > I think that sounds OK. The only thing to be careful of is that on other > platforms all ptypes retrieve it, I believe. To make sure I understood: - I need to extend install_static::InitializeProcessType and g_process_type to finer process granularity - I need to make MakeProductDetails have the "only for X process types" logic. Can I take a dependency on chrome/common's ProcessNeedsProfileDir for that?
On 2017/05/16 20:45:52, manzagop wrote: > On 2017/05/16 15:42:15, scottmg wrote: > > On 2017/05/16 07:18:25, grt (UTC plus 2) wrote: > > > On 2017/05/15 14:21:35, manzagop wrote: > > > > Hi Scott, > > > > > > > > Greg mentioned you added the GetUserDataDirectory thunking [1] and might > be > > > able > > > > to offer insight about retrieving the user-data-dir. > > > > > > > > - Greg initially suggested the user data dir could be > > > > accessed via InstallDetails > > > > - we're now wondering whether this a good idea after all. > > > > Part of the answer is whether it'd be used only > > > > in the browser process or all processes. > > > > - I get the impression we call GetUserDataDirectory in > > > > all processes as InitializeUserDataDir [2] is called in all processes > > during > > > > ChromeMainDelegate::PreSandboxStartup and that we could > > > > use install details instead. > > > > > > I just cracked the code! User data dir is found for the browser and for > > > cloud-print-service procs only: > > > > > > https://cs.chromium.org/chromium/src/chrome/common/chrome_paths_win.cc?sq=pac... > > > > > > Since we already have this "only for X process types" logic, it seems okay > to > > me > > > to move that into install_static so that MakeProductDetails checks the > process > > > type and only initializes the user data dir for the proper types. > > install_static > > > already has a concept of the process type, so this could be extended as > > needed. > > > InstallDetails::user_data_dir() (or what have you) could DCHECK the process > > > type. As for the implementation, I think you should ensure that no Chrome > code > > > directly calls GetUserDataDirectory -- it should be used solely by > > > install_static during MakeProductDetails. > > > > > > WDYT, Scott? > > > > I think that sounds OK. The only thing to be careful of is that on other > > platforms all ptypes retrieve it, I believe. > > To make sure I understood: > - I need to extend install_static::InitializeProcessType and g_process_type to > finer process granularity Yes. > - I need to make MakeProductDetails have the "only for X process types" > logic. Can I take a dependency on chrome/common's ProcessNeedsProfileDir for > that? No. install_static can't take a dependency on //chrome/common. ProcessNeedsProfileDir in chrome_paths_win.cc can ask install_static for its answer, though.
On 2017/05/17 08:36:32, grt (UTC plus 2) wrote: > On 2017/05/16 20:45:52, manzagop wrote: > > On 2017/05/16 15:42:15, scottmg wrote: > > > On 2017/05/16 07:18:25, grt (UTC plus 2) wrote: > > > > On 2017/05/15 14:21:35, manzagop wrote: > > > > > Hi Scott, > > > > > > > > > > Greg mentioned you added the GetUserDataDirectory thunking [1] and might > > be > > > > able > > > > > to offer insight about retrieving the user-data-dir. > > > > > > > > > > - Greg initially suggested the user data dir could be > > > > > accessed via InstallDetails > > > > > - we're now wondering whether this a good idea after all. > > > > > Part of the answer is whether it'd be used only > > > > > in the browser process or all processes. > > > > > - I get the impression we call GetUserDataDirectory in > > > > > all processes as InitializeUserDataDir [2] is called in all processes > > > during > > > > > ChromeMainDelegate::PreSandboxStartup and that we could > > > > > use install details instead. > > > > > > > > I just cracked the code! User data dir is found for the browser and for > > > > cloud-print-service procs only: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/common/chrome_paths_win.cc?sq=pac... > > > > > > > > Since we already have this "only for X process types" logic, it seems okay > > to > > > me > > > > to move that into install_static so that MakeProductDetails checks the > > process > > > > type and only initializes the user data dir for the proper types. > > > install_static > > > > already has a concept of the process type, so this could be extended as > > > needed. > > > > InstallDetails::user_data_dir() (or what have you) could DCHECK the > process > > > > type. As for the implementation, I think you should ensure that no Chrome > > code > > > > directly calls GetUserDataDirectory -- it should be used solely by > > > > install_static during MakeProductDetails. > > > > > > > > WDYT, Scott? > > > > > > I think that sounds OK. The only thing to be careful of is that on other > > > platforms all ptypes retrieve it, I believe. > > > > To make sure I understood: > > - I need to extend install_static::InitializeProcessType and g_process_type to > > > finer process granularity > > Yes. > > > - I need to make MakeProductDetails have the "only for X process types" > > logic. Can I take a dependency on chrome/common's ProcessNeedsProfileDir for > > that? > > No. install_static can't take a dependency on //chrome/common. > ProcessNeedsProfileDir in chrome_paths_win.cc can ask install_static for its > answer, though. For reference: I've broken it out to https://chromiumcodereview.appspot.com/2884333004/
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
This now has the change to only initialize the user_data_dir for specific processes. Please have a look.
https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:246: install_static::InstallDetails::Get().user_data_dir(); since this is such a mouthful, i've been putting helpers for these details methods in install_util.{cc,h} -- see, for example, install_static::IsSystemInstall, install_static::GetAppGuid, etc... please do the same (and remove #includes of install_details.h). https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_exe_... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_exe_... chrome/app/chrome_exe_main_win.cc:34: #include "chrome/install_static/user_data_dir.h" unused https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/install_util.cc:536: // DO NOT SUBMIT: GetUserDataDirectory should be called from chrome_elf only. DNS? https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/install_util.cc:542: bool ret = GetUserDataDirectory(InstallDetails::Get().mode(), &user_data_dir, can this just get the dir from InstallDetails now? it will then be safe to use from any module https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/product_install_details.cc:145: // DO NOT SUBMIT: how should we deal with a failure here? based on the comment in GetUserDataDirectoryImpl, we should be sure that there's a CHECK *after* crash handling is set up. +scottmg? https://codereview.chromium.org/2867063002/diff/340001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_main.cc:32: extern "C" void GetUserDataDirectoryThunk(wchar_t* user_data_dir, this thunk isn't needed any more, is it? can its consumers now simply use the value in InstallDetails?
https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. In the CL description "On successful collection the stability file." is missing some words. https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/product_install_details.cc:145: // DO NOT SUBMIT: how should we deal with a failure here? On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > based on the comment in GetUserDataDirectoryImpl, we should be sure that there's > a CHECK *after* crash handling is set up. > > +scottmg? Right, ideally we'd CHECK somewhere later so that a crash could actually be uploaded if this happens. Currently this would be after SignalInitializeCrashReporting() in WinMain().
Description was changed from ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. DETAILS: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. DETAILS: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file is deleted. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
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...
I've addressed comments. Also note: - I've added invalid_user_data_dir to install details - empty user data dir is interpreted as failure - the process type is now auto initialized for simplicity (elf, exe, dll). Should it be added to install details instead? Please have another look! https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_cras... File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/05/19 17:46:52, scottmg wrote: > In the CL description "On successful collection the stability file." is missing > some words. Thanks! ("is deleted.") Done. https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_cras... chrome/app/chrome_crash_reporter_client_win.cc:246: install_static::InstallDetails::Get().user_data_dir(); On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > since this is such a mouthful, i've been putting helpers for these details > methods in install_util.{cc,h} -- see, for example, > install_static::IsSystemInstall, install_static::GetAppGuid, etc... please do > the same (and remove #includes of install_details.h). Done. https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_exe_... File chrome/app/chrome_exe_main_win.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/app/chrome_exe_... chrome/app/chrome_exe_main_win.cc:34: #include "chrome/install_static/user_data_dir.h" On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > unused Done. https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/install_util.cc:536: // DO NOT SUBMIT: GetUserDataDirectory should be called from chrome_elf only. On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > DNS? Done. https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/install_util.cc:542: bool ret = GetUserDataDirectory(InstallDetails::Get().mode(), &user_data_dir, On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > can this just get the dir from InstallDetails now? it will then be safe to use > from any module Yup. The DNS was a reminder of this. Done. https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/product_install_details.cc:145: // DO NOT SUBMIT: how should we deal with a failure here? On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > based on the comment in GetUserDataDirectoryImpl, we should be sure that there's > a CHECK *after* crash handling is set up. > > +scottmg? IIUC this is called before crash reporting is set up [1] and we make use of the result to setup up crash reporting [2]. Do we need to store the success in product details? [1] https://cs.chromium.org/chromium/src/chrome_elf/chrome_elf_main.cc?l=49 [2] https://cs.chromium.org/chromium/src/components/crash/content/app/crashpad_wi... https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/product_install_details.cc:145: // DO NOT SUBMIT: how should we deal with a failure here? On 2017/05/19 17:46:52, scottmg wrote: > On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > > based on the comment in GetUserDataDirectoryImpl, we should be sure that > there's > > a CHECK *after* crash handling is set up. > > > > +scottmg? > > Right, ideally we'd CHECK somewhere later so that a crash could actually be > uploaded if this happens. Currently this would be after > SignalInitializeCrashReporting() in WinMain(). Is an empty string an indicator of invalid user-data-dir? I've seen a few DCHECKs that seem to imply so. Assuming that's the case, I've placed a CHECK(!user_data_dir.empty()) in InitializeUserDataDir which is in PreSandboxStartup. https://codereview.chromium.org/2867063002/diff/340001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_main.cc:32: extern "C" void GetUserDataDirectoryThunk(wchar_t* user_data_dir, On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > this thunk isn't needed any more, is it? can its consumers now simply use the > value in InstallDetails? Done.
https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/product_install_details.cc:145: // DO NOT SUBMIT: how should we deal with a failure here? On 2017/05/19 22:02:07, manzagop wrote: > On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > > based on the comment in GetUserDataDirectoryImpl, we should be sure that > there's > > a CHECK *after* crash handling is set up. > > > > +scottmg? > > IIUC this is called before crash reporting is set up [1] and we make use of the > result to setup up crash reporting [2]. Do we need to store the success in > product details? > > [1] https://cs.chromium.org/chromium/src/chrome_elf/chrome_elf_main.cc?l=49 > [2] > https://cs.chromium.org/chromium/src/components/crash/content/app/crashpad_wi... Wow, this is confusing. I think what you have in ps16 makes sense. Maybe just add a comment to the CHECK in PreSandboxStartup() that an empty string means that no fallback/default user-data-dir could be retrieved either, otherwise it makes it look like the "show a dialog for invalid directory" might not be called. (Also, maybe manually test that `--user-data-dir=q:\bad_drive` does actually work the same as Stable. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2867063002/diff/380001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2867063002/diff/380001/chrome/install_static/... chrome/install_static/install_util.cc:561: return user_data_dir.append(L"\\Crashpad"); this would be bad if |user_data_dir| happened to be empty. is this protected against somehow? https://codereview.chromium.org/2867063002/diff/380001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2867063002/diff/380001/chrome/install_static/... chrome/install_static/product_install_details.cc:146: install_static::GetAndCacheUserDataDirectory(*mode, &user_data_dir, this function no longer needs to do caching. how about removing that and renaming it? https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... components/crash/content/app/crashpad.h:65: const std::string& user_data_dir); please document |user_data_dir|. what is it used for? may it be empty? under what circumstances? https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... components/crash/content/app/crashpad.h:125: const std::string& user_data_dir); please document |user_data_dir| https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... File components/crash/content/app/run_as_crashpad_handler_win.h (right): https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.h:25: const base::FilePath& user_data_dir, please document new args https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.h:25: const base::FilePath& user_data_dir, nit: forward declare FilePath rather than including the header
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! PTAL. https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2867063002/diff/340001/chrome/install_static/... chrome/install_static/product_install_details.cc:145: // DO NOT SUBMIT: how should we deal with a failure here? On 2017/05/19 22:28:06, scottmg (ooo until may23) wrote: > On 2017/05/19 22:02:07, manzagop wrote: > > On 2017/05/19 08:15:50, grt (UTC plus 2) wrote: > > > based on the comment in GetUserDataDirectoryImpl, we should be sure that > > there's > > > a CHECK *after* crash handling is set up. > > > > > > +scottmg? > > > > IIUC this is called before crash reporting is set up [1] and we make use of > the > > result to setup up crash reporting [2]. Do we need to store the success in > > product details? > > > > [1] https://cs.chromium.org/chromium/src/chrome_elf/chrome_elf_main.cc?l=49 > > [2] > > > https://cs.chromium.org/chromium/src/components/crash/content/app/crashpad_wi... > > Wow, this is confusing. I think what you have in ps16 makes sense. Maybe just > add a comment to the CHECK in PreSandboxStartup() that an empty string means > that no fallback/default user-data-dir could be retrieved either, otherwise it > makes it look like the "show a dialog for invalid directory" might not be > called. Done. > (Also, maybe manually test that `--user-data-dir=q:\bad_drive` does actually > work the same as Stable. :) Done. I get a dialog and it falls back to default. https://codereview.chromium.org/2867063002/diff/380001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2867063002/diff/380001/chrome/install_static/... chrome/install_static/install_util.cc:561: return user_data_dir.append(L"\\Crashpad"); On 2017/05/22 10:16:12, grt (UTC plus 2) wrote: > this would be bad if |user_data_dir| happened to be empty. is this protected > against somehow? Done. https://codereview.chromium.org/2867063002/diff/380001/chrome/install_static/... File chrome/install_static/product_install_details.cc (right): https://codereview.chromium.org/2867063002/diff/380001/chrome/install_static/... chrome/install_static/product_install_details.cc:146: install_static::GetAndCacheUserDataDirectory(*mode, &user_data_dir, On 2017/05/22 10:16:12, grt (UTC plus 2) wrote: > this function no longer needs to do caching. how about removing that and > renaming it? Done. https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... components/crash/content/app/crashpad.h:65: const std::string& user_data_dir); On 2017/05/22 10:16:12, grt (UTC plus 2) wrote: > please document |user_data_dir|. what is it used for? may it be empty? under > what circumstances? Done. https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... components/crash/content/app/crashpad.h:125: const std::string& user_data_dir); On 2017/05/22 10:16:12, grt (UTC plus 2) wrote: > please document |user_data_dir| Done. https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... File components/crash/content/app/run_as_crashpad_handler_win.h (right): https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.h:25: const base::FilePath& user_data_dir, On 2017/05/22 10:16:12, grt (UTC plus 2) wrote: > nit: forward declare FilePath rather than including the header Done. https://codereview.chromium.org/2867063002/diff/380001/components/crash/conte... components/crash/content/app/run_as_crashpad_handler_win.h:25: const base::FilePath& user_data_dir, On 2017/05/22 10:16:12, grt (UTC plus 2) wrote: > please document new args Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_...)
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...
https://codereview.chromium.org/2867063002/diff/400001/chrome/install_static/... File chrome/install_static/user_data_dir.h (right): https://codereview.chromium.org/2867063002/diff/400001/chrome/install_static/... chrome/install_static/user_data_dir.h:38: // derivation inconsistencies. |invalid_user_data_directory| may be null if not If the function is only supposed to be called once, it doesn't make much sense for there to be an optional argument, does it?
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Thanks! Another look? https://codereview.chromium.org/2867063002/diff/400001/chrome/install_static/... File chrome/install_static/user_data_dir.h (right): https://codereview.chromium.org/2867063002/diff/400001/chrome/install_static/... chrome/install_static/user_data_dir.h:38: // derivation inconsistencies. |invalid_user_data_directory| may be null if not On 2017/05/23 15:25:02, grt (UTC plus 2) wrote: > If the function is only supposed to be called once, it doesn't make much sense > for there to be an optional argument, does it? And it's now become incorrect. Updated comment. Done.
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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... headless/app/headless_shell.cc:553: ::switches::kProcessType, switches::kUserDataDir); uber-nit: "::switches::kUserDataDir" for consistency with kProcessType
Thanks for the review Greg! Did someone else want another pass? https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... headless/app/headless_shell.cc:553: ::switches::kProcessType, switches::kUserDataDir); On 2017/05/24 06:58:49, grt (UTC plus 2) wrote: > uber-nit: "::switches::kUserDataDir" for consistency with kProcessType Actually, that second one is in the headless namespace (headless::switches::kUserDataDir from headless/app/headless_shell_switches.h). Should I fully qualify it to make it clearer?
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...
manzagop@chromium.org changed reviewers: + alexclarke@chromium.org, rkaplow@chromium.org
Hi Alex, Rob, Robert! Could you have an OWNERS' look? - Robert: chrome_elf/* - RobK: tools\metrics\histograms\histograms.xml - Alex: headless/* Thanks! Pierre-Antoine
https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... headless/app/headless_shell.cc:553: ::switches::kProcessType, switches::kUserDataDir); On 2017/05/24 13:19:53, manzagop wrote: > On 2017/05/24 06:58:49, grt (UTC plus 2) wrote: > > uber-nit: "::switches::kUserDataDir" for consistency with kProcessType > > Actually, that second one is in the headless namespace > (headless::switches::kUserDataDir from headless/app/headless_shell_switches.h). > Should I fully qualify it to make it clearer? Ha. Hrm. I'd be inclined to remove :: from the first, but I'm okay with it as-is.
headless/ LGTM % nit https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... headless/app/headless_shell.cc:553: ::switches::kProcessType, switches::kUserDataDir); On 2017/05/24 13:35:02, grt (UTC plus 2) wrote: > On 2017/05/24 13:19:53, manzagop wrote: > > On 2017/05/24 06:58:49, grt (UTC plus 2) wrote: > > > uber-nit: "::switches::kUserDataDir" for consistency with kProcessType > > > > Actually, that second one is in the headless namespace > > (headless::switches::kUserDataDir from > headless/app/headless_shell_switches.h). > > Should I fully qualify it to make it clearer? > > Ha. Hrm. I'd be inclined to remove :: from the first, but I'm okay with it > as-is. I don't know why this code is using :: If it compiles without I'd prefer that, but its not a big deal :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Alex: quick validation for you about the nit. :) https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... headless/app/headless_shell.cc:553: ::switches::kProcessType, switches::kUserDataDir); On 2017/05/24 13:45:06, alex clarke wrote: > On 2017/05/24 13:35:02, grt (UTC plus 2) wrote: > > On 2017/05/24 13:19:53, manzagop wrote: > > > On 2017/05/24 06:58:49, grt (UTC plus 2) wrote: > > > > uber-nit: "::switches::kUserDataDir" for consistency with kProcessType > > > > > > Actually, that second one is in the headless namespace > > > (headless::switches::kUserDataDir from > > headless/app/headless_shell_switches.h). > > > Should I fully qualify it to make it clearer? > > > > Ha. Hrm. I'd be inclined to remove :: from the first, but I'm okay with it > > as-is. > > I don't know why this code is using :: > If it compiles without I'd prefer that, but its not a big deal :) Hm, I think the author's intent was to highlight this is coming from content/public/common/content_switches.h, and not headless::switches. Based on that, is your preference still the same? I'm happy to go with either.
https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2867063002/diff/460001/headless/app/headless_... headless/app/headless_shell.cc:553: ::switches::kProcessType, switches::kUserDataDir); On 2017/05/24 15:40:29, manzagop wrote: > On 2017/05/24 13:45:06, alex clarke wrote: > > On 2017/05/24 13:35:02, grt (UTC plus 2) wrote: > > > On 2017/05/24 13:19:53, manzagop wrote: > > > > On 2017/05/24 06:58:49, grt (UTC plus 2) wrote: > > > > > uber-nit: "::switches::kUserDataDir" for consistency with kProcessType > > > > > > > > Actually, that second one is in the headless namespace > > > > (headless::switches::kUserDataDir from > > > headless/app/headless_shell_switches.h). > > > > Should I fully qualify it to make it clearer? > > > > > > Ha. Hrm. I'd be inclined to remove :: from the first, but I'm okay with it > > > as-is. > > > > I don't know why this code is using :: > > If it compiles without I'd prefer that, but its not a big deal :) > > Hm, I think the author's intent was to highlight this is coming from > content/public/common/content_switches.h, and not headless::switches. Based on > that, is your preference still the same? I'm happy to go with either. Ah yes you're probably right. Lets leave it as is then.
manzagop@chromium.org changed reviewers: + robertshield@chromium.org
+Robert Shield for realz Hi Robert, Could you have an OWNERS' look for chrome_elf? Thanks! Pierre-Antoine
chrome_elf LGTM + quick suggestion for minor cleanup https://codereview.chromium.org/2867063002/diff/460001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2867063002/diff/460001/chrome/install_static/... chrome/install_static/install_util.cc:530: bool IsProcessTypeInitialized() { actually, could we get rid of this and inline it into EnsureProjectTypeIsInitialized()? https://codereview.chromium.org/2867063002/diff/460001/chrome/install_static/... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2867063002/diff/460001/chrome/install_static/... chrome/install_static/install_util.h:152: bool IsProcessTypeInitialized(); It's not clear to me that this needs to be in the public interface anymore.
lgtm histograms lg
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Thanks for the reviews! Scott, Mark: is this CL OK with you? https://codereview.chromium.org/2867063002/diff/460001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2867063002/diff/460001/chrome/install_static/... chrome/install_static/install_util.cc:530: bool IsProcessTypeInitialized() { On 2017/05/24 18:10:48, robertshield wrote: > actually, could we get rid of this and inline it into > EnsureProjectTypeIsInitialized()? Done. https://codereview.chromium.org/2867063002/diff/460001/chrome/install_static/... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2867063002/diff/460001/chrome/install_static/... chrome/install_static/install_util.h:152: bool IsProcessTypeInitialized(); On 2017/05/24 18:10:48, robertshield wrote: > It's not clear to me that this needs to be in the public interface anymore. Done.
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_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 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: win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)
lgtm I'm not sure how we'll do this for non-Windows, but this looks fine for now.
LGTM as well. I only focused primarily on components/crash.
Thanks for the review!
I've made a final manual test. I see no issues generating a crash or reading it (chrome://inducebrowsercrashforrealz) whether the experiment is on or off. Going ahead with submission.
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, robertshield@chromium.org, alexclarke@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2867063002/#ps520001 (title: "merge")
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": 520001, "attempt_start_ts": 1495740928197270,
"parent_rev": "20e172c34b3685b8f8e6d212c866742ba729d007", "commit_rev":
"fecdf607e706474b65a4d26e554f0504aead3f3e"}
Message was sent while issue was closed.
Description was changed from ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. DETAILS: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file is deleted. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Stability instrumentation Crashpad integration The stability instrumentation is about Chrome recording internal state to a memory mapped file as it is running. It is currently used experimentally on Windows only. This CL makes use of Crashpad's extensibility mechanism to collect information from the stability file on crash, and include it within the crash report as an additional minidump user stream. DETAILS: - The browser process now passes the user data dir to the crash handler's command line. - The handler process creates the Crashpad extension that will search for a stability file on any crash. If the file is found (currently only possible for instrumented browser processes), it is collected and the information is added to the crash report. On successful collection the stability file is deleted. TESTING - launch chrome with the stability instrumentation: chrome.exe --user-data-dir=<data-dir> --enable-features=StabilityDebugging - validate the presence of the stability file in <data-dir>/Stability - trigger a crash (visit chrome://inducebrowsercrashforrealz) - validate the deletion of the the stability file in <data-dir>/Stability - validate the presence of a crash report in <data-dir>/Crashpad/Reports and ensure it contains the additional stream, either with dumpchk.exe or the dump_stability build target. BUG=718437 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2867063002 Cr-Commit-Position: refs/heads/master@{#474759} Committed: https://chromium.googlesource.com/chromium/src/+/fecdf607e706474b65a4d26e554f... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:520001) as https://chromium.googlesource.com/chromium/src/+/fecdf607e706474b65a4d26e554f... |
