Simplify Frame::Swap() to only use Detach().

Frame::Swap() would previously call DetachDocument(), save some state,
and then complete detaching old frame by calling Detach(). However, this
left the old frame in a semi-inconsistent state between DetachDocument()
and Detach() and could trigger some weird edge cases (such as in the
printing code).

Instead, since Detach() already takes a FrameDetachType, improve the
coordination between Detach() and Swap() so that detaching a frame for
swap preserves the state that Swap() will adopt. By doing this,
Frame::Swap() no longer needs to call DetachDocument() at all.

Bug: 1061686, 1063150
Change-Id: I937bbf9d0a6d0de911aa4bd4743a16b86df1b4f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597169Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839626}

third_party/blink/renderer/bindings/core/v8/remote_window_proxy.cc

@@ -74,6 +74,7 @@ void RemoteWindowProxy::DisposeContext(Lifecycle next_status,
   if ((next_status == Lifecycle::kV8MemoryIsForciblyPurged ||
        next_status == Lifecycle::kGlobalObjectIsDetached) &&
       !global_proxy_.IsEmpty()) {
+    v8::HandleScope handle_scope(GetIsolate());
     global_proxy_.Get().SetWrapperClassId(0);
     V8DOMWrapper::ClearNativeInfo(GetIsolate(),
                                   global_proxy_.NewLocal(GetIsolate()));

third_party/blink/renderer/core/frame/frame.cc

@@ -143,25 +143,28 @@ bool Frame::Detach(FrameDetachType type) {
   // simplify this code.
   ScriptForbiddenScope forbid_scripts;
 
-  if (provisional_frame_) {
-    // This path should never be taken for a swap of any sort.
-    // 1. When swapping local->remote, that means the actual navigation is
-    //    committing in a different process. Thus, there should be no
-    //    provisional frame in this process, since there should only be one
-    //    provisional frame for a FrameTreeNode at any given time.
-    // 2. When swapping remote->local, that means the actual navigation is
-    //    committing in this frame tree. However, the committing frame should
-    //    have unset itself as the provisional frame in `Swap()` already, so
-    //    this should never be reached.
-    // 3. Similarly, when swapping local->local, the actual navigation is
-    //    committing in this frame tree, and the same logic applies except for
-    //    one edge case where RenderDocument is enabled and a JS unload handler
-    //    removes its own frame, causing it to be detached during a swap.
-    DCHECK_EQ(FrameDetachType::kRemove, type);
-    provisional_frame_->Detach(type);
+  if (type == FrameDetachType::kRemove) {
+    if (provisional_frame_) {
+      provisional_frame_->Detach(FrameDetachType::kRemove);
+    }
+    SetOpener(nullptr);
+    // Clearing the window proxies can call back into `LocalFrameClient`, so
+    // this must be done before nulling out `client_` below.
+    GetWindowProxyManager()->ClearForClose();
+  } else {
+    // In the case of a swap, detach is carefully coordinated with `Swap()`.
+    // Intentionally avoid clearing the opener with `SetOpener(nullptr)` here,
+    // since `Swap()` needs the original value to clone to the new frame.
+    DCHECK_EQ(FrameDetachType::kSwap, type);
+
+    // Clearing the window proxies can call back into `LocalFrameClient`, so
+    // this must be done before nulling out `client_` below.
+    // `ClearForSwap()` preserves the v8::Objects that represent the global
+    // proxies; `Swap()` will later use `ReleaseGlobalProxies()` +
+    // `SetGlobalProxies()` to adopt the global proxies into the new frame.
+    GetWindowProxyManager()->ClearForSwap();
   }
 
-  SetOpener(nullptr);
   // After this, we must no longer talk to the client since this clears
   // its owning reference back to our owning LocalFrame.
   client_->Detached(type);
@@ -538,7 +541,7 @@ Frame* Frame::Top() {
   return parent;
 }
 
-bool Frame::Swap(WebFrame* frame) {
+bool Frame::Swap(WebFrame* new_web_frame) {
   using std::swap;
   // TODO(dcheng): This should not be reachable. Reaching this implies `Swap()`
   // is being called on an already-detached frame which should never happen...
@@ -546,17 +549,30 @@ bool Frame::Swap(WebFrame* frame) {
     return false;
   // Important: do not cache frame tree pointers (e.g.  `previous_sibling_`,
   // `next_sibling_`, `first_child_`, `last_child_`) here. It is possible for
-  // `DetachDocument()` to mutate the frame tree and cause cached values to
-  // become invalid.
+  // `Detach()` to mutate the frame tree and cause cached values to become
+  // invalid.
   FrameOwner* owner = owner_;
   FrameSwapScope frame_swap_scope(owner);
   Page* page = page_;
   AtomicString name = Tree().GetName();
 
+  // TODO(dcheng): This probably isn't necessary if we fix the ordering of
+  // events in `Swap()`, e.g. `Detach()` should not happen before
+  // `new_web_frame` is swapped in.
+  // If there is a local parent, it might incorrectly declare itself complete
+  // during the detach phase of this swap. Suppress its completion until swap is
+  // over, at which point its completion will be correctly dependent on its
+  // newly swapped-in child.
+  auto* parent_local_frame = DynamicTo<LocalFrame>(parent_.Get());
+  std::unique_ptr<IncrementLoadEventDelayCount> delay_parent_load =
+      parent_local_frame ? std::make_unique<IncrementLoadEventDelayCount>(
+                               *parent_local_frame->GetDocument())
+                         : nullptr;
+
   // Unload the current Document in this frame: this calls unload handlers,
   // detaches child frames, etc. Since this runs script, make sure this frame
   // wasn't detached before continuing with the swap.
-  if (!DetachDocument()) {
+  if (!Detach(FrameDetachType::kSwap)) {
     // If the Swap() fails, it should be because the frame has been detached
     // already. Otherwise the caller will not detach the frame when we return
     // false, and the browser and renderer will disagree about the destruction
@@ -565,48 +581,26 @@ bool Frame::Swap(WebFrame* frame) {
     return false;
   }
 
+  // Otherwise, on a successful `Detach()` for swap, `this` is now detached--but
+  // crucially--still linked into the frame tree.
+
   if (provisional_frame_) {
     // `this` is about to be replaced, so if `provisional_frame_` is set, it
     // should match `frame` which is being swapped in.
-    DCHECK_EQ(provisional_frame_, WebFrame::ToCoreFrame(*frame));
+    DCHECK_EQ(provisional_frame_, WebFrame::ToCoreFrame(*new_web_frame));
     provisional_frame_ = nullptr;
   }
 
-  // TODO(dcheng): This probably isn't necessary if we fix the ordering of
-  // events in `Swap()`, e.g. `Detach()` should not happen before `new_frame` is
-  // swapped in.
-  // If there is a local parent, it might incorrectly declare itself complete
-  // during the detach phase of this swap. Suppress its completion until swap is
-  // over, at which point its completion will be correctly dependent on its
-  // newly swapped-in child.
-  auto* parent_local_frame = DynamicTo<LocalFrame>(parent_.Get());
-  std::unique_ptr<IncrementLoadEventDelayCount> delay_parent_load =
-      parent_local_frame ? std::make_unique<IncrementLoadEventDelayCount>(
-                               *parent_local_frame->GetDocument())
-                         : nullptr;
-
   v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
   WindowProxyManager::GlobalProxyVector global_proxies;
-  GetWindowProxyManager()->ClearForSwap();
   GetWindowProxyManager()->ReleaseGlobalProxies(global_proxies);
 
-  // This must be before Detach so DidChangeOpener is not called.
-  Frame* original_opener = opener_;
-  if (original_opener)
-    SetOpenerDoNotNotify(nullptr);
-
-  // Although the Document in this frame is now unloaded, many resources
-  // associated with the frame itself have not yet been freed yet. Note that
-  // after `Detach()` returns, `this` is detached but *not* yet unlinked from
-  // the frame tree.
-  // TODO(dcheng): Merge this into the `DetachDocument()` step above. Executing
-  // parts of `Detach()` twice is confusing.
-  Detach(FrameDetachType::kSwap);
-  if (frame->IsWebRemoteFrame()) {
-    CHECK(!WebFrame::ToCoreFrame(*frame));
-    To<WebRemoteFrameImpl>(frame)->InitializeCoreFrame(
-        *page, owner, WebFrame::FromCoreFrame(parent_), nullptr,
-        FrameInsertType::kInsertLater, name, &window_agent_factory());
+  if (new_web_frame->IsWebRemoteFrame()) {
+    CHECK(!WebFrame::ToCoreFrame(*new_web_frame));
+    To<WebRemoteFrameImpl>(new_web_frame)
+        ->InitializeCoreFrame(*page, owner, WebFrame::FromCoreFrame(parent_),
+                              nullptr, FrameInsertType::kInsertLater, name,
+                              &window_agent_factory());
     // At this point, a `RemoteFrame` will have already updated
     // `Page::MainFrame()` or `FrameOwner::ContentFrame()` as appropriate, and
     // its `parent_` pointer is also populated.
@@ -618,7 +612,7 @@ bool Frame::Swap(WebFrame* frame) {
     // TODO(dcheng): Make local and remote frame updates more uniform.
   }
 
-  Frame* new_frame = WebFrame::ToCoreFrame(*frame);
+  Frame* new_frame = WebFrame::ToCoreFrame(*new_web_frame);
   CHECK(new_frame);
 
   // At this point, `new_frame->parent_` is correctly set, but `new_frame`'s
@@ -648,8 +642,9 @@ bool Frame::Swap(WebFrame* frame) {
     parent_ = nullptr;
   }
 
-  if (original_opener) {
-    new_frame->SetOpenerDoNotNotify(original_opener);
+  if (Frame* opener = opener_) {
+    SetOpenerDoNotNotify(nullptr);
+    new_frame->SetOpenerDoNotNotify(opener);
   }
   opened_frame_tracker_.TransferTo(new_frame);
 

third_party/blink/renderer/core/frame/local_dom_window.cc

@@ -1068,14 +1068,8 @@ Element* LocalDOMWindow::frameElement() const {
 void LocalDOMWindow::blur() {}
 
 void LocalDOMWindow::print(ScriptState* script_state) {
-  // Don't print after detach begins, even if GetFrame() hasn't been nulled out
-  // yet.
-  // TODO(crbug.com/1063150): When a frame is being detached for a swap, the
-  // document has already been Shutdown() and is no longer in a consistent
-  // state, even though GetFrame() is not yet nulled out. This is an ordering
-  // violation, and checking whether we're in the middle of detach here is
-  // probably not the right long-term fix.
-  if (!GetFrame() || !GetFrame()->IsAttached())
+  // Don't try to print if there's no frame attached anymore.
+  if (!GetFrame())
     return;
 
   if (script_state &&

third_party/blink/renderer/core/frame/local_frame.cc

@@ -666,9 +666,6 @@ bool LocalFrame::DetachImpl(FrameDetachType type) {
 
   DCHECK(!view_->IsAttached());
   Client()->WillBeDetached();
-  // Notify WindowProxyManager that the frame is closing, since its cleanup ends
-  // up calling back to LocalFrameClient via WindowProxy.
-  GetWindowProxyManager()->ClearForClose();
 
   // TODO(crbug.com/729196): Trace why LocalFrameView::DetachFromLayout crashes.
   CHECK(!view_->IsAttached());

third_party/blink/renderer/core/frame/remote_frame.cc

@@ -234,7 +234,6 @@ bool RemoteFrame::DetachImpl(FrameDetachType type) {
   // the parent is a local frame.
   if (view_)
     view_->Dispose();
-  GetWindowProxyManager()->ClearForClose();
   SetView(nullptr);
   // ... the RemoteDOMWindow will need to be informed of detachment,
   // as otherwise it will keep a strong reference back to this RemoteFrame.