Add UMA stats to record when DLLs are successfully blocked in the Browser.

Add UMA stats to record when DLLs are successfully blocked in the Browser.

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

[csharp, 2014-02-20 19:48:37]

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:158: base::MD5Digest hash;
This hashing approach seems pretty silly, but I couldn't think of a better way
to do this. Alexei, any advice/tips? :)

[robertshield, 2014-02-24 15:24:04]

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:65: // Report all successful blacklist
interception.
interceptions

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:68: // Schedule another task to report all
sucessfully interceptions later.
successful

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:69: // This timne delay should be long
enough to catch any dlls that attempt to
time

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:74: base::TimeDelta::FromSeconds(600));
Make the 600 a constant declared at the top with the other constants.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:139: typedef
void(*SuccessfullyBlocked)(const wchar_t**, int*);
s/SuccessfullyBlocked/SuccessfullyBlockedPtr/

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:143: "SuccessfullyBlocked"));
line this up either with a four space indent or with the other parameter to
GetModuleHandle (or use git cl format)

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:149: int num_blocked_dlls;
= 0

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:153: std::vector<const wchar_t *>
blocked_dlls(num_blocked_dlls);
wchar_t* (no space)

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:154:
successfully_blocked(&(blocked_dlls[0]), &num_blocked_dlls);
don't need the parens: &blocked_dlls[0]

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.h:15: // Report with DLLs were prevented from
being loaded.
which

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.h:16: void ReportSuccessfulBlocks();
is this used outside of the .cc file? if not, move it to the anonymous namespace
in the .cc file and take the declaration out of this header.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.cc:330: void SuccessfullyBlocked(const wchar_t**
blocked_dlls, int* size) {
null check the inputs

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.cc:339: for (int i = 0; strings_to_fill !=
g_num_blocked_dlls; ++i) {
be paranoid: also bounds check the index |i| against the length of the list it
is indexing into.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.h:79: // |size| will always be modify to be the
number of dlls that were blocked.
be modified

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.h:80: extern "C" void SuccessfullyBlocked(const
wchar_t** blocked_dlls, int* size);
document the memory management specifics here, i.e.: the caller does not own the
returned strings and is not expected to free them

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/test/bl...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/test/bl...
chrome_elf/blacklist/test/blacklist_test.cc:136: int size;
= 0

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/test/bl...
chrome_elf/blacklist/test/blacklist_test.cc:162: int num_blocked_dlls;
= 0

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/test/bl...
chrome_elf/blacklist/test/blacklist_test.cc:186: const wchar_t* blocked_dll;
= NULL

[csharp, 2014-02-24 21:37:15]

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:65: // Report all successful blacklist
interception.
On 2014/02/24 15:24:05, robertshield wrote:
> interceptions

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:68: // Schedule another task to report all
sucessfully interceptions later.
On 2014/02/24 15:24:05, robertshield wrote:
> successful

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:69: // This timne delay should be long
enough to catch any dlls that attempt to
On 2014/02/24 15:24:05, robertshield wrote:
> time

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:74: base::TimeDelta::FromSeconds(600));
On 2014/02/24 15:24:05, robertshield wrote:
> Make the 600 a constant declared at the top with the other constants.

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:139: typedef
void(*SuccessfullyBlocked)(const wchar_t**, int*);
On 2014/02/24 15:24:05, robertshield wrote:
> s/SuccessfullyBlocked/SuccessfullyBlockedPtr/

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:143: "SuccessfullyBlocked"));
On 2014/02/24 15:24:05, robertshield wrote:
> line this up either with a four space indent or with the other parameter to
> GetModuleHandle (or use git cl format)

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:149: int num_blocked_dlls;
On 2014/02/24 15:24:05, robertshield wrote:
> = 0

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:153: std::vector<const wchar_t *>
blocked_dlls(num_blocked_dlls);
On 2014/02/24 15:24:05, robertshield wrote:
> wchar_t* (no space)

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.cc:154:
successfully_blocked(&(blocked_dlls[0]), &num_blocked_dlls);
On 2014/02/24 15:24:05, robertshield wrote:
> don't need the parens: &blocked_dlls[0]

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.h:15: // Report with DLLs were prevented from
being loaded.
On 2014/02/24 15:24:05, robertshield wrote:
> which

Done.

https://codereview.chromium.org/174013007/diff/1/chrome/browser/chrome_elf_in...
chrome/browser/chrome_elf_init_win.h:16: void ReportSuccessfulBlocks();
On 2014/02/24 15:24:05, robertshield wrote:
> is this used outside of the .cc file? if not, move it to the anonymous
namespace
> in the .cc file and take the declaration out of this header.

