Do not initialize event routers in the Profile Import process.

Do not initialize event routers in the Profile Import process.

BUG=90909
TEST=launch organic browser with empty fresh profile; no message about a bad profile seen before search engine dialog.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98418

[Miranda Callahan, 2011-08-24 15:10:21]

On 2011/08/24 15:00:41, Miranda Callahan wrote:

Hmm, hold on -- not working yet; I thought I had a clean run-through. :-/

[Miranda Callahan, 2011-08-24 15:40:08]

On 2011/08/24 15:10:21, Miranda Callahan wrote:
> On 2011/08/24 15:00:41, Miranda Callahan wrote:
> 
> Hmm, hold on -- not working yet; I thought I had a clean run-through. :-/

Ready, I think -- please use eagle eyes to make sure this place is all right for
initialization of the EventRouters. The problem is this:

The profile in the secondary process creates the WDS and puts a lock on the
directories -- if the EventRouters are initialized before the secondary process
is launched, the primary process has a lock on these directories and the
secondary process can't write to them.

Is it ok for the event routers to be started on Bookmark Model Load like this?

[Yoyo Zhou, 2011-08-24 18:15:35]

By the way, you wrote that this was a crasher -- is that only in Debug
builds? The original bug report 90909 doesn't say that it crashes.

On Wed, Aug 24, 2011 at 8:40 AM,  <mirandac@chromium.org> wrote:
> On 2011/08/24 15:10:21, Miranda Callahan wrote:
>>
>> On 2011/08/24 15:00:41, Miranda Callahan wrote:
>
>> Hmm, hold on -- not working yet; I thought I had a clean run-through. :-/
>
> Ready, I think -- please use eagle eyes to make sure this place is all right
> for
> initialization of the EventRouters. The problem is this:
>
> The profile in the secondary process creates the WDS and puts a lock on the
> directories -- if the EventRouters are initialized before the secondary
> process
> is launched, the primary process has a lock on these directories and the
> secondary process can't write to them.

 Does the secondary process still need to initialize event routers?
(Would you also want the first change that you sent out?)

> Is it ok for the event routers to be started on Bookmark Model Load like
> this?

 Is there an event we could listen for that would tell us import was
finished (writing to those directories), or did not need to run? That
would seem to make the most sense.

 I'm not sure how early bookmarks get loaded. I asked sky about this;
he said it would depend on who asked for bookmarks first, which would
be unpredictable. Two potential issues I can think of are:
- events from between profile creation and bookmark load might be dropped
- tests might not load bookmarks as expected (but it looks like
TestingProfile does at least try to load bookmarks)

 Which event router(s) lock the WDS directories? If it's only, say,
the bookmark event router, then I could see delaying initializing that
one (it should be okay because we don't import bookmarks on first
run).

[Miranda Callahan, 2011-08-25 12:20:27]

On 2011/08/24 18:15:35, Yoyo Zhou wrote:
> By the way, you wrote that this was a crasher -- is that only in Debug
> builds? The original bug report 90909 doesn't say that it crashes.

Sorry for the confusion -- it does crash the import process, but this doesn't
read as a "crash" per se in Chrome itself, which is smart enough to recover and
present the user with a nice dialog.

> 
> On Wed, Aug 24, 2011 at 8:40 AM,  <mailto:mirandac@chromium.org> wrote:
> > On 2011/08/24 15:10:21, Miranda Callahan wrote:
> >>
> >> On 2011/08/24 15:00:41, Miranda Callahan wrote:
> >
> >> Hmm, hold on -- not working yet; I thought I had a clean run-through. :-/
> >
> > Ready, I think -- please use eagle eyes to make sure this place is all right
> > for
> > initialization of the EventRouters. The problem is this:
> >
> > The profile in the secondary process creates the WDS and puts a lock on the
> > directories -- if the EventRouters are initialized before the secondary
> > process
> > is launched, the primary process has a lock on these directories and the
> > secondary process can't write to them.
> 
>  Does the secondary process still need to initialize event routers?
> (Would you also want the first change that you sent out?)

You are correct that I still want that first change!

> 
> > Is it ok for the event routers to be started on Bookmark Model Load like
> > this?
> 
>  Is there an event we could listen for that would tell us import was
> finished (writing to those directories), or did not need to run? That
> would seem to make the most sense.

I agree. I was trying for "minimal change" because this needs to be merged to
14, but I think this is the correct approach, and that's what I did for this CL.

> 
>  I'm not sure how early bookmarks get loaded. I asked sky about this;
> he said it would depend on who asked for bookmarks first, which would
> be unpredictable. Two potential issues I can think of are:
> - events from between profile creation and bookmark load might be dropped
> - tests might not load bookmarks as expected (but it looks like
> TestingProfile does at least try to load bookmarks)
> 
>  Which event router(s) lock the WDS directories? If it's only, say,
> the bookmark event router, then I could see delaying initializing that
> one (it should be okay because we don't import bookmarks on first
> run).

Well, I started writing it this way (you're right, that's the one that blocks),
but I was afraid of messing with the clarity of the extension service's
understanding of event routers as being either all initialized, or all not
initialized; if it doesn't matter if we wait, it seems clear to just hold off on
initialization of all of them until import is done.

My concerns with this patch are that I don't know the consequence of not
initializing the event routers until the profile has finished the import
process; can anything bad happen due to this delay?  It could also be the case
that part of the import process includes initializing bookmarks from a file
silently -- this will only happen in special distributions of Chrome, and will
happen instead of the normal import process. Does this change anything?

