Revert "Revert 249676 "Have the unload event execute in background on cr...""

Revert "Revert 249676 "Have the unload event execute in background on cr...""

This fixes a bug in the original CL which may have caused a crash in
ProfileDestroyer.

BUG=343002, 323528, 342361
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254007

[clamy, 2014-02-26 14:11:49]

@creis, jam: PTAL. The reland now deletes all RFH pending shutdown in the
destructor of WebContentsImpl. This in turn ensures that Shutdown will be on the
corresponding RVH, and that they will be deleted as well. The
WebContentsObservers are now properly notified of the RVH destruction, since
this happens before the list of observers is cleared in WebContentsImpl. This
fixes the memory leak of crbug.com/342361.

[jam, 2014-02-26 17:46:21]

lgtm

[Charlie Reis, 2014-02-26 17:54:29]

Great.  I assume patch set 1 is the original CL.  The diff to patch set 2 LGTM,
with one question below.

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:332:
GetRenderManager()->ClearAllPendingShutdownRFH();
If this deletes the RVHs, does it eliminate the need for the RenderViewDeleted
observer calls on the pending and current RVH below?  That would be fantastic.

[Charlie Reis, 2014-02-26 17:59:18]

Also, glad to see you started this patch with a revert of my revert, rather than
with the original CL.  That means it includes the things I had to change to fix
merge conflicts (e.g., the is_waiting_for_unload_ack_ change in
RenderFrameHostImpl::OnNavigate, which moved after your CL landed).

[jam, 2014-02-26 22:48:46]

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:332:
GetRenderManager()->ClearAllPendingShutdownRFH();
On 2014/02/26 17:54:30, Charlie Reis wrote:
> If this deletes the RVHs, does it eliminate the need for the RenderViewDeleted
> observer calls on the pending and current RVH below?  That would be fantastic.

I had assumed, perhaps incorrectly, that the list of pending shutdown RVHs
doesn't cover pending/current RVH. If it did, and those two got deleted at this
point, then WebContentsObservers would crash when calling
web_contents()->GetRenderViewHost()->Foo() in their destructor

[Charlie Reis, 2014-02-26 23:30:30]

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:332:
GetRenderManager()->ClearAllPendingShutdownRFH();
On 2014/02/26 22:48:47, jam wrote:
> On 2014/02/26 17:54:30, Charlie Reis wrote:
> > If this deletes the RVHs, does it eliminate the need for the
RenderViewDeleted
> > observer calls on the pending and current RVH below?  That would be
fantastic.
> 
> I had assumed, perhaps incorrectly, that the list of pending shutdown RVHs
> doesn't cover pending/current RVH. If it did, and those two got deleted at
this
> point, then WebContentsObservers would crash when calling
> web_contents()->GetRenderViewHost()->Foo() in their destructor

Ah, of course.  ClearAllPendingShutdownRFH() doesn't cover all RVHs, and we'd
hit the unfortunate GetRenderViewHost() crash if it did.  (It's kind of a shame
that we allow observers to call GetRenderViewHost() from RenderViewDeleted.)

After this, I think we're still missing some RenderViewDeleted calls.  We now
cover current and pending (below), pending-shutdown (here), but not those in
swapped_out_hosts_.  I'm less certain that we need RenderViewDeleted calls for
them, because we've already called RenderViewHostChanged for them (signalling
that they're swapped out).

Maybe I should step back and allow this to proceed, and we can separately look
into other issues.  (For example, this change only works for the top-level
RenderFrameHostManager, even though one exists for each frame.  For now, that
only matters for --site-per-process, since we never swap processes in the
subframes without that flag.)

[clamy, 2014-02-27 17:38:06]

Thanks for your reviews! PTAL at the new patchset, this should take care of all
RFH pending shutdown.

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:332:
GetRenderManager()->ClearAllPendingShutdownRFH();
On 2014/02/26 23:30:31, Charlie Reis wrote:
> On 2014/02/26 22:48:47, jam wrote:
> > On 2014/02/26 17:54:30, Charlie Reis wrote:
> > > If this deletes the RVHs, does it eliminate the need for the
> RenderViewDeleted
> > > observer calls on the pending and current RVH below?  That would be
> fantastic.
> > 
> > I had assumed, perhaps incorrectly, that the list of pending shutdown RVHs
> > doesn't cover pending/current RVH. If it did, and those two got deleted at
> this
> > point, then WebContentsObservers would crash when calling
> > web_contents()->GetRenderViewHost()->Foo() in their destructor
> 
> Ah, of course.  ClearAllPendingShutdownRFH() doesn't cover all RVHs, and we'd
> hit the unfortunate GetRenderViewHost() crash if it did.  (It's kind of a
shame
> that we allow observers to call GetRenderViewHost() from RenderViewDeleted.)
> 
> After this, I think we're still missing some RenderViewDeleted calls.  We now
> cover current and pending (below), pending-shutdown (here), but not those in
> swapped_out_hosts_.  I'm less certain that we need RenderViewDeleted calls for
> them, because we've already called RenderViewHostChanged for them (signalling
> that they're swapped out).
> 
> Maybe I should step back and allow this to proceed, and we can separately look
> into other issues.  (For example, this change only works for the top-level
> RenderFrameHostManager, even though one exists for each frame.  For now, that
> only matters for --site-per-process, since we never swap processes in the
> subframes without that flag.)

Yes this only covers the case of RFH pending shutdown. I have uploaded a new
patchset where we iterate over all nodes in the frame tree, so all RFH pending
shutdown should now be cleared.

