Refactor frame navigation/detach state cleanup to be more sane.

Source/core/loader/FrameLoader.cpp

Index: Source/core/loader/FrameLoader.cpp
diff --git a/Source/core/loader/FrameLoader.cpp b/Source/core/loader/FrameLoader.cpp
index 922fb8214a549fff262e326be04cf7f61258e42b..32729f6cd1a2f100b0d8963858c94a6e786e3dd4 100644
--- a/Source/core/loader/FrameLoader.cpp
+++ b/Source/core/loader/FrameLoader.cpp
@@ -242,7 +242,6 @@ void FrameLoader::clear()
 
     m_frame->editor().clear();
     m_frame->document()->cancelParsing();
-    m_frame->document()->prepareForDestruction();
     m_frame->document()->removeFocusedElementOfSubtree(m_frame->document());
     m_frame->selection().prepareForDestruction();
     m_frame->eventHandler().clear();
@@ -279,9 +278,11 @@ void FrameLoader::replaceDocumentWhileExecutingJavaScriptURL(const String& sourc
     init.withNewRegistrationContext();
 
     stopAllLoaders();
+    m_frame->detachChildren();
+    m_frame->document()->prepareForDestruction();
     clear();
 
-    // clear() potentially detaches the frame from the document. The
+    // detachChildren() potentially detaches the frame from the document. The
     // loading cannot continue in that case.
     if (!m_frame->page())
         return;
@@ -964,7 +965,14 @@ void FrameLoader::commitProvisionalLoad()
         client()->dispatchWillClose();
         dispatchUnloadEvent();
     }
-    m_frame->detachChildren();
+    if (m_frame->document()) {
+        m_frame->detachChildren();

[Nate Chapin, 2015/04/03 20:42:32]

It's clear to me why detachChildren() is a noop if m_frame->document() is null,
but it might(?) be more clear if this is outside the if().

[dcheng, 2015/04/03 20:47:20]

The idea is if there's no document, there should be no children to detach either
(because it wouldn't be possible to attach them to the DOM/active Document yet).
That being said, your comment below makes sense (and I added a brief comment
here as well).

+        m_frame->document()->prepareForDestruction();
+    } else {
+        // If there is no document, this is the creation of the initial empty
+        // doc, and there should be no child frames.
+        ASSERT(m_frame->tree().childCount() == 0);

[Nate Chapin, 2015/04/03 20:42:32]

Maybe drop this else{} clause and ASSERT this unconditionally? It should always
be true at this point.

[dcheng, 2015/04/03 20:47:20]

Done.

+    }
     if (pdl != m_provisionalDocumentLoader)
         return;
     if (m_documentLoader)