Move PROFILE_DESTROYED notification to ProfileDestroyer and observe it in ExtensionProcessManager.

Move PROFILE_DESTROYED notification to ProfileDestroyer and observe it in ExtensionProcessManager.

1. EPM was not observing the correct notification to shut down its ExtensionHosts.
2. PROFILE_DESTROYED has to be sent early enough that the EPM can shut down ExtensionHosts before the ProfileDestroyer checks that all renderers are gone.

BUG=138843
TEST=In Debug build, have a split mode incognito extension installed. Open and close an incognito window; no crash. Close the regular profile window; no crash.
TBR=sky@chromium.org

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

[Yoyo Zhou, 2012-07-25 18:13:00]

mad, rlp: please review for profiles
mpcomplete: please review for extensions
willchan: FYI since you seem to have commented on ProfileDestroyer reviews
before

http://codereview.chromium.org/10824020/diff/1/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_destroyer.cc (right):

http://codereview.chromium.org/10824020/diff/1/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_destroyer.cc:29:
content::NotificationService::current()->Notify(
I wanted to put this in the ProfileDestroyer initializer, but that would be too
late for the DCHECK at 45 for normal profiles -- extension background pages
would still be hanging around.

http://codereview.chromium.org/10824020/diff/1/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_destroyer.cc:136: registrar_.Remove(this,
content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
It turns out the current implementation doesn't reach this point, but a previous
attempt did, and this appeared to be missing.

[Matt Perry, 2012-07-25 19:32:08]

lgtm

http://codereview.chromium.org/10824020/diff/1/chrome/browser/profiles/profil...
File chrome/browser/profiles/profile_destroyer.cc (right):

http://codereview.chromium.org/10824020/diff/1/chrome/browser/profiles/profil...
chrome/browser/profiles/profile_destroyer.cc:45: DCHECK(hosts.empty() ||
profile->IsOffTheRecord());
you should change the DCHECK now that it works for OTR

[rpetterson, 2012-07-25 20:44:05]

Perhaps MAD can shed some light on this, but I don't think profile destroyer is
the only way which profiles are deleted (if that's not the case, then this
should be okay). For example, when Chrome shuts down, the profile_manager is
destroyed which results in the destruction of profiles without the use of the
profile_destroyer and thus no notification in this case. I don't know enough
about the services relying on the PROFILE_DESTROYED notification to know if this
is a problem for them.

On 2012/07/25 19:32:08, Matt Perry wrote:
> lgtm
> 
>
http://codereview.chromium.org/10824020/diff/1/chrome/browser/profiles/profil...
> File chrome/browser/profiles/profile_destroyer.cc (right):
> 
>
http://codereview.chromium.org/10824020/diff/1/chrome/browser/profiles/profil...
> chrome/browser/profiles/profile_destroyer.cc:45: DCHECK(hosts.empty() ||
> profile->IsOffTheRecord());
> you should change the DCHECK now that it works for OTR

[willchan no longer on Chromium, 2012-07-25 22:07:44]

I haven't read this CL yet, but ProfileManager does *not* actually destroy
the profile anymore. It releases ownership of the Profile* to the
ProfileDestroyer. I *think* this means Profiles are always destroyed via
the ProfileDestroyer, although I'm not sure there's an edge case I don't
know about.

[Yoyo Zhou, 2012-07-25 22:12:15]

On Wed, Jul 25, 2012 at 3:07 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> I haven't read this CL yet, but ProfileManager does *not* actually destroy
> the profile anymore. It releases ownership of the Profile* to the
> ProfileDestroyer. I *think* this means Profiles are always destroyed via the
> ProfileDestroyer, although I'm not sure there's an edge case I don't know
> about.

Does this happen in unit tests too?

[willchan no longer on Chromium, 2012-07-25 23:13:05]

The main weird profile destruction stuff we have in unit tests is in the
ProfileManager tests. I remember looking at that test and running away
crying. If Profiles are destroyed via the ProfileDestroyer there (and I
think they are), then I would not imagine a unit test that would delete
Profiles directly. That said, I wouldn't guarantee that doesn't happen,
Chromium has a gazillion tests after all.


On Wed, Jul 25, 2012 at 3:12 PM, Yoyo Zhou <yoz@chromium.org> wrote:

> On Wed, Jul 25, 2012 at 3:07 PM, William Chan (陈智昌)
> <willchan@chromium.org> wrote:
> > I haven't read this CL yet, but ProfileManager does *not* actually
> destroy
> > the profile anymore. It releases ownership of the Profile* to the
> > ProfileDestroyer. I *think* this means Profiles are always destroyed via
> the
> > ProfileDestroyer, although I'm not sure there's an edge case I don't know
> > about.
>
> Does this happen in unit tests too?
>

[Yoyo Zhou, 2012-07-26 01:01:43]

On 2012/07/25 23:13:05, willchan wrote:
> The main weird profile destruction stuff we have in unit tests is in the
> ProfileManager tests. I remember looking at that test and running away
> crying. If Profiles are destroyed via the ProfileDestroyer there (and I
> think they are), then I would not imagine a unit test that would delete
> Profiles directly. That said, I wouldn't guarantee that doesn't happen,
> Chromium has a gazillion tests after all.

Unfortunately, the outcome of my discussion with Will was finding that we have
quite a few tests that keep a scoped_ptr<Profile> around, and don't use
ProfileManager.

http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/42591

Fixing all of these is going to be arduous. My thought is to keep some
additional state on each profile so that it sends NOTIFICATION_PROFILE_DESTROYED
only once, and attempt to send it in both ProfileDestroyer and the Profile
destructors. Does this sound reasonable?

[rpetterson, 2012-07-26 02:46:59]

Yes, that sounds reasonable.

On 2012/07/26 01:01:43, Yoyo Zhou wrote:
> On 2012/07/25 23:13:05, willchan wrote:
> > The main weird profile destruction stuff we have in unit tests is in the
> > ProfileManager tests. I remember looking at that test and running away
> > crying. If Profiles are destroyed via the ProfileDestroyer there (and I
> > think they are), then I would not imagine a unit test that would delete
> > Profiles directly. That said, I wouldn't guarantee that doesn't happen,
> > Chromium has a gazillion tests after all.
> 
> Unfortunately, the outcome of my discussion with Will was finding that we have
> quite a few tests that keep a scoped_ptr<Profile> around, and don't use
> ProfileManager.
> 
> http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/42591
> 
> Fixing all of these is going to be arduous. My thought is to keep some
> additional state on each profile so that it sends
NOTIFICATION_PROFILE_DESTROYED
> only once, and attempt to send it in both ProfileDestroyer and the Profile
> destructors. Does this sound reasonable?

[Yoyo Zhou, 2012-07-26 17:35:33]

I've added MaybeSendDestroyedNotification to Profile. Please take another look.

[rpetterson, 2012-07-26 18:04:18]

LGTM

[Yoyo Zhou, 2012-07-26 23:32:29]

I had to fix up the listeneing in the EPM so that the incognito one also
observes the normal profile being destroyed (which happens in a few tests).

[Yoyo Zhou, 2012-07-27 00:17:00]

sky: can you review the trivial change to chrome/test?

[Yoyo Zhou, 2012-07-27 00:36:14]

moving sky to TBR

[Matt Perry, 2012-07-27 00:37:09]

lgtm

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

http://codereview.chromium.org/10824020/diff/13003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_destroyer.cc:37: }
What if we just moved this test and the new notification to the Profile
destructor instead? Would that fix this bug?

[Yoyo Zhou, 2012-07-27 00:45:13]

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

http://codereview.chromium.org/10824020/diff/13003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_destroyer.cc:37: }
On 2012/07/27 00:37:09, Matt Perry wrote:
> What if we just moved this test and the new notification to the Profile
> destructor instead? Would that fix this bug?

Did you mean the test right below this line? (Oops, I forgot to change that per
your earlier comment.)

We'd then have to move/copy GetHostsForProfile into the Profile destructor...
also, I think that might defeat the purpose of ProfileDestroyer, which I think
is to not actually start destroying the profiles until their renderers are gone.

[Matt Perry, 2012-07-27 00:51:24]

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

http://codereview.chromium.org/10824020/diff/13003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_destroyer.cc:37: }
On 2012/07/27 00:45:13, Yoyo Zhou wrote:
> On 2012/07/27 00:37:09, Matt Perry wrote:
> > What if we just moved this test and the new notification to the Profile
> > destructor instead? Would that fix this bug?
> 
> Did you mean the test right below this line? (Oops, I forgot to change that
per
> your earlier comment.)
> 
> We'd then have to move/copy GetHostsForProfile into the Profile destructor...
> also, I think that might defeat the purpose of ProfileDestroyer, which I think
> is to not actually start destroying the profiles until their renderers are
gone.

Ah, nevermind then.