For the swapped out RFH, they were not taken into account before the patch, so I
assume nothing needs to be done for them.

[Charlie Reis, 2014-02-27 20:09:26]

Thanks.  The diff from patch set 1 to patch set 4 LGTM.

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:332:
GetRenderManager()->ClearAllPendingShutdownRFH();
On 2014/02/27 17:38:06, clamy wrote:
> On 2014/02/26 23:30:31, Charlie Reis wrote:
> > On 2014/02/26 22:48:47, jam wrote:
> > > On 2014/02/26 17:54:30, Charlie Reis wrote:
> > > > If this deletes the RVHs, does it eliminate the need for the
> > RenderViewDeleted
> > > > observer calls on the pending and current RVH below?  That would be
> > fantastic.
> > > 
> > > I had assumed, perhaps incorrectly, that the list of pending shutdown RVHs
> > > doesn't cover pending/current RVH. If it did, and those two got deleted at
> > this
> > > point, then WebContentsObservers would crash when calling
> > > web_contents()->GetRenderViewHost()->Foo() in their destructor
> > 
> > Ah, of course.  ClearAllPendingShutdownRFH() doesn't cover all RVHs, and
we'd
> > hit the unfortunate GetRenderViewHost() crash if it did.  (It's kind of a
> shame
> > that we allow observers to call GetRenderViewHost() from RenderViewDeleted.)
> > 
> > After this, I think we're still missing some RenderViewDeleted calls.  We
now
> > cover current and pending (below), pending-shutdown (here), but not those in
> > swapped_out_hosts_.  I'm less certain that we need RenderViewDeleted calls
for
> > them, because we've already called RenderViewHostChanged for them
(signalling
> > that they're swapped out).
> > 
> > Maybe I should step back and allow this to proceed, and we can separately
look
> > into other issues.  (For example, this change only works for the top-level
> > RenderFrameHostManager, even though one exists for each frame.  For now,
that
> > only matters for --site-per-process, since we never swap processes in the
> > subframes without that flag.)
> 
> Yes this only covers the case of RFH pending shutdown. I have uploaded a new
> patchset where we iterate over all nodes in the frame tree, so all RFH pending
> shutdown should now be cleared.

Ah, thanks for taking care of that here.

> For the swapped out RFH, they were not taken into account before the patch, so
I
> assume nothing needs to be done for them.

Agreed.  I don't have any evidence that's a problem.

[Charlie Reis, 2014-02-27 21:24:51]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-02-27 21:25:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/180993003/80001

[jam, 2014-02-27 21:33:10]

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/180993003/diff/60001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:332:
GetRenderManager()->ClearAllPendingShutdownRFH();
On 2014/02/27 20:09:27, Charlie Reis wrote:
> On 2014/02/27 17:38:06, clamy wrote:
> > On 2014/02/26 23:30:31, Charlie Reis wrote:
> > > On 2014/02/26 22:48:47, jam wrote:
> > > > On 2014/02/26 17:54:30, Charlie Reis wrote:
> > > > > If this deletes the RVHs, does it eliminate the need for the
> > > RenderViewDeleted
> > > > > observer calls on the pending and current RVH below?  That would be
> > > fantastic.
> > > > 
> > > > I had assumed, perhaps incorrectly, that the list of pending shutdown
RVHs
> > > > doesn't cover pending/current RVH. If it did, and those two got deleted
at
> > > this
> > > > point, then WebContentsObservers would crash when calling
> > > > web_contents()->GetRenderViewHost()->Foo() in their destructor
> > > 
> > > Ah, of course.  ClearAllPendingShutdownRFH() doesn't cover all RVHs, and
> we'd
> > > hit the unfortunate GetRenderViewHost() crash if it did.  (It's kind of a
> > shame
> > > that we allow observers to call GetRenderViewHost() from
RenderViewDeleted.)
> > > 
> > > After this, I think we're still missing some RenderViewDeleted calls.  We
> now
> > > cover current and pending (below), pending-shutdown (here), but not those
in
> > > swapped_out_hosts_.  I'm less certain that we need RenderViewDeleted calls
> for
> > > them, because we've already called RenderViewHostChanged for them
> (signalling
> > > that they're swapped out).
> > > 
> > > Maybe I should step back and allow this to proceed, and we can separately
> look
> > > into other issues.  (For example, this change only works for the top-level
> > > RenderFrameHostManager, even though one exists for each frame.  For now,
> that
> > > only matters for --site-per-process, since we never swap processes in the
> > > subframes without that flag.)
> > 
> > Yes this only covers the case of RFH pending shutdown. I have uploaded a new
> > patchset where we iterate over all nodes in the frame tree, so all RFH
pending
> > shutdown should now be cleared.
> 
> Ah, thanks for taking care of that here.
> 
> > For the swapped out RFH, they were not taken into account before the patch,
so
> I
> > assume nothing needs to be done for them.
> 
> Agreed.  I don't have any evidence that's a problem.

I think for swapped out RFH, we should call RenderFrameDeleted for them as well,
if we had called RenderFrameCreated for them. It's much easier for
webcontentsobservers to handle leaks and use-after-frees that way, instead of
having to track objects' lifetimes using multiple methods. note i completely
agree that this is beyond this patch, which didn't change that.

[commit-bot: I haz the power, 2014-02-28 01:03:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/180993003/80001

[commit-bot: I haz the power, 2014-02-28 01:52:28]

Change committed as 254007