Move the window destruction and registration out of cleanup and into BrowserProcessImpl::EndSession…

Move the window destruction and registration out of cleanup and into BrowserProcessImpl::EndSession to make sure it happens after profiles are written out.

BUG=127607
TEST=Run on slow computer, open three or more profiles, exit from wrench menu, immediately open chrome again. There should be no reports of profile corruption and logs should show that profile writing is not occuring after second chrome starts.

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

[Jói, 2012-06-14 10:32:49]

This seems close to ready, see comments below.

Cheers,
Jói

http://codereview.chromium.org/10542151/diff/1/chrome/browser/browser_process...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/10542151/diff/1/chrome/browser/browser_process...
chrome/browser/browser_process_impl.cc:353: DCHECK(process_singleton_);
This only checks that the pointer is not NULL (which you might as well DCHECK at
construction time); it does not guarantee that the object pointed to is still
around.

I would suggest moving the DCHECK to the constructor.

Here, you could have an invariant of data like this:

// BrowserProcess::EndSession is only called in response to a window
// message sent by Windows when the user logs out or shuts down.
// When this occurs, all of Chrome is still running, so the pointer
// to process_singleton_ should still be valid.

http://codereview.chromium.org/10542151/diff/1/chrome/browser/browser_process...
chrome/browser/browser_process_impl.cc:354:
process_singleton_->UnregisterWindowClass();
BrowserProcess::EndSession is only called when the user logs off or shuts down
the machine.  We need to make sure we also unregister the window class in the
normal shutdown case (e.g. Alt+F4).

I think this is as simple as also calling UnregisterWindowClass in the
destructor of ProcessSingleton.

[rpetterson, 2012-06-14 22:12:50]

So this does fix the issue in the bug. However, one issue I'm seeing now is that
when I go to open a second Chrome, if the first has not finished
writing/closing, the second one just doesn't open (I get a spinner briefly and
then nothing) and I have to click it a second or third time to actually start up
Chrome once more. Is this expected or should we be waiting so that the user
doesn't have to click a second or third time?

http://codereview.chromium.org/10542151/diff/1/chrome/browser/browser_process...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/10542151/diff/1/chrome/browser/browser_process...
chrome/browser/browser_process_impl.cc:353: DCHECK(process_singleton_);
On 2012/06/14 10:32:49, Jói wrote:
> This only checks that the pointer is not NULL (which you might as well DCHECK
at
> construction time); it does not guarantee that the object pointed to is still
> around.
> 
> I would suggest moving the DCHECK to the constructor.
> 
> Here, you could have an invariant of data like this:
> 
> // BrowserProcess::EndSession is only called in response to a window
> // message sent by Windows when the user logs out or shuts down.
> // When this occurs, all of Chrome is still running, so the pointer
> // to process_singleton_ should still be valid.

Done.

http://codereview.chromium.org/10542151/diff/1/chrome/browser/browser_process...
chrome/browser/browser_process_impl.cc:354:
process_singleton_->UnregisterWindowClass();
On 2012/06/14 10:32:49, Jói wrote:
> BrowserProcess::EndSession is only called when the user logs off or shuts down
> the machine.  We need to make sure we also unregister the window class in the
> normal shutdown case (e.g. Alt+F4).
> 
> I think this is as simple as also calling UnregisterWindowClass in the
> destructor of ProcessSingleton.

Done.

[rpetterson, 2012-06-14 23:59:28]

Adding jam@ since he's been assisting and may have an idea about the new issue
I'm seeing.

[Jói, 2012-06-15 10:04:57]

LGTM

I'm not 100% sure but perhaps the second Chrome instance simply notices the
original Chrome instance, and "opens" that instead, which normally would be the
right thing to do.  Since the original instance is actually in the process of
shutdown in this case, from the user's perspective nothing useful happens.  I'm
not clear on whether this is by design and "as good as we can make it" or not. 
John is likely to know more.

What I can say, is that your change seems to make the window class registration
and unregistration, and the window creation and destruction, logically identical
to what they were before r121480 except the correct module handle is now used
for registration/unregistration.

Cheers,
Jói

http://codereview.chromium.org/10542151/diff/5001/chrome/browser/process_sing...
File chrome/browser/process_singleton_win.cc (right):

http://codereview.chromium.org/10542151/diff/5001/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:202: Cleanup();
nit: since Cleanup() is a no-op now, you could remove this.

[jam, 2012-06-15 14:58:34]

I agree with Joi that this behavior is fine. The first process can't create new
windows since it's already started shutdown. This also restores the previous
behavior.

Now that I think about it more, isn't it unnecessary to do the cleanup in
EndSession? i.e. the user is logging out, so who cares if we leak the window
class. That would make the patch much simpler (i.e. simply move what's in
Cleanup() to the destructor).

Joi: as a Windows expert, wdyt?

[rpetterson, 2012-06-15 19:50:07]

Updated with the discussed changes. Now a much simpler CL :)

[jam, 2012-06-15 19:52:50]

On 2012/06/15 19:50:07, rpetterson wrote:
> Updated with the discussed changes. Now a much simpler CL :)

nit: no need for UnregisterWindowClass, i.e. just move the contents of that
function to the destructor

[rpetterson, 2012-06-15 20:38:01]

And an even simpler CL :)

[jam, 2012-06-15 21:56:22]

lgtm!

http://codereview.chromium.org/10542151/diff/4004/chrome/browser/process_sing...
File chrome/browser/process_singleton_win.cc (right):

http://codereview.chromium.org/10542151/diff/4004/chrome/browser/process_sing...
chrome/browser/process_singleton_win.cc:209: window_ = NULL;
nit: this was never needed before, so feel free to take it out

[commit-bot: I haz the power, 2012-06-15 23:50:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10542151/20002

[commit-bot: I haz the power, 2012-06-16 00:22:47]

Try job failure for 10542151-20002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-06-18 07:13:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10542151/20002

[commit-bot: I haz the power, 2012-06-18 08:47:58]

Change committed as 142682