Add export to query for blacklisted-ness.

Add export to query for blacklisted-ness.

BUG=394352
TEST=unit_tests

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

[robertshield, 2014-08-05 19:55:18]

Please take a look.

[csharp, 2014-08-05 21:08:59]

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_unittest_win.cc:212:
RetrieveBlacklistedModules(&module_names);
Shouldn't this just return an empty list? Could we just check the size instead?

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_unittest_win.cc:227: bool found_test_module =
false;
lines 227-236 can probably be replace by just checking that 1 module was found
and that it was the expected value (I don't think we need the for loop)

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:212: bool
RetrieveBlacklistedModules(std::vector<base::string16>* module_names) {
std::vector<base::string16>* -> const std::vector<const base::string16>*

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:217: if (GetLoadedModules(&module_info)) {
Maybe have "if (!GetLoadedModules(&module_info)) return false" instead (less
indentation later and a clearer exit condition)

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:223:
blacklist::GetBlacklistIndex(module_file_name.c_str());
Maybe just move this directly into the if statement since the variable is only
used once

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:224: if (blacklist_index > -1) {
why > and not !=?

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.h:25: bool
RetrieveBlacklistedModules(std::vector<base::string16>* module_names);
This names doesn't quite seem to match, maybe RetrieveLoadedBlacklistedModules?
(The current name just sounds like it will get a list of all the modules we
blacklist)

https://codereview.chromium.org/444543002/diff/40001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/444543002/diff/40001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:2510: # Next line used by
chrome_elf_init_unittest_win.cc
I don't see many comment in gypi files, is this really going to be useful?

[robertshield, 2014-08-06 16:38:32]

Thanks! PTAL

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_unittest_win.cc:212:
RetrieveBlacklistedModules(&module_names);
On 2014/08/05 21:08:59, csharp wrote:
> Shouldn't this just return an empty list? Could we just check the size
instead?

Done (also added logging to help identify any non-test blacklist bypassing as
discussed).

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_unittest_win.cc:227: bool found_test_module =
false;
On 2014/08/05 21:08:59, csharp wrote:
> lines 227-236 can probably be replace by just checking that 1 module was found
> and that it was the expected value (I don't think we need the for loop)

Done.

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:212: bool
RetrieveBlacklistedModules(std::vector<base::string16>* module_names) {
On 2014/08/05 21:08:59, csharp wrote:
> std::vector<base::string16>* -> const std::vector<const base::string16>*

As discussed, this is an out param and there's not much value in enforcing that
the return strings be themselves const.

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:217: if (GetLoadedModules(&module_info)) {
On 2014/08/05 21:08:59, csharp wrote:
> Maybe have "if (!GetLoadedModules(&module_info)) return false" instead (less
> indentation later and a clearer exit condition)

Done.

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:223:
blacklist::GetBlacklistIndex(module_file_name.c_str());
On 2014/08/05 21:08:59, csharp wrote:
> Maybe just move this directly into the if statement since the variable is only
> used once

Done.

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:224: if (blacklist_index > -1) {
On 2014/08/05 21:08:59, csharp wrote:
> why > and not !=?

Done.

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/444543002/diff/40001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.h:25: bool
RetrieveBlacklistedModules(std::vector<base::string16>* module_names);
On 2014/08/05 21:08:59, csharp wrote:
> This names doesn't quite seem to match, maybe
RetrieveLoadedBlacklistedModules?
> (The current name just sounds like it will get a list of all the modules we
> blacklist)

Renamed to GetLoadedBlacklistModules.

https://codereview.chromium.org/444543002/diff/40001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/444543002/diff/40001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:2510: # Next line used by
chrome_elf_init_unittest_win.cc
On 2014/08/05 21:08:59, csharp wrote:
> I don't see many comment in gypi files, is this really going to be useful?

Perhaps not, git blame does just as well. Removed.

[csharp, 2014-08-06 17:52:57]

https://codereview.chromium.org/444543002/diff/60001/chrome/browser/chrome_el...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/444543002/diff/60001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:215: std::set<ModuleInfo> module_info;
nit: module_info -> module_info_set

https://codereview.chromium.org/444543002/diff/60001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:230: return module_found;
I think we should return True here and only return False when we failed to run
successfully.

[robertshield, 2014-08-06 17:54:49]

Thanks!

https://codereview.chromium.org/444543002/diff/60001/chrome/browser/chrome_el...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/444543002/diff/60001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:215: std::set<ModuleInfo> module_info;
On 2014/08/06 17:52:57, csharp wrote:
> nit: module_info -> module_info_set

Done.

https://codereview.chromium.org/444543002/diff/60001/chrome/browser/chrome_el...
chrome/browser/chrome_elf_init_win.cc:230: return module_found;
On 2014/08/06 17:52:57, csharp wrote:
> I think we should return True here and only return False when we failed to run
> successfully.

Done.

[robertshield, 2014-08-06 17:54:50]

Thanks!

[csharp, 2014-08-06 17:55:50]

lgtm

[robertshield, 2014-08-06 18:04:41]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2014-08-06 18:06:44]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/robertshield@chromium.org/444543002/80001

[robertshield, 2014-08-06 19:47:40]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2014-08-06 19:49:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/robertshield@chromium.org/444543002/10...

[commit-bot: I haz the power, 2014-08-06 23:38:59]

Change committed as 287899

[bdibello, 2014-08-07 09:16:08]

I think this may have the wrong bug ID.

[robertshield, 2014-08-07 11:49:34]

On 2014/08/07 09:16:08, bdibello wrote:
> I think this may have the wrong bug ID.

Indeed! My apologies, correct bug id for posterity: http://crbug.com/394352

[Nico, 2014-10-28 02:55:46]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-10-28 02:55:47]

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

https://codereview.chromium.org/444543002/diff/100001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:234: for (int i = 0; i <
kTroublesomeDllsMaxCount, g_troublesome_dlls[i]; ++i) {
you mean &&, not ,

[robertshield, 2014-10-28 03:27:30]

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

https://codereview.chromium.org/444543002/diff/100001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:234: for (int i = 0; i <
kTroublesomeDllsMaxCount, g_troublesome_dlls[i]; ++i) {
On 2014/10/28 02:55:47, Nico wrote:
> you mean &&, not ,

indeed! fixed with https://codereview.chromium.org/678193003/