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

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 309 matching lines @@
   for (size_t i = 0; i < g_created_callbacks.Get().size(); i++)
     g_created_callbacks.Get().at(i).Run(this);
   frame_tree_.SetFrameRemoveListener(
       base::Bind(&WebContentsImpl::OnFrameRemoved,
                  base::Unretained(this)));
 }
 
 WebContentsImpl::~WebContentsImpl() {
   is_being_destroyed_ = true;
 
+  // Delete all RFH pending shutdown, which will lead the corresponding RVH to
+  // shutdown and be deleted as well.
+  GetRenderManager()->ClearAllPendingShutdownRFH();

[Charlie Reis, 2014/02/26 17:54:30]

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.

[jam, 2014/02/26 22:48:47]

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:31]

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]

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:27]

Ah, thanks for taking care of that here.
Agreed.  I don't have any evidence that's a problem.

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

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.

+
   ClearAllPowerSaveBlockers();
 
   for (std::set<RenderWidgetHostImpl*>::iterator iter =
            created_widgets_.begin(); iter != created_widgets_.end(); ++iter) {
     (*iter)->DetachDelegate();
   }
   created_widgets_.clear();
 
   // Clear out any JavaScript state.
   if (dialog_manager_)
@@ skipping 2432 matching lines @@
 
 void WebContentsImpl::RemoveBrowserPluginEmbedder() {
   if (browser_plugin_embedder_)
     browser_plugin_embedder_.reset();
 }
 
 void WebContentsImpl::RenderViewCreated(RenderViewHost* render_view_host) {
   // Don't send notifications if we are just creating a swapped-out RVH for
   // the opener chain.  These won't be used for view-source or WebUI, so it's
   // ok to return early.
-  if (static_cast<RenderViewHostImpl*>(render_view_host)->is_swapped_out())
+  if (static_cast<RenderViewHostImpl*>(render_view_host)->IsSwappedOut())
     return;
 
   if (delegate_)
     view_->SetOverscrollControllerEnabled(delegate_->CanOverscrollContent());
 
   NotificationService::current()->Notify(
       NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED,
       Source<WebContents>(this),
       Details<RenderViewHost>(render_view_host));
 
@@ skipping 261 matching lines @@
 
 void WebContentsImpl::RequestOpenURL(RenderViewHost* rvh,
                                      const GURL& url,
                                      const Referrer& referrer,
                                      WindowOpenDisposition disposition,
                                      int64 source_frame_id,
                                      bool should_replace_current_entry,
                                      bool user_gesture) {
   // If this came from a swapped out RenderViewHost, we only allow the request
   // if we are still in the same BrowsingInstance.
-  if (static_cast<RenderViewHostImpl*>(rvh)->is_swapped_out() &&
+  if (static_cast<RenderViewHostImpl*>(rvh)->IsSwappedOut() &&
       !rvh->GetSiteInstance()->IsRelatedSiteInstance(GetSiteInstance())) {
     return;
   }
 
   // Delegate to RequestTransferURL because this is just the generic
   // case where |old_request_id| is empty.
   // TODO(creis): Pass the redirect_chain into this method to support client
   // redirects.  http://crbug.com/311721.
   std::vector<GURL> redirect_chain;
   RequestTransferURL(url, redirect_chain, referrer, PAGE_TRANSITION_LINK,
@@ skipping 150 matching lines @@
     const base::string16& message,
     const base::string16& default_prompt,
     const GURL& frame_url,
     JavaScriptMessageType javascript_message_type,
     IPC::Message* reply_msg,
     bool* did_suppress_message) {
   // Suppress JavaScript dialogs when requested. Also suppress messages when
   // showing an interstitial as it's shown over the previous page and we don't
   // want the hidden page's dialogs to interfere with the interstitial.
   bool suppress_this_message =
-      static_cast<RenderViewHostImpl*>(rvh)->is_swapped_out() ||
+      static_cast<RenderViewHostImpl*>(rvh)->IsSwappedOut() ||
       ShowingInterstitialPage() ||
       !delegate_ ||
       delegate_->ShouldSuppressDialogs() ||
       !delegate_->GetJavaScriptDialogManager();
 
   if (!suppress_this_message) {
     std::string accept_lang = GetContentClient()->browser()->
       GetAcceptLangs(GetBrowserContext());
     dialog_manager_ = delegate_->GetJavaScriptDialogManager();
     dialog_manager_->RunJavaScriptDialog(
@@ skipping 24 matching lines @@
 
 void WebContentsImpl::RunBeforeUnloadConfirm(RenderViewHost* rvh,
                                              const base::string16& message,
                                              bool is_reload,
                                              IPC::Message* reply_msg) {
   RenderViewHostImpl* rvhi = static_cast<RenderViewHostImpl*>(rvh);
   if (delegate_)
     delegate_->WillRunBeforeUnloadConfirm();
 
   bool suppress_this_message =
-      rvhi->is_swapped_out() ||
+      rvhi->rvh_state() != RenderViewHostImpl::STATE_DEFAULT ||
       !delegate_ ||
       delegate_->ShouldSuppressDialogs() ||
       !delegate_->GetJavaScriptDialogManager();
   if (suppress_this_message) {
     // The reply must be sent to the RVH that sent the request.
     rvhi->JavaScriptDialogClosed(reply_msg, true, base::string16());
     return;
   }
 
   is_showing_before_unload_dialog_ = true;
@@ skipping 371 matching lines @@
 
 void WebContentsImpl::OnPreferredSizeChanged(const gfx::Size& old_size) {
   if (!delegate_)
     return;
   const gfx::Size new_size = GetPreferredSize();
   if (new_size != old_size)
     delegate_->UpdatePreferredSize(this, new_size);
 }
 
 }  // namespace content