Have the unload event execute in background on cross-site navigations

content/browser/renderer_host/render_view_host_impl.h

Index: content/browser/renderer_host/render_view_host_impl.h
diff --git a/content/browser/renderer_host/render_view_host_impl.h b/content/browser/renderer_host/render_view_host_impl.h
index 52f36ea55d0886212f23bfd47401ced3a7540508..c171f0faac8da0e0a73c6a5f7085aba918a55898 100644
--- a/content/browser/renderer_host/render_view_host_impl.h
+++ b/content/browser/renderer_host/render_view_host_impl.h
@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ -64,6 +65,7 @@ class RenderWidgetHostDelegate;
 class SessionStorageNamespace;
 class SessionStorageNamespaceImpl;
 class TestRenderViewHost;
+class TimeoutMonitor;
 struct ContextMenuParams;
 struct FileChooserParams;
 struct Referrer;
@@ -99,6 +101,19 @@ class CONTENT_EXPORT RenderViewHostImpl
     : public RenderViewHost,
       public RenderWidgetHostImpl {
  public:
+  enum RenderViewHostImplState {

[nasko, 2014/01/23 23:34:58]

Having a good comment for each of those states will be awesome. Since this is
hard to understand area, anything we can do to help our future selves will be
great!

[clamy, 2014/01/24 17:01:54]

Done.

+    RVH_STATE_LIVE = 0,

[nasko, 2014/01/23 23:34:58]

nit: Drop the RVH_ prefix, as this is already accessed through
RenderViewHostImpl:: prefix.

[clamy, 2014/01/24 17:01:54]

Done.

+    RVH_STATE_WAITING_FOR_UNLOAD_ACK,
+    RVH_STATE_WAITING_FOR_COMMIT,
+    RVH_STATE_WAITING_FOR_CLOSE,
+    RVH_STATE_PENDING_SWAP_OUT,
+    RVH_STATE_PENDING_SHUTDOWN,
+    RVH_STATE_SWAPPED_OUT,
+  };
+  // Helper function to determine whether the RVH state should contribute to the
+  // number of active views of a SiteInstance or not.
+  static bool IsRVHStateActive(RenderViewHostImplState rvh_state);
+
   // Convenience function, just like RenderViewHost::FromID.
   static RenderViewHostImpl* FromID(int render_process_id, int render_view_id);
 
@@ -282,9 +297,8 @@ class CONTENT_EXPORT RenderViewHostImpl
     return has_accessed_initial_document_;
   }
 
-  // Whether this RenderViewHost has been swapped out to be displayed by a
-  // different process.
-  bool is_swapped_out() const { return is_swapped_out_; }

[nasko, 2014/01/23 23:34:58]

nit: This might be useful method to keep around as an easy way to check for
swapped out state.

[clamy, 2014/01/24 17:01:54]

Do we want it to return true only for STATE_SWAPPED_OUT, or include
STATE_PENDING_SWAP_OUT as well? When the RVH is in STATE_PENDING_SWAP_OUT, it is
stored in the map of swapped_out_hosts in the RenderFrameHostManager (though it
does not filter all IPCs as in STATE_SWAPPED_OUT).

[nasko, 2014/01/27 19:09:51]

Based on the replaced usages, it looks that returning only true for
STATE_SWAPPED_OUT is the best thing. Also, it will match the name and be
conceptually simple.

[clamy, 2014/01/28 12:27:21]

Done.

+  // The current state of this RVH.
+  RenderViewHostImplState rvh_state() const { return rvh_state_; }
 
   // Called on the pending RenderViewHost when the network response is ready to
   // commit.  We should ensure that the old RenderViewHost runs its unload
@@ -317,11 +331,15 @@ class CONTENT_EXPORT RenderViewHostImpl
   // out.
   void OnSwappedOut(bool timed_out);
 
-  // Called to notify the renderer that it has been visibly swapped out and
-  // replaced by another RenderViewHost, after an earlier call to SwapOut.
-  // It is now safe for the process to exit if there are no other active
-  // RenderViews.
-  void WasSwappedOut();
+  // Called when the RenderFrameHostManager has swapped in a new
+  // RenderFrameHost. Should |this| RVH switch to the pending shutdown state,
+  // |pending_delete_on_swap_out| will be executed upon reception of the
+  // SwapOutACK, or when the unload timer times out.
+  void WasSwappedOut(const base::Closure& pending_delete_on_swap_out);
+
+  // Set |this| as pending shutdown. |on_swap_out| will be called
+  // when the swap out ack is received, or when the unload timer times out.
+  void SetPendingShutdown(const base::Closure& on_swap_out);
 
   // Close the page ignoring whether it has unload events registers.
   // This is called after the beforeunload and unload events have fired
@@ -455,9 +473,8 @@ class CONTENT_EXPORT RenderViewHostImpl
     return is_waiting_for_beforeunload_ack_;
   }
 
