Delay loading the SigninManagerFactory until the first-run Import process has completed.

chrome/browser/policy/user_policy_signin_service.cc

 // Copyright (c) 2012 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/user_policy_signin_service.h"
 
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
@@ skipping 239 matching lines @@
   // TokenService should not yet have loaded its tokens since this happens in
   // the background after PKS initialization - so this service should always be
   // created before the oauth token is available.
   DCHECK(!TokenServiceFactory::GetForProfile(profile_)->HasOAuthLoginToken());
 
   // Register a listener to be called back once the current profile has finished
   // initializing, so we can startup the UserCloudPolicyManager.
   registrar_.Add(this,
                  chrome::NOTIFICATION_PROFILE_ADDED,
                  content::Source<Profile>(profile));
+
+  // Register a listener for the import finished notification in a first run
+  // scenario, which indicates the profile is ready to be further initialized.
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_IMPORT_FINISHED,
+                 content::Source<Profile>(profile));
 }
 
 UserPolicySigninService::~UserPolicySigninService() {}
 
 void UserPolicySigninService::RegisterPolicyClient(
     const std::string& username,
     const std::string& oauth2_refresh_token,
     const PolicyRegistrationCallback& callback) {
   DCHECK(!username.empty());
   DCHECK(!oauth2_refresh_token.empty());
@@ skipping 74 matching lines @@
   // Manager should be fully initialized by now.
   DCHECK(manager);
   DCHECK(manager->core()->service());
   manager->core()->service()->AddObserver(this);
 }
 
 void UserPolicySigninService::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
+  // If an import process is running, wait for NOTIFICATION_IMPORT_FINISHED
+  // before potentially creating the SigninManager. Its dependencies can access
+  // databases that the import process is also accessing, causing conflicts.
+  // Note that the profile manager is NULL in unit tests.
+  if (g_browser_process->profile_manager() &&
+      g_browser_process->profile_manager()->will_import()) {
+    DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type);

[Bernhard Bauer, 2013/03/26 12:29:32]

Wait, doesn't this DCHECK fail when we get *any* other notification while the
import process is running?

[Joao da Silva, 2013/03/26 12:52:00]

will_import() signals that the first run import will be performed, which happens
synchronously on the UI thread. So other notifications can't be issued. See
ChromeBrowserMainParts::PreMainMessageLoopRunImpl().

If do_first_run_tasks_ is true, then it sets will_import, creates the profile
(which triggers NOTIFICATION_PROFILE_ADDED), and later runs the import process,
and then unsets the flag (which triggers NOTIFICATION_IMPORT_FINISHED, and by
that time will_import is already returning false). All of this happens in the
same function, and the UI loop isn't being pumped.

Does this make sense to you, or do you see other ways that could possibly end up
with another notification here?

[Bernhard Bauer, 2013/03/26 14:18:22]

Well, I ran into this with a local build that tries to set up Sync for managed
users, which requires passing in an OAuth token, thus triggering the
TOKEN_AVAILABLE notification.

It just seems strange (and brittle) to me that we assume no other notification
will happen, even if it may be completely independent of profile loading.

[Joao da Silva, 2013/03/26 14:35:27]

IIUC, that would only be a problem if you do that while the import is in
progress (or if you post to UI before startup has finished). Can you delay that
until the first_run_tasks are done?
IMO the larger problem is that the import process spins the UI loop. A lot more
places have similar stopgaps to handle the import process spinning the loop
before startup has finished.

I'm not sure what's the best fix here, and Drew is away for a couple of weeks. I
think it's OK to remove the DCHECK, since InitializeForSignedInUser() is called
anyway once the import process is done. If you change this then please check
back with Drew once he's back.

+    return;
+  }
+
   // If using a TestingProfile with no SigninManager or UserCloudPolicyManager,
   // skip initialization.
   if (!GetManager() || !SigninManagerFactory::GetForProfile(profile_)) {
     DVLOG(1) << "Skipping initialization for tests due to missing components.";
     return;
   }
 
   switch (type) {
     case chrome::NOTIFICATION_GOOGLE_SIGNED_OUT:
       ShutdownUserCloudPolicyManager();
       break;
+    case chrome::NOTIFICATION_IMPORT_FINISHED:
     case chrome::NOTIFICATION_PROFILE_ADDED: {
       // A new profile has been loaded - if it's signed in, then initialize the
       // UCPM, otherwise shut down the UCPM (which deletes any cached policy
       // data). This must be done here instead of at constructor time because
       // the Profile is not fully initialized when this object is constructed
       // (DoFinalInit() has not yet been called, so ProfileIOData and
       // SSLConfigServiceManager have not been created yet).
       // TODO(atwilson): Switch to using a timer instead, to avoid contention
       // with other services at startup (http://crbug.com/165468).
       SigninManager* signin_manager =
@@ skipping 160 matching lines @@
   // before UserCloudPolicyManager shuts down the CloudPolicyClient.
   registration_helper_.reset();
   StopObserving();
 }
 
 UserCloudPolicyManager* UserPolicySigninService::GetManager() {
   return UserCloudPolicyManagerFactory::GetForProfile(profile_);
 }
 
 }  // namespace policy