Use a Finch Experiment to control the Browser Blacklist

Use a Finch Experiment to control the Browser Blacklist

Also increase the states stored in the register to prevent
the setup code from running multiple times per version if
it fails to start one time.

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

[csharp, 2013-12-24 19:27:15]

robertshield as owner for chrome_elf/*
jwd as owner for histograms.xml
thakis as owner chrome/browser*

[Ilya Sherman, 2013-12-27 01:22:50]

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:80: };
Please document that this histogram is used to back an UMA_HISTOGRAM_ENUMERATION
metric, and hence (a) existing constants should never be deleted or re-arranged;
and (b) new constants should only ever be added at the end of the histogram. 
See [ https://sites.google.com/a/chromium.org/dev/Home/user-metrics ] if you'd
like a template comment.  (Feel free to improve the template as well, I just now
wrote it, so it's only had one pair of eyes on it.)

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:299: BLACKLIST_SETUP_EVENT_MAX);
Please create a wrapper method to emit to this histogram, so that all of the
shared parts of the call are just in one place.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:350: }
I've mostly only looked at the histograms-related code in this CL, but it seems
wrong to me to include so much extra code in chrome_browser_main_win.cc,
especially inline in this method.  Can you factor this out into a separate
method, preferably in a more specialized implementation file?

https://codereview.chromium.org/120963002/diff/70001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/120963002/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:1274: +    code. Used to determine if
the blacklist is working as intended  (since the
nit: Extraneous second space after "intended"

https://codereview.chromium.org/120963002/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:1275: +    blacklist runs before crash
reporting is setup).
nit: "is setup" -> "is set up" ("setup" is a noun, "set up" is a verb)

[robertshield, 2013-12-27 03:24:28]

Nice patch, a few comments and the like.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:280: DWORD blacklist_state;
= 0

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:282: ::RegGetValue(HKEY_CURRENT_USER,
Please use base/win/registry.h instead of the advapi32 functions while in
chrome/

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:313: wchar_t key_data[255];
 = {}

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:350: }
On 2013/12/27 01:22:50, Ilya Sherman (Away thru Jan 6) wrote:
> I've mostly only looked at the histograms-related code in this CL, but it
seems
> wrong to me to include so much extra code in chrome_browser_main_win.cc,
> especially inline in this method.  Can you factor this out into a separate
> method, preferably in a more specialized implementation file?

+1

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
File chrome/browser/chrome_main_browsertest.cc (right):

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:182: DWORD blacklist_state;
 = 0

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:185: ::RegGetValue(HKEY_CURRENT_USER,
use base/win/registry here too

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:196: wchar_t key_data[255];
 = {}

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:214: class
ChromeBlacklistTrialFirstRun : public ChromeBlacklistTrialTest {
This class appears to be empty, does it need to exist, or can
ChromeBlacklistTrialTest be used as the fixture directly below.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:237: // Set the registry to indicate
that the blacklist setup is  running,
nit: double space before running

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:151: DWORD blacklist_state;
 = 0

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:153: ::RegGetValue(HKEY_CURRENT_USER,
Please ensure that the addition of these advapi calls maintains the correct
loading order. Running the tests here is an easy way to do that:
https://codereview.chromium.org/109483003/

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:43: // Looks for the beacon that
LeaveSetupBeacon() creates and reset it to
s/reset/resets

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:45: // Returns true if the beacon was
successfully deleted.
Update this last sentence.

[csharp, 2014-01-02 19:55:36]

Moved the setup code into it's own file (chrome_elf_init_win) and converted the
browser tests I had added into unit tests (same coverage).

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:80: };
On 2013/12/27 01:22:50, Ilya Sherman (Away thru Jan 6) wrote:
> Please document that this histogram is used to back an
UMA_HISTOGRAM_ENUMERATION
> metric, and hence (a) existing constants should never be deleted or
re-arranged;
> and (b) new constants should only ever be added at the end of the histogram. 
> See [ https://sites.google.com/a/chromium.org/dev/Home/user-metrics ] if you'd
> like a template comment.  (Feel free to improve the template as well, I just
now
> wrote it, so it's only had one pair of eyes on it.)

Added the comment although I reworded part b to say new elements should be
inserted in front of BLACKLIST_SETUP_EVENT_MAX. I'm not quite sure how or if the
comment template should be adjusted to mention this fact (since the _MAX value
isn't really part of the histogram and it can be moved further down).

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:280: DWORD blacklist_state;
On 2013/12/27 03:24:29, robertshield wrote:
> = 0

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:282: ::RegGetValue(HKEY_CURRENT_USER,
On 2013/12/27 03:24:29, robertshield wrote:
> Please use base/win/registry.h instead of the advapi32 functions while in
> chrome/

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:299: BLACKLIST_SETUP_EVENT_MAX);
On 2013/12/27 01:22:50, Ilya Sherman (Away thru Jan 6) wrote:
> Please create a wrapper method to emit to this histogram, so that all of the
> shared parts of the call are just in one place.

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:313: wchar_t key_data[255];
On 2013/12/27 03:24:29, robertshield wrote:
>  = {}

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main_win.cc:350: }
On 2013/12/27 03:24:29, robertshield wrote:
> On 2013/12/27 01:22:50, Ilya Sherman (Away thru Jan 6) wrote:
> > I've mostly only looked at the histograms-related code in this CL, but it
> seems
> > wrong to me to include so much extra code in chrome_browser_main_win.cc,
> > especially inline in this method.  Can you factor this out into a separate
> > method, preferably in a more specialized implementation file?
> 
> +1

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
File chrome/browser/chrome_main_browsertest.cc (right):

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:182: DWORD blacklist_state;
On 2013/12/27 03:24:29, robertshield wrote:
>  = 0

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:185: ::RegGetValue(HKEY_CURRENT_USER,
On 2013/12/27 03:24:29, robertshield wrote:
> use base/win/registry here too

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:196: wchar_t key_data[255];
On 2013/12/27 03:24:29, robertshield wrote:
>  = {}

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:214: class
ChromeBlacklistTrialFirstRun : public ChromeBlacklistTrialTest {
On 2013/12/27 03:24:29, robertshield wrote:
> This class appears to be empty, does it need to exist, or can
> ChromeBlacklistTrialTest be used as the fixture directly below.

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome/browser/chrome_ma...
chrome/browser/chrome_main_browsertest.cc:237: // Set the registry to indicate
that the blacklist setup is  running,
On 2013/12/27 03:24:29, robertshield wrote:
> nit: double space before running

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:151: DWORD blacklist_state;
On 2013/12/27 03:24:29, robertshield wrote:
>  = 0

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:153: ::RegGetValue(HKEY_CURRENT_USER,
On 2013/12/27 03:24:29, robertshield wrote:
> Please ensure that the addition of these advapi calls maintains the correct
> loading order. Running the tests here is an easy way to do that:
> https://codereview.chromium.org/109483003/ 

Will do once that cl is submitted.

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:43: // Looks for the beacon that
LeaveSetupBeacon() creates and reset it to
On 2013/12/27 03:24:29, robertshield wrote:
> s/reset/resets

Done.

https://codereview.chromium.org/120963002/diff/70001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:45: // Returns true if the beacon was
successfully deleted.
On 2013/12/27 03:24:29, robertshield wrote:
> Update this last sentence.

Done.

https://codereview.chromium.org/120963002/diff/70001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/120963002/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:1274: +    code. Used to determine if
the blacklist is working as intended  (since the
On 2013/12/27 01:22:50, Ilya Sherman (Away thru Jan 6) wrote:
> nit: Extraneous second space after "intended"

Done.

https://codereview.chromium.org/120963002/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:1275: +    blacklist runs before crash
reporting is setup).
On 2013/12/27 01:22:50, Ilya Sherman (Away thru Jan 6) wrote:
> nit: "is setup" -> "is set up" ("setup" is a noun, "set up" is a verb)

Done.

[csharp, 2014-01-02 20:39:38]

Made a small change were the success or failed metrics are only reported if the
version didn't change.

If the version did change, then we'd be reporting stats for the wrong version
(since it would get counted for the new version).

[robertshield, 2014-01-02 20:42:21]

https://codereview.chromium.org/120963002/diff/360001/base/win/registry.h
File base/win/registry.h (right):

https://codereview.chromium.org/120963002/diff/360001/base/win/registry.h#new...
base/win/registry.h:60: // Returns the number of values for this key, or 0 if
the number cannot be
recommend extracting this change into its own CL and TBRing OWNERs for it.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
2014

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:16: const char
kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";
nit: alphabetize these

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:59: KEY_READ | KEY_WRITE);
I believe this should be KEY_QUERY_VALUE | KEY_SET_VALUE.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:68:
RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED);
as discussed, can we check the version here to make sure we don't misreport it
across an upgrade?

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:83: base::string16 current_version =
base::UTF8ToUTF16(version_info.Version());
nit: prefer direct construction vs assignment (it makes it clearer that a
constructor is being called):
base::string16 current_version(base::UTF8ToUTF16(version_info.Version()));

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:10: void SetupChromeElfInitialization();
How about InitializeChromeElf()? Having "Initialize" and "Setup" in the same
name seems redundant.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:12: // Correctly set the required state for
an enabled browser blacklist.
Drop the word "Correctly" since that is hopefully implicit.

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:151: DWORD blacklist_state = 0;
= BLACKLIST_DISABLED;

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:159: &blacklist_state_size);
Check the return value of RegGetValue and also fail on non-ERROR_SUCCESS in the
check immediately below.

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:62:
EXPECT_FALSE(blacklist::LeaveSetupBeacon());
Also check that ResetBeacon works after calling LeaveSetupBeacon.

[robertshield, 2014-01-02 20:42:21]

https://codereview.chromium.org/120963002/diff/360001/base/win/registry.h
File base/win/registry.h (right):

https://codereview.chromium.org/120963002/diff/360001/base/win/registry.h#new...
base/win/registry.h:60: // Returns the number of values for this key, or 0 if
the number cannot be
recommend extracting this change into its own CL and TBRing OWNERs for it.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
2014

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:16: const char
kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";
nit: alphabetize these

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:59: KEY_READ | KEY_WRITE);
I believe this should be KEY_QUERY_VALUE | KEY_SET_VALUE.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:68:
RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED);
as discussed, can we check the version here to make sure we don't misreport it
across an upgrade?

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:83: base::string16 current_version =
base::UTF8ToUTF16(version_info.Version());
nit: prefer direct construction vs assignment (it makes it clearer that a
constructor is being called):
base::string16 current_version(base::UTF8ToUTF16(version_info.Version()));

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:10: void SetupChromeElfInitialization();
How about InitializeChromeElf()? Having "Initialize" and "Setup" in the same
name seems redundant.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:12: // Correctly set the required state for
an enabled browser blacklist.
Drop the word "Correctly" since that is hopefully implicit.

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:151: DWORD blacklist_state = 0;
= BLACKLIST_DISABLED;

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:159: &blacklist_state_size);
Check the return value of RegGetValue and also fail on non-ERROR_SUCCESS in the
check immediately below.

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:62:
EXPECT_FALSE(blacklist::LeaveSetupBeacon());
Also check that ResetBeacon works after calling LeaveSetupBeacon.

[robertshield, 2014-01-02 20:46:19]

Looks great, couple more nits

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:62: std::wstring blacklist_version(L"");
Use base::string16 rather than wstring.

Also, don't need the (L"") bit since wstrings are by default initialized to the
empty string.

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:78: // Don't try to record the results of
the last setup since it could have
explicitly state here that by "results" we mean either
BLACKLIST_SETUP_RAN_SUCCESSFULLY or BLACKLIST_SETUP_FAILED

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:84: DWORD blacklist_state = 0;
= BLACKLIST_SETUP_ENABLED;

[csharp, 2014-01-02 21:57:58]

https://codereview.chromium.org/120963002/diff/360001/base/win/registry.h
File base/win/registry.h (right):

https://codereview.chromium.org/120963002/diff/360001/base/win/registry.h#new...
base/win/registry.h:60: // Returns the number of values for this key, or 0 if
the number cannot be
On 2014/01/02 20:42:22, robertshield wrote:
> recommend extracting this change into its own CL and TBRing OWNERs for it.

Done.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
On 2014/01/02 20:42:22, robertshield wrote:
> 2014

Done.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:16: const char
kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";
On 2014/01/02 20:42:22, robertshield wrote:
> nit: alphabetize these

I looked at a couple files and this order (trial name followed by group names)
seems to be the general pattern. 

Although maybe jwd@ or isherman@ can shed more light on this.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:59: KEY_READ | KEY_WRITE);
On 2014/01/02 20:42:22, robertshield wrote:
> I believe this should be KEY_QUERY_VALUE | KEY_SET_VALUE.

Yup, that seems to work.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:68:
RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED);
On 2014/01/02 20:42:22, robertshield wrote:
> as discussed, can we check the version here to make sure we don't misreport it
> across an upgrade?

Done.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:83: base::string16 current_version =
base::UTF8ToUTF16(version_info.Version());
On 2014/01/02 20:42:22, robertshield wrote:
> nit: prefer direct construction vs assignment (it makes it clearer that a
> constructor is being called):
> base::string16 current_version(base::UTF8ToUTF16(version_info.Version()));

Done.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:10: void SetupChromeElfInitialization();
On 2014/01/02 20:42:22, robertshield wrote:
> How about InitializeChromeElf()? Having "Initialize" and "Setup" in the same
> name seems redundant.

I wasn't quite sure since we aren't really initializing here, but are setting up
code for the next initialization.

Changed since the old name wasn't really expressing the desired meaning.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:12: // Correctly set the required state for
an enabled browser blacklist.
On 2014/01/02 20:42:22, robertshield wrote:
> Drop the word "Correctly" since that is hopefully implicit.

Done.

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:151: DWORD blacklist_state = 0;
On 2014/01/02 20:42:22, robertshield wrote:
> = BLACKLIST_DISABLED;

Done.

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:159: &blacklist_state_size);
On 2014/01/02 20:42:22, robertshield wrote:
> Check the return value of RegGetValue and also fail on non-ERROR_SUCCESS in
the
> check immediately below.

Done.

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:62:
EXPECT_FALSE(blacklist::LeaveSetupBeacon());
On 2014/01/02 20:42:22, robertshield wrote:
> Also check that ResetBeacon works after calling LeaveSetupBeacon.

Done.

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:62: std::wstring blacklist_version(L"");
On 2014/01/02 20:46:20, robertshield wrote:
> Use base::string16 rather than wstring.
> 
> Also, don't need the (L"") bit since wstrings are by default initialized to
the
> empty string.

Hmm, I thought I got a compile error when I tried that before, but it does work.
Clearly I'm going insane :)

Done.

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:78: // Don't try to record the results of
the last setup since it could have
On 2014/01/02 20:46:20, robertshield wrote:
> explicitly state here that by "results" we mean either
> BLACKLIST_SETUP_RAN_SUCCESSFULLY or BLACKLIST_SETUP_FAILED

Done.

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:84: DWORD blacklist_state = 0;
On 2014/01/02 20:46:20, robertshield wrote:
> = BLACKLIST_SETUP_ENABLED;

We are reading the value here, not writing. Setting blacklist_state to a dummy
value so we won't be anything if we fail to read it from the registry.

[robertshield, 2014-01-03 03:21:41]

Awesome, nice work! LGTM with just a couple more nits.

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/360001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:16: const char
kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";
On 2014/01/02 21:57:59, csharp wrote:
> On 2014/01/02 20:42:22, robertshield wrote:
> > nit: alphabetize these
> 
> I looked at a couple files and this order (trial name followed by group names)
> seems to be the general pattern. 
> 
> Although maybe jwd@ or isherman@ can shed more light on this.

Ok, not a strong preference. If this is conventional I'm fine with it.

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/420001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:84: DWORD blacklist_state = 0;
On 2014/01/02 21:57:59, csharp wrote:
> On 2014/01/02 20:46:20, robertshield wrote:
> > = BLACKLIST_SETUP_ENABLED;
> 
> We are reading the value here, not writing. Setting blacklist_state to a dummy
> value so we won't be anything if we fail to read it from the registry.

0 isn't a dummy value though, it's a legitimate enum value - in this case
BLACKLIST_DISABLED (not BLACKLIST_SETUP_ENABLED, sorry I mixed up the enums
earlier).

Since the DWORD is effectively holding one of these enum values, you should
initialize blacklist_state with an identifier from the enum, or a value not
present in it (like -1)...

And you already did this in the latest patch, so huzzah!

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:86: base::string16 version =
base::UTF8ToUTF16(version_info.Version());
base::string16 version(base...

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:116: base::string16
expected_version = base::UTF8ToUTF16(version_info.Version());
same as above

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:10: void InitializeChromeElf();
Hmm, I see what you mean, maybe InitializeChromeElfState()? If you don't like
that more then you can also just ask me to stop complaining about names and
leave it as is :-)

[csharp, 2014-01-03 14:59:35]

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:86: base::string16 version =
base::UTF8ToUTF16(version_info.Version());
On 2014/01/03 03:21:43, robertshield wrote:
> base::string16 version(base...

Done.

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:116: base::string16
expected_version = base::UTF8ToUTF16(version_info.Version());
On 2014/01/03 03:21:43, robertshield wrote:
> same as above

Done.

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/120963002/diff/530001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:10: void InitializeChromeElf();
On 2014/01/03 03:21:43, robertshield wrote:
> Hmm, I see what you mean, maybe InitializeChromeElfState()? If you don't like
> that more then you can also just ask me to stop complaining about names and
> leave it as is :-)

I think I'll leave it as is, State still makes me think we are referring to the
current state.

[Alexei Svitkine (slow), 2014-01-06 20:43:04]

histograms and field trial lgtm % comments

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:9: #include
"chrome/browser/chrome_elf_init_win.h"
This should be the first include. (For a while both ways were used for tests,
but now the best practice is to put this first in the test, supported by the
style guide.)

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:21: virtual void SetUp() {
Nit: OVERRIDE

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:68:
blacklist_registry_key_.ReadValueDW(blacklist::kBeaconState,
Why do you both have helpers in the test class (GetBlacklistState()) while using
the key directly. Either do it one way consistently or the other. Also, the
helper functions don't seem to be checking the return values.

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:63:
blacklist_registry_key.ReadValue(blacklist::kBeaconVersion,
Can this fail? (Should this check the return value?).

https://codereview.chromium.org/120963002/diff/650001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/120963002/diff/650001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1274: +    code. Used to determine if
the blacklist is working as intended (since the
Mention that this is done on startup on and is specific to Windows.

[csharp, 2014-01-07 20:04:02]

thakis@ ping for owner's approval of chrome/browser/*

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:9: #include
"chrome/browser/chrome_elf_init_win.h"
On 2014/01/06 20:43:05, Alexei Svitkine wrote:
> This should be the first include. (For a while both ways were used for tests,
> but now the best practice is to put this first in the test, supported by the
> style guide.)

Done.

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:21: virtual void SetUp() {
On 2014/01/06 20:43:05, Alexei Svitkine wrote:
> Nit: OVERRIDE

Done.

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:68:
blacklist_registry_key_.ReadValueDW(blacklist::kBeaconState,
On 2014/01/06 20:43:05, Alexei Svitkine wrote:
> Why do you both have helpers in the test class (GetBlacklistState()) while
using
> the key directly. Either do it one way consistently or the other. Also, the
> helper functions don't seem to be checking the return values.

Modified this code to use the test helpers.
The test helpers still don't directly check for the return value (ASSERT_EQ
can't be compiled in non-void helper functions), but they will return either an
empty string or an invalid enum (which should be good enough signals that the
get failed).

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/650001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:63:
blacklist_registry_key.ReadValue(blacklist::kBeaconVersion,
On 2014/01/06 20:43:05, Alexei Svitkine wrote:
> Can this fail? (Should this check the return value?).

This can potentially fail, but I don't think that matters.

If it fails blacklist_version will be empty, so we will act as if the blacklist
code hasn't been setup yet and set it up (which seems ok to me, since if we
can't read the value, we probably need to set it up again).

https://codereview.chromium.org/120963002/diff/650001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/120963002/diff/650001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1274: +    code. Used to determine if
the blacklist is working as intended (since the
On 2014/01/06 20:43:05, Alexei Svitkine wrote:
> Mention that this is done on startup on and is specific to Windows.

Done.

[Nico, 2014-01-07 21:27:44]

Sorry, I lack context to say anything useful. What is chrome_elf? Any reason
it's not in chrome/elf? "elf" is usually the linux binary format, but here it's
used on windows and it sounds like this isn't what it means?

Will this additional dependency add to browser binary size, or is this code
already linked in?

Then this is writing to the windows registry, which I wasn't aware we're doing
much in new code.

All-in-all, you might be happier with a more windows-knowledgeable chrome owner
(sky, probably)

[csharp, 2014-01-07 21:44:18]

Adding sky for owner's approval of chrome/browser/*

quick fyi (mainly for thakis),
chrome_elf is the chrome early loading framework and its purpose in life is to
startup very early in the chrome process so we can do certain thing before too
much has happened (such as setting up a browser dll blacklist before
appinit_dlls are loaded).
We use the registry because elf needs to know if it should activate the
blacklist or not and reading state from files would be too slow (and not easy
due to a lot of functionality that wouldn't be loaded yet).

[sky, 2014-01-07 23:15:08]

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_win.cc:257: InitializeChromeElf();
Should we only do this in official builds?

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:17: ChromeBlacklistTrialTest() :
blacklist_registry_key_(
nit: since multiline start member initialize on next line.

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:69: ASSERT_EQ(L"",
GetBlacklistVersion());
L"" -> base::string16()

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:63:
blacklist_registry_key.ReadValue(blacklist::kBeaconVersion,
What will the effects be of someone running two versions of chrome with
different beacon versions (say a dev channel and stable channel with different
profile dirs)? Seems like the values will bounce around and that there may be
race conditions.

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:80: if (set_version == ERROR_SUCCESS &&
set_state == ERROR_SUCCESS) {
nit: no {}

[csharp, 2014-01-08 21:07:06]

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_win.cc:257: InitializeChromeElf();
On 2014/01/07 23:15:08, sky wrote:
> Should we only do this in official builds?

I don't think so since the chrome elf is present in unofficial builds as well.

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:17: ChromeBlacklistTrialTest() :
blacklist_registry_key_(
On 2014/01/07 23:15:08, sky wrote:
> nit: since multiline start member initialize on next line.

Done.

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:69: ASSERT_EQ(L"",
GetBlacklistVersion());
On 2014/01/07 23:15:08, sky wrote:
> L"" -> base::string16()

Done.

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:63:
blacklist_registry_key.ReadValue(blacklist::kBeaconVersion,
On 2014/01/07 23:15:08, sky wrote:
> What will the effects be of someone running two versions of chrome with
> different beacon versions (say a dev channel and stable channel with different
> profile dirs)? Seems like the values will bounce around and that there may be
> race conditions.

Different user installs should interact with each other since these values are
nested in HKEY_CURRENT_KEY. However, we could run into trouble with users that
canary and another channels installed.

I've fixed this issue by adding a check in the code that actually checks if it
is enabled or disabled to only be enabled if the proper beacon is set and the
version match. The collisions that occur just mean that the blacklist might be
disabled for some runs for no good reason, but a working version of chrome's
blacklist won't ever enable the blacklist for a broken version.

https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:80: if (set_version == ERROR_SUCCESS &&
set_state == ERROR_SUCCESS) {
On 2014/01/07 23:15:08, sky wrote:
> nit: no {}

Done.

[sky, 2014-01-08 21:28:35]

Would it be better to have per channel registry values?

On Wed, Jan 8, 2014 at 1:07 PM,  <csharp@chromium.org> wrote:
>
>
https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_b...
> File chrome/browser/chrome_browser_main_win.cc (right):
>
>
https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_b...
> chrome/browser/chrome_browser_main_win.cc:257: InitializeChromeElf();
> On 2014/01/07 23:15:08, sky wrote:
>>
>> Should we only do this in official builds?
>
>
> I don't think so since the chrome elf is present in unofficial builds as
> well.
>
>
>
https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
> File chrome/browser/chrome_elf_init_unittest_win.cc (right):
>
>
https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
> chrome/browser/chrome_elf_init_unittest_win.cc:17:
> ChromeBlacklistTrialTest() : blacklist_registry_key_(
> On 2014/01/07 23:15:08, sky wrote:
>>
>> nit: since multiline start member initialize on next line.
>
>
> Done.
>
>
>
https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
> chrome/browser/chrome_elf_init_unittest_win.cc:69: ASSERT_EQ(L"",
> GetBlacklistVersion());
> On 2014/01/07 23:15:08, sky wrote:
>>
>> L"" -> base::string16()
>
>
> Done.
>
>
>
https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
> File chrome/browser/chrome_elf_init_win.cc (right):
>
>
https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
> chrome/browser/chrome_elf_init_win.cc:63:
> blacklist_registry_key.ReadValue(blacklist::kBeaconVersion,
> On 2014/01/07 23:15:08, sky wrote:
>>
>> What will the effects be of someone running two versions of chrome
>
> with
>>
>> different beacon versions (say a dev channel and stable channel with
>
> different
>>
>> profile dirs)? Seems like the values will bounce around and that there
>
> may be
>>
>> race conditions.
>
>
> Different user installs should interact with each other since these
> values are nested in HKEY_CURRENT_KEY. However, we could run into
> trouble with users that canary and another channels installed.
>
> I've fixed this issue by adding a check in the code that actually checks
> if it is enabled or disabled to only be enabled if the proper beacon is
> set and the version match. The collisions that occur just mean that the
> blacklist might be disabled for some runs for no good reason, but a
> working version of chrome's blacklist won't ever enable the blacklist
> for a broken version.
>
>
>
https://codereview.chromium.org/120963002/diff/780001/chrome/browser/chrome_e...
> chrome/browser/chrome_elf_init_win.cc:80: if (set_version ==
> ERROR_SUCCESS && set_state == ERROR_SUCCESS) {
> On 2014/01/07 23:15:08, sky wrote:
>>
>> nit: no {}
>
>
> Done.
>
> https://codereview.chromium.org/120963002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[csharp, 2014-01-08 21:32:37]

On 2014/01/08 21:28:35, sky wrote:
> Would it be better to have per channel registry values?
>

Potentially, but I don't there is a good way for blacklist.cc to easily figure
out what channel it is on (since it is executed early in Chrome's startup). The
amount of users that might get affected by this should be pretty small, and we
do hope to remove this switch in the future (once we've seen this function
working and hopefully added breakpad support to elf) so it seems like an
acceptable problem to live with.

[sky, 2014-01-08 23:36:14]

Ok, LGTM

[commit-bot: I haz the power, 2014-01-09 14:36:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/120963002/900001

[commit-bot: I haz the power, 2014-01-09 14:58:32]

Retried try job too often on linux_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-09 15:18:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/120963002/900001

[commit-bot: I haz the power, 2014-01-09 20:36:41]

Change committed as 243967