-  bool is_waiting_for_unload_ack() {
-    return is_waiting_for_unload_ack_;
-  }
+  // Whether the RVH is waiting for the unload ack from the renderer.
+  bool IsWaitingForUnloadACK() const;
 
   // Returns whether the given URL is allowed to commit in the current process.
   // This is a more conservative check than RenderProcessHost::FilterURL, since
@@ -481,6 +498,18 @@ class CONTENT_EXPORT RenderViewHostImpl
                                          bool main_frame,
                                          const GURL& url);
 
+  // Increases the refcounting on this RVH. This is done by the FrameTree on
+  // creation of a RenderFrameHost.
+  void increment_ref_count() { ++ref_count_; }
+
+  // Decreases the refcounting on this RVH. This is done by the FrameTree on
+  // destruction of a RenderFrameHost.
+  void decrement_ref_count() { --ref_count_; }
+
+  // Returns the refcount on this RVH, that is the number of RenderFrameHosts
+  // currently using it.
+  int ref_count() { return ref_count_; }
+
   // NOTE: Do not add functions that just send an IPC message that are called in
   // one or two places. Have the caller send the IPC message directly (unless
   // the caller places are in different platforms, in which case it's better
@@ -597,12 +626,15 @@ class CONTENT_EXPORT RenderViewHostImpl
   FRIEND_TEST_ALL_PREFIXES(RenderViewHostTest, BasicRenderFrameHost);
   FRIEND_TEST_ALL_PREFIXES(RenderViewHostTest, RoutingIdSane);
 
-  // Sets whether this RenderViewHost is swapped out in favor of another,
-  // and clears any waiting state that is no longer relevant.
-  void SetSwappedOut(bool is_swapped_out);
+  // Updates the state of this RenderViewHost and clears any waiting state
+  // that is no longer relevant.
+  void SetRVHState(RenderViewHostImplState rvh_state);

[nasko, 2014/01/23 23:34:58]

nit: Why not just SetState? The method is already called on RVH object, so that
part is redundant.

[clamy, 2014/01/24 17:01:54]

Done.

 
   bool CanAccessFilesOfPageState(const PageState& state) const;
 
+  // The number of RenderFrameHost which have a pointer to this RVH.

[nasko, 2014/01/23 23:34:58]

nit: s/pointer/reference/

[clamy, 2014/01/24 17:01:54]

Done.

+  int ref_count_;

[nasko, 2014/01/23 23:34:58]

The name implies generic ref count, which this isn't. Maybe call it
frames_ref_count_?

[clamy, 2014/01/24 17:01:54]

Done.

+
   // Our delegate, which wants to know about changes in the RenderView.
   RenderViewHostDelegate* delegate_;
 
@@ -637,9 +669,8 @@ class CONTENT_EXPORT RenderViewHostImpl
   // first commit.
   bool has_accessed_initial_document_;
 
-  // Whether this RenderViewHost is currently swapped out, such that the view is
-  // being rendered by another process.
-  bool is_swapped_out_;
+  // The current state of this RVH.
+  RenderViewHostImplState rvh_state_;
 
   // The frame id of the main (top level) frame. This value is set on the
   // initial navigation of a RenderView and reset when the RenderView's
@@ -658,20 +689,13 @@ class CONTENT_EXPORT RenderViewHostImpl
 
   // Set to true when there is a pending ViewMsg_ShouldClose message.  This
   // ensures we don't spam the renderer with multiple beforeunload requests.
-  // When either this value or is_waiting_for_unload_ack_ is true, the value of
+  // When either this value or IsWaitingForUnloadACK is true, the value of
   // unload_ack_is_for_cross_site_transition_ indicates whether this is for a
   // cross-site transition or a tab close attempt.
   bool is_waiting_for_beforeunload_ack_;
 
-  // Set to true when there is a pending ViewMsg_Close message.  Also see
-  // is_waiting_for_beforeunload_ack_, unload_ack_is_for_cross_site_transition_.
-  bool is_waiting_for_unload_ack_;
-
-  // Set to true when waiting for ViewHostMsg_SwapOut_ACK has timed out.
-  bool has_timed_out_on_unload_;
-
   // Valid only when is_waiting_for_beforeunload_ack_ or
-  // is_waiting_for_unload_ack_ is true.  This tells us if the unload request
+  // IsWaitingForUnloadACK is true.  This tells us if the unload request
   // is for closing the entire tab ( = false), or only this RenderViewHost in
   // the case of a cross-site transition ( = true).
   bool unload_ack_is_for_cross_site_transition_;
@@ -706,6 +730,16 @@ class CONTENT_EXPORT RenderViewHostImpl
   scoped_ptr<BrowserMediaPlayerManager> media_player_manager_;
 #endif
 
+  // Used to shutdown this RVH when the unload event is taking too long to
+  // execute.
+  scoped_ptr<TimeoutMonitor> unload_event_monitor_timeout_;
+
+  // Called after receiving the swap out ack when the RVH is in state pending
+  // shutdown. Also called if the unload timer times out.
+  base::Closure pending_shutdown_on_swap_out_;
+
+  base::WeakPtrFactory<RenderViewHostImpl> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(RenderViewHostImpl);
 };