Added a test for crash in DeviceDisabledScreen

chrome/browser/chromeos/system/device_disabling_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <memory>
 #include <string>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ skipping 26 matching lines @@
 namespace chromeos {
 namespace system {
 
 namespace {
 
 void ErrorCallbackFunction(const std::string& error_name,
                            const std::string& error_message) {
   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
     : public OobeBaseTest,
       public NetworkStateInformer::NetworkStateInformerObserver {
  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();
 
   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;
 
   // 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);
 };
 
 
 DeviceDisablingTest::DeviceDisablingTest()
     : fake_session_manager_client_(new FakeSessionManagerClient) {
 }
 
+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());
-  // 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());
+  SetDeviceDisabledPolicy();
   // Trigger a policy fetch.
   fake_session_manager_client_->OnPropertyChangeComplete(true);
   // Wait for the policy fetch to complete and the disabled setting to change.
   run_loop.Run();
 }
 
 std::string DeviceDisablingTest::GetCurrentScreenName(
     content::WebContents* web_contents ) {
   std::string screen_name;
   if (!content::ExecuteScriptAndExtractString(
@@ skipping 23 matching lines @@
   // Set up fake networks.
   DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface()->
       SetupDefaultEnvironment();
 }
 
 void DeviceDisablingTest::UpdateState(NetworkError::ErrorReason reason) {
   network_state_change_wait_run_loop_->Quit();
 }
 
 IN_PROC_BROWSER_TEST_F(DeviceDisablingTest, DisableDuringNormalOperation) {
   // Mark the device as disabled and wait until cros settings update.

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

nit: I'd rather remove these comments here and in the new test - these comments
seem to duplicate what is already said in the function names.

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

Done.

   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
 // enabled. This case warrants its own test because the UI behaves somewhat
 // differently when the policy is set: A background job runs on startup that
 // causes the UI to try and show the login screen after some delay. It must
 // be ensured that the login screen does not show and does not clobber the
 // disabled screen.
 IN_PROC_BROWSER_TEST_F(DeviceDisablingTest, DisableWithEphemeralUsers) {
   // Connect to the fake Ethernet network. This ensures that Chrome OS will not
@@ skipping 53 matching lines @@
   network_state_change_wait_run_loop_->Run();
   network_state_informer->RemoveObserver(this);
   base::RunLoop().RunUntilIdle();
 
   // Verify that the offline error screen was not shown and the device disabled
   // screen is still being shown instead.
   EXPECT_EQ(GetOobeScreenName(OobeScreen::SCREEN_DEVICE_DISABLED),
             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