Added a test for crash in DeviceDisabledScreen

chrome/browser/chromeos/system/device_disabling_browsertest.cc

Index: chrome/browser/chromeos/system/device_disabling_browsertest.cc
diff --git a/chrome/browser/chromeos/system/device_disabling_browsertest.cc b/chrome/browser/chromeos/system/device_disabling_browsertest.cc
index 23a0d934e63094b417d5b620de0388b74ffc8134..c01aec8a063ca80440676e5871bc7ec46634ab7a 100644
--- a/chrome/browser/chromeos/system/device_disabling_browsertest.cc
+++ b/chrome/browser/chromeos/system/device_disabling_browsertest.cc
@@ -44,6 +44,14 @@ void ErrorCallbackFunction(const std::string& error_name,
   LOG(ERROR) << "Shill Error: " << error_name << " : " << error_message;
 }
 
+bool DeviceDisabledScreenShown() {
+  WizardController* const wizard_controller =
+      WizardController::default_controller();
+  EXPECT_TRUE(wizard_controller);
+  return wizard_controller->current_screen() ==

[emaxx, 2017/04/24 16:49:34]

nit: This will crash if wizard_controller is null, as EXPECT_TRUE doesn't return
from the function. So something like adding "wizard_controller &&" into this
condition will make it a little bit more robust.

[Ivan Šandrk, 2017/04/24 17:18:24]

Done.

+         wizard_controller->GetScreen(OobeScreen::SCREEN_DEVICE_DISABLED);
+}
+
 }  // namespace
 
 class DeviceDisablingTest
@@ -52,6 +60,9 @@ class DeviceDisablingTest
  public:
   DeviceDisablingTest();
 
+  // Sets up a device state blob that indicates the device is disabled.
+  void SetDeviceDisabledPolicy();
+
   // Sets up a device state blob that indicates the device is disabled, triggers
   // a policy plus device state fetch and waits for it to succeed.
   void MarkDisabledAndWaitForPolicyFetch();
@@ -59,9 +70,6 @@ class DeviceDisablingTest
   std::string GetCurrentScreenName(content::WebContents* web_contents);
 
  protected:
-  std::unique_ptr<base::RunLoop> network_state_change_wait_run_loop_;
-
- private:
   // OobeBaseTest:
   void SetUpInProcessBrowserTestFixture() override;
   void SetUpOnMainThread() override;
@@ -69,9 +77,11 @@ class DeviceDisablingTest
   // NetworkStateInformer::NetworkStateInformerObserver:
   void UpdateState(NetworkError::ErrorReason reason) override;
 
+  std::unique_ptr<base::RunLoop> network_state_change_wait_run_loop_;
   FakeSessionManagerClient* fake_session_manager_client_;
   policy::DevicePolicyCrosTestHelper test_helper_;

[emaxx, 2017/04/24 16:49:34]

nit: Please make as private all members that aren't used in the inherited class.

[Ivan Šandrk, 2017/04/24 17:18:24]

Done.

network_state_change_wait_run_loop_ is used in
IN_PROC_BROWSER_TEST_F(DeviceDisablingTest, DisableWithEphemeralUsers)
-> gives an error if I make it private

 
+ private:
   DISALLOW_COPY_AND_ASSIGN(DeviceDisablingTest);
 };
 
@@ -80,18 +90,22 @@ DeviceDisablingTest::DeviceDisablingTest()
     : fake_session_manager_client_(new FakeSessionManagerClient) {
 }
 
-void DeviceDisablingTest::MarkDisabledAndWaitForPolicyFetch() {
-  base::RunLoop run_loop;
-  // Set up an |observer| that will wait for the disabled setting to change.
-  std::unique_ptr<CrosSettings::ObserverSubscription> observer =
-      CrosSettings::Get()->AddSettingsObserver(kDeviceDisabled,
-                                               run_loop.QuitClosure());
+void DeviceDisablingTest::SetDeviceDisabledPolicy() {
   // Prepare a policy fetch response that indicates the device is disabled.
   test_helper_.device_policy()->policy_data().mutable_device_state()->
       set_device_mode(enterprise_management::DeviceState::DEVICE_MODE_DISABLED);
   test_helper_.device_policy()->Build();
   fake_session_manager_client_->set_device_policy(
       test_helper_.device_policy()->GetBlob());
+}
+
+void DeviceDisablingTest::MarkDisabledAndWaitForPolicyFetch() {
+  base::RunLoop run_loop;
+  // Set up an |observer| that will wait for the disabled setting to change.
+  std::unique_ptr<CrosSettings::ObserverSubscription> observer =
+      CrosSettings::Get()->AddSettingsObserver(kDeviceDisabled,
+                                               run_loop.QuitClosure());
+  SetDeviceDisabledPolicy();
   // Trigger a policy fetch.
   fake_session_manager_client_->OnPropertyChangeComplete(true);
   // Wait for the policy fetch to complete and the disabled setting to change.
@@ -139,11 +153,7 @@ IN_PROC_BROWSER_TEST_F(DeviceDisablingTest, DisableDuringNormalOperation) {
   MarkDisabledAndWaitForPolicyFetch();
 
   // Verify that the device disabled screen is being shown.
-  WizardController* const wizard_controller =
-      WizardController::default_controller();
-  ASSERT_TRUE(wizard_controller);
-  EXPECT_EQ(wizard_controller->GetScreen(OobeScreen::SCREEN_DEVICE_DISABLED),
-            wizard_controller->current_screen());
+  EXPECT_TRUE(DeviceDisabledScreenShown());
 }
 
 // Verifies that device disabling works when the ephemeral users policy is
@@ -217,5 +227,30 @@ IN_PROC_BROWSER_TEST_F(DeviceDisablingTest, DisableWithEphemeralUsers) {
             GetCurrentScreenName(web_contents));
 }
 
+class PresetPolicyDeviceDisablingTest : public DeviceDisablingTest {

[emaxx, 2017/04/24 16:49:34]

nit: Please add a short comment about what is specific with this class. E.g.:
// Sets the device disabled policy before the browser is started.

[Ivan Šandrk, 2017/04/24 17:18:24]

Done.

+ public:
+  PresetPolicyDeviceDisablingTest() {}
+
+ protected:
+  // DeviceDisablingTest:
+  void SetUpInProcessBrowserTestFixture() override {
+    DeviceDisablingTest::SetUpInProcessBrowserTestFixture();
+    SetDeviceDisabledPolicy();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PresetPolicyDeviceDisablingTest);
+};
+
+// Same test as the one in DeviceDisablingTest, except the policy is being set
+// before Chrome process is started. This test covers a crash (crbug.com/709518)
+// in DeviceDisabledScreen where it would try to access DeviceDisablingManager
+// even though it wasn't yet constructed fully.
+IN_PROC_BROWSER_TEST_F(PresetPolicyDeviceDisablingTest,
+                       DisableDuringNormalOperation) {

[emaxx, 2017/04/24 16:49:34]

nit: Please find a more relevant name for the test (words
"DuringNormalOperation" don't seem to correspond to the fact that the browser is
started with the preset policy here).

[Ivan Šandrk, 2017/04/24 17:18:24]

How about DisableBeforeStartup?

+  // Verify that the device disabled screen is being shown.
+  EXPECT_TRUE(DeviceDisabledScreenShown());
+}
+
 }  // namespace system
 }  // namespace chromeos