Adding sampling support to installer crash reporter client.

Adding sampling support to installer crash reporter client.

BUG=654771
Committed: https://crrev.com/7d2b5433e0e583fc5edb40f16abc9566ade06f14
Cr-Commit-Position: refs/heads/master@{#427702}

[jwd, 2016-10-17 20:34:47]

The CQ bit was checked by jwd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 20:35:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jwd, 2016-10-17 21:21:18]

jwd@chromium.org changed reviewers:
+ grt@chromium.org

[jwd, 2016-10-17 21:21:52]

On 2016/10/17 21:21:19, jwd wrote:

Full disclosure, I have no idea how to test this manually.

[commit-bot: I haz the power, 2016-10-17 23:46:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 23:46:27]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2016-10-24 09:34:33]

do you have access to a windows machine and windbg or devenv debugging skillz?

https://codereview.chromium.org/2422103004/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/installer_crash_reporter_client.cc (right):

https://codereview.chromium.org/2422103004/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/installer_crash_reporter_client.cc:128: return
install_static::GetCollectStatsInSample();
it looks like install_static::GetCollectStatsInSample uses
install_static::IsSxSChrome, which doesn't seem to do the right thing when
called from the context of setup.exe. i fear that you'll need to instead
duplicate the code in install_static, but using regular Chrome-isms. something
like:

{
  // TODO(grt): remove duplication of code.
  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
  base::win::RegKey key(HKEY_CURRENT_USER, dist->GetRegistryPath().c_str(),
KEY_QUERY_VALUE | KEY_WOW64_32KEY);
  if (!key.Valid())
    return true;
  DWORD out_value = 0;
  if (key.ReadValueDW(kRegValueChromeStatsSample, &out_value) != ERROR_SUCCESS)
    return true;
  return out_value == 1;
}

(i have a change out for review that will allow this to use install_static)

[jwd, 2016-10-24 19:59:24]

On 2016/10/24 09:34:33, grt (UTC plus 2) wrote:
> do you have access to a windows machine and windbg or devenv debugging skillz?
I have a windows machine (on corp, and a VM in a cloud somewhere), I've USED
windbg.
> 
>
https://codereview.chromium.org/2422103004/diff/1/chrome/installer/setup/inst...
> File chrome/installer/setup/installer_crash_reporter_client.cc (right):
> 
>
https://codereview.chromium.org/2422103004/diff/1/chrome/installer/setup/inst...
> chrome/installer/setup/installer_crash_reporter_client.cc:128: return
> install_static::GetCollectStatsInSample();
> it looks like install_static::GetCollectStatsInSample uses
> install_static::IsSxSChrome, which doesn't seem to do the right thing when
> called from the context of setup.exe. i fear that you'll need to instead
> duplicate the code in install_static, but using regular Chrome-isms. something
> like:
> 
> {
>   // TODO(grt): remove duplication of code.
>   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
>   base::win::RegKey key(HKEY_CURRENT_USER, dist->GetRegistryPath().c_str(),
> KEY_QUERY_VALUE | KEY_WOW64_32KEY);
>   if (!key.Valid())
>     return true;
>   DWORD out_value = 0;
>   if (key.ReadValueDW(kRegValueChromeStatsSample, &out_value) !=
ERROR_SUCCESS)
>     return true;
>   return out_value == 1;
> }
> 
> (i have a change out for review that will allow this to use install_static)

[jwd, 2016-10-24 21:28:52]

https://codereview.chromium.org/2422103004/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/installer_crash_reporter_client.cc (right):

https://codereview.chromium.org/2422103004/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/installer_crash_reporter_client.cc:128: return
install_static::GetCollectStatsInSample();
On 2016/10/24 09:34:33, grt (UTC plus 2) wrote:
> it looks like install_static::GetCollectStatsInSample uses
> install_static::IsSxSChrome, which doesn't seem to do the right thing when
> called from the context of setup.exe. i fear that you'll need to instead
> duplicate the code in install_static, but using regular Chrome-isms. something
> like:
> 
> {
>   // TODO(grt): remove duplication of code.
>   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
>   base::win::RegKey key(HKEY_CURRENT_USER, dist->GetRegistryPath().c_str(),
> KEY_QUERY_VALUE | KEY_WOW64_32KEY);
>   if (!key.Valid())
>     return true;
>   DWORD out_value = 0;
>   if (key.ReadValueDW(kRegValueChromeStatsSample, &out_value) !=
ERROR_SUCCESS)
>     return true;
>   return out_value == 1;
> }
> 
> (i have a change out for review that will allow this to use install_static)

