Add ScopedStubEnterpriseInstallAttributes for tests to set test install attributes.

Add ScopedStubEnterpriseInstallAttributes for tests to set test install attributes.

This change is motivated by the need to set the test install attributes before the browser policy connector is initialized for the first time. In particular, test class members may cause the policy connector to be initialized in their constructors, so we also need to set the install attributes in the constructor of a member.

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

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

[Mattias Nissler (ping if slow), 2014-04-24 07:21:57]

The code here LGTM, but I've put a suggestions for your consideration. Feel free
to ignore if you conclude that clean shutdown in tests doesn't matter.

https://codereview.chromium.org/247283007/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/stub_enterprise_install_attributes.cc
(right):

https://codereview.chromium.org/247283007/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/stub_enterprise_install_attributes.cc:47:
BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(attributes);
Hm, the whole SetInstallAttributesForTesting seems to be broken after the
browser gets torn down. Have you tried what happens if you run 2 browser test
cases that call SetInstallAttributesForTesting with the --single_process flag?

If the only failure you see is due to SetInstallAttributesForTesting, it may
make sense to make a call to
BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(NULL) in the
destructor (and change the DCHECK in the implementation to allow that). Or
alternatively, reset g_testing_install_attributes in the
BrowserPolicyConnectorChromeOS ctor after grabbing the reference.

[Mattias Nissler (ping if slow), 2014-04-24 07:25:00]

Oh, one more thing: please update the BUG line to reference the bug that
requires this change before committing.

[Nikita (slow), 2014-04-24 12:53:02]

cc:merkulova@

[Steve Condie, 2014-04-24 18:14:28]

https://codereview.chromium.org/247283007/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/stub_enterprise_install_attributes.cc
(right):

https://codereview.chromium.org/247283007/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/stub_enterprise_install_attributes.cc:47:
BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(attributes);
On 2014/04/24 07:21:57, Mattias Nissler wrote:
> Hm, the whole SetInstallAttributesForTesting seems to be broken after the
> browser gets torn down. Have you tried what happens if you run 2 browser test
> cases that call SetInstallAttributesForTesting with the --single_process flag?
> 
> If the only failure you see is due to SetInstallAttributesForTesting, it may
> make sense to make a call to
> BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(NULL) in the
> destructor (and change the DCHECK in the implementation to allow that). Or
> alternatively, reset g_testing_install_attributes in the
> BrowserPolicyConnectorChromeOS ctor after grabbing the reference.

Good call, done. I was thinking that resetting g_testing_install_attributes
would not be useful since BrowserPolicyConnectorChromeOS only reads it when it's
initialized. However, since a new BrowserPolicyConnectorChromeOS is built for
each browser test within the same process we can set a new
g_testing_install_attributes each time.

[Mattias Nissler (ping if slow), 2014-04-24 18:33:47]

LGTM

[Steve Condie, 2014-04-24 18:34:51]

The CQ bit was checked by stepco@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:52:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/247283007/20001

[commit-bot: I haz the power, 2014-04-25 02:39:51]

Change committed as 266094

[Alexander Potapenko, 2014-04-25 12:40:08]

On 2014/04/25 02:39:51, I haz the power (commit-bot) wrote:
> Change committed as 266094

This CL has caused memory leaks on the CrOS ASan bots, e.g.
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...:

Direct leak of 64 byte(s) in 1 object(s) allocated from:
    #0 0x57ab3b in operator new(unsigned long)
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
    #1 0x35e5f63 in
policy::ScopedStubEnterpriseInstallAttributes::ScopedStubEnterpriseInstallAttributes(std::string
const&, std::string const&, std::string const&, policy::DeviceMode)
chrome/browser/chromeos/policy/stub_enterprise_install_attributes.cc:41
    #2 0xc41d2a in
policy::DeviceStatusCollectorTest::DeviceStatusCollectorTest()
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:160
    #3 0xc443ca in DeviceStatusCollectorTest_ActivityTimesDisabledByDefault_Test
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:428
    #4 0xc443ca in
testing::internal::TestFactoryImpl\u003Cpolicy::DeviceStatusCollectorTest_ActivityTimesDisabledByDefault_Test>::CreateTest()
testing/gtest/include/gtest/internal/gtest-internal.h:443
    #5 0x403a0ef in
HandleExceptionsInMethodIfSupported\u003Ctesting::internal::TestFactoryBase,
testing::Test *> testing/gtest/src/gtest.cc:2045
    #6 0x403a0ef in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2228
    #7 0x403ae46 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344
    #8 0x404b63a in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4065
    #9 0x404ac80 in
HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2045
    #10 0x404ac80 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697
    #11 0x382d0c4 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231
    #12 0x382d0c4 in base::TestSuite::Run() base/test/test_suite.cc:206
    #13 0x2361e66 in ChromeBrowserTestSuiteRunner::RunTestSuite(int, char**)
chrome/test/base/browser_tests_main.cc:14
    #14 0xd473cf4 in content::LaunchTests(content::TestLauncherDelegate*, int,
int, char**) content/public/test/test_launcher.cc:467
    #15 0x3699429 in LaunchChromeTests(int, ChromeTestSuiteRunner*, int, char**)
chrome/test/base/chrome_test_launcher.cc:129
    #16 0x2361d7a in main chrome/test/base/browser_tests_main.cc:21
    #17 0x7f972a6ee76c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226

Indirect leak of 41 byte(s) in 1 object(s) allocated from:
    #0 0x57ab3b in operator new(unsigned long)
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
    #1 0x7f972ad5c739 in __gnu_cxx::new_allocator\u003Cchar>::allocate(unsigned
