Fix a race condition where rapid back/forward clicks could close a tab

This can be triggered when you're on the new tab page, going to *two* other
sites, then rapidly hitting back and forward randomly. If a cross-site
transition was canceled before the original page responds with an "OK to close
me" message, it will mistakenly categorize the close as not just for the
RenderView (correspondong to one side of the cross-site transition) but for the
entire tab.

This change adds an explicit parameter on the messages indicating whether it's
for interstials or for the tab so we don't have to rely on the request still
being active.

This also adds the "requesting process + route" in addition to the
"new process + request" so we can be more clear about sending the messages to
the correct place. The previous patch conbimed these in a confusing way.

BUG=16246
TEST=none
Review URL: http://codereview.chromium.org/159255

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@21531 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 80 insertions, 8 deletions

diff --git a/chrome/common/render_messages.h b/chrome/common/render_messages.h
index 6835b49..2ca15fd 100644
--- a/chrome/common/render_messages.h
+++ b/chrome/common/render_messages.h
@@ -223,6 +223,42 @@ struct ViewMsg_UploadFile_Params {
   std::wstring other_values;
 };
 
+// Information on closing a tab. This is used both for ViewMsg_ClosePage, and
+// the corresponding ViewHostMsg_ClosePage_ACK.
+struct ViewMsg_ClosePage_Params {
+  // The identifier of the RenderProcessHost for the currently closing view.
+  //
+  // These first two parameters are technically redundant since they are
+  // needed only when processing the ACK message, and the processor
+  // theoretically knows both the process and route ID. However, this is
+  // difficult to figure out with our current implementation, so this
+  // information is duplicate here.
+  int closing_process_id;
+
+  // The route identifier for the currently closing RenderView.
+  int closing_route_id;
+
+  // True when this close is for the first (closing) tab of a cross-site
+  // transition where we switch processes. False indicates the close is for the
+  // entire tab.
+  //
+  // When true, the new_* variables below must be filled in. Otherwise they must
+  // both be -1.
+  bool for_cross_site_transition;
+
+  // The identifier of the RenderProcessHost for the new view attempting to
+  // replace the closing one above. This must be valid when
+  // for_cross_site_transition is set, and must be -1 otherwise.
+  int new_render_process_host_id;
+
+  // The identifier of the *request* the new view made that is causing the
+  // cross-site transition. This is *not* a route_id, but the request that we
+  // will resume once the ACK from the closing view has been received. This
+  // must be valid when for_cross_site_transition is set, and must be -1
+  // otherwise.
+  int new_request_id;
+};
+
 // Parameters for a resource request.
 struct ViewHostMsg_Resource_Request {
   // The request method: GET, POST, etc.
@@ -1243,6 +1279,41 @@ struct ParamTraits<NavigationGesture> {
   }
 };
 
+// Traits for ViewMsg_Close_Params.
+template <>
+struct ParamTraits<ViewMsg_ClosePage_Params> {
+  typedef ViewMsg_ClosePage_Params param_type;
+  static void Write(Message* m, const param_type& p) {
+    WriteParam(m, p.closing_process_id);
+    WriteParam(m, p.closing_route_id);
+    WriteParam(m, p.for_cross_site_transition);
+    WriteParam(m, p.new_render_process_host_id);
+    WriteParam(m, p.new_request_id);
+  }
+
+  static bool Read(const Message* m, void** iter, param_type* r) {
+    return ReadParam(m, iter, &r->closing_process_id) &&
+           ReadParam(m, iter, &r->closing_route_id) &&
+           ReadParam(m, iter, &r->for_cross_site_transition) &&
+           ReadParam(m, iter, &r->new_render_process_host_id) &&
+           ReadParam(m, iter, &r->new_request_id);
+  }
+
+  static void Log(const param_type& p, std::wstring* l) {
+    l->append(L"(");
+    LogParam(p.closing_process_id, l);
+    l->append(L", ");
+    LogParam(p.closing_route_id, l);
+    l->append(L", ");
+    LogParam(p.for_cross_site_transition, l);
+    l->append(L", ");
+    LogParam(p.new_render_process_host_id, l);
+    l->append(L", ");
+    LogParam(p.new_request_id, l);
+    l->append(L")");
+  }
+};
+
 // Traits for ViewHostMsg_Resource_Request
 template <>
 struct ParamTraits<ViewHostMsg_Resource_Request> {
diff --git a/chrome/common/render_messages_internal.h b/chrome/common/render_messages_internal.h
index f240a34..36afb03 100644
--- a/chrome/common/render_messages_internal.h
+++ b/chrome/common/render_messages_internal.h
@@ -466,10 +466,12 @@ IPC_BEGIN_MESSAGES(View)
   IPC_MESSAGE_ROUTED0(ViewMsg_ShouldClose)
 
   // Instructs the renderer to close the current page, including running the
-  // onunload event handler.  Expects a ClosePage_ACK message when finished.
-  IPC_MESSAGE_ROUTED2(ViewMsg_ClosePage,
-                      int /* new_render_process_host_id */,
-                      int /* new_request_id */)
+  // onunload event handler. See the struct in render_messages.h for more.
+  //
+  // Expects a ClosePage_ACK message when finished, where the parameters are
+  // echoed back.
+  IPC_MESSAGE_ROUTED1(ViewMsg_ClosePage,
+                      ViewMsg_ClosePage_Params)
 
   // Asks the renderer to send back stats on the WebCore cache broken down by
   // resource types.
@@ -1222,10 +1224,9 @@ IPC_BEGIN_MESSAGES(ViewHost)
                       bool /* proceed */)
 
   // Indicates that the current page has been closed, after a ClosePage
-  // message.
-  IPC_MESSAGE_ROUTED2(ViewHostMsg_ClosePage_ACK,
-                      int /* new_render_process_host_id */,
-                      int /* new_request_id */)
+  // message. The parameters are just echoed from the ClosePage request.
+  IPC_MESSAGE_ROUTED1(ViewHostMsg_ClosePage_ACK,
+                      ViewMsg_ClosePage_Params)
 
   IPC_MESSAGE_ROUTED4(ViewHostMsg_DidDownloadFavIcon,
                       int /* Identifier of the request */,