Modify ProfileDestroyer to prevent leaks.

Now doesn't wait to destroy non-incognito profiles, and allow immediate destruction of incognito profiles from their parent profile. Also, doesn't wait infinitely for render process hosts to go away, uses a timer to complete destruction. And finally, adds DCHECKs to identify when render process hosts are dead soon enough.

BUG=130772
TEST=Make sure Chrome exits without a crash and that Profile info properly got saved. Also make sure info doesn't leak from one incognito session to another.
Tests-Missing

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

[MAD, 2012-06-22 20:06:48]

Is this OK with you?

Thanks!

BYE
MAD...

[willchan no longer on Chromium, 2012-06-22 23:24:13]

Sorry, I don't have time to do a detailed review. Rachel, do you mind reviewing
this?

I'm fine with the principle that mad@ and I agreed to in an offline email. I
assume this is what he implemented here.

[MAD, 2012-06-26 14:27:38]

Ping?

If you can't review this, can you suggest another reviewer?

Thanks!

BYE
MAD...

[rpetterson, 2012-06-26 16:09:23]

Hi, mad,

I'm sheriffing yesterday and today so I haven't had a chance to get to it
yet. If I have some down time today I will try to look at it, but maybe
Elliot can get to it before me.

- Rachel

On Tue, Jun 26, 2012 at 7:27 AM, <mad@chromium.org> wrote:

> Ping?
>
> If you can't review this, can you suggest another reviewer?
>
> Thanks!
>
> BYE
> MAD...
>
>
http://codereview.chromium.**org/10645005/<http://codereview.chromium.org/106...
>

[rpetterson, 2012-06-26 18:35:34]

This seems reasonable. A couple of clarification questions . . .

http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_destroyer.cc (right):

http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_destroyer.cc:39: if (hosts.empty() ||
!profile->IsOffTheRecord()) {
I don't see the need for this added "!profile->IsOffTheRecord" since you
will/should never have the case where hosts are not empty and we aren't OTR. So
the only condition this is actually looking at is whether hosts are empty or not
and the OTRness of the profile is handled inside the statement. Is this just an
extra safeguard for release builds?

http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_destroyer.cc:43: delete profile;
Is there any chance we could be deleting a profile at this point with an
OTRprofile?

[MAD, 2012-06-26 18:53:20]

Thanks...

Answers to questions below, and update to comments on the way...

BYE
MAD

http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_destroyer.cc (right):

http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_destroyer.cc:39: if (hosts.empty() ||
!profile->IsOffTheRecord()) {
On 2012/06/26 18:35:34, rpetterson wrote:
> I don't see the need for this added "!profile->IsOffTheRecord" since you
> will/should never have the case where hosts are not empty and we aren't OTR.
So
> the only condition this is actually looking at is whether hosts are empty or
not
> and the OTRness of the profile is handled inside the statement. Is this just
an
> extra safeguard for release builds?

Safegard for release builds... For those cases where we need to fix a leak of a
host that still had a pointer to the profile being deleted... I'll update the
comment to make it clearer...

http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_destroyer.cc:43: delete profile;
On 2012/06/26 18:35:34, rpetterson wrote:
> Is there any chance we could be deleting a profile at this point with an
> OTRprofile? 

Yes, but then the profile_impl destructor will call
ProfileDestroyer::DestroyOffTheRecordProfileNow()...

OK?

[rpetterson, 2012-06-26 18:54:31]

Got it. LGTM

On 2012/06/26 18:53:20, MAD wrote:
> Thanks...
> 
> Answers to questions below, and update to comments on the way...
> 
> BYE
> MAD
> 
>
http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_destroyer.cc (right):
> 
>
http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_destroyer.cc:39: if (hosts.empty() ||
> !profile->IsOffTheRecord()) {
> On 2012/06/26 18:35:34, rpetterson wrote:
> > I don't see the need for this added "!profile->IsOffTheRecord" since you
> > will/should never have the case where hosts are not empty and we aren't OTR.
> So
> > the only condition this is actually looking at is whether hosts are empty or
> not
> > and the OTRness of the profile is handled inside the statement. Is this just
> an
> > extra safeguard for release builds?
> 
> Safegard for release builds... For those cases where we need to fix a leak of
a
> host that still had a pointer to the profile being deleted... I'll update the
> comment to make it clearer...
> 
>
http://codereview.chromium.org/10645005/diff/11007/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_destroyer.cc:43: delete profile;
> On 2012/06/26 18:35:34, rpetterson wrote:
> > Is there any chance we could be deleting a profile at this point with an
> > OTRprofile? 
> 
> Yes, but then the profile_impl destructor will call
> ProfileDestroyer::DestroyOffTheRecordProfileNow()...
> 
> OK?

[commit-bot: I haz the power, 2012-06-26 18:55:39]

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

[commit-bot: I haz the power, 2012-06-26 20:09:17]

Change committed as 144251