Experimentally disable termination on heap corrruption in order to measure the contribution of this…

Experimentally disable termination on heap corruption in order to measure the contribution of this feature to missing crash reports.

Because this feature is configured very early in the process lifetime it cannot be directly controlled by a field-trial. Rather, we query the status during a given execution, store that status in the registry, and then query the registry during startup. This means the experiment will only take effect the 2nd time it is executed.

BUG=394842
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284100

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285931

[erikwright (departed), 2014-07-17 18:52:27]

jhawkins: PTAL chrome/*
wfh@: PTAL for security
brettw@: PTAL for content/

See
https://docs.google.com/a/google.com/document/d/15yJ4UeHpkXz_qe460F9LaSiYHuM1...
for background.

[brettw, 2014-07-17 20:15:04]

content lgtm with name change

https://codereview.chromium.org/402723002/diff/1/content/public/app/content_m...
File content/public/app/content_main.h (right):

https://codereview.chromium.org/402723002/diff/1/content/public/app/content_m...
content/public/app/content_main.h:28:
disable_termination_on_heap_corruption(false),
I find this kind of thing much easier to follow if everything is positive (i.e.
"enable_termination_on_heap_corruption(true)"). Otherwise everything becomes a
double-negative which is difficult to reason about.

[erikwright (departed), 2014-07-17 21:10:14]

On 2014/07/17 20:15:04, brettw wrote:
> content lgtm with name change
> 
>
https://codereview.chromium.org/402723002/diff/1/content/public/app/content_m...
> File content/public/app/content_main.h (right):
> 
>
https://codereview.chromium.org/402723002/diff/1/content/public/app/content_m...
> content/public/app/content_main.h:28:
> disable_termination_on_heap_corruption(false),
> I find this kind of thing much easier to follow if everything is positive
(i.e.
> "enable_termination_on_heap_corruption(true)"). Otherwise everything becomes a
> double-negative which is difficult to reason about.

jhawkins: I had to duplicate some strings in terminate...win.cc that also appear
in chrome_constants.cc (and chrome_elf/chrome_elf_constants.cc). It's pretty
normal for chrome_elf_constants.cc to duplicate them as it does not link to
chrome/common, but theoretically my code could share them.

Since I plan to yank out this code shortly, anyway, I propose to not introduce
an extra constant in chrome_constants.{h,cc}. But I'll follow your guidance.

[Will Harris, 2014-07-17 22:17:59]

LGTM from security perspective.

You could use BrowserDistribution::GetDistribution()->GetStateKey() to store the
configuration data a place in HKCU that gets deleted for you on uninstall, but
it seems chrome_elf already uses these same registry keys (and leaves them
behind after uninstall), so it's probably fine. :)

[erikwright (departed), 2014-07-18 13:00:30]

jochen: PTAL for chrome/*

[jochen (gone - plz use gerrit), 2014-07-18 13:05:46]

lgtm

[erikwright (departed), 2014-07-18 13:07:11]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-07-18 13:08:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/402723002/40001

[commit-bot: I haz the power, 2014-07-18 15:49:07]

Change committed as 284100

[tonyg, 2014-07-18 21:11:23]

A revert of this CL has been created in
https://codereview.chromium.org/401913002/ by tonyg@chromium.org.

The reason for reverting is: Crashes chromium.perf bots.

	ChildEBP RetAddr  
	0052f9f4 6a47d2ff chrome_69f00000!base::debug::BreakDebugger+0x10
	0052fa64 6a47df08 chrome_69f00000!CheckIsChromeSxSProcess+0x26
	0052fa68 6a47e664 chrome_69f00000!InstallUtil::IsChromeSxSProcess+0x16
	0052fa70 6a47e53b
chrome_69f00000!BrowserDistribution::GetSpecificDistribution+0x3b
	0052fa78 6a47bb77 chrome_69f00000!BrowserDistribution::GetDistribution+0x7
	0052fb64 6a47bb2e chrome_69f00000!`anonymous
namespace'::GetChromeChannelInternal+0x2c
	0052fb7c 6a2d03f8 chrome_69f00000!GoogleUpdateSettings::GetChromeChannel+0x21
	0052fbe8 6a2cd0d8 chrome_69f00000!chrome::VersionInfo::GetChannel+0x60
	0052fbec 6a2cd1ab chrome_69f00000!`anonymous namespace'::GetBeaconKeyPath+0x5
	0052fc08 6a2cc624
chrome_69f00000!ShouldExperimentallyDisableTerminateOnHeapCorruption+0xe
	0052fc50 0020720b chrome_69f00000!ChromeMain+0x42
	0052fce0 00206c36 chrome!MainDllLoader::Launch+0x15f
	0052fd24 00228c9e chrome!wWinMain+0x5a
	0052fd70 766e338a chrome!__tmainCRTStartup+0xfd
	WARNING: Stack unwind information not available. Following frames may be wrong.
	0052fd7c 76f99f72 kernel32!BaseThreadInitThunk+0x12
	0052fdbc 76f99f45 ntdll!RtlInitializeExceptionChain+0x63
	0052fdd4 00000000 ntdll!RtlInitializeExceptionChain+0x36.

[erikwright (departed), 2014-07-18 23:17:58]

jochen: PTAL for chrome/* changes since patchset 3.

This failed on the perf bots because they do an official Chrome build. I guess
everything else was Chromium. So the check for IsCanary is very simple in
Chromium builds but in Chrome it requires the CommandLine, which wasn't
initialized.

I added some code to temporarily initialize it, if necessary, to allow the check
for Canary to pass.

In case you are wondering if I should instead just randomly assign the group
locally instead of doing it via the server and then checking in the registry,
here is a message I sent to WFH on that same subject:

(1) According to the Finch team I can't even locally assign a field trial that
early. So even if I do what you suggest, in fact I need to do something strange
to persist the data locally temporarily, and then subsequently store it in the
field trial list for uploading. In fact, Alexei and I decided that the current
approach is simpler.
(2) Like this I get the ability to very quickly turn it off if the TPMs get
concerned by the (expected) changes in crash data. Keep in mind that if my
theory is right I will totally mess with their numbers and I want to be able to
make things right fast.
(3) Also, I conserve in my back pocket the ability to turn this on for Dev if
the Canary data is insufficient.

When it comes time to turn it off I will first disable the experiment for a good
while before removing the code. So any client that connects in that period will
delete the key. And leaving the key is harmless (it will be deleted if Chrome is
uninstalled).

Again, although it's a bit unpleasant, this code will be reverted in the next
few weeks.

[erikwright (departed), 2014-07-19 00:46:32]

On 2014/07/18 23:17:58, erikwright OOO until July 28th wrote:
> jochen: PTAL for chrome/* changes since patchset 3.
> 
> This failed on the perf bots because they do an official Chrome build. I guess
> everything else was Chromium. So the check for IsCanary is very simple in
> Chromium builds but in Chrome it requires the CommandLine, which wasn't
> initialized.
> 
> I added some code to temporarily initialize it, if necessary, to allow the
check
> for Canary to pass.
> 
> In case you are wondering if I should instead just randomly assign the group
> locally instead of doing it via the server and then checking in the registry,
> here is a message I sent to WFH on that same subject:
> 
> (1) According to the Finch team I can't even locally assign a field trial that
> early. So even if I do what you suggest, in fact I need to do something
strange
> to persist the data locally temporarily, and then subsequently store it in the
> field trial list for uploading. In fact, Alexei and I decided that the current
> approach is simpler.
> (2) Like this I get the ability to very quickly turn it off if the TPMs get
> concerned by the (expected) changes in crash data. Keep in mind that if my
> theory is right I will totally mess with their numbers and I want to be able
to
> make things right fast.
> (3) Also, I conserve in my back pocket the ability to turn this on for Dev if
> the Canary data is insufficient.
> 
> When it comes time to turn it off I will first disable the experiment for a
good
> while before removing the code. So any client that connects in that period
will
> delete the key. And leaving the key is harmless (it will be deleted if Chrome
is
> uninstalled).
> 
> Again, although it's a bit unpleasant, this code will be reverted in the next
> few weeks.

I did an official build and manually tested this with and without the
--chrome-sxs flag.  The crash is resolved.

[erikwright (departed), 2014-07-22 02:45:52]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-07-22 02:47:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/402723002/100001

[commit-bot: I haz the power, 2014-07-22 02:50:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-22 02:52:52]

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

[commit-bot: I haz the power, 2014-07-22 02:52:53]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[erikwright (departed), 2014-07-28 13:38:28]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-07-28 13:39:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/402723002/100001

[erikwright (departed), 2014-07-28 14:38:03]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-07-28 14:39:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/402723002/140001

[commit-bot: I haz the power, 2014-07-28 17:15:52]

Change committed as 285931

[grt (UTC plus 2), 2014-07-30 21:05:12]

Hi Eric. Just sending out what we discussed via chat so that it's on the record.
Not sure if this makes sense or not. Cheers.

https://codereview.chromium.org/402723002/diff/140001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/402723002/diff/140001/content/app/content_mai...
content/app/content_main_runner.cc:558:
base::EnableTerminationOnHeapCorruption();
if it's okay for this to happen on line 574 instead of here, consider doing
something like this down there after the AtExitManager has been created:

  if (delegate_ &&
!delegate_->ShouldExperimentallyDisableTerminateOnHeapCorruption()) {
    base::EnableTerminationOnHeapCorruption();
  }

i haven't understood exactly what's going on here, so maybe that's tricky to do,
but it would allow you to keep the ShouldFoo check in chrome-land but do the
heap corruption thing here early in content-land.

[Nico, 2014-08-03 05:56:03]

https://codereview.chromium.org/402723002/diff/140001/chrome/common/terminate...
File chrome/common/terminate_on_heap_corruption_experiment_win.cc (right):

https://codereview.chromium.org/402723002/diff/140001/chrome/common/terminate...
chrome/common/terminate_on_heap_corruption_experiment_win.cc:42: return
L"SOFTWARE\\" PRODUCT_STRING_PATH
..\..\chrome\common\terminate_on_heap_corruption_experiment_win.cc(42,12) : 
warning(clang): ISO C++11 does not allow conversion from string literal to
'wchar_t *' [-Wwritable-strings]
    return L"SOFTWARE\\" PRODUCT_STRING_PATH
           ^
..\..\chrome\common\terminate_on_heap_corruption_experiment_win.cc(45,10) : 
warning(clang): ISO C++11 does not allow conversion from string literal to
'wchar_t *' [-Wwritable-strings]
  return L"SOFTWARE\\" PRODUCT_STRING_PATH
         ^


Can you make this return a const wchar_t*?