Invert the owning relationship between WebFrame and Frame.

Source/core/loader/FrameLoader.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2008 Alp Toker <alp@atoker.com>
  * Copyright (C) Research In Motion Limited 2009. All rights reserved.
  * Copyright (C) 2011 Kris Jordan <krisjordan@gmail.com>
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ skipping 141 matching lines @@
     , m_shouldCallCheckCompleted(false)
     , m_opener(0)
     , m_didAccessInitialDocument(false)
     , m_didAccessInitialDocumentTimer(this, &FrameLoader::didAccessInitialDocumentTimerFired)
     , m_forcedSandboxFlags(SandboxNone)
 {
 }
 
 FrameLoader::~FrameLoader()
 {
-    setOpener(0);
-
     HashSet<Frame*>::iterator end = m_openedFrames.end();
     for (HashSet<Frame*>::iterator it = m_openedFrames.begin(); it != end; ++it)
         (*it)->loader().m_opener = 0;
-
-    m_client->frameLoaderDestroyed();
 }
 
 void FrameLoader::init()
 {
     // This somewhat odd set of steps gives the frame an initial empty document.
     m_provisionalDocumentLoader = m_client->createDocumentLoader(ResourceRequest(KURL(ParsedURLString, emptyString())), SubstituteData());
     m_provisionalDocumentLoader->setFrame(m_frame);
     m_provisionalDocumentLoader->startLoadingMainResource();
     m_frame->document()->cancelParsing();
     m_stateMachine.advanceTo(FrameLoaderStateMachine::DisplayingInitialEmptyDocument);
@@ skipping 232 matching lines @@
 void FrameLoader::finishedParsing()
 {
     if (m_stateMachine.creatingInitialEmptyDocument())
         return;
 
     // This can be called from the Frame's destructor, in which case we shouldn't protect ourselves
     // because doing so will cause us to re-enter the destructor when protector goes out of scope.
     // Null-checking the FrameView indicates whether or not we're in the destructor.
     RefPtr<Frame> protector = m_frame->view() ? m_frame : 0;
 
-    m_client->dispatchDidFinishDocumentLoad();
+    if (m_client)
+        m_client->dispatchDidFinishDocumentLoad();
 
     checkCompleted();
 
     if (!m_frame->view())
         return; // We are being destroyed by something checkCompleted called.
 
     // Check if the scrollbars are really needed for the content.
     // If not, remove them, relayout, and repaint.
     m_frame->view()->restoreScrollbar();
     scrollToFragmentWithParentBoundary(m_frame->document()->url());
@@ skipping 384 matching lines @@
         m_documentLoader->stopLoading();
 
     if (m_provisionalDocumentLoader)
         m_provisionalDocumentLoader->detachFromFrame();
     m_provisionalDocumentLoader = 0;
 
     m_checkTimer.stop();
 
     m_inStopAllLoaders = false;
 
-    m_client->didStopAllLoaders();
+    // detachFromParent() can be called multiple times on same Frame, which
+    // means we may no longer have a FrameLoaderClient to talk to.
+    if (m_client)
+        m_client->didStopAllLoaders();
 }
 
 DocumentLoader* FrameLoader::activeDocumentLoader() const
 {
     if (m_state == FrameStateProvisional)
         return m_provisionalDocumentLoader.get();
     return m_documentLoader.get();
 }
 
 void FrameLoader::didAccessInitialDocument()
@@ skipping 281 matching lines @@
     // stopAllLoaders() needs to be called after detachChildren(), because detachedChildren()
     // will trigger the unload event handlers of any child frames, and those event
     // handlers might start a new subresource load in this frame.
     stopAllLoaders();
 
     InspectorInstrumentation::frameDetachedFromParent(m_frame);
 
     if (m_documentLoader)
         m_documentLoader->detachFromFrame();
     m_documentLoader = 0;
-    m_client->detachedFromParent();
 
     m_progressTracker.clear();
 
+    if (m_client)
+        m_client->willDetachParent();
+
     // FIXME: All this code belongs up in Page.
     if (Frame* parent = m_frame->tree().parent()) {
         parent->loader().closeAndRemoveChild(m_frame);
         parent->loader().scheduleCheckCompleted();
     } else {
         m_frame->setView(0);
         m_frame->willDetachFrameHost();
         m_frame->detachFromFrameHost();
     }
+
+    if (m_client) {
+        setOpener(0);
+
+        // Close down the proxy. The purpose of this change is to make the call to

[eseidel, 2013/12/28 01:18:40]

I think you mean "call" instead of change.

[dcheng, 2013/12/28 01:51:05]

I don't have any clue what the original author meant (this was moved verbatim
from FrameLoaderClientImpl).

+        // ScriptController::clearWindowShell a no-op when called from Frame::pageDestroyed.
+        // Without this change, this call to clearWindowShell will cause a crash. If you

[eseidel, 2013/12/28 01:18:40]

clearWindowShell is called by clearForClose? or did you mean clearForClose?

[dcheng, 2013/12/28 01:51:05]

This comment seems to have suffered from significant bitrot. For example, there
is no such thing as Frame::pageDestroyed() anymore. I've rewritten it so
hopefully it makes sense now.

+        // remove/modify this, just ensure that you can go to a page and then navigate to a new
+        // page without getting any asserts or crashes.
+        m_frame->script().clearForClose();
+
+        // After this, we must no longer talk to the client since this clears
+        // its owning reference back to us.
+        m_client->detachedFromParent();
+        m_client = 0;
+    }
 }
 
 void FrameLoader::addHTTPOriginIfNeeded(ResourceRequest& request, const AtomicString& origin)
 {
     if (!request.httpOrigin().isEmpty())
         return;  // Request already has an Origin header.
 
     // Don't send an Origin header for GET or HEAD to avoid privacy issues.
     // For example, if an intranet page has a hyperlink to an external web
     // site, we don't want to include the Origin of the request because it
@@ skipping 132 matching lines @@
         return;
 
     // We skip dispatching the beforeload event on the frame owner if we've already committed a real
     // document load because the event would leak subsequent activity by the frame which the parent
     // frame isn't supposed to learn. For example, if the child frame navigated to  a new URL, the
     // parent frame shouldn't learn the URL.
     const ResourceRequest& request = action.resourceRequest();
     if (!m_stateMachine.committedFirstRealDocumentLoad() && m_frame->ownerElement() && !m_frame->ownerElement()->dispatchBeforeLoadEvent(request.url().string()))
         return;
 
+    // Dispatching the beforeload event could have blown away the frame.
+    if (!m_client)
+        return;
+
     if (!m_stateMachine.startedFirstRealLoad())
         m_stateMachine.advanceTo(FrameLoaderStateMachine::StartedFirstRealLoad);
 
     // The current load should replace the history item if it is the first real
     // load of the frame.
     bool replacesCurrentHistoryItem = false;
     if (type == FrameLoadTypeRedirectWithLockedBackForwardList
         || !m_stateMachine.committedFirstRealDocumentLoad()) {
         replacesCurrentHistoryItem = true;
     }
@@ skipping 199 matching lines @@
 {
     SandboxFlags flags = m_forcedSandboxFlags;
     if (Frame* parentFrame = m_frame->tree().parent())
         flags |= parentFrame->document()->sandboxFlags();
     if (HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement())
         flags |= ownerElement->sandboxFlags();
     return flags;
 }
 
 } // namespace WebCore