Create RenderFrameProxyHost at time of swap-out instead of commit.

content/renderer/render_frame_impl.cc

 // Copyright 2013 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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 147 matching lines @@
 using base::Time;
 using base::TimeDelta;
 using webkit_glue::WebURLResponseExtraDataImpl;
 
 namespace content {
 
 namespace {
 
 const size_t kExtraCharsBeforeAndAfterSelection = 100;
 
+typedef std::map<int, RenderFrameImpl*> RoutingIDFrameMap;
+static base::LazyInstance<RoutingIDFrameMap> g_routing_id_frame_map =
+    LAZY_INSTANCE_INITIALIZER;
+
 typedef std::map<blink::WebFrame*, RenderFrameImpl*> FrameMap;
 base::LazyInstance<FrameMap> g_frame_map = LAZY_INSTANCE_INITIALIZER;
 
 int64 ExtractPostId(const WebHistoryItem& item) {
   if (item.isNull())
     return -1;
 
   if (item.httpBody().isNull())
     return -1;
 
@@ skipping 173 matching lines @@
                                          int32 routing_id) {
   DCHECK(routing_id != MSG_ROUTING_NONE);
 
   if (g_create_render_frame_impl)
     return g_create_render_frame_impl(render_view, routing_id);
   else
     return new RenderFrameImpl(render_view, routing_id);
 }
 
 // static
+RenderFrameImpl* RenderFrameImpl::FromRoutingID(int32 routing_id) {

[Charlie Reis, 2014/05/15 00:32:50]

This code looks fine, but any reason that it differs from FromWebFrame below?

[nasko, 2014/05/15 18:47:13]

Yes, it is identical to the code in RenderViewImpl ;). I've changed it to
conform to the one in this file.

+  RoutingIDFrameMap* frames = g_routing_id_frame_map.Pointer();
+  RoutingIDFrameMap::iterator it = frames->find(routing_id);
+  return it == frames->end() ? NULL : it->second;
+}
+
+// static
 RenderFrame* RenderFrame::FromWebFrame(blink::WebFrame* web_frame) {
   return RenderFrameImpl::FromWebFrame(web_frame);
 }
 
 RenderFrameImpl* RenderFrameImpl::FromWebFrame(blink::WebFrame* web_frame) {
   FrameMap::iterator iter = g_frame_map.Get().find(web_frame);
   if (iter != g_frame_map.Get().end())
     return iter->second;
   return NULL;
 }
 
 // static
 void RenderFrameImpl::InstallCreateHook(
     RenderFrameImpl* (*create_render_frame_impl)(RenderViewImpl*, int32)) {
   CHECK(!g_create_render_frame_impl);
   g_create_render_frame_impl = create_render_frame_impl;
 }
 
 // RenderFrameImpl ----------------------------------------------------------
 RenderFrameImpl::RenderFrameImpl(RenderViewImpl* render_view, int routing_id)
     : frame_(NULL),
       render_view_(render_view->AsWeakPtr()),
+      render_frame_proxy_(NULL),
       routing_id_(routing_id),
       is_swapped_out_(false),
       is_detaching_(false),
       cookie_jar_(this),
       selection_text_offset_(0),
       selection_range_(gfx::Range::InvalidRange()),
       handling_select_range_(false),
       notification_provider_(NULL),
       media_stream_client_(NULL),
       web_user_media_client_(NULL),
       weak_factory_(this) {
   RenderThread::Get()->AddRoute(routing_id_, this);
 
+  std::pair<RoutingIDFrameMap::iterator, bool> result =
+      g_routing_id_frame_map.Get().insert(std::make_pair(routing_id_, this));
+  CHECK(result.second) << "Inserting a duplicate item.";
+
 #if defined(OS_ANDROID)
   new JavaBridgeDispatcher(this);
 #endif
 
 #if defined(ENABLE_NOTIFICATIONS)
   notification_provider_ = new NotificationProvider(this);
 #endif
 }
 
 RenderFrameImpl::~RenderFrameImpl() {
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, RenderFrameGone());
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, OnDestruct());
+  g_routing_id_frame_map.Get().erase(routing_id_);
   RenderThread::Get()->RemoveRoute(routing_id_);
 }
 
 void RenderFrameImpl::SetWebFrame(blink::WebLocalFrame* web_frame) {
   DCHECK(!frame_);
 
   std::pair<FrameMap::iterator, bool> result = g_frame_map.Get().insert(
       std::make_pair(web_frame, this));
   CHECK(result.second) << "Inserting a duplicate item.";
 
@@ skipping 199 matching lines @@
 #endif  // ENABLE_PLUGINS
 
 void RenderFrameImpl::SetMediaStreamClientForTesting(
     MediaStreamClient* media_stream_client) {
   DCHECK(!media_stream_client_);
   DCHECK(!web_user_media_client_);
   media_stream_client_ = media_stream_client;
 }
 
 bool RenderFrameImpl::Send(IPC::Message* message) {
-  if (is_detaching_ ||
-      ((is_swapped_out_ || render_view_->is_swapped_out()) &&
-       !SwappedOutMessages::CanSendWhileSwappedOut(message))) {
+  if (is_detaching_) {
     delete message;
     return false;
   }
+  if (is_swapped_out_ || render_view_->is_swapped_out()) {
+    if (render_frame_proxy_) {
+      // Some call sites use the associated RenderViewImpl routing id to send

[Charlie Reis, 2014/05/15 00:32:50]

Whew, this is tricky!  Maybe start with a sentence like "In most cases, send
IPCs through the proxy when swapped out."

[nasko, 2014/05/15 18:47:13]

Done.

+      // messages, so it shouldn't be overwritten in that case. Additionally,
+      // The SwapOut_ACK message must be delivered to the RenderFrameHost

[Charlie Reis, 2014/05/15 00:32:50]

nit: the

[nasko, 2014/05/15 18:47:13]

Done.

+      // instead of the proxy object and needs to be special-cased here.
+      if (message->routing_id() == routing_id_ &&
+          message->type() != FrameHostMsg_SwapOut_ACK::ID) {
+        return render_frame_proxy_->Send(message);
+      }
+    }
+
+    if (!SwappedOutMessages::CanSendWhileSwappedOut(message)) {

[Charlie Reis, 2014/05/15 00:32:50]

I don't think this is right.  When CanSendWhileSwappedOut returns false, we'll
have already sent the message through the proxy on line 656.  We should probably
leave it in the initial if statement as it was before.

[Edit: I see that RenderFrameProxy::Send calls CanSendWhileSwappedOut as well,
and deletes the message if not.  Wouldn't that be a double free in most cases,
apart from SwapOut_ACK, etc?]

[nasko, 2014/05/15 18:47:13]

If we ever get to call Send on the RenderFrameProxy, we will return and never
hit this statement or am I missing something?
The double free could happen if the call to RenderFrameProxy::Send didn't do an
early return out of this method.

+      delete message;
+      return false;
+    }
+  }
 
   return RenderThread::Get()->Send(message);
 }
 
 bool RenderFrameImpl::OnMessageReceived(const IPC::Message& msg) {
   GetContentClient()->SetActiveURL(frame_->document().url());
 
   ObserverListBase<RenderFrameObserver>::Iterator it(observers_);
   RenderFrameObserver* observer;
   while ((observer = it.GetNext()) != NULL) {
@@ skipping 241 matching lines @@
   CHECK(!frame_->parent());
 
   base::TimeTicks before_unload_start_time = base::TimeTicks::Now();
   bool proceed = frame_->dispatchBeforeUnloadEvent();
   base::TimeTicks before_unload_end_time = base::TimeTicks::Now();
   Send(new FrameHostMsg_BeforeUnload_ACK(routing_id_, proceed,
                                          before_unload_start_time,
                                          before_unload_end_time));
 }
 
-void RenderFrameImpl::OnSwapOut() {
+void RenderFrameImpl::OnSwapOut(int proxy_routing_id) {
   // Only run unload if we're not swapped out yet, but send the ack either way.
   if (!is_swapped_out_ || !render_view_->is_swapped_out_) {
     // Swap this RenderFrame out so the frame can navigate to a page rendered by
     // a different process.  This involves running the unload handler and
     // clearing the page.  Once WasSwappedOut is called, we also allow this
     // process to exit if there are no other active RenderFrames in it.
+    RenderFrameProxy* proxy = RenderFrameProxy::CreateFrameProxy(

[Charlie Reis, 2014/05/15 00:32:50]

I'm unclear on why this is declared up here, rather than as part of the
set_render_frame_proxy call below.  Does it need to happen before we get swapped
out, similar to SyncNavigationState?  If so, let's put it in that block (of
things to do before swap out actually occurs).

[nasko, 2014/05/15 18:47:13]

I think it should be created prior to running the unload handler, which can
potentially run for a long time. The reason is that if the browser starts
sending IPCs, the routing id is registered and messages aren't dropped.

+        proxy_routing_id, routing_id_);
 
     // Send an UpdateState message before we get swapped out.
     render_view_->SyncNavigationState();
 
     // Synchronously run the unload handler before sending the ACK.
     // TODO(creis): Call dispatchUnloadEvent unconditionally here to support
     // unload on subframes as well.
     if (!frame_->parent())
       frame_->dispatchUnloadEvent();
 
     // Swap out and stop sending any IPC messages that are not ACKs.
     if (!frame_->parent())
       render_view_->SetSwappedOut(true);
     is_swapped_out_ = true;
+    set_render_frame_proxy(proxy);
 
     // Now that we're swapped out and filtering IPC messages, stop loading to
     // ensure that no other in-progress navigation continues.  We do this here
     // to avoid sending a DidStopLoading message to the browser process.
     // TODO(creis): Should we be stopping all frames here and using
     // StopAltErrorPageFetcher with RenderView::OnStop, or just stopping this
     // frame?
     if (!frame_->parent())
       render_view_->OnStop();
     else
@@ skipping 2545 matching lines @@
       weak_factory_.GetWeakPtr(),
       render_view_->media_player_manager_,
       stream_texture_factory,
       RenderThreadImpl::current()->GetMediaThreadMessageLoopProxy(),
       new RenderMediaLog());
 }
 
 #endif
 
 }  // namespace content