Reland "Handle race of SwapIn() and browser destroying the speculative MainFrame"

This is a reland of e7d5981628

TestExpectations added for the portals test that is hitting crbug.com/838348

TBR=avi@chromium.org, dcheng@chromium.org

Original change's description:
> Handle race of SwapIn() and browser destroying the speculative MainFrame
>
> When WebContentsImpl destroys, it deletes the speculative main frame,
> but the renderer may have taken ownership of it already, and the notice
> of such action is in flight to the browser still. This leads to DCHECKs
> failing in the renderer. So inform the FrameMsg_Delete IPC what the
> intention of the browser is, there are 3 modes:
>
> - Deleting a non-main frame. This is the common case. These frames are
> all owned by the browser so it's all good, no races.
> - Deleting a speculative main frame at shutdown. This is the race we
> address here.
> - Deleting a speculative main frame because it's no longer needed. This
> race is not handled by this CL but we CHECK() it explicitly now instead
> of letting the renderer continue with a missing RenderFrame that it
> expects to be present until it crashes somewhere random later.
>
> TBR=avi@chromium.org, dcheng@chromium.org
>
> Bug: 957858, 838348
> Change-Id: I2110bdaf8b116df48037f69db6cb992fa3796e29
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1594834
> Commit-Queue: danakj <danakj@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#660025}

Bug: 957858, 838348
Change-Id: Iba424cc7be7db053c7ca617677b3cacf972fa067
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614132
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660128}

content/browser/frame_host/render_frame_host_impl.cc

@@ -1758,12 +1758,12 @@ bool RenderFrameHostImpl::CreateRenderFrame(int previous_routing_id,
   return true;
 }
 
