Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
228553002:
Preference dis/allowing supervised users creation is now available as owner setting, not only as de… (Closed)
|
Created:
6 years, 8 months ago by merkulova Modified:
6 years, 6 months ago Reviewers:
bartfab (slow), David Roche, Nikita (slow), Mattias Nissler (ping if slow), Paweł Hajdan Jr., pastarmovj CC:
chromium-reviews, dbeam+watch-options_chromium.org, nkostylev+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPreference dis/allowing supervised users creation is now available as owner setting, not only as device policy.
BUG=243341
TBR=phajdan.jr@chromium.org,bartfab@chromium.org,davidroche@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275033
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Default enable-supervised logic fixed. Duplicated subscriptions removed. #
Total comments: 4
Patch Set 3 : Logic for default settings moved to device_settings_provider. #
Total comments: 4
Patch Set 4 : Rebased #Patch Set 5 : Unused preference cache removed. #Patch Set 6 : Rebase #Patch Set 7 : Unused declarations removed. #Patch Set 8 : Test-specific logic added: check for BrowserPolicyConnector existence used. #
Total comments: 5
Patch Set 9 : Using EnterpriseInstallAttributes for getting device status. #
Total comments: 1
Patch Set 10 : GetDeviceMode callback added into DeviceSettingsService constructor. #Patch Set 11 : Unnecessary callback removed. Patch applied for failing tests. #Messages
Total messages: 84 (0 generated)
pastarmovj@ as owner of chrome/browser/chromeos/settings/*
c/b/chromeos/settings LGTM.
https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/app_launch_signin_screen.h (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/login/app_launch_signin_screen.h:77: virtual bool IsShowSupervised() const OVERRIDE; I don't see much value in adding IsShowSupervised() here and passing it as a parameter to Init(). You may always get this value by accessing UserManager::AreLocallyManagedUsersAllowed(). So I suggest dropping this new method. https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_manager_impl.cc:1833: bool locally_managed_users_allowed = true; As discussed: this is not the correct way to set the default value for the consumer device. Please find a proper way to set correct defaults for this feature for enterprise and consumer cases. I think defaults should be different: consumer: enabled by default enterprise: disabled by default (i.e. enabled only by the policy). https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/webui_login_display.h (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/login/webui_login_display.h:115: // Whether to show supervised login. nit: show supervised users. https://codereview.chromium.org/228553002/diff/50001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:43: params->SetBoolean("managedUsersEnabled", allow_supervised); managedUsersEnabled is already set on line 71, you should drop it here. I suggest integrated code of UpdateAuthParamsFromSettings() into UpdateAuthParams() since all it does is also updating AuthParams from CrosSettings (mostly supervised users). https://codereview.chromium.org/228553002/diff/50001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:333: base::Bind(&SigninScreenHandler::UserSettingsChanged, nit: Please double check the code that changing this setting is actually reflected when UserSettingsChanged() is called.
https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/app_launch_signin_screen.h (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/login/app_launch_signin_screen.h:77: virtual bool IsShowSupervised() const OVERRIDE; On 2014/04/08 14:02:09, Nikita Kostylev wrote: > I don't see much value in adding IsShowSupervised() here and passing it as a > parameter to Init(). > > You may always get this value by accessing > UserManager::AreLocallyManagedUsersAllowed(). > > So I suggest dropping this new method. Done. https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_manager_impl.cc:1833: bool locally_managed_users_allowed = true; On 2014/04/08 14:02:09, Nikita Kostylev wrote: > As discussed: this is not the correct way to set the default value for the > consumer device. > > Please find a proper way to set correct defaults for this feature for enterprise > and consumer cases. > > I think defaults should be different: > > consumer: enabled by default > enterprise: disabled by default (i.e. enabled only by the policy). Done. https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/webui_login_display.h (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/login/webui_login_display.h:115: // Whether to show supervised login. On 2014/04/08 14:02:09, Nikita Kostylev wrote: > nit: show supervised users. Done. https://codereview.chromium.org/228553002/diff/50001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:43: params->SetBoolean("managedUsersEnabled", allow_supervised); On 2014/04/08 14:02:09, Nikita Kostylev wrote: > managedUsersEnabled is already set on line 71, you should drop it here. > > I suggest integrated code of UpdateAuthParamsFromSettings() into > UpdateAuthParams() since all it does is also updating AuthParams from > CrosSettings (mostly supervised users). Done. https://codereview.chromium.org/228553002/diff/50001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/228553002/diff/50001/chrome/browser/ui/webui/... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:333: base::Bind(&SigninScreenHandler::UserSettingsChanged, On 2014/04/08 14:02:09, Nikita Kostylev wrote: > nit: Please double check the code that changing this setting is actually > reflected when UserSettingsChanged() is called. Removed as discussed offline because we have duplicated subscriptions with ExistingUserController that exists all along SigninScreenHandler lifetime.
https://codereview.chromium.org/228553002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/228553002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_manager_impl.cc:1840: policy::BrowserPolicyConnectorChromeOS* connector = nit: As discussed, please remove lines 1840..1845 since these defaults will be set in device_settings_provider.cc https://codereview.chromium.org/228553002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:470: kAccountsPrefSupervisedUsersEnabled, As discussed, please set different defaults here: * enterprise device: supervised users are disabled (safe value) * consumer device: supervised users are enabled (convenient value)
https://codereview.chromium.org/228553002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/228553002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/login/user_manager_impl.cc:1840: policy::BrowserPolicyConnectorChromeOS* connector = On 2014/04/14 11:48:59, Nikita Kostylev wrote: > nit: As discussed, please remove lines 1840..1845 since these defaults will be > set in device_settings_provider.cc Done. https://codereview.chromium.org/228553002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:470: kAccountsPrefSupervisedUsersEnabled, On 2014/04/14 11:48:59, Nikita Kostylev wrote: > As discussed, please set different defaults here: > > * enterprise device: supervised users are disabled (safe value) > > * consumer device: supervised users are enabled (convenient value) Done.
lgtm Julian, please take another look.
Please see the comment I think there is something mixed up in the last version of this CL. https://codereview.chromium.org/228553002/diff/90001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:486: kAccountsPrefSupervisedUsersEnabled, supervised_users_enabled); There is some issue here - the code below will overwrite what you set here. I guess you wanted to replace the code below? https://codereview.chromium.org/228553002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:489: kAccountsPrefSupervisedUsersEnabled, ...this here I mean.
Rebased. https://codereview.chromium.org/228553002/diff/90001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:486: kAccountsPrefSupervisedUsersEnabled, supervised_users_enabled); On 2014/04/15 08:54:28, pastarmovj wrote: > There is some issue here - the code below will overwrite what you set here. I > guess you wanted to replace the code below? Done. https://codereview.chromium.org/228553002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:489: kAccountsPrefSupervisedUsersEnabled, On 2014/04/15 08:54:28, pastarmovj wrote: > ...this here I mean. Done.
lgtm
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
Julian, can you have a look onto this CL one more time? As you see tests are failing, e.g. DeviceStatusCollectorTest.ReportManagedUser and DeviceStatusCollectorTest.ReportUsers I found that the fail is caused with the change in device_settings_provider file. Adding call for policy connector (no matter what is added then to the new_values_cache) at this place somehow breaks the test flow. To be more precise it leads to some bad initialization (or lack of it) of g_testing_install_attributes. Why we can't call policy connector there? Is something broken or I misused the call?
On 2014/04/17 15:01:15, merkulova wrote: > Julian, can you have a look onto this CL one more time? > > As you see tests are failing, e.g. DeviceStatusCollectorTest.ReportManagedUser > and DeviceStatusCollectorTest.ReportUsers > > I found that the fail is caused with the change in device_settings_provider > file. Adding call for policy connector (no matter what is added then to the > new_values_cache) at this place somehow breaks the test flow. > > To be more precise it leads to some bad initialization (or lack of it) of > g_testing_install_attributes. > > Why we can't call policy connector there? Is something broken or I misused the > call? You are not the first case of using the BrowserPolicyConnector from this class but the first to use the install attributes. I don't have an idea out of the top of my head why is this a problem but I would guess this tests so far didn't need to mock/initialize the InstallAttributes and therefore you might just need to adjust the failing tests. However before you adjust the tests to make them "work" I would recommend to run the failing tests in debugger and see what is the state of those objects at the time they are used with and without your code to make sure it is missing initialization or an actual violation of the initialization order. Btw I am on vacation till the end of next week but mnissler knows this part of the code well too and can help you in more real time then me. :)
Mattias, can you have a look at the issue in device_settings_provider file?
device_settings_provider.cc LGTM
On 2014/04/22 08:19:33, Mattias Nissler wrote: > device_settings_provider.cc LGTM No-no-no! Sorry, actually we have a problem at device_settings_provider, description at my comments for Julian above. "As you see tests are failing, e.g. DeviceStatusCollectorTest.ReportManagedUser and DeviceStatusCollectorTest.ReportUsers I found that the fail is caused with the change in device_settings_provider file. Adding call for policy connector (no matter what is added then to the new_values_cache) at this place somehow breaks the test flow. To be more precise it leads to some bad initialization (or lack of it) of g_testing_install_attributes. Why we can't call policy connector there? Is something broken or I misused the call?" He recommended to address you with this question. Sorry for unclear request.
On 2014/04/22 08:22:57, merkulova wrote: > On 2014/04/22 08:19:33, Mattias Nissler wrote: > > device_settings_provider.cc LGTM > > No-no-no! Sorry, actually we have a problem at device_settings_provider, > description at my comments for Julian above. > > "As you see tests are failing, e.g. DeviceStatusCollectorTest.ReportManagedUser > and DeviceStatusCollectorTest.ReportUsers > > I found that the fail is caused with the change in device_settings_provider > file. Adding call for policy connector (no matter what is added then to the > new_values_cache) at this place somehow breaks the test flow. > > To be more precise it leads to some bad initialization (or lack of it) of > g_testing_install_attributes. > > Why we can't call policy connector there? Is something broken or I misused the > call?" > > He recommended to address you with this question. > > Sorry for unclear request. Sorry, it seems I was on auto-pilot :) Unfortunately, I have no idea why that test is failing now. The test code seems odd to me in that the user_size() expectations seem off by one. Adding stepco@ who wrote that test to comment. Steve, if the test code turns out to be correct, please add a comment there why your expectations on user list size are off by one.
Actually adding stepco@.
The test which is breaking depends on fake enterprise install attributes to be
set on the browser policy connector. Setting the fake attributes must occur
before the browser policy connector is initialized. This CL caused the browser
policy connector to be initialized before the fake attributes were set in this
test.
Mattias, the size expectations in the tests are based on the limit on the number
of users that can be reported; not the entire user history is considered in
creating the report.
Here is a patch which fixes the test. If you prefer me to check in this patch
separately, I can do that, but it seems like the fastest option is to just
include this in your CL.
diff --git
a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
index b2f62e9..1b66e19 100644
--- a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
+++ b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
@@ -173,13 +173,6 @@ class DeviceStatusCollectorTest : public testing::Test {
cros_settings_->RemoveSettingsProvider(device_settings_provider_));
cros_settings_->AddSettingsProvider(&stub_settings_provider_);
- // Set up fake install attributes.
- StubEnterpriseInstallAttributes* attributes =
- new StubEnterpriseInstallAttributes();
- attributes->SetDomain("managed.com");
- attributes->SetRegistrationUser("user@managed.com");
- BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(attributes);
-
RestartStatusCollector();
}
@@ -253,6 +246,17 @@ class DeviceStatusCollectorTest : public testing::Test {
return policy::DeviceStatusCollector::kIdlePollIntervalSeconds * 1000;
}
+ // Sets the test enterprise install attributes.
+ struct TestInstallAttributesSetter {
+ TestInstallAttributesSetter() {
+ StubEnterpriseInstallAttributes* attributes =
+ new StubEnterpriseInstallAttributes();
+ attributes->SetDomain("managed.com");
+ attributes->SetRegistrationUser("user@managed.com");
+
BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(attributes);
+ }
+ };
+
// Since this is a unit test running in browser_tests we must do additional
// unit test setup and make a TestingBrowserProcess. Must be first member.
TestingBrowserProcessInitializer initializer_;
@@ -261,6 +265,7 @@ class DeviceStatusCollectorTest : public testing::Test {
content::TestBrowserThread file_thread_;
content::TestBrowserThread io_thread_;
+ TestInstallAttributesSetter install_attributes_setter_;
TestingPrefServiceSimple prefs_;
chromeos::system::MockStatisticsProvider statistics_provider_;
chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;
Steve, thanks for clarifying and your suggestion for a fix. Instead of rolling a
local TestInstallAttributesSetter class, let's add a
ScopedStupEnterpriseInstallAttributes in
stub_enterprise_install_attributes.{h,cc} instead (if possible, do we need to
set the domain and user up front?) so it can be used elsewhere.
On 2014/04/22 20:34:44, stepco wrote:
> The test which is breaking depends on fake enterprise install attributes to be
> set on the browser policy connector. Setting the fake attributes must occur
> before the browser policy connector is initialized. This CL caused the browser
> policy connector to be initialized before the fake attributes were set in this
> test.
>
> Mattias, the size expectations in the tests are based on the limit on the
number
> of users that can be reported; not the entire user history is considered in
> creating the report.
OK, that wasn't clear to me just looking at the test code, I should have checked
the implementation as well.
>
> Here is a patch which fixes the test. If you prefer me to check in this patch
> separately, I can do that, but it seems like the fastest option is to just
> include this in your CL.
>
> diff --git
> a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
> b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
> index b2f62e9..1b66e19 100644
> --- a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
> +++ b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
> @@ -173,13 +173,6 @@ class DeviceStatusCollectorTest : public testing::Test {
> cros_settings_->RemoveSettingsProvider(device_settings_provider_));
> cros_settings_->AddSettingsProvider(&stub_settings_provider_);
>
> - // Set up fake install attributes.
> - StubEnterpriseInstallAttributes* attributes =
> - new StubEnterpriseInstallAttributes();
> - attributes->SetDomain("managed.com");
> - mailto:attributes->SetRegistrationUser("user@managed.com");
> -
BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(attributes);
> -
> RestartStatusCollector();
> }
>
> @@ -253,6 +246,17 @@ class DeviceStatusCollectorTest : public testing::Test {
> return policy::DeviceStatusCollector::kIdlePollIntervalSeconds * 1000;
> }
>
> + // Sets the test enterprise install attributes.
> + struct TestInstallAttributesSetter {
> + TestInstallAttributesSetter() {
> + StubEnterpriseInstallAttributes* attributes =
> + new StubEnterpriseInstallAttributes();
> + attributes->SetDomain("managed.com");
> + mailto:attributes->SetRegistrationUser("user@managed.com");
> +
> BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(attributes);
> + }
> + };
> +
> // Since this is a unit test running in browser_tests we must do additional
> // unit test setup and make a TestingBrowserProcess. Must be first member.
> TestingBrowserProcessInitializer initializer_;
> @@ -261,6 +265,7 @@ class DeviceStatusCollectorTest : public testing::Test {
> content::TestBrowserThread file_thread_;
> content::TestBrowserThread io_thread_;
>
> + TestInstallAttributesSetter install_attributes_setter_;
> TestingPrefServiceSimple prefs_;
> chromeos::system::MockStatisticsProvider statistics_provider_;
> chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;
Created a CL to fix the test in preparation for this change: https://codereview.chromium.org/247283007/ Thanks for your patience Tatiana.
Thanks a lot. Expected an idea to try, you got the whole solution. Wow! On Wed, Apr 23, 2014 at 12:53 PM, <stepco@chromium.org> wrote: > Created a CL to fix the test in preparation for this change: > https://codereview.chromium.org/247283007/ > Thanks for your patience Tatiana. > > https://codereview.chromium.org/228553002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/190001
The CQ bit was unchecked by glider@chromium.org
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
Julian, Mattias, please have another look. Another bunch of tests is failing. DeviceOAuth2TokenServiceTest.* And again it's caused by getting browser-policy-connector lines 471-472 at device_settings_provider.cc This time DCHECK fails at observer_list.h. As some obveservers are not removed properly.
On 2014/04/30 14:05:54, merkulova wrote:
> Julian, Mattias, please have another look.
>
> Another bunch of tests is failing. DeviceOAuth2TokenServiceTest.*
>
> And again it's caused by getting browser-policy-connector lines 471-472 at
> device_settings_provider.cc
>
> This time DCHECK fails at observer_list.h. As some obveservers are not removed
> properly.
I've patched in your change locally, and the offending observer is added from
the BrowserPolicyConnectorChromeOS contructor:
Breakpoint 1, chromeos::DeviceSettingsService::AddObserver (this=0x7fffe0481aa0,
observer=0x16f58405c5f0)
at ../../chrome/browser/chromeos/settings/device_settings_service.cc:248
248 observers_.AddObserver(observer);
(gdb) bt
#0 chromeos::DeviceSettingsService::AddObserver (this=0x7fffe0481aa0,
observer=0x16f58405c5f0)
at ../../chrome/browser/chromeos/settings/device_settings_service.cc:248
#1 0x0000000005c07d0e in
policy::DeviceCloudPolicyStoreChromeOS::DeviceCloudPolicyStoreChromeOS (
this=0x16f58405c520, device_settings_service=0x7fffe0481aa0,
install_attributes=0x16f584023ea0,
background_task_runner=...) at
../../chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:26
#2 0x0000000005be46ba in
policy::BrowserPolicyConnectorChromeOS::BrowserPolicyConnectorChromeOS (
this=0x16f583ebca20) at
../../chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:107
#3 0x0000000002aa21d5 in
BrowserProcessPlatformPart::CreateBrowserPolicyConnector (this=0x16f584038f20)
at ../../chrome/browser/browser_process_platform_part_chromeos.cc:61
#4 0x0000000003e3b7ee in TestingBrowserProcess::browser_policy_connector
(this=0x16f583fd8910)
at ../../chrome/test/base/testing_browser_process.cc:149
#5 0x0000000002aa216a in
BrowserProcessPlatformPart::browser_policy_connector_chromeos
(this=0x16f584038f20)
at ../../chrome/browser/browser_process_platform_part_chromeos.cc:52
#6 0x0000000005c8a617 in chromeos::DeviceSettingsProvider::DecodeLoginPolicies
(this=0x16f583ebd5a0, policy=
..., new_values_cache=0x7fffffffcba0)
at ../../chrome/browser/chromeos/settings/device_settings_provider.cc:472
#7 0x0000000005c89c64 in chromeos::DeviceSettingsProvider::UpdateValuesCache
(this=0x16f583ebd5a0, policy_data=
..., settings=...,
trusted_status=chromeos::CrosSettingsProvider::TEMPORARILY_UNTRUSTED)
at ../../chrome/browser/chromeos/settings/device_settings_provider.cc:789
#8 0x0000000005c8774f in chromeos::DeviceSettingsProvider::RetrieveCachedData
(this=0x16f583ebd5a0)
at ../../chrome/browser/chromeos/settings/device_settings_provider.cc:182
#9 0x0000000005c870a6 in
chromeos::DeviceSettingsProvider::DeviceSettingsProvider(base::Callback<void
(std::string const&)> const&, chromeos::DeviceSettingsService*)
(this=0x16f583ebd5a0, notify_cb=...,
device_settings_service=0x7fffe0481aa0)
at ../../chrome/browser/chromeos/settings/device_settings_provider.cc:101
#10 0x0000000005c7735c in chromeos::CrosSettings::CrosSettings
(this=0x16f583fbe520,
device_settings_service=0x7fffe0481aa0) at
../../chrome/browser/chromeos/settings/cros_settings.cc:57
#11 0x0000000005c77016 in chromeos::CrosSettings::Initialize ()
at ../../chrome/browser/chromeos/settings/cros_settings.cc:27
#12 0x0000000000ed8dae in chromeos::DeviceOAuth2TokenServiceTest::SetUp
(this=0x16f583fdc020)
at
../../chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:96
#13 0x0000000003d3d6c3 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (
object=0x16f583fdc020, method=&virtual testing::Test::SetUp(),
location=0x735dc08 <.L.str104> "SetUp()")
at ../../testing/gtest/src/gtest.cc:1989
#14 0x0000000003d2aa2e in
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (
object=0x16f583fdc020, method=&virtual testing::Test::SetUp(),
location=0x735dc08 <.L.str104> "SetUp()")
at ../../testing/gtest/src/gtest.cc:2042
#15 0x0000000003d218a3 in testing::Test::Run (this=0x16f583fdc020) at
../../testing/gtest/src/gtest.cc:2057
#16 0x0000000003d2200b in testing::TestInfo::Run (this=0x16f5838db8e0) at
../../testing/gtest/src/gtest.cc:2237
#17 0x0000000003d225fa in testing::TestCase::Run (this=0x16f5838cc720) at
../../testing/gtest/src/gtest.cc:2344
#18 0x0000000003d26c08 in testing::internal::UnitTestImpl::RunAllTests
(this=0x7fffe0537520)
at ../../testing/gtest/src/gtest.cc:4065
#19 0x0000000003d36453 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool> (object=0x7fffe0537520,
method=(bool
(testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const))
0x3d268b0 <testing::internal::UnitTestImpl::RunAllTests()>,
location=0x735e342 <.L.str214> "auxiliary test code (environments or event
listeners)")
at ../../testing/gtest/src/gtest.cc:1989
#20 0x0000000003d2c5ee in
testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool> (object=0x7fffe0537520,
The problem seems to be that a BrowserPolicyConnector on g_browser_process gets
created implicitly via the test code, while all other dependencies are handled
in test setup. Then, we get bad shutdown ordering when the test gets torn down:
BrowserPolicyConnector::Shutdown() doesn't get called before our
DeviceSettingsService instance gets torn down.
On 2014/05/02 12:47:12, Mattias Nissler wrote: > On 2014/04/30 14:05:54, merkulova wrote: > > Julian, Mattias, please have another look. > > > > Another bunch of tests is failing. DeviceOAuth2TokenServiceTest.* > > > > And again it's caused by getting browser-policy-connector lines 471-472 at > > device_settings_provider.cc > > > > This time DCHECK fails at observer_list.h. As some obveservers are not removed > > properly. > > I've patched in your change locally, and the offending observer is added from > the BrowserPolicyConnectorChromeOS contructor: > > Breakpoint 1, chromeos::DeviceSettingsService::AddObserver (this=0x7fffe0481aa0, > observer=0x16f58405c5f0) > at ../../chrome/browser/chromeos/settings/device_settings_service.cc:248 > 248 observers_.AddObserver(observer); > (gdb) bt > #0 chromeos::DeviceSettingsService::AddObserver (this=0x7fffe0481aa0, > observer=0x16f58405c5f0) > at ../../chrome/browser/chromeos/settings/device_settings_service.cc:248 > #1 0x0000000005c07d0e in > policy::DeviceCloudPolicyStoreChromeOS::DeviceCloudPolicyStoreChromeOS ( > this=0x16f58405c520, device_settings_service=0x7fffe0481aa0, > install_attributes=0x16f584023ea0, > background_task_runner=...) at > ../../chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:26 > #2 0x0000000005be46ba in > policy::BrowserPolicyConnectorChromeOS::BrowserPolicyConnectorChromeOS ( > this=0x16f583ebca20) at > ../../chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:107 > #3 0x0000000002aa21d5 in > BrowserProcessPlatformPart::CreateBrowserPolicyConnector (this=0x16f584038f20) > at ../../chrome/browser/browser_process_platform_part_chromeos.cc:61 > #4 0x0000000003e3b7ee in TestingBrowserProcess::browser_policy_connector > (this=0x16f583fd8910) > at ../../chrome/test/base/testing_browser_process.cc:149 > #5 0x0000000002aa216a in > BrowserProcessPlatformPart::browser_policy_connector_chromeos > (this=0x16f584038f20) > at ../../chrome/browser/browser_process_platform_part_chromeos.cc:52 > #6 0x0000000005c8a617 in chromeos::DeviceSettingsProvider::DecodeLoginPolicies > (this=0x16f583ebd5a0, policy= > ..., new_values_cache=0x7fffffffcba0) > at ../../chrome/browser/chromeos/settings/device_settings_provider.cc:472 > #7 0x0000000005c89c64 in chromeos::DeviceSettingsProvider::UpdateValuesCache > (this=0x16f583ebd5a0, policy_data= > ..., settings=..., > trusted_status=chromeos::CrosSettingsProvider::TEMPORARILY_UNTRUSTED) > at ../../chrome/browser/chromeos/settings/device_settings_provider.cc:789 > #8 0x0000000005c8774f in chromeos::DeviceSettingsProvider::RetrieveCachedData > (this=0x16f583ebd5a0) > at ../../chrome/browser/chromeos/settings/device_settings_provider.cc:182 > #9 0x0000000005c870a6 in > chromeos::DeviceSettingsProvider::DeviceSettingsProvider(base::Callback<void > (std::string const&)> const&, chromeos::DeviceSettingsService*) > (this=0x16f583ebd5a0, notify_cb=..., > device_settings_service=0x7fffe0481aa0) > at ../../chrome/browser/chromeos/settings/device_settings_provider.cc:101 > #10 0x0000000005c7735c in chromeos::CrosSettings::CrosSettings > (this=0x16f583fbe520, > device_settings_service=0x7fffe0481aa0) at > ../../chrome/browser/chromeos/settings/cros_settings.cc:57 > #11 0x0000000005c77016 in chromeos::CrosSettings::Initialize () > at ../../chrome/browser/chromeos/settings/cros_settings.cc:27 > #12 0x0000000000ed8dae in chromeos::DeviceOAuth2TokenServiceTest::SetUp > (this=0x16f583fdc020) > at > ../../chrome/browser/chromeos/settings/device_oauth2_token_service_unittest.cc:96 > #13 0x0000000003d3d6c3 in > testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> ( > object=0x16f583fdc020, method=&virtual testing::Test::SetUp(), > location=0x735dc08 <.L.str104> "SetUp()") > at ../../testing/gtest/src/gtest.cc:1989 > #14 0x0000000003d2aa2e in > testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> ( > object=0x16f583fdc020, method=&virtual testing::Test::SetUp(), > location=0x735dc08 <.L.str104> "SetUp()") > at ../../testing/gtest/src/gtest.cc:2042 > #15 0x0000000003d218a3 in testing::Test::Run (this=0x16f583fdc020) at > ../../testing/gtest/src/gtest.cc:2057 > #16 0x0000000003d2200b in testing::TestInfo::Run (this=0x16f5838db8e0) at > ../../testing/gtest/src/gtest.cc:2237 > #17 0x0000000003d225fa in testing::TestCase::Run (this=0x16f5838cc720) at > ../../testing/gtest/src/gtest.cc:2344 > #18 0x0000000003d26c08 in testing::internal::UnitTestImpl::RunAllTests > (this=0x7fffe0537520) > at ../../testing/gtest/src/gtest.cc:4065 > #19 0x0000000003d36453 in > testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, > bool> (object=0x7fffe0537520, > method=(bool > (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) > 0x3d268b0 <testing::internal::UnitTestImpl::RunAllTests()>, > location=0x735e342 <.L.str214> "auxiliary test code (environments or event > listeners)") > at ../../testing/gtest/src/gtest.cc:1989 > #20 0x0000000003d2c5ee in > testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, > bool> (object=0x7fffe0537520, > > The problem seems to be that a BrowserPolicyConnector on g_browser_process gets > created implicitly via the test code, while all other dependencies are handled > in test setup. Then, we get bad shutdown ordering when the test gets torn down: > BrowserPolicyConnector::Shutdown() doesn't get called before our > DeviceSettingsService instance gets torn down. One option to fix could be to bring up and tear down a TestingBrowserProcess in the test. Alternatively, one could try to convert these tests to browser_tests.
It seems TestingBrowserProcess is already used at this test. Or you mean some specific init? Also is it normal, that because of device_settings_provider change some not direct unit-test should be changed? Converting to browsertests seems like overload. As these tests don't need to be browser-tests in the core. Nikita proposed to create method that would check if BrowserPolicyConnector already created (meaning browser is alive). We can use this check at DeviceSettingsProvider and skip its implicit creation supposing we don't need it at unit-tests. What do you think on it?
friendly ping
Sorry, I had typed a long reply but seem to have failed to send it. Typing it again... :) On 2014/05/06 12:42:12, merkulova wrote: > It seems TestingBrowserProcess is already used at this test. Or you mean some > specific init? I was suggesting to bring up and tear down a test-specific instance, so we can get correct shutdown ordering. > Also is it normal, that because of device_settings_provider change some not > direct unit-test should be changed? Definitively not - but note that this depends on your definition of "not direct": You've essentially introduced a hidden dependency from DeviceSettingsProvider to BrowserPolicyConnector. It's not surprising that this potentially affects all unit tests relying on DeviceSettingsProvider. In a world were dependencies are explicit (for example, all dependencies passed in the constructor), you'd even have found out that your change affects the test that fails now, because you'd have to fix the code that brings up DeviceSettingsProvider in the test to provide the new BrowserPolicyConnector dependency. Unfortunately, we don't live in such a world - the code uses global singletons liberally (more than it should IMHO) and so we have lots of hidden dependencies (and even cyclic dependencies!), so tests do break if you touch code that many people rely on. Note that BrowserPolicyConnector is the *only* global singleton in the policy code, and adding a dependency to it means you're essentially pulling in *the entire* policy infrastructure - no surprise this causes problems in unit tests :) It might be better to depend on just EnterpriseInstallAttributes and mock them for unit tests that rely on them via DeviceSettingsProvider. > > Converting to browsertests seems like overload. As these tests don't need to be > browser-tests in the core. Agreed. > > Nikita proposed to create method that would check if BrowserPolicyConnector > already created (meaning browser is alive). We can use this check at > DeviceSettingsProvider and skip its implicit creation supposing we don't need it > at unit-tests. > > What do you think on it? The NULL-in-tests idiom is indeed commonly used. IMHO that's just adding in another hack to work around the larger problem I described above. I don't expect you to fix the global singletons problem in this CL though, so I'll not block you if you decide to go with the NULL check solution.
+jam@ for chrome/browser/browser_process* files +phajdan.jr@ for chrome/test/base/testing_browser_process Explicit check used for covering unit-tests logic.
https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:472: if (g_browser_process->is_browser_policy_connector_created()) { why this check? it's not deterministic when you're in this method whether this object is created or not? if it's always created, then this check isn't needed. if it's not created, can its constructor be made cheap, and any expensive work can be done lazily?
https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:472: if (g_browser_process->is_browser_policy_connector_created()) { On 2014/05/13 21:09:11, jam wrote: > why this check? it's not deterministic when you're in this method whether this > object is created or not? > > if it's always created, then this check isn't needed. > if it's not created, can its constructor be made cheap, and any expensive work > can be done lazily? It's a check that covers unit tests where browser is not created. Implicit creation at this step led to tests failing due to wrong Shutdown order. Had a discussion with Mattias on it. Comments #55 and #59 contain the info that might help understand the case.
https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:472: if (g_browser_process->is_browser_policy_connector_created()) { On 2014/05/14 06:30:10, merkulova wrote: > On 2014/05/13 21:09:11, jam wrote: > > why this check? it's not deterministic when you're in this method whether this > > object is created or not? > > > > if it's always created, then this check isn't needed. > > if it's not created, can its constructor be made cheap, and any expensive work > > can be done lazily? > > It's a check that covers unit tests where browser is not created. Implicit > creation at this step led to tests failing due to wrong Shutdown order. > > Had a discussion with Mattias on it. Comments #55 and #59 contain the info that > might help understand the case. > Can you find another way of doing this without adding yet another method to BrowserProcess? why can't this unit test check be localized to policy code without adding another getter to the global BrowserProcess?
Implemented enterprise-status check using EnterpriseInstallAttributes. Works on a device. DeviceOAuth2TokenServiceTest.* tests pass. https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:472: if (g_browser_process->is_browser_policy_connector_created()) { On 2014/05/14 17:29:46, jam wrote: > On 2014/05/14 06:30:10, merkulova wrote: > > On 2014/05/13 21:09:11, jam wrote: > > > why this check? it's not deterministic when you're in this method whether > this > > > object is created or not? > > > > > > if it's always created, then this check isn't needed. > > > if it's not created, can its constructor be made cheap, and any expensive > work > > > can be done lazily? > > > > It's a check that covers unit tests where browser is not created. Implicit > > creation at this step led to tests failing due to wrong Shutdown order. > > > > Had a discussion with Mattias on it. Comments #55 and #59 contain the info > that > > might help understand the case. > > > > Can you find another way of doing this without adding yet another method to > BrowserProcess? why can't this unit test check be localized to policy code > without adding another getter to the global BrowserProcess? Done.
https://codereview.chromium.org/228553002/diff/310001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/310001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:477: DBusThreadManager::Get()->GetCryptohomeClient())); EnterpriseInstallAttributes is not meant to be instantiated on demand. You should rather use the instanced vended by BrowserPolicyConnectorChromeOS
As bartfab@ proposed adding callback into the constructor. DeviceOAuth2TokenServiceTest tests pass.
https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/228553002/diff/290001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:472: if (g_browser_process->is_browser_policy_connector_created()) { On 2014/05/23 09:27:15, merkulova wrote: > On 2014/05/14 17:29:46, jam wrote: > > On 2014/05/14 06:30:10, merkulova wrote: > > > On 2014/05/13 21:09:11, jam wrote: > > > > why this check? it's not deterministic when you're in this method whether > > this > > > > object is created or not? > > > > > > > > if it's always created, then this check isn't needed. > > > > if it's not created, can its constructor be made cheap, and any expensive > > work > > > > can be done lazily? > > > > > > It's a check that covers unit tests where browser is not created. Implicit > > > creation at this step led to tests failing due to wrong Shutdown order. > > > > > > Had a discussion with Mattias on it. Comments #55 and #59 contain the info > > that > > > might help understand the case. > > > > > > > Can you find another way of doing this without adding yet another method to > > BrowserProcess? why can't this unit test check be localized to policy code > > without adding another getter to the global BrowserProcess? > > Done. thanks, removing myself as reviewer
Julian, can you have one more look?
lgtm
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/330001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
+David and Bartosz
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/350001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/228553002/350001
Message was sent while issue was closed.
Change committed as 275033 |
