Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

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 <utility>
 #include <vector>
@@ skipping 938 matching lines @@
     return nullptr;
 
   // Opener routing ID could refer to either a RenderFrameProxy or a
   // RenderFrame, so need to check both.
   RenderFrameProxy* opener_proxy =
       RenderFrameProxy::FromRoutingID(opener_frame_routing_id);
   if (opener_proxy) {
     if (opener_view_routing_id)
       *opener_view_routing_id = opener_proxy->render_view()->GetRoutingID();
 
-    // TODO(nasko,alexmos): This check won't be needed once swappedout:// is
-    // gone.
-    if (opener_proxy->IsMainFrameDetachedFromTree()) {
-      DCHECK(!SiteIsolationPolicy::IsSwappedOutStateForbidden());
-      return opener_proxy->render_view()->webview()->mainFrame();
-    } else {
-      return opener_proxy->web_frame();
-    }
+    return opener_proxy->web_frame();
   }
 
   RenderFrameImpl* opener_frame =
       RenderFrameImpl::FromRoutingID(opener_frame_routing_id);
   if (opener_frame) {
     if (opener_view_routing_id)
       *opener_view_routing_id = opener_frame->render_view()->GetRoutingID();
     return opener_frame->GetWebFrame();
   }
 
   return nullptr;
 }
 
 // RenderFrameImpl ----------------------------------------------------------
 RenderFrameImpl::RenderFrameImpl(const CreateParams& params)
     : frame_(NULL),
       is_main_frame_(true),
       in_browser_initiated_detach_(false),
       in_frame_tree_(false),
       render_view_(params.render_view->AsWeakPtr()),
       routing_id_(params.routing_id),
       is_swapped_out_(false),

[dmazzoni, 2016/03/11 20:31:34]

Why not delete this as part of the same change?

[Charlie Reis, 2016/03/11 21:56:56]

We're just doing it in pieces to keep the size reasonable.  That'll happen soon.
 :)

[nasko, 2016/03/11 22:35:33]

Indeed. It is already removed in my next iteration :).

       render_frame_proxy_(NULL),
       is_detaching_(false),
       proxy_routing_id_(MSG_ROUTING_NONE),
 #if defined(ENABLE_PLUGINS)
       plugin_power_saver_helper_(nullptr),
       plugin_find_handler_(nullptr),
 #endif
       cookie_jar_(this),
       selection_text_offset_(0),
       selection_range_(gfx::Range::InvalidRange()),
@@ skipping 52 matching lines @@
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, OnDestruct());
 
   base::trace_event::TraceLog::GetInstance()->RemoveProcessLabel(routing_id_);
 
 #if defined(VIDEO_HOLE)
   if (contains_media_player_)
     render_view_->UnregisterVideoHoleFrame(this);
 #endif
 
   if (is_main_frame_) {
-    // When using swapped out frames, RenderFrameProxy is owned by
-    // RenderFrameImpl in the case it is the main frame. Ensure it is deleted
-    // along with this object.
-    if (render_frame_proxy_ &&
-        !SiteIsolationPolicy::IsSwappedOutStateForbidden()) {
-      // The following method calls back into this object and clears
-      // |render_frame_proxy_|.
-      render_frame_proxy_->frameDetached(
-          blink::WebRemoteFrameClient::DetachType::Remove);
-    }
-
     // Ensure the RenderView doesn't point to this object, once it is destroyed.
     // TODO(nasko): Add a check that the |main_render_frame_| of |render_view_|
     // is |this|, once the object is no longer leaked.
     // See https://crbug.com/464764.
     render_view_->main_render_frame_ = nullptr;
   }
 
   render_view_->UnregisterRenderFrame(this);
   g_routing_id_frame_map.Get().erase(routing_id_);
   RenderThread::Get()->RemoveRoute(routing_id_);