-void RenderFrameHostImpl::DeleteRenderFrame() {
+void RenderFrameHostImpl::DeleteRenderFrame(FrameDeleteIntention intent) {
   if (!is_active())
     return;
 
   if (render_frame_created_) {
-    Send(new FrameMsg_Delete(routing_id_));
+    Send(new FrameMsg_Delete(routing_id_, intent));
 
     if (!frame_tree_node_->IsMainFrame() && IsCurrent()) {
       // If this subframe has an unload handler (and isn't speculative), ensure
@@ -2036,7 +2036,8 @@ void RenderFrameHostImpl::RemoveChild(FrameTreeNode* child) {
       // observers are notified of its deletion.
       std::unique_ptr<FrameTreeNode> node_to_delete(std::move(*iter));
       children_.erase(iter);
-      node_to_delete->current_frame_host()->DeleteRenderFrame();
+      node_to_delete->current_frame_host()->DeleteRenderFrame(
+          FrameDeleteIntention::kNotMainFrame);
       // Speculative RenderFrameHosts are deleted by the FrameTreeNode's
       // RenderFrameHostManager's destructor. RenderFrameProxyHosts send
       // FrameMsg_Delete automatically in the destructor.
@@ -2059,7 +2060,8 @@ void RenderFrameHostImpl::ResetChildren() {
   // this RenderFrameHostImpl to detach the current frame's children, rather
   // than messaging each child's current frame host...
   for (auto& child : children)
-    child->current_frame_host()->DeleteRenderFrame();
+    child->current_frame_host()->DeleteRenderFrame(
+        FrameDeleteIntention::kNotMainFrame);
 }
 
 void RenderFrameHostImpl::SetLastCommittedUrl(const GURL& url) {
@@ -2389,7 +2391,7 @@ void RenderFrameHostImpl::DetachFromProxy() {
     return;
 
   // Start pending deletion on this frame and its children.
-  DeleteRenderFrame();
+  DeleteRenderFrame(FrameDeleteIntention::kNotMainFrame);
   StartPendingDeletionOnSubtree();
   // Some children with no unload handler may be eligible for immediate
   // deletion. Cut the dead branches now. This is a performance optimization.
@@ -4525,7 +4527,7 @@ void RenderFrameHostImpl::StartPendingDeletionOnSubtree() {
           local_ancestor = rfh;
       }
 
-      local_ancestor->DeleteRenderFrame();
+      local_ancestor->DeleteRenderFrame(FrameDeleteIntention::kNotMainFrame);
       if (local_ancestor != child) {
         child->unload_state_ =
             child->GetSuddenTerminationDisablerState(blink::kUnloadHandler)

content/browser/frame_host/render_frame_host_impl.h

@@ -41,6 +41,7 @@
 #include "content/common/content_export.h"
 #include "content/common/content_security_policy/csp_context.h"
 #include "content/common/frame.mojom.h"
+#include "content/common/frame_delete_intention.h"
 #include "content/common/frame_message_enums.h"
 #include "content/common/frame_replication_state.h"
 #include "content/common/image_downloader/image_downloader.mojom.h"
@@ -332,7 +333,7 @@ class CONTENT_EXPORT RenderFrameHostImpl
 
   // Deletes the RenderFrame in the renderer process.
   // Postcondition: |is_active()| will return false.
-  void DeleteRenderFrame();
+  void DeleteRenderFrame(FrameDeleteIntention intent);
 
   // Tracks whether the RenderFrame for this RenderFrameHost has been created in
   // the renderer process.  This is currently only used for subframes.

content/browser/frame_host/render_frame_host_manager.cc

@@ -823,7 +823,10 @@ void RenderFrameHostManager::CleanUpNavigation() {
 std::unique_ptr<RenderFrameHostImpl>
 RenderFrameHostManager::UnsetSpeculativeRenderFrameHost() {
   speculative_render_frame_host_->GetProcess()->RemovePendingView();
-  speculative_render_frame_host_->DeleteRenderFrame();
+  speculative_render_frame_host_->DeleteRenderFrame(
+      frame_tree_node_->parent()
+          ? FrameDeleteIntention::kNotMainFrame
+          : FrameDeleteIntention::kSpeculativeMainFrameForNavigationCancelled);
   return std::move(speculative_render_frame_host_);
 }
 

content/browser/web_contents/web_contents_impl.cc

@@ -689,7 +689,8 @@ WebContentsImpl::~WebContentsImpl() {
   // Do not update state as the WebContents is being destroyed.
   frame_tree_.root()->ResetNavigationRequest(true, true);
   if (root->speculative_frame_host()) {
-    root->speculative_frame_host()->DeleteRenderFrame();
+    root->speculative_frame_host()->DeleteRenderFrame(
+        FrameDeleteIntention::kSpeculativeMainFrameForShutdown);
     root->speculative_frame_host()->SetRenderFrameCreated(false);
     root->speculative_frame_host()->ResetNavigationRequests();
   }

content/common/BUILD.gn

@@ -112,6 +112,7 @@ source_set("common") {
     "font_list_fontconfig.cc",
     "font_list_mac.mm",
     "font_list_win.cc",
+    "frame_delete_intention.h",
     "frame_message_enums.h",
     "frame_message_structs.cc",
     "frame_message_structs.h",

content/common/frame_delete_intention.h

@@ -0,0 +1,27 @@
+// Copyright 2019 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.
+
+#ifndef CONTENT_COMMON_FRAME_DELETE_INTENTION_H_
+#define CONTENT_COMMON_FRAME_DELETE_INTENTION_H_
+
+namespace content {
+
+enum class FrameDeleteIntention {
+  // The frame being deleted isn't a (speculative) main frame.
+  kNotMainFrame,
+  // The frame being deleted is a speculative main frame, and it is being
+  // deleted as part of the shutdown for that WebContents. The entire RenderView
+  // etc will be destroyed by a separate IPC sent later.
+  kSpeculativeMainFrameForShutdown,
+  // The frame being deleted is a speculative main frame, and it is being
+  // deleted because the speculative navigation was cancelled. This is not part
+  // of shutdown.
+  kSpeculativeMainFrameForNavigationCancelled,
+
+  kMaxValue = kSpeculativeMainFrameForNavigationCancelled
+};
+
+}  // namespace content
+
+#endif  // CONTENT_COMMON_FRAME_DELETE_INTENTION_H_

content/common/frame_messages.h

@@ -26,6 +26,7 @@
 #include "content/common/content_param_traits.h"
 #include "content/common/content_security_policy/csp_context.h"
 #include "content/common/content_security_policy_header.h"
+#include "content/common/frame_delete_intention.h"
 #include "content/common/frame_message_enums.h"
 #include "content/common/frame_message_structs.h"
 #include "content/common/frame_owner_properties.h"
@@ -101,6 +102,8 @@ using FrameMsg_GetSerializedHtmlWithLocalLinks_FrameRoutingIdMap =
 #define IPC_MESSAGE_EXPORT CONTENT_EXPORT
 
 #define IPC_MESSAGE_START FrameMsgStart
+IPC_ENUM_TRAITS_MAX_VALUE(content::FrameDeleteIntention,
+                          content::FrameDeleteIntention::kMaxValue)
 IPC_ENUM_TRAITS_MAX_VALUE(blink::FrameOwnerElementType,
                           blink::FrameOwnerElementType::kMaxValue)
 IPC_ENUM_TRAITS_MAX_VALUE(
@@ -813,7 +816,7 @@ IPC_MESSAGE_ROUTED1(FrameMsg_UpdateOpener, int /* opener_routing_id */)
 IPC_MESSAGE_ROUTED1(FrameMsg_VisualStateRequest, uint64_t /* id */)
 
 // Instructs the renderer to delete the RenderFrame.
-IPC_MESSAGE_ROUTED0(FrameMsg_Delete)
+IPC_MESSAGE_ROUTED1(FrameMsg_Delete, content::FrameDeleteIntention)
 
 // Instructs the renderer to invoke the frame's beforeunload event handler.
 // Expects the result to be returned via FrameHostMsg_BeforeUnload_ACK.

content/renderer/render_frame_impl.cc

@@ -2418,7 +2418,47 @@ void RenderFrameImpl::OnSwapIn() {
   SwapIn();
 }
 
-void RenderFrameImpl::OnDeleteFrame() {
+void RenderFrameImpl::OnDeleteFrame(FrameDeleteIntention intent) {
+  // The main frame (when not provisional) is owned by the renderer's frame tree
+  // via WebViewImpl. When a provisional main frame is swapped in, the ownership
+  // moves from the browser to the renderer, but this happens in the renderer
+  // process and is then the browser is informed.
+  // If the provisional main frame is swapped in while the browser is destroying
+  // it, the browser may request to delete |this|, thinking it has ownership
+  // of it, but the renderer has already taken ownership via SwapIn().
+  switch (intent) {
+    case FrameDeleteIntention::kNotMainFrame:
+      // The frame was not a main frame, so the browser should always have
+      // ownership of it and we can just proceed with deleting it on
+      // request.
+      DCHECK(!is_main_frame_);
+      break;
+    case FrameDeleteIntention::kSpeculativeMainFrameForShutdown:
+      // In this case the renderer has taken ownership of the provisional main
+      // frame but the browser did not know yet and is shutting down. We can
+      // ignore this request as the frame will be destroyed when the RenderView
+      // is. This handles the shutdown case of https://crbug.com/957858.
+      DCHECK(is_main_frame_);
+      if (in_frame_tree_)
+        return;
+      break;
+    case FrameDeleteIntention::kSpeculativeMainFrameForNavigationCancelled:
+      // In this case the browser was navigating and cancelled the speculative
+      // navigation. The renderer *should* undo the SwapIn() but the old state
+      // has already been destroyed. Both ignoring the message or handling it
+      // would leave the renderer in an inconsistent state now. If we ignore it
+      // then the browser thinks the RenderView has a remote main frame, but it
+      // is incorrect. If we handle it, then we are deleting a local main frame
+      // out from under the RenderView and we will have bad pointers in the
+      // renderer. So all we can do is crash. We should instead prevent this
+      // scenario by blocking the browser from dropping the speculative main
+      // frame when a commit (and ownership transfer) is imminent.
+      // TODO(dcheng): This is the case of https://crbug.com/838348.
+      DCHECK(is_main_frame_);
+      CHECK(!in_frame_tree_);
+      break;
+  }
+
   // This will result in a call to RenderFrameImpl::FrameDetached, which
   // deletes the object. Do not access |this| after detach.
   frame_->Detach();

content/renderer/render_frame_impl.h

@@ -34,6 +34,7 @@
 #include "content/common/buildflags.h"
 #include "content/common/download/mhtml_file_writer.mojom.h"
 #include "content/common/frame.mojom.h"
+#include "content/common/frame_delete_intention.h"
 #include "content/common/frame_message_enums.h"
 #include "content/common/host_zoom.mojom.h"
 #include "content/common/media/renderer_audio_input_stream_factory.mojom.h"
@@ -1108,7 +1109,7 @@ class CONTENT_EXPORT RenderFrameImpl
   void OnSwapOut(int proxy_routing_id,
                  bool is_loading,
                  const FrameReplicationState& replicated_frame_state);
-  void OnDeleteFrame();
+  void OnDeleteFrame(FrameDeleteIntention intent);
   void OnStop();
   void OnCollapse(bool collapse);
   void OnShowContextMenu(const gfx::Point& location);

content/renderer/render_frame_impl_browsertest.cc

@@ -497,7 +497,7 @@ TEST_F(RenderFrameImplTest, NoCrashWhenDeletingFrameDuringFind) {
       1, "foo", true /* match_case */, true /* forward */,
       false /* find_next */, true /* force */, false /* wrap_within_frame */);
 
-  FrameMsg_Delete delete_message(0);
+  FrameMsg_Delete delete_message(0, FrameDeleteIntention::kNotMainFrame);
   frame()->OnMessageReceived(delete_message);
 }
 

third_party/blink/web_tests/TestExpectations

@@ -5505,6 +5505,11 @@ crbug.com/953153 external/wpt/speech-api/idlharness.window.html [ Pass Failure T
 # portals-adopt-predecessor occasionally times out.
 crbug.com/952584 external/wpt/portals/portals-adopt-predecessor.html [ Pass Timeout ]
 
+# portals/csp/frame-src.sub.html crashes because of a race between cancelling a
+# speculative navigation in the browser, and committing the navigation in the
+# renderer.
+crbug.com/838348 external/wpt/portals/csp/frame-src.sub.html [ Pass Crash ]
+
 # Sheriff 2019-04-17
 crbug.com/953534 [ Mac ] virtual/layout_ng_experimental/external/wpt/css/css-flexbox/ttwf-reftest-flex-wrap.html [ Failure ]