Also delay regular profile destruction...

Also delay regular profile destruction...

A previous CL delayed the destruction of Off The Record profiles, but there are cases where they are delayed post destruction of their original profile... no good...

So we now delay the destruction of both the off the record profile and it's original profile when either of them is still referenced by a render process host.

BUG=114245
TEST=unittest --gtest_filter=ProfileDestroyerTest.*

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

Got reverted because of failures in extension api tests, so trying again with fix in extension tests to avoid an infobar leak, which was keeping a render process host alive for too long and causing an assert that not all profiles were destroyed...

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

[MAD, 2012-02-17 01:35:03]

Please validate the approach (and the code of course :-), I asked marja@ for the
code review since you seem the be the one that touched this code the most
recently...

Also, I'll need approvals from owners of both browser/UI and browser/PROFILES...
So I put two of each on the CC line... Let me know if you would like me to ask
others...

Thanks!

BYE
MAD

[marja, 2012-02-17 08:34:42]

I haven't worked on profile deletion / profile lifetime / incognito profiles, so
I guess rlp / sail (added as cc) / ... should review this instead.

[MAD, 2012-02-17 15:52:08]

OK, Rachel, can you CR this please?

Otherwise, please let me know, and I'll put sail@ as a reviewer instead...

I still need OWNERS approval for both browser/ui and browser/profiles changes...

Thanks!

BYE
MAD

[sky, 2012-02-17 16:53:09]

https://chromiumcodereview.appspot.com/9420036/diff/2003/chrome/browser/profi...
File chrome/browser/profiles/profile_destroyer.cc (right):

https://chromiumcodereview.appspot.com/9420036/diff/2003/chrome/browser/profi...
chrome/browser/profiles/profile_destroyer.cc:19: // Some tests try to destroy
their profile on other threads than the UI.
I hate to have to change around code like this for a test. Could we change the
tests to destroy on the UI thread?

[MAD, 2012-02-17 17:06:09]

OK, I'll find and fix all the tests that destroy a profile from the wrong thread
and send an updated patch...

BYE
MAD

https://chromiumcodereview.appspot.com/9420036/diff/2003/chrome/browser/profi...
File chrome/browser/profiles/profile_destroyer.cc (right):

https://chromiumcodereview.appspot.com/9420036/diff/2003/chrome/browser/profi...
chrome/browser/profiles/profile_destroyer.cc:19: // Some tests try to destroy
their profile on other threads than the UI.
On 2012/02/17 16:53:09, sky wrote:
> I hate to have to change around code like this for a test. Could we change the
> tests to destroy on the UI thread?

Yeah, I agree with you... I was lazy... I was scared that there would be many of
them... But you are right, it's worth going through all of them... I'll do that
and send another patch...

Thanks! ~:-)

[rpetterson, 2012-02-17 21:22:10]

I have limited computer access today so if you need this reviewed before
next week, hopefully Sail can take it.
On Feb 17, 2012 9:06 AM, <mad@chromium.org> wrote:

