Prevent app_controller_mac.mm access lastProfile when EnterpriseStartupDialog is displayed.

Because the dialog must be displayed before the Profile initalization as
the process might load the extensions which might open browser windows.

TEST=Set up a invalid token by setting any non-empty string in policy.
     MachineLevelUserCloudPolicyEnrollmentToken.
     a) Login mac.
     b) Create com.google.Chrome.plist (org.chromium.Chromium.plist for chromium) for the
        following content:
         <?xml version="1.0" encoding="UTF-8"?>
         <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
         <plist version="1.0">
         <dict>
            <key>MachineLevelUserCloudPolicyEnrollmentToken</key>
            <string>invalid-token</string>
         </dict>
         </plist>
     (More details in https://support.google.com/chrome/a/answer/7550274?hl=en)
     Launch Chrome. Instead of the chrome window, there should be an error dialog indicates
     that launching is blocked.
     1) The error dialog could be hidden/shown, lose or gain focus without any issue.
     2) Clicking any option in the menu bar or dock menu shouldn't open any browser window.

Bug: 860540
Change-Id: Ibf4b7c2794a8ad78b9fedb2da67512508d3d7621
Reviewed-on: https://chromium-review.googlesource.com/1145862
Commit-Queue: Owen Min <zmin@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580242}

chrome/browser/app_controller_mac.mm

