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

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

SigninManagerFactory depends on services that initialize databases
accessed by the import process. This change ignores profile added
notifications that occur while the import process is still running,
instead waiting for the import finished notification to complete signin
initialization.

BUG=180459
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186990

[tapted, 2013-03-08 12:26:53]

I think has a good chance of fixing the lingering first-run "profile could not
be opened" error, and isn't introducing any regressions.

[Joao da Silva, 2013-03-08 12:51:49]

lgtm

I got that error earlier this week, I'll see if it still repros and if this
fixes it.

[tapted, 2013-03-08 13:22:58]

I've just managed to convince hive to let me run a test case on XP.

With an unpatched official/branded chrome at r186898 I could reliably get the
"your profile could not be opened" error, with

chrome.exe --force-first-run --user-data-dir=d1
chrome.exe --force-first-run --user-data-dir=d2
etc.

After patching this in, the problem goes away.

[tapted, 2013-03-08 15:08:08]

Sorry for the pre-emptive publish. This should now keep all the tests green
(tested unit_tests locally, and others are green in earlier patch sets). PTAL.

[Joao da Silva, 2013-03-08 15:12:54]

lgtm

[commit-bot: I haz the power, 2013-03-08 15:14:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/12461010/20001

[commit-bot: I haz the power, 2013-03-08 16:05:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/12461010/29001

[commit-bot: I haz the power, 2013-03-08 18:16:37]

Change committed as 186990

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

Post-commit drive-by review!

https://chromiumcodereview.appspot.com/12461010/diff/29001/chrome/browser/pol...
File chrome/browser/policy/user_policy_signin_service.cc (right):

https://chromiumcodereview.appspot.com/12461010/diff/29001/chrome/browser/pol...
chrome/browser/policy/user_policy_signin_service.cc:366:
DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type);
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:51:59]

https://chromiumcodereview.appspot.com/12461010/diff/29001/chrome/browser/pol...
File chrome/browser/policy/user_policy_signin_service.cc (right):

https://chromiumcodereview.appspot.com/12461010/diff/29001/chrome/browser/pol...
chrome/browser/policy/user_policy_signin_service.cc:366:
DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type);
On 2013/03/26 12:29:32, Bernhard Bauer wrote:
> Wait, doesn't this DCHECK fail when we get *any* other notification while the
> import process is running?

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]

https://chromiumcodereview.appspot.com/12461010/diff/29001/chrome/browser/pol...
File chrome/browser/policy/user_policy_signin_service.cc (right):

https://chromiumcodereview.appspot.com/12461010/diff/29001/chrome/browser/pol...
chrome/browser/policy/user_policy_signin_service.cc:366:
DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type);
On 2013/03/26 12:52:00, Joao da Silva wrote:
> On 2013/03/26 12:29:32, Bernhard Bauer wrote:
> > Wait, doesn't this DCHECK fail when we get *any* other notification while
the
> > import process is running?
> 
> 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?

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]

https://chromiumcodereview.appspot.com/12461010/diff/29001/chrome/browser/pol...
File chrome/browser/policy/user_policy_signin_service.cc (right):

https://chromiumcodereview.appspot.com/12461010/diff/29001/chrome/browser/pol...
chrome/browser/policy/user_policy_signin_service.cc:366:
DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type);
On 2013/03/26 14:18:22, Bernhard Bauer wrote:
> On 2013/03/26 12:52:00, Joao da Silva wrote:
> > On 2013/03/26 12:29:32, Bernhard Bauer wrote:
> > > Wait, doesn't this DCHECK fail when we get *any* other notification while
> the
> > > import process is running?
> > 
> > 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?
> 
> 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.
> 

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?

> 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.

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.