Done.

[grt (UTC plus 2), 2016-10-25 06:58:31]

can you test this manually by setting the value(s) in the registry and then
seeing how the crashpad settings database is modified after running setup.exe?
for example, you could install some older version of chromium/chrome, make sure
uploads are enabled, tweak the registry for the sample thing, then run your
fresh mini_installer to update chromium/chrome. if all goes well, it will flip
the state in the settings file to uploads not enabled, correct?

alternatively, set a breakpoint in setup_exe!crash_reporter::SetUploadConsent
and step through.

https://codereview.chromium.org/2422103004/diff/20001/chrome/installer/setup/...
File chrome/installer/setup/installer_crash_reporter_client.cc (right):

https://codereview.chromium.org/2422103004/diff/20001/chrome/installer/setup/...
chrome/installer/setup/installer_crash_reporter_client.cc:137: ERROR_SUCCESS)
nit: braces around the body since the conditional spans multiple lines.

[jwd, 2016-10-25 20:36:30]

On 2016/10/25 06:58:31, grt (UTC plus 2) wrote:
> can you test this manually by setting the value(s) in the registry and then
> seeing how the crashpad settings database is modified after running setup.exe?
> for example, you could install some older version of chromium/chrome, make
sure
> uploads are enabled, tweak the registry for the sample thing, then run your
> fresh mini_installer to update chromium/chrome. if all goes well, it will flip
> the state in the settings file to uploads not enabled, correct?

Alright, I've done it this way, with some help from gab for setup.exe flags.
I've confirmed that setup.exe will use the sampling registry value. I've also
figured out what byte in Crashpad\settings.dat gets flipped when enabling or
disabling reporting, it is byte offset 8.
> 
> alternatively, set a breakpoint in setup_exe!crash_reporter::SetUploadConsent
> and step through.
> 
>
https://codereview.chromium.org/2422103004/diff/20001/chrome/installer/setup/...
> File chrome/installer/setup/installer_crash_reporter_client.cc (right):
> 
>
https://codereview.chromium.org/2422103004/diff/20001/chrome/installer/setup/...
> chrome/installer/setup/installer_crash_reporter_client.cc:137: ERROR_SUCCESS)
> nit: braces around the body since the conditional spans multiple lines.

[jwd, 2016-10-25 20:36:45]

https://codereview.chromium.org/2422103004/diff/20001/chrome/installer/setup/...
File chrome/installer/setup/installer_crash_reporter_client.cc (right):

https://codereview.chromium.org/2422103004/diff/20001/chrome/installer/setup/...
chrome/installer/setup/installer_crash_reporter_client.cc:137: ERROR_SUCCESS)
On 2016/10/25 06:58:31, grt (UTC plus 2) wrote:
> nit: braces around the body since the conditional spans multiple lines.

Done.

[grt (UTC plus 2), 2016-10-26 07:58:08]

Awesome. LGTM.

[jwd, 2016-10-26 15:06:22]

The CQ bit was checked by jwd@chromium.org

[commit-bot: I haz the power, 2016-10-26 15:06:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 16:06:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-26 16:15:33]

Description was changed from

==========
Adding sampling support to installer crash reporter client.

BUG=654771
==========

to

==========
Adding sampling support to installer crash reporter client.

BUG=654771

Committed: https://crrev.com/7d2b5433e0e583fc5edb40f16abc9566ade06f14
Cr-Commit-Position: refs/heads/master@{#427702}
==========

[commit-bot: I haz the power, 2016-10-26 16:15:34]

Patchset 3 (id:??) landed as
https://crrev.com/7d2b5433e0e583fc5edb40f16abc9566ade06f14
Cr-Commit-Position: refs/heads/master@{#427702}