Check that we do not call RenderFrameCreated while in RenderFrameDeleted

This was happening in https://crbug.com/1146573 and could happen if a
WebContentsObserver triggers synchronous navigation in response to a
the RenderFrameDeleted call.

This adds a RenderFrameState::kDeleting that is only used during
RenderFrameDeleted.

Bug: 1146573,1163815
Change-Id: I6f3ff264d5788135a79d90a1dba0e7e6a16adad3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2599568
Commit-Queue: Fergal Daly <fergal@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Auto-Submit: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841398}

content/browser/renderer_host/render_frame_host_impl.cc

@@ -2477,6 +2477,12 @@ void RenderFrameHostImpl::DeleteRenderFrame(FrameDeleteIntention intent) {
 }
 
 void RenderFrameHostImpl::RenderFrameCreated() {
+  // In https://crbug.com/1146573 a WebContentsObserver was causing the frame to
+  // be reinitialized during deletion. It is not valid to re-enter navigation
+  // code like that and it led to an invalid state. This is not a DCHECK because
+  // the corruption will not be visible until later, making the bug very
+  // difficult to understand.
+  CHECK_NE(render_frame_state_, RenderFrameState::kDeleting);
   // We should not create new RenderFrames while our delegate is being destroyed
   // (e.g., via a WebContentsObserver during WebContents shutdown).  This seems
   // to have caused crashes in https://crbug.com/717650.
@@ -2522,17 +2528,25 @@ void RenderFrameHostImpl::RenderFrameCreated() {
 }
 
 void RenderFrameHostImpl::RenderFrameDeleted() {
+  // In https://crbug.com/1146573 a WebContentsObserver was causing the frame to
+  // be reinitialized during deletion. It is not valid to re-enter navigation
+  // code like that and it led to an invalid state. This is not a DCHECK because
+  // the corruption will cause a crash but later, making the bug very
+  // difficult to understand.
+  CHECK_NE(render_frame_state_, RenderFrameState::kDeleting);
   bool was_created = is_render_frame_created();
-  render_frame_state_ = RenderFrameState::kDeleted;
+  render_frame_state_ = RenderFrameState::kDeleting;
 
   // If the current status is different than the new status, the delegate
   // needs to be notified.
   if (was_created) {
     delegate_->RenderFrameDeleted(this);
   }
+
   if (web_ui_) {
     web_ui_->InvalidateMojoConnection();
   }
+  render_frame_state_ = RenderFrameState::kDeleted;
 }
 
 void RenderFrameHostImpl::SwapIn() {

content/browser/renderer_host/render_frame_host_impl.h

@@ -2737,11 +2737,21 @@ class CONTENT_EXPORT RenderFrameHostImpl
     // state will be kCreated or kDeleted.
     kNeverCreated = 0,
     // A RenderFrame has been created in the renderer and is still in that
-    // state. The next state will be kDeleted.
+    // state. The next state will be kDeleting.
     kCreated,
-    // A RenderFrame has either been cleanly deleted or its renderer process has
-    // exited or crashed. The next state may be kCreated or the RenderFrameHost
-    // may be destroyed.
+    // A RenderFrame has either
+    // - been cleanly deleted
+    // - its renderer process has exited or crashed
+    // We will call observers of RenderFrameDeleted in this state and this
+    // allows us to CHECK if an observer causes us to attempt to change state
+    // during deletion. See https://crbug.com/1146573. The next state will
+    // be kDeleted and we will move to that before exiting RenderFrameDeleted.
+    kDeleting,
+    // A RenderFrame has either
+    // - been cleanly deleted
+    // - its renderer process has exited or crashed
+    // The next state may be kCreated if the RenderFrameHost is being reused
+    // after a crash or the RenderFrameHost may be destroyed.
     kDeleted,
   };
   RenderFrameState render_frame_state_ = RenderFrameState::kNeverCreated;