@@ skipping 454 matching lines @@
                                          before_unload_start_time,
                                          before_unload_end_time));
 }
 
 void RenderFrameImpl::OnSwapOut(
     int proxy_routing_id,
     bool is_loading,
     const FrameReplicationState& replicated_frame_state) {
   TRACE_EVENT1("navigation", "RenderFrameImpl::OnSwapOut", "id", routing_id_);
   RenderFrameProxy* proxy = NULL;
-  bool swapped_out_forbidden =
-      SiteIsolationPolicy::IsSwappedOutStateForbidden();
 
   // This codepath should only be hit for subframes when in --site-per-process.
   CHECK(is_main_frame_ || SiteIsolationPolicy::AreCrossProcessFramesPossible());
 
   // Only run unload if we're not swapped out yet, but send the ack either way.
   if (!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.  We also allow this process to exit if there are no
     // other active RenderFrames in it.
@@ skipping 21 matching lines @@
     if (is_main_frame_)
       render_view_->SetSwappedOut(true);
     is_swapped_out_ = true;
 
     // Set the proxy here, since OnStop() below could cause an onload event
     // handler to execute, which could trigger code such as
     // willCheckAndDispatchMessageEvent() that needs the proxy.
     if (proxy)
       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 (!swapped_out_forbidden)
-      OnStop();
-
     // Transfer settings such as initial drawing parameters to the remote frame,
     // if one is created, that will replace this frame.
     if (!is_main_frame_ && proxy)
       proxy->web_frame()->initializeFromFrame(frame_);
 
-    // Replace the page with a blank dummy URL. The unload handler will not be
-    // run a second time, thanks to a check in FrameLoader::stopLoading.
-    // TODO(creis): Need to add a better way to do this that avoids running the
-    // beforeunload handler. For now, we just run it a second time silently.
-    if (!swapped_out_forbidden)
-      NavigateToSwappedOutURL();
-
     // Let WebKit know that this view is hidden so it can drop resources and
     // stop compositing.
     // TODO(creis): Support this for subframes as well.
     if (is_main_frame_) {
       render_view_->webview()->setVisibilityState(
           blink::WebPageVisibilityStateHidden, false);
     }
   }
 
   // It is now safe to show modal dialogs again.
   // TODO(creis): Deal with modal dialogs from subframes.
   if (is_main_frame_)
     render_view_->suppress_dialogs_until_swap_out_ = false;
 
   Send(new FrameHostMsg_SwapOut_ACK(routing_id_));
 
   RenderViewImpl* render_view = render_view_.get();
   bool is_main_frame = is_main_frame_;
   int routing_id = GetRoutingID();
 
   // Now that all of the cleanup is complete and the browser side is notified,
   // start using the RenderFrameProxy, if one is created.
-  if (proxy && swapped_out_forbidden) {
+  if (proxy) {
     // The swap call deletes this RenderFrame via frameDetached.  Do not access
     // any members after this call.
     // TODO(creis): WebFrame::swap() can return false.  Most of those cases
     // should be due to the frame being detached during unload (in which case
     // the necessary cleanup has happened anyway), but it might be possible for
     // it to return false without detaching.  Catch those cases below to track
     // down https://crbug.com/575245.
     frame_->swap(proxy->web_frame());
 
     // For main frames, the swap should have cleared the RenderView's pointer to
     // this frame.
     if (is_main_frame) {
       base::debug::SetCrashKeyValue("swapout_frame_id",
                                     base::IntToString(routing_id));
       base::debug::SetCrashKeyValue("swapout_proxy_id",
                                     base::IntToString(proxy->routing_id()));
       base::debug::SetCrashKeyValue(
           "swapout_view_id", base::IntToString(render_view->GetRoutingID()));
       CHECK(!render_view->main_render_frame_);
     }
 
     if (is_loading)
       proxy->OnDidStartLoading();
   }
 
-  // In --site-per-process, initialize the WebRemoteFrame with the replication
-  // state passed by the process that is now rendering the frame.
-  // TODO(alexmos): We cannot yet do this for swapped-out main frames, because
-  // in that case we leave the LocalFrame as the main frame visible to Blink
-  // and don't call swap() above. Because swap() is what creates a RemoteFrame
-  // in proxy->web_frame(), the RemoteFrame will not exist for main frames.
-  // When we do an unconditional swap for all frames, we can remove
-  // !is_main_frame below.

[dcheng, 2016/03/11 20:32:28]

This TODO is stale right? I don't see a !is_main_frame case at all anywhere, but
I want to make sure I'm not missing something.

[nasko, 2016/03/11 22:35:33]

Indeed it no longer applies, which is why I've removed it.

-  if (proxy && swapped_out_forbidden)
+  // Initialize the WebRemoteFrame with the replication state passed by the
+  // process that is now rendering the frame.
+  if (proxy)
     proxy->SetReplicatedState(replicated_frame_state);
 
   // Safe to exit if no one else is using the process.
   // TODO(nasko): Remove the dependency on RenderViewImpl here and ref count
   // the process based on the lifetime of this RenderFrameImpl object.
-  if (is_main_frame) {
+  if (is_main_frame)
     render_view->WasSwappedOut();
-  }
 }
 
 void RenderFrameImpl::OnDeleteFrame() {
   // TODO(nasko): If this message is received right after a commit has
   // swapped a RenderFrameProxy with this RenderFrame, the proxy needs to be
   // recreated in addition to the RenderFrame being deleted.
   // See https://crbug.com/569683 for details.
   in_browser_initiated_detach_ = true;
 
   // This will result in a call to RendeFrameImpl::frameDetached, which
@@ skipping 417 matching lines @@
       WebString::fromUTF8(params.text_track_text_size));
 }
 
 void RenderFrameImpl::OnPostMessageEvent(
     const FrameMsg_PostMessage_Params& params) {
   // Find the source frame if it exists.
   WebFrame* source_frame = NULL;
   if (params.source_routing_id != MSG_ROUTING_NONE) {
     RenderFrameProxy* source_proxy =
         RenderFrameProxy::FromRoutingID(params.source_routing_id);
-    if (source_proxy) {
-      // Currently, navigating a top-level frame cross-process does not swap
-      // the WebLocalFrame for a WebRemoteFrame in the frame tree, and the
-      // WebRemoteFrame will not have an associated blink::Frame. If this is
-      // the case for |source_proxy|, use the corresponding (swapped-out)
-      // WebLocalFrame instead, so that event.source for this message can be
-      // set and used properly.
-      if (source_proxy->IsMainFrameDetachedFromTree())
-        source_frame = source_proxy->render_view()->webview()->mainFrame();
-      else
-        source_frame = source_proxy->web_frame();
-    }
+    if (source_proxy)
+      source_frame = source_proxy->web_frame();
   }
 
   // If the message contained MessagePorts, create the corresponding endpoints.
   blink::WebMessagePortChannelArray channels =
       WebMessagePortChannelImpl::CreatePorts(
           params.message_ports, params.new_routing_ids,
           base::ThreadTaskRunnerHandle::Get().get());
 
   WebSerializedScriptValue serialized_script_value;
   if (params.is_data_raw_string) {
@@ skipping 2644 matching lines @@
           info.urlRequest, info.navigationType, info.defaultPolicy,
           is_redirect)) {
     return blink::WebNavigationPolicyIgnore;
   }
 #endif
 
   Referrer referrer(
       RenderViewImpl::GetReferrerFromRequest(frame_, info.urlRequest));
 
   // TODO(nick): Is consulting |is_main_frame| here correct?
