Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221833005:
Adding support for sampling crashes in Chrome on Windows. (Closed)
|
Created:
4 years, 4 months ago by jwd Modified:
4 years, 4 months ago Reviewers:
rkaplow, Lei Zhang, ananta, scottmg, Mark Mentovai, robertshield, Alexei Svitkine (slow), Ilya Sherman CC:
chromium-reviews, sadrul, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, kalyank, gayane -on leave until 09-2017 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding support for sampling crashes in Chrome on Windows.
BUG=609987
Committed: https://crrev.com/c882e48d0f01744dd5e9de83395f45baa38ce731
Cr-Commit-Position: refs/heads/master@{#412984}
Patch Set 1 #Patch Set 2 : Adding support for sampling crashes in Chrome on Windows. #Patch Set 3 : Adding support for sampling crashes in Chrome on Windows. #
Total comments: 38
Patch Set 4 : rkaplow's comments #
Total comments: 18
Patch Set 5 : Alexei's and Marks comments #Patch Set 6 : scottmg's comments #
Total comments: 25
Patch Set 7 : Adding support for sampling crashes in Chrome on Windows. #Patch Set 8 : Straggler comments #Patch Set 9 : rename, move enable checks #
Total comments: 17
Patch Set 10 : Alexei's and Mark's comments, including function rename #Patch Set 11 : fixing type #
Total comments: 8
Patch Set 12 : scottmg's comments, adding DCHECK #Messages
Total messages: 62 (21 generated)
The CQ bit was checked by jwd@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...
jwd@chromium.org changed reviewers: + asvitkine@chromium.org, mark@chromium.org, rkaplow@chromium.org
mark@ for crashpad related work rkaplow@ for metrics side of things asvitkine@ for opinions if available
https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:45: // Registry key for sampling state. This is uesd to communicate with crashpad. used https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:46: const wchar_t kRegUsageStatsInSample[] = L"usagestatsinsample"; is lowercasestyle the right style for keys? Poking around, it seems to mostly be UpperCamelCase... ex https://codesearch.chromium.org/chromium/src/components/startup_metric_utils/... or https://codesearch.chromium.org/chromium/src/chrome/browser/metrics/chrome_me... https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:203: base::win::RegKey reg_key(HKEY_CURRENT_USER, registry_path.c_str(), it looks like we can check for creation success of a regkey, ex. https://codesearch.chromium.org/chromium/src/components/metrics/clean_exit_be... maybe we can use that sample pattern here for safety https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:208: // Next, get crashpad to pickup the sampling state for this run. what do you mean here 'this run'? Do you mean session? https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:213: GetProcAddress(elf_module, "SetUploadsEnabledImpl")); should we define this as a const and make a note how this is synced with the string in crashpad? https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:219: // This isn't a problem though, since they will be consistent. hm... not fully following the point of the comment. We're setting the reg key to store the value - so why do we need to pass the bool in the call here? https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:59: // On windows, the client controls if crashpad can upload crash reports. nit: Windows https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.h:84: // Returns false if the current executable is affected by stats collection this is ok, but IMO would be more intuitive as Returns true if the current executable is in the chosen sample that will report stats and crashes. Especially since everything else here is discussing the true output, and currently there's a bit of double negation..
https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:206: reg_key.WriteValue(kRegUsageStatsInSample, IsClientInSample() ? 1 : 0); Can the registry code be a helper function in installer_util so that the code that gets and sets the key is in the same place? https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:58: const wchar_t kChromeMetrics[] = L"\\MetricsReporting"; Are just using this for the one sub-key? Seems strange given that usagestats above isn't under this. I would just put the key below next to usagestats and not have the intermediate dir. Unless there's a good reason for this grouping? https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:59: const wchar_t kChromeStatsSampleKey[] = L"usagestatsinsample"; I know there's no comments here currently, but please add some about this to explain what this means and what possible values it can have. Or in the header file I guess. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:278: std::wstring registry_path = L"Software\\"; Seems strange to have this hardcoded. Can this be a constant? Or maybe a function that returns the chrome install registry path so each function doesn't need to append it themselves. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:285: out_value == 1); What happens if the registry key isn't set? Would this return "not in sample" - which seems not correct. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.h:45: extern const wchar_t kChromeStatsSampleKey[]; Are you missing the bit that sets these? https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... File components/crash/content/app/crash_reporter_client.cc (right): https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crash_reporter_client.cc:126: return true; Maybe add a comment about why true as default value makes sense. https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crash_reporter_client.h:142: // crashes. Otherwise, returns true. Please expand comment to mention interaction with GetCollectStatsConsent(). Should this only be queried if that function returns true? https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); Maybe I didn't follow all the discussion, but what's the reason for having both GetCollectStatsConsent() and GetCollectStatsInSample() exposed? (as opposed to something like ShouldUploadCrashes() where the implementation can check both of those). If there's a good reason, worth mentioning somewhere in a comment. https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crashpad.cc:465: // changes to if crash uploads are enabled. The last bit of this sentence is a bit hard to understand. I think you're talking specifically about this setting changing at runtime (e.g. if user toggles their checkbox) - is that right? What does "if crash uploads are enabled" mean if it also takes a param? Please rephrase comment and clarify things.
https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:45: // Registry key for sampling state. This is uesd to communicate with crashpad. On 2016/08/10 20:16:28, rkaplow wrote: > used Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:46: const wchar_t kRegUsageStatsInSample[] = L"usagestatsinsample"; On 2016/08/10 20:16:28, rkaplow wrote: > is lowercasestyle the right style for keys? Poking around, it seems to mostly be > UpperCamelCase... ex > > https://codesearch.chromium.org/chromium/src/components/startup_metric_utils/... > > or > https://codesearch.chromium.org/chromium/src/chrome/browser/metrics/chrome_me... hm, I was using the usagestats key as example. Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:203: base::win::RegKey reg_key(HKEY_CURRENT_USER, registry_path.c_str(), On 2016/08/10 20:16:28, rkaplow wrote: > it looks like we can check for creation success of a regkey, ex. > > https://codesearch.chromium.org/chromium/src/components/metrics/clean_exit_be... > > maybe we can use that sample pattern here for safety Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:208: // Next, get crashpad to pickup the sampling state for this run. On 2016/08/10 20:16:28, rkaplow wrote: > what do you mean here 'this run'? Do you mean session? yes, went with 'this run' as in the same vein as "first run". Changed to session. https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:213: GetProcAddress(elf_module, "SetUploadsEnabledImpl")); On 2016/08/10 20:16:28, rkaplow wrote: > should we define this as a const and make a note how this is synced with the > string in crashpad? Sure, why not. https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:219: // This isn't a problem though, since they will be consistent. On 2016/08/10 20:16:28, rkaplow wrote: > hm... not fully following the point of the comment. > > We're setting the reg key to store the value - so why do we need to pass the > bool in the call here? set_crash_enabled needs a bool argument to be able to turn crash off, mirroring the SetUploadsEnabled function in crashpad.cc. The value being passed here doesn't need to take the sampling state into account, it just so happens that it does, and it's simpler to use it as is. The comment was intended to point out the redundancy, and say it's ok. https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:59: // On windows, the client controls if crashpad can upload crash reports. On 2016/08/10 20:16:28, rkaplow wrote: > nit: Windows Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.h:84: // Returns false if the current executable is affected by stats collection On 2016/08/10 20:16:29, rkaplow wrote: > this is ok, but IMO would be more intuitive as > > > > Returns true if the current executable is in the chosen sample that will report > stats and crashes. > > Especially since everything else here is discussing the true output, and > currently there's a bit of double negation.. I agree the double negation is undesirable. I don't really like your comment though because I don't like referring to clients that aren't affected by sampling as "in the chosen sample". Note that in Chrome, there is a functional difference between clients that aren't affected by sampling, and those that are, and are in the sample. Namely in-sample clients report a variation and sample rate, whereas unaffected reporting clients don't. But, I don't feel that strongly, so I'll change it.
Patchset #4 (id:60001) has been deleted
mark@chromium.org changed reviewers: + scottmg@chromium.org
+scottmg, as he was more involved in the chrome_elf Crashpad split. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:45: // Registry key for sampling state. This is used to communicate with crashpad. Capital C here and on line 213 too. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right): https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:59: // On Windows, the client controls if crashpad can upload crash reports. if→whether https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:59: // On Windows, the client controls if crashpad can upload crash reports. Crashpad, proper name, capital C https://codereview.chromium.org/2221833005/diff/80001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/80001/components/crash/conten... components/crash/content/app/crashpad.cc:465: // changes to if crash uploads are enabled. trigger changes to the upload-enabled state
scottmg@chromium.org changed reviewers: + ananta@chromium.org
+ananta fyi for install_static. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:45: // Registry key for sampling state. This is used to communicate with crashpad. and line 202 https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:210: ERROR_SUCCESS) nit; {} around multiline if https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:213: // Next, get crashpad to pickup the sampling state for this session. pick up https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:224: // This isn't a problem though, since they will be consistent. It seems like instead of this comment, it'd be better just to tell the chrome_elf side to "check again", so that we don't need to retrieve and then send over the value that we expect to be the same. Would that work? https://codereview.chromium.org/2221833005/diff/80001/chrome/install_static/i... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/80001/chrome/install_static/i... chrome/install_static/install_util.h:84: // Returns true if the current executable is curretnly in the chosen sample that currently
https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:206: reg_key.WriteValue(kRegUsageStatsInSample, IsClientInSample() ? 1 : 0); On 2016/08/10 21:49:33, Alexei Svitkine (very slow) wrote: > Can the registry code be a helper function in installer_util so that the code > that gets and sets the key is in the same place? Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:58: const wchar_t kChromeMetrics[] = L"\\MetricsReporting"; On 2016/08/10 21:49:33, Alexei Svitkine (very slow) wrote: > Are just using this for the one sub-key? Seems strange given that usagestats > above isn't under this. > > I would just put the key below next to usagestats and not have the intermediate > dir. Unless there's a good reason for this grouping? Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:59: const wchar_t kChromeStatsSampleKey[] = L"usagestatsinsample"; On 2016/08/10 21:49:33, Alexei Svitkine (very slow) wrote: > I know there's no comments here currently, but please add some about this to > explain what this means and what possible values it can have. Or in the header > file I guess. Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:278: std::wstring registry_path = L"Software\\"; On 2016/08/10 21:49:33, Alexei Svitkine (very slow) wrote: > Seems strange to have this hardcoded. Can this be a constant? Or maybe a > function that returns the chrome install registry path so each function doesn't > need to append it themselves. Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:285: out_value == 1); On 2016/08/10 21:49:33, Alexei Svitkine (very slow) wrote: > What happens if the registry key isn't set? Would this return "not in sample" - > which seems not correct. Done. https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/40001/chrome/install_static/i... chrome/install_static/install_util.h:45: extern const wchar_t kChromeStatsSampleKey[]; On 2016/08/10 21:49:33, Alexei Svitkine (very slow) wrote: > Are you missing the bit that sets these? Moved setting from chrome_metrics_services_client to here. https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... File components/crash/content/app/crash_reporter_client.cc (right): https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crash_reporter_client.cc:126: return true; On 2016/08/10 21:49:33, Alexei Svitkine (very slow) wrote: > Maybe add a comment about why true as default value makes sense. Done. https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crash_reporter_client.h:142: // crashes. Otherwise, returns true. On 2016/08/10 21:49:33, Alexei Svitkine (very slow) wrote: > Please expand comment to mention interaction with GetCollectStatsConsent(). > Should this only be queried if that function returns true? Done. https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); On 2016/08/10 21:49:34, Alexei Svitkine (very slow) wrote: > Maybe I didn't follow all the discussion, but what's the reason for having both > GetCollectStatsConsent() and GetCollectStatsInSample() exposed? (as opposed to > something like ShouldUploadCrashes() where the implementation can check both of > those). > > If there's a good reason, worth mentioning somewhere in a comment. My understanding is that SetUploadsEnabled expects to see the value of the checkbox. A ShouldUploadCrashes instead of both a consent and InSample would mean either (1)making that check in SetUploadsEnabled, probably removing the bool argument, or (2)doing no check in SetUploadsEnabled and just passing in the right value. (2) was explicitly rejected by Mark (1) seems implicitly rejected by Mark, and would involve complexity w.r.t. modifying the initialization logic. In conclusion, I don't have a good reason. https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crashpad.cc:465: // changes to if crash uploads are enabled. On 2016/08/10 21:49:34, Alexei Svitkine (very slow) wrote: > The last bit of this sentence is a bit hard to understand. I think you're > talking specifically about this setting changing at runtime (e.g. if user > toggles their checkbox) - is that right? > > What does "if crash uploads are enabled" mean if it also takes a param? > > Please rephrase comment and clarify things. Done. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:45: // Registry key for sampling state. This is used to communicate with crashpad. On 2016/08/11 18:32:24, Mark Mentovai wrote: > Capital C here and on line 213 too. Done. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.h (right): https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:59: // On Windows, the client controls if crashpad can upload crash reports. On 2016/08/11 18:32:25, Mark Mentovai wrote: > Crashpad, proper name, capital C Done. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.h:59: // On Windows, the client controls if crashpad can upload crash reports. On 2016/08/11 18:32:25, Mark Mentovai wrote: > if→whether Done. https://codereview.chromium.org/2221833005/diff/80001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/80001/components/crash/conten... components/crash/content/app/crashpad.cc:465: // changes to if crash uploads are enabled. On 2016/08/11 18:32:25, Mark Mentovai wrote: > trigger changes to the upload-enabled state Done.
https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); On 2016/08/11 21:41:03, jwd wrote: > On 2016/08/10 21:49:34, Alexei Svitkine (very slow) wrote: > > Maybe I didn't follow all the discussion, but what's the reason for having > both > > GetCollectStatsConsent() and GetCollectStatsInSample() exposed? (as opposed to > > something like ShouldUploadCrashes() where the implementation can check both > of > > those). > > > > If there's a good reason, worth mentioning somewhere in a comment. > > My understanding is that SetUploadsEnabled expects to see the value of the > checkbox. A ShouldUploadCrashes instead of both a consent and InSample would > mean either (1)making that check in SetUploadsEnabled, probably removing the > bool argument, or (2)doing no check in SetUploadsEnabled and just passing in the > right value. > > (2) was explicitly rejected by Mark > (1) seems implicitly rejected by Mark, and would involve complexity w.r.t. > modifying the initialization logic. > > In conclusion, I don't have a good reason. Mark, could you clarify your thoughts on this? Would it be simpler to just have a single ShouldUploadCrashes() crashpad client API or is there a good reason for having them split?
https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:45: // Registry key for sampling state. This is used to communicate with crashpad. On 2016/08/11 21:38:03, scottmg wrote: > and line 202 Done. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:210: ERROR_SUCCESS) On 2016/08/11 21:38:03, scottmg wrote: > nit; {} around multiline if Done. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:213: // Next, get crashpad to pickup the sampling state for this session. On 2016/08/11 21:38:03, scottmg wrote: > pick up Done. https://codereview.chromium.org/2221833005/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:224: // This isn't a problem though, since they will be consistent. On 2016/08/11 21:38:03, scottmg wrote: > It seems like instead of this comment, it'd be better just to tell the > chrome_elf side to "check again", so that we don't need to retrieve and then > send over the value that we expect to be the same. Would that work? Not too sure exactly what you mean. The value passed to set_crash_enabled won't always be the same as the sampling value, it also has the consent baked in. It just happens that, by this point, MetricsServicesManager[Client] expects may_record to be consent given && in sample. Are you suggesting a new "check again" function in crashpad.cc and not call SetUploadsEnabled here? To be strictly right, it would need to apply all the same logic that InitializeCrashpadImpl does for the enable_uploads value. This seems like more work since it will have to retrieve the consent bit from registry and check policy and such. https://codereview.chromium.org/2221833005/diff/80001/chrome/install_static/i... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/80001/chrome/install_static/i... chrome/install_static/install_util.h:84: // Returns true if the current executable is curretnly in the chosen sample that On 2016/08/11 21:38:03, scottmg wrote: > currently Done.
https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/40001/components/crash/conten... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); On 2016/08/11 21:46:34, Alexei Svitkine (very slow) wrote: > On 2016/08/11 21:41:03, jwd wrote: > > On 2016/08/10 21:49:34, Alexei Svitkine (very slow) wrote: > > > Maybe I didn't follow all the discussion, but what's the reason for having > > both > > > GetCollectStatsConsent() and GetCollectStatsInSample() exposed? (as opposed > to > > > something like ShouldUploadCrashes() where the implementation can check both > > of > > > those). > > > > > > If there's a good reason, worth mentioning somewhere in a comment. > > > > My understanding is that SetUploadsEnabled expects to see the value of the > > checkbox. A ShouldUploadCrashes instead of both a consent and InSample would > > mean either (1)making that check in SetUploadsEnabled, probably removing the > > bool argument, or (2)doing no check in SetUploadsEnabled and just passing in > the > > right value. > > > > (2) was explicitly rejected by Mark > > (1) seems implicitly rejected by Mark, and would involve complexity w.r.t. > > modifying the initialization logic. > > > > In conclusion, I don't have a good reason. > > Mark, could you clarify your thoughts on this? Would it be simpler to just have > a single ShouldUploadCrashes() crashpad client API or is there a good reason for > having them split? SetUploadsEnabled sees the value of the checkbox. It could be turned into something that sees the value of the checkbox && !SampledOut, but then every caller would need to do checkbox && !SampledOut, or that logic would need to be consolidated somewhere else. There are presently two callers, and one of them is in this file, so this file needs to know something about sampling either way. To me, that makes this file, and this function in particular, the ideal place to do the consolidation. In the near future, there will be at least one other caller into here (the sad tab page), so the need for consolidation of logic will only increase over time. I think that the way it's written now provides good consolidation and prevents misusing the interface by people who don't understand all of the logical bits that go into the decision we tell Crashpad about. But if you've got an alternative that addresses logic centralization and API use, I'll entertain it. I made my recommendations based solely on the Crashpad side of things, but once UMA is mixed in, what's best might change.
jwd@chromium.org changed reviewers: + robertshield@chromium.org
+robertshield@ for install_static
https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.cc:58: const wchar_t kChromeStatsSampleKey[] = L"UsageStatsInSample"; How about using a similar variable naming convention as kRegValueUsageStats? e.g. kRegValueUsageStatsInSample (Although, I guess this convention isn't constently followed - still I prefer you at least make it match some existing style rather than adding a whole new one.) https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.cc:430: // implicitly meaning this install in in-sample. Nit: "this install in sample" -> "this install is in the sample" https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.h:50: extern const wchar_t kChromeStatsSampleKey[]; Since this is no longer used outside of the .cc file, I would remove it from the .h file and put it in an anon namespace in the .cc file with the comment. https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:469: crash_reporter::SetUploadsEnabled(enable_uploads); So this will be called as a result of UpdateRunningServices() in metrics code. At which point, it should override whatever was done by line 278, right? Could there be an inconsistency there? For example, I see the line 278 code has some extra checks, like: - should_initialize_database_and_set_upload_policy - crash_reporter_client->ReportingIsEnforcedByPolicy() - crash_reporter_client->IsRunningUnattended() Seems like all those things that may have caused line 278 previously to not enable uploads may suddenly get overridden here. Am I reading it right? If so, it seems like an issue. Can we make the two go through the same code path that checks all these things?
Some structural thoughts about exposing multiple bits on CrashReporterClient that determine whether uploads are enabled. I, and others who have worked on the installer, have spent long hours over the years untangling the logic to determine whether crash reporting is enabled or not, I would try to keep that logic centralized as much as possible. https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:46: const char kCrashpadSetEnabledFunctionName[] = "SetUploadsEnabledImpl"; s/through/exported by/ https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:207: }(); This is harder to read than: HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName); static SetCrashUploadsEnabledPointer set_crash_enabled = reinterpret_cast<SetCrashUploadsEnabledPointer>( GetProcAddress(elf_module, kCrashpadSetEnabledFunctionName)); https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.h:46: // otherwise, uploads will be dissabled. It is used to sample clients, to reduce s/dissabled/disabled/ https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.h:50: extern const wchar_t kChromeStatsSampleKey[]; What is the relationship between the value stored here and the value stored at kRegValueUsageStats which also seems to control UMA and crash reporting? https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:144: virtual bool GetCollectStatsInSample(); It seems weird to me that consumers need to know about implementation details to figure out whether to upload crashes or not. I'm guessing that anywhere this is used, callers will have to call GetCollectStatsConsent() && GetCollectStatsInSample() to determine whether to upload crashes? That seems like odd leakage. https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); The logic for whether uploads are enabled or not is now split between SetUploadsEnabled() and InitializeCrashpadImpl(). I think it should all be in InitializeCrashpadImpl(). It's surprising that SetUploadsEnabled(true) might not actually enable uploads.
https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:46: const char kCrashpadSetEnabledFunctionName[] = "SetUploadsEnabledImpl"; On 2016/08/12 17:49:25, robertshield wrote: > s/through/exported by/ Done. https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:207: }(); On 2016/08/12 17:49:25, robertshield wrote: > This is harder to read than: > > HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName); > static SetCrashUploadsEnabledPointer set_crash_enabled = > reinterpret_cast<SetCrashUploadsEnabledPointer>( > GetProcAddress(elf_module, kCrashpadSetEnabledFunctionName)); I agree, and had to spend time understanding what was going on in the code that inspired this (in crash_upload_list_crashpad.cc), but is there no significant cost to getting the handle? https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.cc:58: const wchar_t kChromeStatsSampleKey[] = L"UsageStatsInSample"; On 2016/08/12 17:47:48, Alexei Svitkine (very slow) wrote: > How about using a similar variable naming convention as kRegValueUsageStats? > > e.g. kRegValueUsageStatsInSample > > (Although, I guess this convention isn't constently followed - still I prefer > you at least make it match some existing style rather than adding a whole new > one.) Done. https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.cc:430: // implicitly meaning this install in in-sample. On 2016/08/12 17:47:48, Alexei Svitkine (very slow) wrote: > Nit: "this install in sample" -> "this install is in the sample" Done. https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... File chrome/install_static/install_util.h (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.h:46: // otherwise, uploads will be dissabled. It is used to sample clients, to reduce On 2016/08/12 17:49:25, robertshield wrote: > s/dissabled/disabled/ Done. https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.h:50: extern const wchar_t kChromeStatsSampleKey[]; On 2016/08/12 17:47:48, Alexei Svitkine (very slow) wrote: > Since this is no longer used outside of the .cc file, I would remove it from the > .h file and put it in an anon namespace in the .cc file with the comment. Done. https://codereview.chromium.org/2221833005/diff/120001/chrome/install_static/... chrome/install_static/install_util.h:50: extern const wchar_t kChromeStatsSampleKey[]; On 2016/08/12 17:49:25, robertshield wrote: > What is the relationship between the value stored here and the value stored at > kRegValueUsageStats which also seems to control UMA and crash reporting? kRegValueUsageStats is the source of consent described in the above comment. I've added a comment to that const. https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); On 2016/08/12 17:49:25, robertshield wrote: > The logic for whether uploads are enabled or not is now split between > SetUploadsEnabled() and InitializeCrashpadImpl(). I think it should all be in > InitializeCrashpadImpl(). > > It's surprising that SetUploadsEnabled(true) might not actually enable uploads. I don't disagree. But, keeping SetUploadsEnabled as is, would mean anyone calling it needs to know about sampling to decide the value passed. Mac calls it when the consent setting is changed in settings. https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:469: crash_reporter::SetUploadsEnabled(enable_uploads); On 2016/08/12 17:47:48, Alexei Svitkine (very slow) wrote: > So this will be called as a result of UpdateRunningServices() in metrics code. > > At which point, it should override whatever was done by line 278, right? > > Could there be an inconsistency there? > > For example, I see the line 278 code has some extra checks, like: > - should_initialize_database_and_set_upload_policy > - crash_reporter_client->ReportingIsEnforcedByPolicy() > - crash_reporter_client->IsRunningUnattended() > > Seems like all those things that may have caused line 278 previously to not > enable uploads may suddenly get overridden here. Am I reading it right? > > If so, it seems like an issue. Can we make the two go through the same code path > that checks all these things? Yes, you're right. This is already the case, since SetUploadsEnabled is being called on Mac through GoogleUpdateSettings::SetCollectStatsConsent. This extern was just intended to make SetUploadsEnabled available to windows in the same way that it is on Mac.
https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); On 2016/08/12 20:04:31, jwd wrote: > On 2016/08/12 17:49:25, robertshield wrote: > > The logic for whether uploads are enabled or not is now split between > > SetUploadsEnabled() and InitializeCrashpadImpl(). I think it should all be in > > InitializeCrashpadImpl(). > > > > It's surprising that SetUploadsEnabled(true) might not actually enable > uploads. > > I don't disagree. But, keeping SetUploadsEnabled as is, would mean anyone > calling it needs to know about sampling to decide the value passed. Mac calls it > when the consent setting is changed in settings. The intent is to do this on Windows too. https://crbug.com/629618.
https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:207: }(); On 2016/08/12 20:04:30, jwd wrote: > On 2016/08/12 17:49:25, robertshield wrote: > > This is harder to read than: > > > > HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName); > > static SetCrashUploadsEnabledPointer set_crash_enabled = > > reinterpret_cast<SetCrashUploadsEnabledPointer>( > > GetProcAddress(elf_module, kCrashpadSetEnabledFunctionName)); > > I agree, and had to spend time understanding what was going on in the code that > inspired this (in crash_upload_list_crashpad.cc), but is there no significant > cost to getting the handle? GetModuleHandle is a lookup in a table, all user mode, it is not significantly expensive. https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); On 2016/08/12 20:16:27, Mark Mentovai wrote: > On 2016/08/12 20:04:31, jwd wrote: > > On 2016/08/12 17:49:25, robertshield wrote: > > > The logic for whether uploads are enabled or not is now split between > > > SetUploadsEnabled() and InitializeCrashpadImpl(). I think it should all be > in > > > InitializeCrashpadImpl(). > > > > > > It's surprising that SetUploadsEnabled(true) might not actually enable > > uploads. > > > > I don't disagree. But, keeping SetUploadsEnabled as is, would mean anyone > > calling it needs to know about sampling to decide the value passed. Mac calls > it > > when the consent setting is changed in settings. > > The intent is to do this on Windows too. https://crbug.com/629618. If this method is to contain logic that can cause it to ignore the value of |enable_uploads|, then it shouldn't be called SetUploadsEnabled(), it should be called MaybeSetUploadsEnabled() or TryToEnableUploads() or some such. As is, the implementation is surprising.
https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:299: enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample()); On 2016/08/12 21:04:43, robertshield wrote: > On 2016/08/12 20:16:27, Mark Mentovai wrote: > > On 2016/08/12 20:04:31, jwd wrote: > > > On 2016/08/12 17:49:25, robertshield wrote: > > > > The logic for whether uploads are enabled or not is now split between > > > > SetUploadsEnabled() and InitializeCrashpadImpl(). I think it should all be > > in > > > > InitializeCrashpadImpl(). > > > > > > > > It's surprising that SetUploadsEnabled(true) might not actually enable > > > uploads. > > > > > > I don't disagree. But, keeping SetUploadsEnabled as is, would mean anyone > > > calling it needs to know about sampling to decide the value passed. Mac > calls > > it > > > when the consent setting is changed in settings. > > > > The intent is to do this on Windows too. https://crbug.com/629618. > > If this method is to contain logic that can cause it to ignore the value of > |enable_uploads|, then it shouldn't be called SetUploadsEnabled(), it should be > called MaybeSetUploadsEnabled() or TryToEnableUploads() or some such. > > As is, the implementation is surprising. Changing the name to something more suitable is fine. SetUploadsEnabledUserPreference()?
On 2016/08/12 22:01:58, Mark Mentovai wrote: > https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... > File components/crash/content/app/crashpad.cc (right): > > https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... > components/crash/content/app/crashpad.cc:299: enable_uploads && > GetCrashReporterClient()->GetCollectStatsInSample()); > On 2016/08/12 21:04:43, robertshield wrote: > > On 2016/08/12 20:16:27, Mark Mentovai wrote: > > > On 2016/08/12 20:04:31, jwd wrote: > > > > On 2016/08/12 17:49:25, robertshield wrote: > > > > > The logic for whether uploads are enabled or not is now split between > > > > > SetUploadsEnabled() and InitializeCrashpadImpl(). I think it should all > be > > > in > > > > > InitializeCrashpadImpl(). > > > > > > > > > > It's surprising that SetUploadsEnabled(true) might not actually enable > > > > uploads. > > > > > > > > I don't disagree. But, keeping SetUploadsEnabled as is, would mean anyone > > > > calling it needs to know about sampling to decide the value passed. Mac > > calls > > > it > > > > when the consent setting is changed in settings. > > > > > > The intent is to do this on Windows too. https://crbug.com/629618. > > > > If this method is to contain logic that can cause it to ignore the value of > > |enable_uploads|, then it shouldn't be called SetUploadsEnabled(), it should > be > > called MaybeSetUploadsEnabled() or TryToEnableUploads() or some such. > > > > As is, the implementation is surprising. > > Changing the name to something more suitable is fine. > SetUploadsEnabledUserPreference()? sgtm
Plan for moving forward: - Rename SetUploadsEnabled to SetUploadsEnabledUserPreference. - The semantics of that function will remain that it trusts its enable_uploads argument, and doesn't validate it. It will be up to the callers to ensure it's being given a correct value. This is currently the case on Mac, as it's only called from crash reporter initialization and through settings UI flow. The particular UI that calls it is only shown when the extra conditions are met (e.g. policy allows it). - The rest of the change essentially as it is, using the reg value to communicate sampling, keeping the sampling check as public in the crash reporter client api, calling the sampling check in the Set...Enabled function, and calling the Set...Enabled when updating the running state of other metrics services on windows (triggered on startup and when consent changes). Mark and Robert how does that sound to you? https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:207: }(); On 2016/08/12 21:04:43, robertshield wrote: > On 2016/08/12 20:04:30, jwd wrote: > > On 2016/08/12 17:49:25, robertshield wrote: > > > This is harder to read than: > > > > > > HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName); > > > static SetCrashUploadsEnabledPointer set_crash_enabled = > > > reinterpret_cast<SetCrashUploadsEnabledPointer>( > > > GetProcAddress(elf_module, kCrashpadSetEnabledFunctionName)); > > > > I agree, and had to spend time understanding what was going on in the code > that > > inspired this (in crash_upload_list_crashpad.cc), but is there no significant > > cost to getting the handle? > > GetModuleHandle is a lookup in a table, all user mode, it is not significantly > expensive. Done. https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:144: virtual bool GetCollectStatsInSample(); On 2016/08/12 17:49:25, robertshield wrote: > It seems weird to me that consumers need to know about implementation details to > figure out whether to upload crashes or not. > > I'm guessing that anywhere this is used, callers will have to call > GetCollectStatsConsent() && GetCollectStatsInSample() to determine whether to > upload crashes? That seems like odd leakage. Callers of this from outside crash_reporter shouldn't be making the full decision of whether to upload. I'm going to have a larger reply with some details. I could also make this method private in the chrome implementation.
On 2016/08/16 19:29:11, jwd wrote: > Plan for moving forward: > > - Rename SetUploadsEnabled to SetUploadsEnabledUserPreference. > - The semantics of that function will remain that it trusts its enable_uploads > argument, and doesn't validate it. It will be up to the callers to ensure it's > being given a correct value. This is currently the case on Mac, as it's only > called from crash reporter initialization and through settings UI flow. The > particular UI that calls it is only shown when the extra conditions are met > (e.g. policy allows it). This sounds much better to me, though if it treats enable_uploads as gospel, I would keep calling it SetUploadsEnabled(). > - The rest of the change essentially as it is, using the reg value to > communicate sampling, keeping the sampling check as public in the crash reporter > client api, calling the sampling check in the Set...Enabled function, and > calling the Set...Enabled when updating the running state of other metrics > services on windows (triggered on startup and when consent changes). This part is a little weirder to me. It seems like it is forcing an artifact of Chrome's usage (consent + sampling) of crashpad onto the crashpad embedder API. I don't have full context on Crashpad's embedder api design though and Mark can probably elaborate more if my understanding is correct. > > Mark and Robert how does that sound to you? > > https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... > File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): > > https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... > chrome/browser/metrics/chrome_metrics_services_manager_client.cc:207: }(); > On 2016/08/12 21:04:43, robertshield wrote: > > On 2016/08/12 20:04:30, jwd wrote: > > > On 2016/08/12 17:49:25, robertshield wrote: > > > > This is harder to read than: > > > > > > > > HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName); > > > > static SetCrashUploadsEnabledPointer set_crash_enabled = > > > > reinterpret_cast<SetCrashUploadsEnabledPointer>( > > > > GetProcAddress(elf_module, kCrashpadSetEnabledFunctionName)); > > > > > > I agree, and had to spend time understanding what was going on in the code > > that > > > inspired this (in crash_upload_list_crashpad.cc), but is there no > significant > > > cost to getting the handle? > > > > GetModuleHandle is a lookup in a table, all user mode, it is not significantly > > expensive. > > Done. > > https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... > File components/crash/content/app/crash_reporter_client.h (right): > > https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... > components/crash/content/app/crash_reporter_client.h:144: virtual bool > GetCollectStatsInSample(); > On 2016/08/12 17:49:25, robertshield wrote: > > It seems weird to me that consumers need to know about implementation details > to > > figure out whether to upload crashes or not. > > > > I'm guessing that anywhere this is used, callers will have to call > > GetCollectStatsConsent() && GetCollectStatsInSample() to determine whether to > > upload crashes? That seems like odd leakage. > Callers of this from outside crash_reporter shouldn't be making the full > decision of whether to upload. I'm going to have a larger reply with some > details. > > I could also make this method private in the chrome implementation.
On 2016/08/16 23:35:22, robertshield wrote: > On 2016/08/16 19:29:11, jwd wrote: > > Plan for moving forward: > > > > - Rename SetUploadsEnabled to SetUploadsEnabledUserPreference. > > - The semantics of that function will remain that it trusts its enable_uploads > > argument, and doesn't validate it. It will be up to the callers to ensure it's > > being given a correct value. This is currently the case on Mac, as it's only > > called from crash reporter initialization and through settings UI flow. The > > particular UI that calls it is only shown when the extra conditions are met > > (e.g. policy allows it). > > This sounds much better to me, though if it treats enable_uploads as gospel, I > would keep calling it SetUploadsEnabled(). Ah, I left ambiguity! By trust, I didn't intend to mean gospel. I meant it would trust that it's not violating anything like consent or policy. I didn't mean it would just apply it. > > > - The rest of the change essentially as it is, using the reg value to > > communicate sampling, keeping the sampling check as public in the crash > reporter > > client api, calling the sampling check in the Set...Enabled function, and > > calling the Set...Enabled when updating the running state of other metrics > > services on windows (triggered on startup and when consent changes). > > This part is a little weirder to me. It seems like it is forcing an artifact of > Chrome's usage (consent + sampling) of crashpad onto the crashpad embedder API. I see this change as adding a sampling functionality to the crashpad component, more so than just jamming chrome's sampling into the crashpad component. In this context, I don't think it makes sense for the embedder of the component to override the sampling state without interacting directly with the sampling control in the crash reporter client. What about changing the Set..Enabled function to be called UpdateReportingWithConsent(bool consent). All this is saying is "here's the consent value, do whatever", this makes the concept of consent explicit (which the Set..Enabled didn't have), and it's no longer a directive since consent means "don't do it if I say no" and not "do it if I say yes". > > I don't have full context on Crashpad's embedder api design though and Mark can > probably elaborate more if my understanding is correct. > > > > > Mark and Robert how does that sound to you? > > > > > https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... > > File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): > > > > > https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... > > chrome/browser/metrics/chrome_metrics_services_manager_client.cc:207: }(); > > On 2016/08/12 21:04:43, robertshield wrote: > > > On 2016/08/12 20:04:30, jwd wrote: > > > > On 2016/08/12 17:49:25, robertshield wrote: > > > > > This is harder to read than: > > > > > > > > > > HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName); > > > > > static SetCrashUploadsEnabledPointer set_crash_enabled = > > > > > reinterpret_cast<SetCrashUploadsEnabledPointer>( > > > > > GetProcAddress(elf_module, kCrashpadSetEnabledFunctionName)); > > > > > > > > I agree, and had to spend time understanding what was going on in the code > > > that > > > > inspired this (in crash_upload_list_crashpad.cc), but is there no > > significant > > > > cost to getting the handle? > > > > > > GetModuleHandle is a lookup in a table, all user mode, it is not > significantly > > > expensive. > > > > Done. > > > > > https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... > > File components/crash/content/app/crash_reporter_client.h (right): > > > > > https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... > > components/crash/content/app/crash_reporter_client.h:144: virtual bool > > GetCollectStatsInSample(); > > On 2016/08/12 17:49:25, robertshield wrote: > > > It seems weird to me that consumers need to know about implementation > details > > to > > > figure out whether to upload crashes or not. > > > > > > I'm guessing that anywhere this is used, callers will have to call > > > GetCollectStatsConsent() && GetCollectStatsInSample() to determine whether > to > > > upload crashes? That seems like odd leakage. > > Callers of this from outside crash_reporter shouldn't be making the full > > decision of whether to upload. I'm going to have a larger reply with some > > details. > > > > I could also make this method private in the chrome implementation.
On 2016/08/17 14:48:36, jwd wrote: > On 2016/08/16 23:35:22, robertshield wrote: > > On 2016/08/16 19:29:11, jwd wrote: > > > Plan for moving forward: > > > > > > - Rename SetUploadsEnabled to SetUploadsEnabledUserPreference. > > > - The semantics of that function will remain that it trusts its > enable_uploads > > > argument, and doesn't validate it. It will be up to the callers to ensure > it's > > > being given a correct value. This is currently the case on Mac, as it's only > > > called from crash reporter initialization and through settings UI flow. The > > > particular UI that calls it is only shown when the extra conditions are met > > > (e.g. policy allows it). > > > > This sounds much better to me, though if it treats enable_uploads as gospel, I > > would keep calling it SetUploadsEnabled(). > > Ah, I left ambiguity! By trust, I didn't intend to mean gospel. I meant it would > trust that it's not violating anything like consent or policy. I didn't mean it > would just apply it. > > > > > - The rest of the change essentially as it is, using the reg value to > > > communicate sampling, keeping the sampling check as public in the crash > > reporter > > > client api, calling the sampling check in the Set...Enabled function, and > > > calling the Set...Enabled when updating the running state of other metrics > > > services on windows (triggered on startup and when consent changes). > > > > This part is a little weirder to me. It seems like it is forcing an artifact > of > > Chrome's usage (consent + sampling) of crashpad onto the crashpad embedder > API. > > I see this change as adding a sampling functionality to the crashpad component, > more so than just jamming chrome's sampling into the crashpad component. In this > context, I don't think it makes sense for the embedder of the component to > override the sampling state without interacting directly with the sampling > control in the crash reporter client. Ok, my apologies, it wasn't clear to me that there adding sampling support to crashpad proper was also the goal. The approach proposed sounds good in this case. > > What about changing the Set..Enabled function to be called > UpdateReportingWithConsent(bool consent). All this is saying is "here's the > consent value, do whatever", this makes the concept of consent explicit (which > the Set..Enabled didn't have), and it's no longer a directive since consent > means "don't do it if I say no" and not "do it if I say yes". Changing "Enabled" for "Consent" sounds good to me. I have no other issues with the proposed solution. Thanks for spelling it out for me :-) > > > > > I don't have full context on Crashpad's embedder api design though and Mark > can > > probably elaborate more if my understanding is correct. > > > > > > > > Mark and Robert how does that sound to you? > > > > > > > > > https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... > > > File chrome/browser/metrics/chrome_metrics_services_manager_client.cc > (right): > > > > > > > > > https://codereview.chromium.org/2221833005/diff/120001/chrome/browser/metrics... > > > chrome/browser/metrics/chrome_metrics_services_manager_client.cc:207: }(); > > > On 2016/08/12 21:04:43, robertshield wrote: > > > > On 2016/08/12 20:04:30, jwd wrote: > > > > > On 2016/08/12 17:49:25, robertshield wrote: > > > > > > This is harder to read than: > > > > > > > > > > > > HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName); > > > > > > static SetCrashUploadsEnabledPointer set_crash_enabled = > > > > > > reinterpret_cast<SetCrashUploadsEnabledPointer>( > > > > > > GetProcAddress(elf_module, kCrashpadSetEnabledFunctionName)); > > > > > > > > > > I agree, and had to spend time understanding what was going on in the > code > > > > that > > > > > inspired this (in crash_upload_list_crashpad.cc), but is there no > > > significant > > > > > cost to getting the handle? > > > > > > > > GetModuleHandle is a lookup in a table, all user mode, it is not > > significantly > > > > expensive. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... > > > File components/crash/content/app/crash_reporter_client.h (right): > > > > > > > > > https://codereview.chromium.org/2221833005/diff/120001/components/crash/conte... > > > components/crash/content/app/crash_reporter_client.h:144: virtual bool > > > GetCollectStatsInSample(); > > > On 2016/08/12 17:49:25, robertshield wrote: > > > > It seems weird to me that consumers need to know about implementation > > details > > > to > > > > figure out whether to upload crashes or not. > > > > > > > > I'm guessing that anywhere this is used, callers will have to call > > > > GetCollectStatsConsent() && GetCollectStatsInSample() to determine whether > > to > > > > upload crashes? That seems like odd leakage. > > > Callers of this from outside crash_reporter shouldn't be making the full > > > decision of whether to upload. I'm going to have a larger reply with some > > > details. > > > > > > I could also make this method private in the chrome implementation.
Ok, I've renamed SetUploadsEnabled to UpdateUploadsEnabled, and the argument to consent. I've also moved the enable logic that was in the initialize code into UpdateUploadsEnabled. The functional change here is: - on initialization, GetCollectStatsConsent is called before policy check - policy and IsRunningUnattended are checked on each update. The intention here is to consolidate the enabling logic, and make it clearer that the Enable/Disable function is not guaranteed to turn on uploads. PTAL
LGTM % nits Thanks! https://codereview.chromium.org/2221833005/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/180001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:41: typedef void (*SetCrashUploadsEnabledPointer)(bool); Nit: Rename. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.cc:285: if (g_database) { Nit: Early return instead. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.cc:296: } Nit: Indent is off. Run git cl format. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.cc:298: Nit: Remove extra blank line.
LGTM otherwise. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.cc:289: // Breakpad provided a --disable-breakpad switch to disable crash What’s up with this indentation? https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.h:66: // consent, but vill only be enabled without consent when policy enforces crash vill → will https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.h:67: // reporting. Enable or disable crash report upload is a property of the Whether report upload is enabled is a property https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.h:68: // Crashpad database. In a newly- created database, uploads will be disabled. Remove the newly- introduced space after the dash. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.h:72: void UpdateUploadsEnabled(bool consent); I don’t know that this name is any better than SetUploadsEnabled (and I’m afraid it might be worse), but if everyone else likes it, OK… My concern is that if you want to update this thing to reflect that it’s about consent, then you need to put the word “consent” in the function name, because nobody sees this “consent” parameter name at call sites. So maybe SetUploadConsent()?
The CQ bit was checked by jwd@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/2221833005/diff/180001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.cc:285: if (g_database) { On 2016/08/17 20:37:48, Alexei Svitkine (very slow) wrote: > Nit: Early return instead. Done. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.cc:289: // Breakpad provided a --disable-breakpad switch to disable crash On 2016/08/17 20:59:07, Mark Mentovai wrote: > What’s up with this indentation? Done. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.cc:296: } On 2016/08/17 20:37:48, Alexei Svitkine (very slow) wrote: > Nit: Indent is off. Run git cl format. Done. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.cc:298: On 2016/08/17 20:37:48, Alexei Svitkine (very slow) wrote: > Nit: Remove extra blank line. Done. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.h:66: // consent, but vill only be enabled without consent when policy enforces crash On 2016/08/17 20:59:07, Mark Mentovai wrote: > vill → will Done. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.h:67: // reporting. Enable or disable crash report upload is a property of the On 2016/08/17 20:59:07, Mark Mentovai wrote: > Whether report upload is enabled is a property Done. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.h:68: // Crashpad database. In a newly- created database, uploads will be disabled. On 2016/08/17 20:59:07, Mark Mentovai wrote: > Remove the newly- introduced space after the dash. Done. https://codereview.chromium.org/2221833005/diff/180001/components/crash/conte... components/crash/content/app/crashpad.h:72: void UpdateUploadsEnabled(bool consent); On 2016/08/17 20:59:07, Mark Mentovai wrote: > I don’t know that this name is any better than SetUploadsEnabled (and I’m afraid > it might be worse), but if everyone else likes it, OK… > > My concern is that if you want to update this thing to reflect that it’s about > consent, then you need to put the word “consent” in the function name, because > nobody sees this “consent” parameter name at call sites. > > So maybe SetUploadConsent()? Done.
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/...)
lgtm
jwd@chromium.org changed reviewers: + isherman@chromium.org
+isherman@ for google_update_settings_posix.cc
https://codereview.chromium.org/2221833005/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/220001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:41: typedef void (*SetCrashUploadConsentPointer)(bool); SetUploadConsentPointer https://codereview.chromium.org/2221833005/diff/220001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:205: static SetCrashUploadConsentPointer set_crash_enabled = set_upload_consent instead of set_crash_enabled. https://codereview.chromium.org/2221833005/diff/220001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/220001/components/crash/conte... components/crash/content/app/crashpad.cc:469: void __declspec(dllexport) __cdecl SetUploadConsentImpl(bool consent) { I'm sorry, I feel like I missed this along the way, but could you explain (probably again) why this takes a bool instead of calling crash_reporter_client->GetCollectStatsConsent()?
https://codereview.chromium.org/2221833005/diff/220001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/220001/components/crash/conte... components/crash/content/app/crashpad.cc:469: void __declspec(dllexport) __cdecl SetUploadConsentImpl(bool consent) { On 2016/08/18 19:49:10, scottmg wrote: > I'm sorry, I feel like I missed this along the way, but could you explain > (probably again) why this takes a bool instead of calling > crash_reporter_client->GetCollectStatsConsent()? The reason I kept the argument is to avoid making it a requirement that the consent value be fully persisted before calling this. On mac, GetCollectStatsConsent relies on the consent file being written, on windows it will be the registry values. The other parts of the logic don't really change during a session (does policy?), and wouldn't trigger a call to this if they did. It wouldn't be the biggest deal for Mac, just moving the call to the bottom of GoogleUpdateSettings::SetCollectStatsConsent instead of the top, and my windows code wouldn't need to change, it's already called after that point. The downsides are: We wouldn't get crash coverage for the 21 lines of GoogleUpdateSettings::SetCollectStatsConsent on Mac, and the windows code doesn't make it exactly clear that it IS doing the right thing. And of course it seemed a bit messy to me to have the required two steps.
https://codereview.chromium.org/2221833005/diff/220001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/220001/components/crash/conte... components/crash/content/app/crashpad.cc:469: void __declspec(dllexport) __cdecl SetUploadConsentImpl(bool consent) { On 2016/08/18 20:21:33, jwd wrote: > On 2016/08/18 19:49:10, scottmg wrote: > > I'm sorry, I feel like I missed this along the way, but could you explain > > (probably again) why this takes a bool instead of calling > > crash_reporter_client->GetCollectStatsConsent()? > > The reason I kept the argument is to avoid making it a requirement that the > consent value be fully persisted before calling this. On mac, > GetCollectStatsConsent relies on the consent file being written, on windows it > will be the registry values. The other parts of the logic don't really change > during a session (does policy?), and wouldn't trigger a call to this if they > did. > > It wouldn't be the biggest deal for Mac, just moving the call to the bottom of > GoogleUpdateSettings::SetCollectStatsConsent instead of the top, and my windows > code wouldn't need to change, it's already called after that point. > > The downsides are: > We wouldn't get crash coverage for the 21 lines of > GoogleUpdateSettings::SetCollectStatsConsent on Mac, and the windows code > doesn't make it exactly clear that it IS doing the right thing. And of course it > seemed a bit messy to me to have the required two steps. OK, I hadn't been looking at Mac. I sort of prefer one place that knows how to read the consent bit, but your argument makes sense too. Since this function is Windows-only, could you add DCHECK_EQ(consent, crash_reporter_client->GetCollectStatsConsent()) here?
The CQ bit was checked by jwd@chromium.org to run a CQ dry run
https://codereview.chromium.org/2221833005/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2221833005/diff/220001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:41: typedef void (*SetCrashUploadConsentPointer)(bool); On 2016/08/18 19:49:10, scottmg wrote: > SetUploadConsentPointer Done. https://codereview.chromium.org/2221833005/diff/220001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:205: static SetCrashUploadConsentPointer set_crash_enabled = On 2016/08/18 19:49:10, scottmg wrote: > set_upload_consent instead of set_crash_enabled. Done. https://codereview.chromium.org/2221833005/diff/220001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/2221833005/diff/220001/components/crash/conte... components/crash/content/app/crashpad.cc:469: void __declspec(dllexport) __cdecl SetUploadConsentImpl(bool consent) { On 2016/08/18 20:33:18, scottmg wrote: > On 2016/08/18 20:21:33, jwd wrote: > > On 2016/08/18 19:49:10, scottmg wrote: > > > I'm sorry, I feel like I missed this along the way, but could you explain > > > (probably again) why this takes a bool instead of calling > > > crash_reporter_client->GetCollectStatsConsent()? > > > > The reason I kept the argument is to avoid making it a requirement that the > > consent value be fully persisted before calling this. On mac, > > GetCollectStatsConsent relies on the consent file being written, on windows it > > will be the registry values. The other parts of the logic don't really change > > during a session (does policy?), and wouldn't trigger a call to this if they > > did. > > > > It wouldn't be the biggest deal for Mac, just moving the call to the bottom of > > GoogleUpdateSettings::SetCollectStatsConsent instead of the top, and my > windows > > code wouldn't need to change, it's already called after that point. > > > > The downsides are: > > We wouldn't get crash coverage for the 21 lines of > > GoogleUpdateSettings::SetCollectStatsConsent on Mac, and the windows code > > doesn't make it exactly clear that it IS doing the right thing. And of course > it > > seemed a bit messy to me to have the required two steps. > > OK, I hadn't been looking at Mac. I sort of prefer one place that knows how to > read the consent bit, but your argument makes sense too. > > Since this function is Windows-only, could you add DCHECK_EQ(consent, > crash_reporter_client->GetCollectStatsConsent()) here? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/18 19:22:11, jwd wrote: > +isherman@ for google_update_settings_posix.cc chrome/browser/google/google_update_settings_posix.cc lgtm, and probably fine to TBR similar changes in the future, since you're just following through on renaming a depended-on function.
lgtm
jwd@chromium.org changed reviewers: + thestig@chromium.org
+thestig@ for chrome/DEPS change
On 2016/08/18 21:52:31, jwd wrote: > +thestig@ for chrome/DEPS change chrome/browser/DEPS lgtm since scottmg approved and ananta has not complained.
The CQ bit was unchecked by jwd@chromium.org
The CQ bit was checked by jwd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, asvitkine@chromium.org, robertshield@chromium.org Link to the patchset: https://codereview.chromium.org/2221833005/#ps240001 (title: "scottmg's comments, adding DCHECK")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Adding support for sampling crashes in Chrome on Windows. BUG=609987 ========== to ========== Adding support for sampling crashes in Chrome on Windows. BUG=609987 Committed: https://crrev.com/c882e48d0f01744dd5e9de83395f45baa38ce731 Cr-Commit-Position: refs/heads/master@{#412984} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c882e48d0f01744dd5e9de83395f45baa38ce731 Cr-Commit-Position: refs/heads/master@{#412984} |