> OK, I'll find and fix all the tests that destroy a profile from the wrong
> thread
> and send an updated patch...
>
> BYE
> MAD
>
>
>
> https://chromiumcodereview.**appspot.com/9420036/diff/2003/**
>
chrome/browser/profiles/**profile_destroyer.cc<https://chromiumcodereview.appspot.com/9420036/diff/2003/chrome/browser/profiles/profile_destroyer.cc>
> File chrome/browser/profiles/**profile_destroyer.cc (right):
>
> https://chromiumcodereview.**appspot.com/9420036/diff/2003/**
>
chrome/browser/profiles/**profile_destroyer.cc#newcode19<https://chromiumcodereview.appspot.com/9420036/diff/2003/chrome/browser/profiles/profile_destroyer.cc#newcode19>
> chrome/browser/profiles/**profile_destroyer.cc:19: // Some tests try to
> destroy their profile on other threads than the UI.
> On 2012/02/17 16:53:09, sky wrote:
>
>> I hate to have to change around code like this for a test. Could we
>>
> change the
>
>> tests to destroy on the UI thread?
>>
>
> Yeah, I agree with you... I was lazy... I was scared that there would be
> many of them... But you are right, it's worth going through all of
> them... I'll do that and send another patch...
>
> Thanks! ~:-)
>
>
https://chromiumcodereview.**appspot.com/9420036/<https://chromiumcodereview....
>

[MAD, 2012-02-17 21:22:50]

How about this for making sure that the testing browser process always destroy
the profile manager while running from the UI thread?

The problem is that some of the tests are correctly setting the UI thread, and
others aren't... For those who are, we can't set the UI thread at the beginning
of the testing browser process because they, they won't be able to set it
again... And I don't think that removing it from all these tests is a good
idea... Some of them set it with their own message loop, so I didn't dare trying
to do that cleanup...

And I thought that since the testing browser process is the one owning and
destroying the profile manager, it should be the one making sure it's doing it
from the UI thread, right?

Otherwise, I would need to find all the tests that are not setting the UI thread
properly, and also make sure that they reset the testing browser process'
profile manager before their instance of the UI thread is destroyed... Not
trivial... Unless I'm missing something obvious (which is far from being
unlikely, ;-)...

Thanks!

BYE
MAD

[MAD, 2012-02-17 21:27:26]

On Fri, Feb 17, 2012 at 4:22 PM, Rachel Weinstein Petterson <
rlp@chromium.org> wrote:

> I have limited computer access today so if you need this reviewed before
> next week, hopefully Sail can take it.
>
No problem, it can wait until next week... I'm on the east coast, so am
about to leave for the long weekend anyway...

Thanks!


> On Feb 17, 2012 9:06 AM, <mad@chromium.org> wrote:
>
>> OK, I'll find and fix all the tests that destroy a profile from the wrong
>> thread
>> and send an updated patch...
>>
>> BYE
>> MAD
>>
>>
>>
>> https://chromiumcodereview.**appspot.com/9420036/diff/2003/**
>>
chrome/browser/profiles/**profile_destroyer.cc<https://chromiumcodereview.appspot.com/9420036/diff/2003/chrome/browser/profiles/profile_destroyer.cc>
>> File chrome/browser/profiles/**profile_destroyer.cc (right):
>>
>> https://chromiumcodereview.**appspot.com/9420036/diff/2003/**
>>
chrome/browser/profiles/**profile_destroyer.cc#newcode19<https://chromiumcodereview.appspot.com/9420036/diff/2003/chrome/browser/profiles/profile_destroyer.cc#newcode19>
>> chrome/browser/profiles/**profile_destroyer.cc:19: // Some tests try to
>> destroy their profile on other threads than the UI.
>> On 2012/02/17 16:53:09, sky wrote:
>>
>>> I hate to have to change around code like this for a test. Could we
>>>
>> change the
>>
>>> tests to destroy on the UI thread?
>>>
>>
>> Yeah, I agree with you... I was lazy... I was scared that there would be
>> many of them... But you are right, it's worth going through all of
>> them... I'll do that and send another patch...
>>
>> Thanks! ~:-)
>>
>>
https://chromiumcodereview.**appspot.com/9420036/<https://chromiumcodereview....
>>
>

[MAD, 2012-02-21 19:51:10]

Friendly post long weekend ping... All bots are happy... :-)

Now waiting for a full review and OWNERs reviews for both browser/UI and
browser/PROFILES...

Thanks!

BYE
MAD

[rpetterson, 2012-02-21 22:41:50]

LGTM for profiles.

[sky, 2012-02-22 17:52:11]

LGTM

[commit-bot: I haz the power, 2012-02-22 18:09:26]

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

[MAD, 2012-02-22 18:11:55]

Thanks all... :)

On Wed, Feb 22, 2012 at 1:09 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>
https://chromium-status.**appspot.com/cq/mad@chromium.**org/9420036/9007<http...
>
>
>
http://codereview.chromium.**org/9420036/<http://codereview.chromium.org/9420...
>

[commit-bot: I haz the power, 2012-02-22 19:42:00]

Change committed as 123082

[MAD, 2012-05-11 17:04:30]

Don't delay the destruction of the TestProfile

[MAD, 2012-05-11 17:10:11]

Don't delay the destruction of the TestProfile

[MAD, 2012-05-11 17:18:46]

Can you take a look at the latest change I did to the profile destroyer? I added
a special case for the TestProfile so that we don't delay the destruction of the
profile in tests since this was causing trouble in a number of places... I
didn't make any other changes... Just merges...

I kept the changes I did to the get_views extension tests anyway, but I could
remove them if we prefer to keep the tests as is now that they shouldn't trip on
the rest of the patch (but my change does fix a test leak so we might as well
keep it).

I unfortunately forgot to upload a "merge only" patch, so you won't be able to
confirm I didn't change the other files without going through all the merges
that happened... Really sorry about that... I could try to do some git magic if
you want... But when I tried, I had trouble with the moved/renamed files...
:-(... I'm open to suggestions on how to do this though... :-)

Thanks!

BYE
MAD
P.S.: Note that someone flagged the bug that this fixes as a BetaBlocker for
M20... so... you know... :-)

[MAD, 2012-05-11 17:25:47]

On 2012/05/11 17:18:46, MAD wrote:
> Can you take a look at the latest change I did to the profile destroyer? I
added
> a special case for the TestProfile so that we don't delay the destruction of
the
> profile in tests since this was causing trouble in a number of places... I
> didn't make any other changes... Just merges...
> 
> I kept the changes I did to the get_views extension tests anyway, but I could
> remove them if we prefer to keep the tests as is now that they shouldn't trip
on
> the rest of the patch (but my change does fix a test leak so we might as well
> keep it).
> 
> I unfortunately forgot to upload a "merge only" patch, so you won't be able to
> confirm I didn't change the other files without going through all the merges
> that happened... Really sorry about that... I could try to do some git magic
if
> you want... But when I tried, I had trouble with the moved/renamed files...
> :-(... I'm open to suggestions on how to do this though... :-)
> 
> Thanks!
> 
> BYE
> MAD
> P.S.: Note that someone flagged the bug that this fixes as a BetaBlocker for
> M20... so... you know... :-)

Wait, I found how to do it... Stay tuned, I'll have a cleaner set of patches in
a minute...

BYE
MAD

[MAD, 2012-05-11 17:28:31]

Now handle TestProfile correctly

[MAD, 2012-05-11 17:30:53]

OK, now you can look at it and see all the merges in Patch Set 7 and only the
most recent changes in Patch Set 8...

Thanks!

BYE
MAD

[sky, 2012-05-11 17:35:47]

LGTM

[commit-bot: I haz the power, 2012-05-11 17:52:42]

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

[MAD, 2012-05-11 17:52:45]

Thanks sky@!...

rpetterson@, I'll hit the commit queue now, let me know if you have
objections to this new change...

BYE
MAD

On Fri, May 11, 2012 at 1:35 PM, <sky@chromium.org> wrote:

> LGTM
>
>
http://codereview.chromium.**org/9420036/<http://codereview.chromium.org/9420...
>

[rpetterson, 2012-05-11 18:21:23]

lgtm

[MAD, 2012-05-11 18:36:48]

Thanks!

On Fri, May 11, 2012 at 2:21 PM, <rlp@chromium.org> wrote:

> lgtm
>
>
>
>
http://codereview.chromium.**org/9420036/<http://codereview.chromium.org/9420...
>

[commit-bot: I haz the power, 2012-05-11 18:54:42]

Try job failure for 9420036-49018 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-16 19:36:45]

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

[commit-bot: I haz the power, 2012-05-16 20:04:22]

Try job failure for 9420036-57019 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-16 20:26:17]

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

[commit-bot: I haz the power, 2012-05-16 21:17:31]

Try job failure for 9420036-57036 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-19 13:22:47]

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

[commit-bot: I haz the power, 2012-05-19 14:37:10]

Change committed as 138038

[willchan no longer on Chromium, 2012-06-06 21:31:03]

This change effectively makes Profile refcounted, which I think is undesirable.
Can we fix the root problem instead, that renderer processes are not getting
destroyed on time / getting leaked?

[MAD, 2012-06-06 21:34:51]

On 2012/06/06 21:31:03, willchan wrote:
> This change effectively makes Profile refcounted, which I think is
undesirable.
> Can we fix the root problem instead, that renderer processes are not getting
> destroyed on time / getting leaked?

I couldn't find a way to do that reliably, so I decided to delay the profile
destruction instead... Which I felt was safe, since profiles are usually
destroyed pretty late in the game anyway... I guess I was wrong... :-(

Any idea on how we could reliably make sure the render process host are
destroyed on time?

[willchan no longer on Chromium, 2012-06-06 21:45:19]

I think this is a very good question. I know why you did your fix, because
getting the ownership/lifetime management correct is difficult. But the
refcounting bandaid often causes other issues. I got alerted to it due to
http://code.google.com/p/chromium/issues/detail?id=130772. Also, I have to
worry that since Profile is getting leaked due to leaked RPHs, we aren't
persisting some important Profile related state on shutdown. So we really
ought to simply solve the RPH leakage issue.

I think the problem is that RPH itself is RefCountedThreadSafe, which makes
reasoning about its lifetime management difficult. What we really want is
for RenderProcessHost to be a node in the dependency tree where the Profile
is the root. That way, when the Profile goes away, we can kill the RPH. My
suspicion is that there is no direct path from Profile to RPHs, so the the
lifetime is not well-defined, leading to RPH leaks / delayed destructions.
Establishing a direct path will require reading a lot of code to understand
the dependency chain, but is the "right" solution IMO.

On Wed, Jun 6, 2012 at 2:34 PM, <mad@chromium.org> wrote:

> On 2012/06/06 21:31:03, willchan wrote:
>
>> This change effectively makes Profile refcounted, which I think is
>>
> undesirable.
>
>> Can we fix the root problem instead, that renderer processes are not
>> getting
>> destroyed on time / getting leaked?
>>
>
> I couldn't find a way to do that reliably, so I decided to delay the
> profile
> destruction instead... Which I felt was safe, since profiles are usually
> destroyed pretty late in the game anyway... I guess I was wrong... :-(
>
> Any idea on how we could reliably make sure the render process host are
> destroyed on time?
>
>
https://chromiumcodereview.**appspot.com/9420036/<https://chromiumcodereview....
>

[Elliot Glaysher, 2012-06-06 21:48:15]

On 2012/06/06 21:45:19, willchan wrote:
> Also, I have to worry that since Profile is getting leaked due
> to leaked RPHs, we aren't persisting some important Profile
> related state on shutdown.

Given previous bugs involving Profile objects not being deleted, it's fairly
likely that we aren't writing stuff to disk in some cases.

[MAD, 2012-06-06 22:00:55]

The problem with releasing the RPH when the profile gets destroyed is that
the RPH has a pointer back to the profile, and it might try to use it as
the profile is being destroyed... So we would need some way to make sure
the RPH goes away, before we enter the Profile destructor...

On Wed, Jun 6, 2012 at 5:45 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> I think this is a very good question. I know why you did your fix, because
> getting the ownership/lifetime management correct is difficult. But the
> refcounting bandaid often causes other issues. I got alerted to it due to
> http://code.google.com/p/chromium/issues/detail?id=130772. Also, I have
> to worry that since Profile is getting leaked due to leaked RPHs, we aren't
> persisting some important Profile related state on shutdown. So we really
> ought to simply solve the RPH leakage issue.
>
> I think the problem is that RPH itself is RefCountedThreadSafe, which
> makes reasoning about its lifetime management difficult. What we really
> want is for RenderProcessHost to be a node in the dependency tree where the
> Profile is the root. That way, when the Profile goes away, we can kill the
> RPH. My suspicion is that there is no direct path from Profile to RPHs, so
> the the lifetime is not well-defined, leading to RPH leaks / delayed
> destructions. Establishing a direct path will require reading a lot of code
> to understand the dependency chain, but is the "right" solution IMO.
>
>
> On Wed, Jun 6, 2012 at 2:34 PM, <mad@chromium.org> wrote:
>
>> On 2012/06/06 21:31:03, willchan wrote:
>>
>>> This change effectively makes Profile refcounted, which I think is
>>>
>> undesirable.
>>
>>> Can we fix the root problem instead, that renderer processes are not
>>> getting
>>> destroyed on time / getting leaked?
>>>
>>
>> I couldn't find a way to do that reliably, so I decided to delay the
>> profile
>> destruction instead... Which I felt was safe, since profiles are usually
>> destroyed pretty late in the game anyway... I guess I was wrong... :-(
>>
>> Any idea on how we could reliably make sure the render process host are
>> destroyed on time?
>>
>>
https://chromiumcodereview.**appspot.com/9420036/<https://chromiumcodereview....
>>
>
>

[rpetterson, 2012-06-07 18:21:48]

I've actually been banging my head over a situation similar to what Elliot
pointed out:  http://code.google.com/p/chromium/issues/detail?id=127607

It might be related to Marc-Andre's change, but the revision numbers don't
quite match up. In any case, since it seems related to the profile
destruction/writing issue at hand, wanted to bring it to attention.

On Wed, Jun 6, 2012 at 3:00 PM, Marc-Andre Decoste <mad@chromium.org> wrote:

> The problem with releasing the RPH when the profile gets destroyed is that
> the RPH has a pointer back to the profile, and it might try to use it as
> the profile is being destroyed... So we would need some way to make sure
> the RPH goes away, before we enter the Profile destructor...
>
> On Wed, Jun 6, 2012 at 5:45 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
>
>> I think this is a very good question. I know why you did your fix,
>> because getting the ownership/lifetime management correct is difficult. But
>> the refcounting bandaid often causes other issues. I got alerted to it due
>> to http://code.google.com/p/chromium/issues/detail?id=130772. Also, I
>> have to worry that since Profile is getting leaked due to leaked RPHs, we
>> aren't persisting some important Profile related state on shutdown. So we
>> really ought to simply solve the RPH leakage issue.
>>
>> I think the problem is that RPH itself is RefCountedThreadSafe, which
>> makes reasoning about its lifetime management difficult. What we really
>> want is for RenderProcessHost to be a node in the dependency tree where the
>> Profile is the root. That way, when the Profile goes away, we can kill the
>> RPH. My suspicion is that there is no direct path from Profile to RPHs, so
>> the the lifetime is not well-defined, leading to RPH leaks / delayed
>> destructions. Establishing a direct path will require reading a lot of code
>> to understand the dependency chain, but is the "right" solution IMO.
>>
>>
>> On Wed, Jun 6, 2012 at 2:34 PM, <mad@chromium.org> wrote:
>>
>>> On 2012/06/06 21:31:03, willchan wrote:
>>>
>>>> This change effectively makes Profile refcounted, which I think is
>>>>
>>> undesirable.
>>>
>>>> Can we fix the root problem instead, that renderer processes are not
>>>> getting
>>>> destroyed on time / getting leaked?
>>>>
>>>
>>> I couldn't find a way to do that reliably, so I decided to delay the
>>> profile
>>> destruction instead... Which I felt was safe, since profiles are usually
>>> destroyed pretty late in the game anyway... I guess I was wrong... :-(
>>>
>>> Any idea on how we could reliably make sure the render process host are
>>> destroyed on time?
>>>
>>>
https://chromiumcodereview.**appspot.com/9420036/<https://chromiumcodereview....
>>>
>>
>>
>

[rpetterson, 2012-06-18 21:53:32]

The issue I was investigating turned out to be unrelated to this, just FYI.

On 2012/06/07 18:21:48, rpetterson wrote:
> I've actually been banging my head over a situation similar to what Elliot
> pointed out:  http://code.google.com/p/chromium/issues/detail?id=127607
> 
> It might be related to Marc-Andre's change, but the revision numbers don't
> quite match up. In any case, since it seems related to the profile
> destruction/writing issue at hand, wanted to bring it to attention.
> 
> On Wed, Jun 6, 2012 at 3:00 PM, Marc-Andre Decoste <mailto:mad@chromium.org>
wrote:
> 
> > The problem with releasing the RPH when the profile gets destroyed is that
> > the RPH has a pointer back to the profile, and it might try to use it as
> > the profile is being destroyed... So we would need some way to make sure
> > the RPH goes away, before we enter the Profile destructor...
> >
> > On Wed, Jun 6, 2012 at 5:45 PM, William Chan (陈智昌)
> <willchan@chromium.org>wrote:
> >
> >> I think this is a very good question. I know why you did your fix,
> >> because getting the ownership/lifetime management correct is difficult. But
> >> the refcounting bandaid often causes other issues. I got alerted to it due
> >> to http://code.google.com/p/chromium/issues/detail?id=130772. Also, I
> >> have to worry that since Profile is getting leaked due to leaked RPHs, we
> >> aren't persisting some important Profile related state on shutdown. So we
> >> really ought to simply solve the RPH leakage issue.
> >>
> >> I think the problem is that RPH itself is RefCountedThreadSafe, which
> >> makes reasoning about its lifetime management difficult. What we really
> >> want is for RenderProcessHost to be a node in the dependency tree where the
> >> Profile is the root. That way, when the Profile goes away, we can kill the
> >> RPH. My suspicion is that there is no direct path from Profile to RPHs, so
> >> the the lifetime is not well-defined, leading to RPH leaks / delayed
> >> destructions. Establishing a direct path will require reading a lot of code
> >> to understand the dependency chain, but is the "right" solution IMO.
> >>
> >>
> >> On Wed, Jun 6, 2012 at 2:34 PM, <mailto:mad@chromium.org> wrote:
> >>
> >>> On 2012/06/06 21:31:03, willchan wrote:
> >>>
> >>>> This change effectively makes Profile refcounted, which I think is
> >>>>
> >>> undesirable.
> >>>
> >>>> Can we fix the root problem instead, that renderer processes are not
> >>>> getting
> >>>> destroyed on time / getting leaked?
> >>>>
> >>>
> >>> I couldn't find a way to do that reliably, so I decided to delay the
> >>> profile
> >>> destruction instead... Which I felt was safe, since profiles are usually
> >>> destroyed pretty late in the game anyway... I guess I was wrong... :-(
> >>>
> >>> Any idea on how we could reliably make sure the render process host are
> >>> destroyed on time?
> >>>
> >>>
>
https://chromiumcodereview.**appspot.com/9420036/%3Chttps://chromiumcoderevie...>
> >>>
> >>
> >>
> >