-  if (SiteIsolationPolicy::IsSwappedOutStateForbidden() && !is_main_frame_) {
-    // There's no reason to ignore navigations on subframes, since the swap out
-    // logic no longer applies.
-  } else {
-    if (is_swapped_out_) {
-      if (info.urlRequest.url() != GURL(kSwappedOutURL)) {
-        // Targeted links may try to navigate a swapped out frame.  Allow the
-        // browser process to navigate the tab instead.  Note that it is also
-        // possible for non-targeted navigations (from this view) to arrive
-        // here just after we are swapped out.  It's ok to send them to the
-        // browser, as long as they're for the top level frame.
-        // TODO(creis): Ensure this supports targeted form submissions when
-        // fixing http://crbug.com/101395.
-        if (frame_->parent() == NULL) {
-          OpenURL(info.urlRequest.url(), referrer, info.defaultPolicy,
-                  info.replacesCurrentHistoryItem, false);
-          return blink::WebNavigationPolicyIgnore;  // Suppress the load here.
-        }
-
-        // We should otherwise ignore in-process iframe navigations, if they
-        // arrive just after we are swapped out.
-        return blink::WebNavigationPolicyIgnore;
+  if (is_main_frame_ && is_swapped_out_) {
+    if (info.urlRequest.url() != GURL(kSwappedOutURL)) {

[Charlie Reis, 2016/03/11 21:56:56]

I suppose this is next on the chopping block?  Maybe add a TODO.

[nasko, 2016/03/11 22:35:33]

Yes, it is. As mentioned elsewhere, I want to rip these out quickly and I'm
intentionally not putting TODOs. If you feel I should, let me know.

+      // Targeted links may try to navigate a swapped out frame.  Allow the
+      // browser process to navigate the tab instead.  Note that it is also
+      // possible for non-targeted navigations (from this view) to arrive
+      // here just after we are swapped out.  It's ok to send them to the
+      // browser, as long as they're for the top level frame.
+      // TODO(creis): Ensure this supports targeted form submissions when
+      // fixing http://crbug.com/101395.
+      if (frame_->parent() == NULL) {
+        OpenURL(info.urlRequest.url(), referrer, info.defaultPolicy,
+                info.replacesCurrentHistoryItem, false);
+        return blink::WebNavigationPolicyIgnore;  // Suppress the load here.
       }
 
-      // Allow kSwappedOutURL to complete.
-      return info.defaultPolicy;
+      // We should otherwise ignore in-process iframe navigations, if they
+      // arrive just after we are swapped out.
+      return blink::WebNavigationPolicyIgnore;
     }
+
+    // Allow kSwappedOutURL to complete.
+    return info.defaultPolicy;
   }
 
   // Webkit is asking whether to navigate to a new URL.
   // This is fine normally, except if we're showing UI from one security
   // context and they're trying to navigate to a different context.
   const GURL& url = info.urlRequest.url();
 
   // If the browser is interested, then give it a chance to look at the request.
   if (is_content_initiated && IsTopLevelNavigation(frame_) &&
       render_view_->renderer_preferences_
@@ skipping 1307 matching lines @@
                                     int match_count,
                                     int ordinal,
                                     const WebRect& selection_rect,
                                     bool final_status_update) {
   Send(new FrameHostMsg_Find_Reply(routing_id_, request_id, match_count,
                                    selection_rect, ordinal,
                                    final_status_update));
 }
 
 }  // namespace content