long, void const*)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/ext/new_allocator.h:92
    #2 0x7f972ad5bd2c in std::string::_Rep::_S_create(unsigned long, unsigned
long, std::allocator\u003Cchar> const&)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609
    #3 0x7f972ad5c5b7 in char* std::string::_S_construct\u003Cchar const*>(char
const*, char const*, std::allocator\u003Cchar> const&,
std::forward_iterator_tag)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:140
    #4 0x7f972ad5c90b in char* std::string::_S_construct_aux\u003Cchar
const*>(char const*, char const*, std::allocator\u003Cchar> const&,
std::__false_type) (/usr/lib/x86_64-linux-gnu/debug/libstdc++.so.6+0xb990b)
    #5 0x7f972ad5c680 in char* std::string::_S_construct\u003Cchar const*>(char
const*, char const*, std::allocator\u003Cchar> const&)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.h:1713
    #6 0x7f972ad58563 in std::basic_string\u003Cchar,
std::char_traits\u003Cchar>, std::allocator\u003Cchar> >::basic_string(char
const*, std::allocator\u003Cchar> const&)
/build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:217
    #7 0xc41cfc in
policy::DeviceStatusCollectorTest::DeviceStatusCollectorTest()
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:160
    #8 0xc443ca in DeviceStatusCollectorTest_ActivityTimesDisabledByDefault_Test
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:428
    #9 0xc443ca in
testing::internal::TestFactoryImpl\u003Cpolicy::DeviceStatusCollectorTest_ActivityTimesDisabledByDefault_Test>::CreateTest()
testing/gtest/include/gtest/internal/gtest-internal.h:443
    #10 0x403a0ef in
HandleExceptionsInMethodIfSupported\u003Ctesting::internal::TestFactoryBase,
testing::Test *> testing/gtest/src/gtest.cc:2045
    #11 0x403a0ef in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2228
    #12 0x403ae46 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344
    #13 0x404b63a in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4065
    #14 0x404ac80 in
HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2045
    #15 0x404ac80 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697
    #16 0x382d0c4 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231
    #17 0x382d0c4 in base::TestSuite::Run() base/test/test_suite.cc:206
    #18 0x2361e66 in ChromeBrowserTestSuiteRunner::RunTestSuite(int, char**)
chrome/test/base/browser_tests_main.cc:14
    #19 0xd473cf4 in content::LaunchTests(content::TestLauncherDelegate*, int,
int, char**) content/public/test/test_launcher.cc:467
    #20 0x3699429 in LaunchChromeTests(int, ChromeTestSuiteRunner*, int, char**)
chrome/test/base/chrome_test_launcher.cc:129
    #21 0x2361d7a in main chrome/test/base/browser_tests_main.cc:21
    #22 0x7f972a6ee76c in __libc_start_main
/build/buildd/eglibc-2.15/csu/libc-start.c:226

Reverting.

[Alexander Potapenko, 2014-04-25 12:41:07]

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

The reason for reverting is: The CL has caused memory leaks on the CrOS ASan
bots originating from policy::DeviceStatusCollectorTest..

[Steve Condie, 2014-04-25 20:38:58]

Please take another look. I didn't realize that some of the tests never
initialized the browser policy connector. Now the test attributes will be
cleaned up even if they're not used.

[Steve Condie, 2014-04-25 20:42:49]

The CQ bit was checked by stepco@chromium.org

[Steve Condie, 2014-04-25 20:42:49]

The CQ bit was unchecked by stepco@chromium.org

[Steve Condie, 2014-04-25 20:43:30]

The CQ bit was checked by stepco@chromium.org

[Steve Condie, 2014-04-25 20:43:30]

The CQ bit was unchecked by stepco@chromium.org

[Mattias Nissler (ping if slow), 2014-04-28 11:37:06]

LGTM assuming you've verified that the leaks are no longer present with the new
version.

[Steve Condie, 2014-04-28 17:24:03]

The CQ bit was checked by stepco@chromium.org

[commit-bot: I haz the power, 2014-04-28 17:24:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/247283007/60001

[commit-bot: I haz the power, 2014-04-28 17:26:47]

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

[commit-bot: I haz the power, 2014-04-28 17:26:47]

Try jobs failed on following builders:
  tryserver.chromium on android_clang_dbg
  tryserver.chromium on linux_chromium_chromeos_clang_dbg
  tryserver.chromium on linux_chromium_clang_dbg
  tryserver.chromium on linux_chromium_gn_rel
  tryserver.chromium on linux_chromium_rel

[Steve Condie, 2014-04-28 17:33:14]

The CQ bit was checked by stepco@chromium.org

[commit-bot: I haz the power, 2014-04-28 17:33:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/247283007/80001

[commit-bot: I haz the power, 2014-04-28 20:26:00]

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

[commit-bot: I haz the power, 2014-04-28 20:26:01]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Steve Condie, 2014-04-28 20:43:12]

The CQ bit was checked by stepco@chromium.org

[Steve Condie, 2014-04-28 20:43:23]

The CQ bit was unchecked by stepco@chromium.org

[Steve Condie, 2014-04-28 21:27:04]

The CQ bit was checked by stepco@chromium.org

[commit-bot: I haz the power, 2014-04-28 21:27:23]

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

[commit-bot: I haz the power, 2014-04-28 22:06:00]

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

[commit-bot: I haz the power, 2014-04-28 22:06:00]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Steve Condie, 2014-04-28 23:52:39]

The CQ bit was checked by stepco@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:55:44]

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

[commit-bot: I haz the power, 2014-04-29 07:39:37]

Change committed as 266771