Detach all subframes before swapping a frame.

Source/web/WebFrame.cpp

Index: Source/web/WebFrame.cpp
diff --git a/Source/web/WebFrame.cpp b/Source/web/WebFrame.cpp
index b6832d30b735811ea7c4940a581f6364b0182073..ec68cbbddb21025e059e7f4cc9ef50e38fd06c05 100644
--- a/Source/web/WebFrame.cpp
+++ b/Source/web/WebFrame.cpp
@@ -29,8 +29,11 @@ void WebFrame::swap(WebFrame* frame)
 {
     using std::swap;
 
-    // All child frames must have been detached first.
-    ASSERT(!m_firstChild && !m_lastChild);
+    Frame* oldFrame = toCoreFrame(this);
+
+    // All child frames must be detached first.
+    oldFrame->detachChildren();

[kenrb, 2014/09/04 19:04:50]

I haven't spent time thinking about the right sequence of things here, and from
the link Daniel provided I see he has been thinking about it but hasn't gotten
to a firm conclusion, so I have some questions/comments.

For one, I suspect you need to RefPtr any WebCore objects you are referencing
here because detachChildren() will fire child frame unload handlers which have
arbitrary behavior.

My main question is where does the unload handler run for the old frame? If this
is a remote frame getting swapped for a local frame then the handler should run
in the other process, but what if it is a local frame getting swapped for a
remote frame? Child detachment is part of the unload process, which is usually
handled by FrameLoader, and I am not sure what the story is for keeping the
behavior consistent.

[dcheng, 2014/09/04 19:18:11]

Ken makes some good points. This reminds me of why I didn't implement this as
part of the original change. Down below, we reach into the Frame and pull out
some of its pointers (like the owner, frame host, name, etc). So we need to
figure out if its possible for this frame to be blown away from underneath
itself in swap()--and what we should do in that case.
Swap is part of frame tree mirroring, so the idea is any renderers that need to
be updated will get a swap() as appropriate and do the "right thing" for
detaching that subtree.

We already break the abstraction for swapping out render views; there's an
escape hatch in WebFrame to trigger FrameLoader's detach. Since we're moving the
detach functionality up into Frame, so it works for both local and remote
frames, it makes sense that we would start using Frame to break this
abstraction.

[nasko, 2014/09/04 23:46:28]

I've implemented an idea based on our discussion. If the frame itself is
detached while detaching its children, the method returns false to the embedder.
The embedder can assume the client object no longer exists, so it should be
returning immediately without executing more code.
Yes, the unload handler for the frame that is being swapped is already called by
the embedder. We should change that and hide this detail here, but it belongs to
a different CL.

+
     // The frame being swapped in should not have a Frame associated
     // with it yet.
     ASSERT(!toCoreFrame(frame));
@@ -66,7 +69,6 @@ void WebFrame::swap(WebFrame* frame)
     // the type of the passed in WebFrame.
     // FIXME: This is a bit clunky; this results in pointless decrements and
     // increments of connected subframes.
-    Frame* oldFrame = toCoreFrame(this);
     FrameOwner* owner = oldFrame->owner();
     oldFrame->disconnectOwnerElement();
     if (frame->isWebLocalFrame()) {