Bug 348156: Remove unbinds from nsDocument::Destroy as they cause badness. Break cycles using cycle collector instead. r/sr/a=jst
authorjonas@sicking.cc
Fri, 28 Sep 2007 16:57:00 -0700
changeset 6425 76cfc68c3bcc9b42f75d28e3d4498209e98cd5ba
parent 6424 c0b1cdb429aa1cbff9e88c63562b6f20b32a8600
child 6426 a3a889a4053613d700efc74acfd48b94725d3394
push idunknown
push userunknown
push dateunknown
bugs348156
milestone1.9a9pre
Bug 348156: Remove unbinds from nsDocument::Destroy as they cause badness. Break cycles using cycle collector instead. r/sr/a=jst
content/base/src/nsDocument.cpp
content/base/src/nsGenericElement.cpp
content/base/src/nsNodeUtils.cpp
content/base/src/nsNodeUtils.h
content/html/content/src/nsHTMLFormElement.cpp
content/xtf/src/nsXTFElementWrapper.cpp
content/xtf/src/nsXTFElementWrapper.h
--- a/content/base/src/nsDocument.cpp
+++ b/content/base/src/nsDocument.cpp
@@ -5531,36 +5531,25 @@ nsDocument::CanSavePresentation(nsIReque
 void
 nsDocument::Destroy()
 {
   // The ContentViewer wants to release the document now.  So, tell our content
   // to drop any references to the document so that it can be destroyed.
   if (mIsGoingAway)
     return;
 
-  PRInt32 count = mChildren.ChildCount();
-
   mIsGoingAway = PR_TRUE;
-  DestroyLinkMap();
-  for (PRInt32 indx = 0; indx < count; ++indx) {
-    // XXXbz what we _should_ do here is to clear mChildren and null out
-    // mRootContent.  If we did this (or at least the latter), we could remove
-    // the silly null-checks in nsHTMLDocument::MatchLinks.  Unfortunately,
-    // doing that introduces several problems:
-    // 1) Focus issues (see bug 341730).  The fix for bug 303260 may fix these.
-    // 2) Crashes in OnPageHide if it fires after Destroy.  See bug 303260
-    //    comments 9 and 10.
-    // So we're just creating an inconsistent DOM for now and hoping.  :(
-    mChildren.ChildAt(indx)->UnbindFromTree();
-  }
+  PRUint32 i, count = mChildren.ChildCount();
+  for (i = 0; i < count; ++i) {
+    nsNodeUtils::DestroySubtree(mChildren.ChildAt(i));
+  }
+
   mLayoutHistoryState = nsnull;
 
   nsContentList::OnDocumentDestroy(this);
-  delete mContentWrapperHash;
-  mContentWrapperHash = nsnull;
 }
 
 already_AddRefed<nsILayoutHistoryState>
 nsDocument::GetLayoutHistoryState() const
 {
   nsILayoutHistoryState* state = nsnull;
   if (!mScriptGlobalObject) {
     NS_IF_ADDREF(state = mLayoutHistoryState);
--- a/content/base/src/nsGenericElement.cpp
+++ b/content/base/src/nsGenericElement.cpp
@@ -3343,18 +3343,21 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ns
       tmp->mAttrsAndChildren.ChildAt(i-1)->UnbindFromTree();
       tmp->mAttrsAndChildren.RemoveChildAt(i-1);    
     }
   }  
 
   // Unlink any DOM slots of interest.
   {
     nsDOMSlots *slots = tmp->GetExistingDOMSlots();
-    if (slots)
+    if (slots) {
       slots->mAttributeMap = nsnull;
+      if (tmp->IsNodeOfType(nsINode::eXUL))
+        NS_IF_RELEASE(slots->mControllers);
+    }
   }
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsGenericElement)
   nsIDocument* currentDoc = tmp->GetCurrentDoc();
   if (currentDoc && nsCCUncollectableMarker::InGeneration(
                       currentDoc->GetMarkedCCGeneration())) {
     return NS_OK;
--- a/content/base/src/nsNodeUtils.cpp
+++ b/content/base/src/nsNodeUtils.cpp
@@ -682,8 +682,32 @@ nsNodeUtils::UnlinkUserData(nsINode *aNo
   // delete the document.
   nsCOMPtr<nsIDocument> document = aNode->GetOwnerDoc();
   if (document) {
     document->PropertyTable()->DeleteAllPropertiesFor(aNode, DOM_USER_DATA);
     document->PropertyTable()->DeleteAllPropertiesFor(aNode,
                                                       DOM_USER_DATA_HANDLER);
   }
 }
+
+/* static */
+void
+nsNodeUtils::DestroySubtree(nsIContent* aRoot)
+{
+  nsXULElement* xul = nsXULElement::FromContent(aRoot);
+  if (xul) {
+    nsGenericElement::nsDOMSlots* slots = xul->GetExistingDOMSlots();
+    if (slots) {
+      NS_IF_RELEASE(slots->mControllers);
+    }
+  }
+
+  nsIDocument *document = aRoot->GetOwnerDoc();
+  if (document) {
+    document->BindingManager()->ChangeDocumentFor(aRoot, document, nsnull);
+    document->ClearBoxObjectFor(aRoot);
+  }
+
+  PRUint32 i, count = aRoot->GetChildCount();
+  for (i = 0; i < count; ++i) {
+    DestroySubtree(aRoot->GetChildAt(i));
+  }
+}
--- a/content/base/src/nsNodeUtils.h
+++ b/content/base/src/nsNodeUtils.h
@@ -264,16 +264,24 @@ public:
 
   /**
    * Release the UserData and UserDataHandlers for aNode.
    *
    * @param aNode the node to release the UserData and UserDataHandlers for
    */
   static void UnlinkUserData(nsINode *aNode);
 
+  /**
+   * Remove neccesary components of all nodes in a subtree to avoid leaking.
+   * So far this removes XBL bindings and XUL controllers.
+   *
+   * @param aRoot the node that is the root of the subtree to clear.
+   */
+  static void DestroySubtree(nsIContent* aRoot);
+
 private:
   friend PLDHashOperator PR_CALLBACK
     AdoptFunc(nsAttrHashKey::KeyType aKey, nsIDOMNode *aData, void* aUserArg);
 
   /**
    * Walks aNode, its attributes and, if aDeep is PR_TRUE, its descendant nodes.
    * If aClone is PR_TRUE the nodes will be cloned. If aNewNodeInfoManager is
    * not null, it is used to create new nodeinfos for the nodes. Also reparents
--- a/content/html/content/src/nsHTMLFormElement.cpp
+++ b/content/html/content/src/nsHTMLFormElement.cpp
@@ -318,17 +318,17 @@ protected:
   /** Keep track of whether a submission was user-initiated or not */
   PRBool mSubmitInitiatedFromUserInput;
 
   /** The pending submission object */
   nsCOMPtr<nsIFormSubmission> mPendingSubmission;
   /** The request currently being submitted */
   nsCOMPtr<nsIRequest> mSubmittingRequest;
   /** The web progress object we are currently listening to */
-  nsCOMPtr<nsIWebProgress> mWebProgress;
+  nsWeakPtr mWebProgress;
 
   /** The default submit element -- WEAK */
   nsIFormControl* mDefaultSubmitElement;
 
   /** The first submit element in mElements -- WEAK */
   nsIFormControl* mFirstSubmitInElements;
 
   /** The first submit element in mNotInElements -- WEAK */
@@ -1020,20 +1020,22 @@ nsHTMLFormElement::SubmitSubmission(nsIF
   // Even if the submit succeeds, it's possible for there to be no docshell
   // or request; for example, if it's to a named anchor within the same page
   // the submit will not really do anything.
   if (docShell) {
     // If the channel is pending, we have to listen for web progress.
     PRBool pending = PR_FALSE;
     mSubmittingRequest->IsPending(&pending);
     if (pending && !schemeIsJavaScript) {
-      mWebProgress = do_GetInterface(docShell);
-      NS_ASSERTION(mWebProgress, "nsIDocShell not converted to nsIWebProgress!");
-      rv = mWebProgress->AddProgressListener(this, nsIWebProgress::NOTIFY_STATE_ALL);
+      nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(docShell);
+      NS_ASSERTION(webProgress, "nsIDocShell not converted to nsIWebProgress!");
+      rv = webProgress->AddProgressListener(this, nsIWebProgress::NOTIFY_STATE_ALL);
       NS_ENSURE_SUBMIT_SUCCESS(rv);
+      mWebProgress = do_GetWeakReference(webProgress);
+      NS_ASSERTION(mWebProgress, "can't hold weak ref to webprogress!");
     } else {
       ForgetCurrentSubmission();
     }
   } else {
     ForgetCurrentSubmission();
   }
 
   return rv;
@@ -1640,20 +1642,21 @@ nsHTMLFormElement::GetLength(PRInt32* aL
 }
 
 void
 nsHTMLFormElement::ForgetCurrentSubmission()
 {
   mNotifiedObservers = PR_FALSE;
   mIsSubmitting = PR_FALSE;
   mSubmittingRequest = nsnull;
-  if (mWebProgress) {
-    mWebProgress->RemoveProgressListener(this);
-    mWebProgress = nsnull;
+  nsCOMPtr<nsIWebProgress> webProgress = do_QueryReferent(mWebProgress);
+  if (webProgress) {
+    webProgress->RemoveProgressListener(this);
   }
+  mWebProgress = nsnull;
 }
 
 // nsIWebProgressListener
 NS_IMETHODIMP
 nsHTMLFormElement::OnStateChange(nsIWebProgress* aWebProgress,
                                  nsIRequest* aRequest,
                                  PRUint32 aStateFlags,
                                  PRUint32 aStatus)
--- a/content/xtf/src/nsXTFElementWrapper.cpp
+++ b/content/xtf/src/nsXTFElementWrapper.cpp
@@ -99,18 +99,25 @@ nsXTFElementWrapper::Init()
   if (innerHandlesAttribs)
     mAttributeHandler = do_QueryInterface(GetXTFElement());
   return NS_OK;
 }
 
 //----------------------------------------------------------------------
 // nsISupports implementation
 
-NS_IMPL_ADDREF_INHERITED(nsXTFElementWrapper,nsXTFElementWrapperBase)
-NS_IMPL_RELEASE_INHERITED(nsXTFElementWrapper,nsXTFElementWrapperBase)
+NS_IMPL_ADDREF_INHERITED(nsXTFElementWrapper, nsXTFElementWrapperBase)
+NS_IMPL_RELEASE_INHERITED(nsXTFElementWrapper, nsXTFElementWrapperBase)
+
+NS_IMPL_CYCLE_COLLECTION_CLASS(nsXTFElementWrapper)
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsXTFElementWrapper,
+                                                  nsXTFElementWrapperBase)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mXTFElement)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mAttributeHandler)
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 NS_IMETHODIMP
 nsXTFElementWrapper::QueryInterface(REFNSIID aIID, void** aInstancePtr)
 {
   NS_PRECONDITION(aInstancePtr, "null out param");
 
   if (aIID.Equals(NS_GET_IID(nsIClassInfo))) {
     *aInstancePtr = static_cast<nsIClassInfo*>(this);
--- a/content/xtf/src/nsXTFElementWrapper.h
+++ b/content/xtf/src/nsXTFElementWrapper.h
@@ -60,16 +60,18 @@ public:
   nsXTFElementWrapper(nsINodeInfo* aNodeInfo, nsIXTFElement* aXTFElement);
   virtual ~nsXTFElementWrapper();
   nsresult Init();
 
   NS_DECLARE_STATIC_IID_ACCESSOR(NS_XTFELEMENTWRAPPER_IID)
 
   // nsISupports interface
   NS_DECL_ISUPPORTS_INHERITED
+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED_NO_UNLINK(nsXTFElementWrapper,
+                                                     nsXTFElementWrapperBase)
 
   // nsIXTFElementWrapper
   NS_DECL_NSIXTFELEMENTWRAPPER
     
   // nsIContent specializations:
   virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                               nsIContent* aBindingParent,
                               PRBool aCompileEventHandlers);