Done.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.cc:330: void SuccessfullyBlocked(const wchar_t**
blocked_dlls, int* size) {
On 2014/02/24 15:24:05, robertshield wrote:
> null check the inputs

Done.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.cc:339: for (int i = 0; strings_to_fill !=
g_num_blocked_dlls; ++i) {
On 2014/02/24 15:24:05, robertshield wrote:
> be paranoid: also bounds check the index |i| against the length of the list it
> is indexing into.

Done.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.h:79: // |size| will always be modify to be the
number of dlls that were blocked.
On 2014/02/24 15:24:05, robertshield wrote:
> be modified

Done.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.h:80: extern "C" void SuccessfullyBlocked(const
wchar_t** blocked_dlls, int* size);
On 2014/02/24 15:24:05, robertshield wrote:
> document the memory management specifics here, i.e.: the caller does not own
the
> returned strings and is not expected to free them

Done.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/test/bl...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/test/bl...
chrome_elf/blacklist/test/blacklist_test.cc:136: int size;
On 2014/02/24 15:24:05, robertshield wrote:
> = 0

Done.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/test/bl...
chrome_elf/blacklist/test/blacklist_test.cc:162: int num_blocked_dlls;
On 2014/02/24 15:24:05, robertshield wrote:
> = 0

Done.

https://codereview.chromium.org/174013007/diff/1/chrome_elf/blacklist/test/bl...
chrome_elf/blacklist/test/blacklist_test.cc:186: const wchar_t* blocked_dll;
On 2014/02/24 15:24:05, robertshield wrote:
> = NULL

Done.

[robertshield, 2014-02-25 03:21:39]

Looks really good. Few more comments.

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

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:31: bool
g_blocked_dlls[kTroublesomeDllsMaxCount] = {false};
"= {}" works for bool arrays too.

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:337: *size = g_num_blocked_dlls;
The comment on this method states that |size| will always be filled, but the
implementation appears to only do so when the value in |size| starts off too
small?

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:343: strings_to_fill != g_num_blocked_dlls &&
g_troublesome_dlls[i];
|strings_to_fill| indexes into |blocked_dlls| and so should be checked against
|size|, also prefer < to != for integer checks

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:344: ++i) {
I know I said to break here earlier,  but if you can wrap the increment to the
previous line, that would be ok. Also, see what git cl format does.

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:352: void BlockedDll(int blocked_index) {
check |blocked_index| against kTroublesomeDllsMaxCount. Also, use a size_t as a
parameter rather than a signed int (should also change the type of
kTroublesomeDllsMaxCount in the process).

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.h:74: // removed, false on error.
Add a comment here and on AddDll* along the lines of "Exposed for testing."

[csharp, 2014-02-25 14:44:59]

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

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:31: bool
g_blocked_dlls[kTroublesomeDllsMaxCount] = {false};
On 2014/02/25 03:21:40, robertshield wrote:
> "= {}" works for bool arrays too.

Done.

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:337: *size = g_num_blocked_dlls;
On 2014/02/25 03:21:40, robertshield wrote:
> The comment on this method states that |size| will always be filled, but the
> implementation appears to only do so when the value in |size| starts off too
> small?

Comment is correct, fixed code.

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:343: strings_to_fill != g_num_blocked_dlls &&
g_troublesome_dlls[i];
On 2014/02/25 03:21:40, robertshield wrote:
> |strings_to_fill| indexes into |blocked_dlls| and so should be checked against
> |size|, also prefer < to != for integer checks

Replaced != with <, also |size| >= |g_num_blocked_dlls| so we don't need that
check (if that condition is false we would have already exited)

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:344: ++i) {
On 2014/02/25 03:21:40, robertshield wrote:
> I know I said to break here earlier,  but if you can wrap the increment to the
> previous line, that would be ok. Also, see what git cl format does.

git cl format seems to like merging line 342 and 343

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:352: void BlockedDll(int blocked_index) {
On 2014/02/25 03:21:40, robertshield wrote:
> check |blocked_index| against kTroublesomeDllsMaxCount. Also, use a size_t as
a
> parameter rather than a signed int (should also change the type of
> kTroublesomeDllsMaxCount in the process).

Done.

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/174013007/diff/140001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.h:74: // removed, false on error.
On 2014/02/25 03:21:40, robertshield wrote:
> Add a comment here and on AddDll* along the lines of "Exposed for testing."

Done.

[robertshield, 2014-02-25 14:57:48]

lgtm

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

https://codereview.chromium.org/174013007/diff/160001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:355: #if !defined(NDEBUG)
Don't need the debug check here, assert does that itself.

[Alexei Svitkine (slow), 2014-02-25 15:00:19]

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

https://codereview.chromium.org/174013007/diff/160001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:126: int k_desired_blacklist_size =
5;
Nit: This should probably be:

const int kDesiredBlacklistSize = 5;

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

https://codereview.chromium.org/174013007/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1304: +<histogram
name="Blacklist.Blocked" units="dll_hash">
How will someone looking at this on the dashboard make sense of this?

It would be better if we could create an enum entry listing the possible hashes
we know about it, so that the dashboard shows more meaningful values. (Though,
you may want to do that in the internal version of this file.)

[csharp, 2014-02-25 15:20:49]

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

https://codereview.chromium.org/174013007/diff/160001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:80: base::MD5Digest hash;
asvitkine@, any hints/tips on a better hash to use? I think this approach work,
but it doesn't seem as clean as I would like.

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

https://codereview.chromium.org/174013007/diff/160001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:355: #if !defined(NDEBUG)
On 2014/02/25 14:57:49, robertshield wrote:
> Don't need the debug check here, assert does that itself.

Done.

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

https://codereview.chromium.org/174013007/diff/160001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:126: int k_desired_blacklist_size =
5;
On 2014/02/25 15:00:20, Alexei Svitkine wrote:
> Nit: This should probably be:
> 
> const int kDesiredBlacklistSize = 5;

Done.

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

https://codereview.chromium.org/174013007/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1304: +<histogram
name="Blacklist.Blocked" units="dll_hash">
On 2014/02/25 15:00:20, Alexei Svitkine wrote:
> How will someone looking at this on the dashboard make sense of this?
> 
> It would be better if we could create an enum entry listing the possible
hashes
> we know about it, so that the dashboard shows more meaningful values. (Though,
> you may want to do that in the internal version of this file.)

Good point, I don't think there is any problem with the enum living here, since
the blacklist is public.

[Alexei Svitkine (slow), 2014-02-25 15:51:54]

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

https://codereview.chromium.org/174013007/diff/160001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:80: base::MD5Digest hash;
On 2014/02/25 15:20:50, csharp wrote:
> asvitkine@, any hints/tips on a better hash to use? I think this approach
work,
> but it doesn't seem as clean as I would like.

Another histogram uses this one, which is indeed cleaner:

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/interf...

[csharp, 2014-02-26 13:13:38]

On 2014/02/25 15:51:54, Alexei Svitkine wrote:
>
https://codereview.chromium.org/174013007/diff/160001/chrome/browser/chrome_e...
> File chrome/browser/chrome_elf_init_win.cc (right):
> 
>
https://codereview.chromium.org/174013007/diff/160001/chrome/browser/chrome_e...
> chrome/browser/chrome_elf_init_win.cc:80: base::MD5Digest hash;
> On 2014/02/25 15:20:50, csharp wrote:
> > asvitkine@, any hints/tips on a better hash to use? I think this approach
> work,
> > but it doesn't seem as clean as I would like.
> 
> Another histogram uses this one, which is indeed cleaner:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/interf...

Updated to use the same hashing as in interface_list

[Alexei Svitkine (slow), 2014-02-26 14:32:13]

lgtm

https://codereview.chromium.org/174013007/diff/200001/chrome_elf/dll_hash/dll...
File chrome_elf/dll_hash/dll_hash_main.cc (right):

https://codereview.chromium.org/174013007/diff/200001/chrome_elf/dll_hash/dll...
chrome_elf/dll_hash/dll_hash_main.cc:21: argv[0]);
Nit: fits on previous line? If not, indent 2 more.

[csharp, 2014-02-26 17:42:55]

robertshield@, does this still lgty?

https://codereview.chromium.org/174013007/diff/200001/chrome_elf/dll_hash/dll...
File chrome_elf/dll_hash/dll_hash_main.cc (right):

https://codereview.chromium.org/174013007/diff/200001/chrome_elf/dll_hash/dll...
chrome_elf/dll_hash/dll_hash_main.cc:21: argv[0]);
On 2014/02/26 14:32:14, Alexei Svitkine wrote:
> Nit: fits on previous line? If not, indent 2 more.

Done.

[robertshield, 2014-02-26 19:57:10]

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash.gypi
File chrome_elf/dll_hash.gypi (right):

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash.gyp...
chrome_elf/dll_hash.gypi:10: '../base/base.gyp:base'
add a trailing comma

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash.gyp...
chrome_elf/dll_hash.gypi:21: 'dll_hash'
trailing comma

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash.gyp...
chrome_elf/dll_hash.gypi:23: 'include_dirs': [
indent.

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash/dll...
File chrome_elf/dll_hash/dll_hash_main.cc (right):

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash/dll...
chrome_elf/dll_hash/dll_hash_main.cc:17: fprintf(stderr, "Usage: %s <dll name>
<dll name> <...>\n", argv[0]);
This is probably fine (it's just a usage string), but be aware that it is
possible on Windows for argv[0] to not be the executable name (see
http://msdn.microsoft.com/en-us/library/88w63h9k.aspx). I don't see a reason to
change this code given the intended usage though.

[csharp, 2014-02-26 21:06:07]

sky@ for chrome/browser owner's approval
maruel@ for PRESUBMIT.py owner's approval

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash.gypi
File chrome_elf/dll_hash.gypi (right):

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash.gyp...
chrome_elf/dll_hash.gypi:10: '../base/base.gyp:base'
On 2014/02/26 19:57:10, robertshield wrote:
> add a trailing comma

Done.

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash.gyp...
chrome_elf/dll_hash.gypi:21: 'dll_hash'
On 2014/02/26 19:57:10, robertshield wrote:
> trailing comma

Done.

https://codereview.chromium.org/174013007/diff/240001/chrome_elf/dll_hash.gyp...
chrome_elf/dll_hash.gypi:23: 'include_dirs': [
On 2014/02/26 19:57:10, robertshield wrote:
> indent.

Done.

[M-A Ruel, 2014-02-26 21:09:54]

Nico, everyone is constantly adding exclusions to _CheckSpamLogging() in
src/PRESUBMIT.py. Is is working as intended?

PRESUBMIT.py rubberstamp lgtm as long Nico agrees.

[robertshield, 2014-02-26 21:30:56]

lgtm

[sky, 2014-02-26 21:52:04]

https://codereview.chromium.org/174013007/diff/280001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/174013007/diff/280001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.h:85: extern "C" void SuccessfullyBlocked(const
wchar_t** blocked_dlls, int* size);
Is there a reason this can't return std::vector<std::wstring> ? Seems like a
such a return type would be much more type safe.

[csharp, 2014-02-26 22:00:01]

https://codereview.chromium.org/174013007/diff/280001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/174013007/diff/280001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.h:85: extern "C" void SuccessfullyBlocked(const
wchar_t** blocked_dlls, int* size);
On 2014/02/26 21:52:05, sky wrote:
> Is there a reason this can't return std::vector<std::wstring> ? Seems like a
> such a return type would be much more type safe.

This is usually called from across a dll boundary and I don't believe we can
easily or consistently pass c++ types.

[sky, 2014-02-26 22:31:13]

Sadness. Ok, LGTM

[csharp, 2014-02-27 20:22:00]

> PRESUBMIT.py rubberstamp lgtm as long Nico agrees.

I'm going to submit now since it appears that Nico is away until next week, and
I'd like to start collecting these stats now.

Nico, let me know if you have any problems with the presubmit change

[csharp, 2014-02-27 20:22:46]

The CQ bit was checked by csharp@chromium.org

[commit-bot: I haz the power, 2014-02-27 20:23:55]

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

[commit-bot: I haz the power, 2014-02-27 21:20:55]

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

[commit-bot: I haz the power, 2014-02-27 21:20:56]

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

[csharp, 2014-02-28 22:39:11]

robertshield@, can you verify that fixes to chrome_elf_init_unittest_win.cc
still look good to you? thanks

[robertshield, 2014-03-03 13:51:28]

On 2014/02/28 22:39:11, csharp wrote:
> robertshield@, can you verify that fixes to chrome_elf_init_unittest_win.cc
> still look good to you? thanks

LGTM on the unit test file

[robertshield, 2014-03-03 13:51:45]

LGTM w/ one nit

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

https://codereview.chromium.org/174013007/diff/340001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:30: KEY_QUERY_VALUE |
KEY_SET_VALUE));
nit: indent

[csharp, 2014-03-03 14:17:37]

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

https://codereview.chromium.org/174013007/diff/340001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:30: KEY_QUERY_VALUE |
KEY_SET_VALUE));
On 2014/03/03 13:51:46, robertshield wrote:
> nit: indent

Done.

[csharp, 2014-03-03 14:17:43]

The CQ bit was checked by csharp@chromium.org

[commit-bot: I haz the power, 2014-03-03 14:17:56]

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

[commit-bot: I haz the power, 2014-03-03 17:01:44]

Change committed as 254480

[Timur Iskhodzhanov, 2015-03-03 14:22:07]

I'm not sure filing a bug on a CL's birthday is a good thing to do, but hey:
https://code.google.com/p/chromium/issues/detail?id=463495