Don't overload the meaning of the RENDERER_PROCESS_TERMINATED notification, i...

Don't overload the meaning of the RENDERER_PROCESS_TERMINATED notification, instead create a new one for crashing.

The old way of using notifications was incorrect since a RenderProcessHost might have sent only one notification even though a new renderer might have been created after a crash.

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

[Charlie Reis, 2009-04-11 16:10:44]

Hmm.  I'll admit that I'm less familiar with the code you're changing here, but
separating out TERMINATED and CRASHED does seem reasonable to me.

However, this doesn't fix the problem with SiteInstances from bug 8085.  It
seems like the RenderProcessHost associated with a given SiteInstance should
never change, so the Observer that was added to SiteInstance (which resets its
process_ to null) probably shouldn't be there.  Are you intending to do that in
a separate changelist?

[jam, 2009-04-13 18:11:35]

On 2009/04/11 16:10:44, creis wrote:
> Hmm.  I'll admit that I'm less familiar with the code you're changing here,
but
> separating out TERMINATED and CRASHED does seem reasonable to me.
> 
> However, this doesn't fix the problem with SiteInstances from bug 8085.  It
> seems like the RenderProcessHost associated with a given SiteInstance should
> never change, so the Observer that was added to SiteInstance (which resets its
> process_ to null) probably shouldn't be there.  Are you intending to do that
in

I think you're misreading the code.  This is the same behavior as before my
original change.  The RenderProcessHost stops being associated with SiteInstance
when it's destructed.  Before this was simply done via the host_id not pointing
to a valid RenderProcessHost, hence a new RPH was created.  Now we just set the
RPH pointer to NULL when it's destructed.  I had verified that the issue in 8085
was fixed from this change.

> a separate changelist?

[Charlie Reis, 2009-04-13 18:32:50]

On 2009/04/13 18:11:35, John Abd-El-Malek wrote:
> On 2009/04/11 16:10:44, creis wrote:
> > Hmm.  I'll admit that I'm less familiar with the code you're changing here,
> but
> > separating out TERMINATED and CRASHED does seem reasonable to me.
> > 
> > However, this doesn't fix the problem with SiteInstances from bug 8085.  It
> > seems like the RenderProcessHost associated with a given SiteInstance should
> > never change, so the Observer that was added to SiteInstance (which resets
its
> > process_ to null) probably shouldn't be there.  Are you intending to do that
> in
> 
> I think you're misreading the code.  This is the same behavior as before my
> original change.  The RenderProcessHost stops being associated with
SiteInstance
> when it's destructed.  Before this was simply done via the host_id not
pointing
> to a valid RenderProcessHost, hence a new RPH was created.  Now we just set
the
> RPH pointer to NULL when it's destructed.  I had verified that the issue in
8085
> was fixed from this change.

Yes, I must have misunderstood it.  I was under the impression that "terminated
vs crashed" could have meant "killed via Task Manager vs hit a bug in the
rendering engine."  But you're saying that "terminated" means the process was
cleanly ended by Chrome because all of its tabs were closed?  In that case, it
does seem like the SiteInstance will be deleted as well, so it's ok to set its
process_ to null.

Is there a reason SiteInstance still needs the explicit observer to set process_
to null, though, and that just relying on it being deleted isn't enough?  (Do we
ever expect the SiteInstance to outlast its RenderProcessHost?)

At any rate, the code looks right to me, but could you add more to the comment
where RENDERER_PROCESS_TERMINATED is declared to explain the intention?  A
similar comment on SiteInstance's process_ field might also help-- something
like "the value of this field should never change until the corresponding
RenderProcessHost is cleanly terminated, just before this SiteInstance is
deleted."  (Assuming that's true.)

Thanks!
Charlie

[Charlie Reis, 2009-04-13 19:44:14]

>> Yes, I must have misunderstood it.  I was under the impression that
>> "terminated vs crashed" could have meant "killed via Task Manager vs hit
>> a bug in the rendering engine."  But you're saying that "terminated"
>> means the process was cleanly ended by Chrome because all of its tabs
>> were closed?
>
>
> Yep that's correct.  I think the problem that caused this bug (and other
> leaks that I found) was overloading "terminated" to mean crashed as well,
> even though the associated RenderProcessHost was still around.

Gotcha.

>>  In that case, it does seem like the SiteInstance will be
>> deleted as well, so it's ok to set its process_ to null.
>>
>> Is there a reason SiteInstance still needs the explicit observer to set
>> process_ to null, though, and that just relying on it being deleted
>> isn't enough?  (Do we ever expect the SiteInstance to outlast its
>> RenderProcessHost?)
>
>
> Yep SiteInstance outlives RPH sometimes, since a pointer to it is cached by
> NavigationEntry.

Ah, right.  That concerned me for a second, but I think it works out.  Consider
navigating two tabs in the same SiteInstance to different sites.  The process
will go away, and the SiteInstance's process_ field will be set to null.  If the
user then goes back in both tabs, we want them to end up in the same process
again.  It works, though, because they share the same SiteInstance object, so it
will only create a new RenderProcessHost once.  Seems ok.

When verifying this, I noticed that the SiteInstance::Observe code gets called
for *every* tab any time a process goes away.  I don't suppose there's a way to
make that more efficient?  (e.g., Giving a RenderProcessHost a list of the tabs
it needs to notify when it goes away?)  Could be n^2 updates if the user is
closing all windows.  Anyway, that's a different issue, if you think it's worth
following up on.

LGTM.