@@ -246,6 +246,10 @@ bool IsProfileSignedOut(Profile* profile) {
 // Given |webContents|, extracts a GURL to be used for Handoff. This may return
 // the empty GURL.
 - (GURL)handoffURLFromWebContents:(content::WebContents*)webContents;
+
+// Return false if Chrome startup is paused by dialog and AppController is
+// called without any initialized Profile.
+- (BOOL)isProfileReady;
 @end
 
 class AppControllerProfileObserver : public ProfileAttributesStorage::Observer {
@@ -433,6 +437,8 @@ static base::mac::ScopedObjCClassSwizzler* g_swizzle_imk_input_session;
 }
 
 - (void)applicationWillHide:(NSNotification*)notification {
+  if (![self isProfileReady])
+    return;
   apps::ExtensionAppShimHandler::OnChromeWillHide();
 }
 
@@ -613,7 +619,7 @@ static base::mac::ScopedObjCClassSwizzler* g_swizzle_imk_input_session;
 }
 
 - (void)windowDidResignMain:(NSNotification*)notify {
-  if (chrome::GetTotalBrowserCount() == 0) {
+  if (chrome::GetTotalBrowserCount() == 0 && [self isProfileReady]) {
     [self windowChangedToProfile:
         g_browser_process->profile_manager()->GetLastUsedProfile()];
   }
@@ -1012,7 +1018,7 @@ static base::mac::ScopedObjCClassSwizzler* g_swizzle_imk_input_session;
   // Ignore commands during session restore's browser creation.  It uses a
   // nested run loop and commands dispatched during this operation cause
   // havoc.
-  if (SessionRestore::IsRestoring(lastProfile) &&
+  if (lastProfile && SessionRestore::IsRestoring(lastProfile) &&
       base::RunLoop::IsNestedOnCurrentThread()) {
     return;
   }
@@ -1322,16 +1328,20 @@ static base::mac::ScopedObjCClassSwizzler* g_swizzle_imk_input_session;
   [app registerServicesMenuSendTypes:types returnTypes:types];
 }
 
+// Return null if Chrome is not ready or there is no ProfileManager.
 - (Profile*)lastProfile {
   // Return the profile of the last-used Browser, if available.
   if (lastProfile_)
     return lastProfile_;
 
+  if (![self isProfileReady])
+    return nullptr;
+
   // On first launch, use the logic that ChromeBrowserMain uses to determine
   // the initial profile.
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   if (!profile_manager)
-    return NULL;
+    return nullptr;
 
   return profile_manager->GetProfile(
       GetStartupProfilePath(profile_manager->user_data_dir(),
@@ -1341,6 +1351,9 @@ static base::mac::ScopedObjCClassSwizzler* g_swizzle_imk_input_session;
 - (Profile*)safeLastProfileForNewWindows {
   Profile* profile = [self lastProfile];
 
+  if (!profile)
+    return nullptr;
+
   // Guest sessions must always be OffTheRecord. Use that when opening windows.
   if (profile->IsGuestSession())
     return profile->GetOffTheRecordProfile();
@@ -1705,6 +1718,12 @@ static base::mac::ScopedObjCClassSwizzler* g_swizzle_imk_input_session;
   return webContents->GetVisibleURL();
 }
 
+- (BOOL)isProfileReady {
+  return !g_browser_process->browser_policy_connector()
+              ->machine_level_user_cloud_policy_controller()
+              ->IsEnterpriseStartupDialogShowing();
+}
+
 #pragma mark - HandoffActiveURLObserverBridgeDelegate
 
 - (void)handoffActiveURLChanged:(content::WebContents*)webContents {

chrome/browser/policy/cloud/machine_level_user_cloud_policy_browsertest.cc

@@ -17,6 +17,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_browser_main.h"
 #include "chrome/browser/chrome_browser_main_extra_parts.h"
@@ -25,6 +26,7 @@
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/policy/machine_level_user_cloud_policy_controller.h"
 #include "chrome/browser/policy/test/local_policy_test_server.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -42,6 +44,10 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/views/test/widget_test.h"
 
+#if defined(OS_MACOSX)
+#include "chrome/browser/policy/cloud/machine_level_user_cloud_policy_browsertest_mac_util.h"
+#endif
+
 using testing::DoAll;
 using testing::Invoke;
 using testing::InvokeWithoutArgs;
@@ -65,6 +71,10 @@ class MachineLevelUserCloudPolicyControllerObserver
  public:
   void OnPolicyRegisterFinished(bool succeeded) override {
     if (!succeeded) {
+      EXPECT_EQ(0u, chrome::GetTotalBrowserCount());
+#if defined(OS_MACOSX)
+      PostAppControllerNSNotifications();
+#endif
       // Close the error dialog.
       ASSERT_EQ(1u, views::test::WidgetTest::GetAllWidgets().size());
       (*views::test::WidgetTest::GetAllWidgets().begin())->Close();
@@ -424,6 +434,7 @@ IN_PROC_BROWSER_TEST_P(MachineLevelUserCloudPolicyEnrollmentTest, Test) {
                                         : std::string(),
             BrowserDMTokenStorage::Get()->RetrieveDMToken());
 
+  // Verify the enrollment result.
   MachineLevelUserCloudPolicyEnrollmentResult expected_result;
   if (is_enrollment_token_valid() && storage_enabled()) {
     expected_result = MachineLevelUserCloudPolicyEnrollmentResult::kSuccess;
@@ -434,6 +445,8 @@ IN_PROC_BROWSER_TEST_P(MachineLevelUserCloudPolicyEnrollmentTest, Test) {
     expected_result =
         MachineLevelUserCloudPolicyEnrollmentResult::kFailedToFetch;
   }
+
+  // Verify the metrics.
   histogram_tester_.ExpectBucketCount(kEnrollmentResultMetrics, expected_result,
                                       1);
   histogram_tester_.ExpectTotalCount(kEnrollmentResultMetrics, 1);

chrome/browser/policy/cloud/machine_level_user_cloud_policy_browsertest_mac_util.h

+// Copyright 2018 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.
+
+#ifndef CHROME_BROWSER_POLICY_CLOUD_MACHINE_LEVEL_USER_CLOUD_POLICY_BROWSERTEST_MAC_UTIL_H_
+#define CHROME_BROWSER_POLICY_CLOUD_MACHINE_LEVEL_USER_CLOUD_POLICY_BROWSERTEST_MAC_UTIL_H_
+
+namespace policy {
+
+// This is a helper function for MachineLevelUserCloudPolicyEnrollmentTest. It
+// sends few system notifications which app_controller_mac.mm listens to make
+// sure they're handle properly when the EnterpriseStartupDialog is being
+// displayed.
+void PostAppControllerNSNotifications();
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_POLICY_CLOUD_MACHINE_LEVEL_USER_CLOUD_POLICY_BROWSERTEST_MAC_UTIL_H_

chrome/browser/policy/cloud/machine_level_user_cloud_policy_browsertest_mac_util.mm

+// Copyright 2018 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 "chrome/browser/policy/cloud/machine_level_user_cloud_policy_browsertest_mac_util.h"
+
+#import <Cocoa/Cocoa.h>
+
+namespace policy {
+
+void PostAppControllerNSNotifications() {
+  // Simulate the user clicking a window other than the dialog.
+  // The Profile is not ready when the dialog is displayed, so it can't be
+  // accessed.
+  [[NSNotificationCenter defaultCenter]
+      postNotificationName:NSWindowDidResignMainNotification
+                    object:nil];
+
+  // Simulate the user hiding Chrome via Cmd+h when the dialog is displayed.
+  // The ExtensionAppShimHandler hasn't been created when the dialog is
+  // displayed, so it must be skipped.
+  [[NSNotificationCenter defaultCenter]
+      postNotificationName:NSApplicationWillHideNotification
+                    object:nil];
+}
+
+}  // namespace

chrome/browser/policy/machine_level_user_cloud_policy_register_watcher.cc

@@ -83,7 +83,7 @@ RegisterResult MachineLevelUserCloudPolicyRegisterWatcher::
 }
 
 bool MachineLevelUserCloudPolicyRegisterWatcher::IsDialogShowing() {
-  return dialog_ && dialog_->IsShowing();
+  return (dialog_ && dialog_->IsShowing()) || run_loop_.running();
 }
 
 void MachineLevelUserCloudPolicyRegisterWatcher::

chrome/test/BUILD.gn

@@ -1890,6 +1890,8 @@ test("browser_tests") {
         "//third_party/ocmock",
       ]
       sources += [
+        "../browser/policy/cloud/machine_level_user_cloud_policy_browsertest_mac_util.h",
+        "../browser/policy/cloud/machine_level_user_cloud_policy_browsertest_mac_util.mm",
         "../browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm",
         "../browser/spellchecker/spell_check_host_chrome_impl_mac_browsertest.cc",
         "../browser/ui/cocoa/applescript/bookmark_applescript_utils_test.h",