Thanks for your incredibly helpful review -- what do you think of this new
patch?

[Miranda Callahan, 2011-08-25 12:38:01]

On 2011/08/25 12:20:27, Miranda Callahan wrote:
> On 2011/08/24 18:15:35, Yoyo Zhou wrote:
> > By the way, you wrote that this was a crasher -- is that only in Debug
> > builds? The original bug report 90909 doesn't say that it crashes.
> 
> Sorry for the confusion -- it does crash the import process, but this doesn't
> read as a "crash" per se in Chrome itself, which is smart enough to recover
and
> present the user with a nice dialog.
> 
> > 
> > On Wed, Aug 24, 2011 at 8:40 AM,  <mailto:mirandac@chromium.org> wrote:
> > > On 2011/08/24 15:10:21, Miranda Callahan wrote:
> > >>
> > >> On 2011/08/24 15:00:41, Miranda Callahan wrote:
> > >
> > >> Hmm, hold on -- not working yet; I thought I had a clean run-through. :-/
> > >
> > > Ready, I think -- please use eagle eyes to make sure this place is all
right
> > > for
> > > initialization of the EventRouters. The problem is this:
> > >
> > > The profile in the secondary process creates the WDS and puts a lock on
the
> > > directories -- if the EventRouters are initialized before the secondary
> > > process
> > > is launched, the primary process has a lock on these directories and the
> > > secondary process can't write to them.
> > 
> >  Does the secondary process still need to initialize event routers?
> > (Would you also want the first change that you sent out?)
> 
> You are correct that I still want that first change!
> 
> > 
> > > Is it ok for the event routers to be started on Bookmark Model Load like
> > > this?
> > 
> >  Is there an event we could listen for that would tell us import was
> > finished (writing to those directories), or did not need to run? That
> > would seem to make the most sense.
> 
> I agree. I was trying for "minimal change" because this needs to be merged to
> 14, but I think this is the correct approach, and that's what I did for this
CL.
> 
> > 
> >  I'm not sure how early bookmarks get loaded. I asked sky about this;
> > he said it would depend on who asked for bookmarks first, which would
> > be unpredictable. Two potential issues I can think of are:
> > - events from between profile creation and bookmark load might be dropped
> > - tests might not load bookmarks as expected (but it looks like
> > TestingProfile does at least try to load bookmarks)
> > 
> >  Which event router(s) lock the WDS directories? If it's only, say,
> > the bookmark event router, then I could see delaying initializing that
> > one (it should be okay because we don't import bookmarks on first
> > run).
> 
> Well, I started writing it this way (you're right, that's the one that
blocks),
> but I was afraid of messing with the clarity of the extension service's
> understanding of event routers as being either all initialized, or all not
> initialized; if it doesn't matter if we wait, it seems clear to just hold off
on
> initialization of all of them until import is done.
> 
> My concerns with this patch are that I don't know the consequence of not
> initializing the event routers until the profile has finished the import
> process; can anything bad happen due to this delay?  It could also be the case
> that part of the import process includes initializing bookmarks from a file
> silently -- this will only happen in special distributions of Chrome, and will
> happen instead of the normal import process. Does this change anything?

Ok, I went back and looked at your first patch to change the initialization of
the eventrouters to the profile, and I see that it happened even later in
BrowserMain:

http://codereview.chromium.org/7400003

So I feel much more confident about moving the initialization in this patch.

> 
> Thanks for your incredibly helpful review -- what do you think of this new
> patch?

[Yoyo Zhou, 2011-08-25 16:34:03]

LGTM
Yes, this version is much clearer.

http://codereview.chromium.org/7717025/diff/10003/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7717025/diff/10003/chrome/browser/browser_main...
chrome/browser/browser_main.cc:1801: FirstRun::AutoImport(profile,
This returns after the import is finished (or it's done locking the WDS
directory), right?

http://codereview.chromium.org/7717025/diff/10003/chrome/browser/browser_main...
chrome/browser/browser_main.cc:1821:
g_browser_process->profile_manager()->SetWillImport(false, profile);
Maybe this function should have a different name -- at first glance it isn't
obvious that this notifies the profile manager that import is finished.

Perhaps
SetWillImport()
and
[On]ImportFinished(profile)
?
(I'm not sure about the naming conventions here.)

[Miranda Callahan, 2011-08-26 08:31:32]

http://codereview.chromium.org/7717025/diff/10003/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7717025/diff/10003/chrome/browser/browser_main...
chrome/browser/browser_main.cc:1801: FirstRun::AutoImport(profile,
On 2011/08/25 16:34:03, Yoyo Zhou wrote:
> This returns after the import is finished (or it's done locking the WDS
> directory), right?

Yes it does; the rest of the startup process continues below.

http://codereview.chromium.org/7717025/diff/10003/chrome/browser/browser_main...
chrome/browser/browser_main.cc:1821:
g_browser_process->profile_manager()->SetWillImport(false, profile);
On 2011/08/25 16:34:03, Yoyo Zhou wrote:
> Maybe this function should have a different name -- at first glance it isn't
> obvious that this notifies the profile manager that import is finished.
> 
> Perhaps
> SetWillImport()
> and
> [On]ImportFinished(profile)
> ?
> (I'm not sure about the naming conventions here.)

Yeah, I wasn't really happy with this either. I'll use your suggestion.

[commit-bot: I haz the power, 2011-08-26 13:17:42]

Change committed as 98418