Bug 348156: Fix leaks by relying on cycle collector rather than calling UnbindFromTree on all nodes. r/sr=jst

content/base/public/nsIContent.h

@@ -62,8 +62,8 @@ class nsIDocShell;
 
 // IID for the nsIContent interface
 #define NS_ICONTENT_IID       \
-{ 0x36b375cb, 0xf01e, 0x4c18, \
+{ 0xe0c5d967, 0x2c15, 0x4097, \
-  { 0xbf, 0x9e, 0xba, 0xad, 0x77, 0x1d, 0xce, 0x22 } }
+  { 0xb0, 0xdc, 0x75, 0xa3, 0xa7, 0xfc, 0xcd, 0x1a } }
 
 // hack to make egcs / gcc 2.95.2 happy
 class nsIContent_base : public nsINode {
@@ -792,6 +792,12 @@ public:
    */
   virtual void UpdateEditableState();
 
+  /**
+   * Destroy this node and it's children. Ideally this shouldn't be needed
+   * but for now we need to do it to break cycles.
+   */
+  virtual void DestroyContent() = 0;
+
 #ifdef DEBUG
   /**
    * List the content (and anything it contains) out to the given

content/base/src/nsDocument.cpp

@@ -1059,6 +1059,11 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDocument)
+  // Tear down linkmap. This is a performance optimization so that we
+  // don't waste time removing links one by one as they are removed
+  // from the doc.
+  DestroyLinkMap();
+
   // Unlink the mChildren nsAttrAndChildArray.
   for (PRInt32 indx = PRInt32(tmp->mChildren.ChildCount()) - 1; 
        indx >= 0; --indx) {
@@ -2057,7 +2062,10 @@ SubDocClearEntry(PLDHashTable *table, PLDHashEntryHdr *entry)
   SubDocMapEntry *e = static_cast<SubDocMapEntry *>(entry);
 
   NS_RELEASE(e->mKey);
-  NS_IF_RELEASE(e->mSubDocument);
+  if (e->mSubDocument) {
+    e->mSubDocument->SetParentDocument(nsnull);
+    NS_RELEASE(e->mSubDocument);
+  }
 }
 
 PR_STATIC_CALLBACK(PRBool)
@@ -2090,8 +2098,6 @@ nsDocument::SetSubDocumentFor(nsIContent *aContent, nsIDocument* aSubDoc)
                                             PL_DHASH_LOOKUP));
 
       if (PL_DHASH_ENTRY_IS_BUSY(entry)) {
-        entry->mSubDocument->SetParentDocument(nsnull);
-
         PL_DHashTableRawRemove(mSubDocuments, entry);
       }
     }
@@ -5580,26 +5586,16 @@ nsDocument::Destroy()
   if (mIsGoingAway)
     return;
 
-  PRInt32 count = mChildren.ChildCount();
-
   mIsGoingAway = PR_TRUE;
-  DestroyLinkMap();
+
-  for (PRInt32 indx = 0; indx < count; ++indx) {
+  PRUint32 i, count = mChildren.ChildCount();
-    // XXXbz what we _should_ do here is to clear mChildren and null out
+  for (i = 0; i < count; ++i) {
-    // mRootContent.  If we did this (or at least the latter), we could remove
+    mChildren.ChildAt(i)->DestroyContent();
-    // 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();
   }
+
   mLayoutHistoryState = nsnull;
 
   nsContentList::OnDocumentDestroy(this);
-  delete mContentWrapperHash;
-  mContentWrapperHash = nsnull;
 }
 
 already_AddRefed<nsILayoutHistoryState>

content/base/src/nsGenericDOMDataNode.cpp

@@ -811,6 +811,10 @@ nsGenericDOMDataNode::IsNodeOfType(PRUint32 aFlags) const
   return !(aFlags & ~(eCONTENT | eDATA_NODE));
 }
 
+void
+nsGenericDOMDataNode::DestroyContent()
+{
+}
 
 #ifdef DEBUG
 void

content/base/src/nsGenericDOMDataNode.h

@@ -225,6 +225,7 @@ public:
                               PRBool aNotify);
   virtual PRBool TextIsOnlyWhitespace();
   virtual void AppendTextTo(nsAString& aResult);
+  virtual void DestroyContent();
 #ifdef DEBUG
   virtual void List(FILE* out, PRInt32 aIndent) const;
   virtual void DumpContent(FILE* out, PRInt32 aIndent, PRBool aDumpAll) const;

content/base/src/nsGenericElement.cpp

@@ -2857,6 +2857,22 @@ nsGenericElement::GetPrimaryFrameFor(nsIContent* aContent,
   return presShell->GetPrimaryFrameFor(aContent);
 }
 
+void
+nsGenericElement::DestroyContent()
+{
+  nsIDocument *document = GetOwnerDoc();
+  if (document) {
+    document->BindingManager()->ChangeDocumentFor(this, document, nsnull);
+    document->ClearBoxObjectFor(this);
+  }
+
+  PRUint32 i, count = mAttrsAndChildren.ChildCount();
+  for (i = 0; i < count; ++i) {
+    // The child can remove itself from the parent in BindToTree.
+    mAttrsAndChildren.ChildAt(i)->DestroyContent();
+  }
+}
+
 //----------------------------------------------------------------------
 
 // Generic DOMNode implementations
@@ -3340,7 +3356,7 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsGenericElement)
     PRUint32 i;
     PRUint32 kids = tmp->mAttrsAndChildren.ChildCount();
     for (i = kids; i > 0; i--) {
-      tmp->mAttrsAndChildren.ChildAt(i-1)->UnbindFromTree();
+      tmp->mAttrsAndChildren.ChildAt(i-1)->UnbindFromTree(PR_FALSE);
       tmp->mAttrsAndChildren.RemoveChildAt(i-1);    
     }
   }  
@@ -3348,8 +3364,11 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsGenericElement)
   // 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
 

content/base/src/nsGenericElement.h

@@ -434,6 +434,7 @@ public:
   virtual PRUint32 GetScriptTypeID() const;
   virtual nsresult SetScriptTypeID(PRUint32 aLang);
 
+  virtual void DestroyContent();
 #ifdef DEBUG
   virtual void List(FILE* out, PRInt32 aIndent) const
   {

content/html/content/src/nsGenericHTMLElement.cpp

@@ -3066,6 +3066,17 @@ nsGenericHTMLFrameElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
   return rv;
 }
 
+void
+nsGenericHTMLFrameElement::DestroyContent()
+{
+  if (mFrameLoader) {
+    mFrameLoader->Destroy();
+    mFrameLoader = nsnull;
+  }
+
+  nsGenericHTMLElement::DestroyContent();
+}
+
 //----------------------------------------------------------------------
 
 void

content/html/content/src/nsGenericHTMLElement.h

@@ -933,6 +933,7 @@ public:
   virtual nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                            nsIAtom* aPrefix, const nsAString& aValue,
                            PRBool aNotify);
+  virtual void DestroyContent();
 
   // nsIDOMNSHTMLElement 
   NS_IMETHOD GetTabIndex(PRInt32 *aTabIndex);

content/html/content/src/nsHTMLFormElement.cpp

@@ -323,7 +323,7 @@ protected:
   /** 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;
@@ -1103,10 +1103,12 @@ nsHTMLFormElement::SubmitSubmission(nsIFormSubmission* aFormSubmission)
     PRBool pending = PR_FALSE;
     mSubmittingRequest->IsPending(&pending);
     if (pending && !schemeIsJavaScript) {
-      mWebProgress = do_GetInterface(docShell);
+      nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(docShell);
-      NS_ASSERTION(mWebProgress, "nsIDocShell not converted to nsIWebProgress!");
+      NS_ASSERTION(webProgress, "nsIDocShell not converted to nsIWebProgress!");
-      rv = mWebProgress->AddProgressListener(this, nsIWebProgress::NOTIFY_STATE_ALL);
+      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();
     }
@@ -1723,10 +1725,11 @@ nsHTMLFormElement::ForgetCurrentSubmission()
   mNotifiedObservers = PR_FALSE;
   mIsSubmitting = PR_FALSE;
   mSubmittingRequest = nsnull;
-  if (mWebProgress) {
+  nsCOMPtr<nsIWebProgress> webProgress = do_QueryReferent(mWebProgress);
-    mWebProgress->RemoveProgressListener(this);
+  if (webProgress) {
-    mWebProgress = nsnull;
+    webProgress->RemoveProgressListener(this);
   }
+  mWebProgress = nsnull;
 }
 
 // nsIWebProgressListener

content/xtf/src/nsXTFElementWrapper.cpp

@@ -104,8 +104,15 @@ nsXTFElementWrapper::Init()
 //----------------------------------------------------------------------
 // nsISupports implementation
 
-NS_IMPL_ADDREF_INHERITED(nsXTFElementWrapper,nsXTFElementWrapperBase)
+NS_IMPL_ADDREF_INHERITED(nsXTFElementWrapper, nsXTFElementWrapperBase)
-NS_IMPL_RELEASE_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)

content/xtf/src/nsXTFElementWrapper.h

@@ -65,6 +65,8 @@ public:
 
   // nsISupports interface
   NS_DECL_ISUPPORTS_INHERITED
+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED_NO_UNLINK(nsXTFElementWrapper,
+                                                     nsXTFElementWrapperBase)
 
   // nsIXTFElementWrapper
   NS_DECL_NSIXTFELEMENTWRAPPER

content/xul/content/src/nsXULElement.cpp

@@ -1471,6 +1471,16 @@ nsXULElement::GetAttrCount() const
     return count;
 }
 
+void
+nsXULElement::DestroyContent()
+{
+    nsDOMSlots* slots = GetExistingDOMSlots();
+    if (slots) {
+      NS_IF_RELEASE(slots->mControllers);
+    }
+
+    nsGenericElement::DestroyContent();
+}
 
 #ifdef DEBUG
 void

content/xul/content/src/nsXULElement.h

@@ -568,6 +568,7 @@ public:
                                PRBool aNotify);
     virtual const nsAttrName* GetAttrNameAt(PRUint32 aIndex) const;
     virtual PRUint32 GetAttrCount() const;
+    virtual void DestroyContent();
 
 #ifdef DEBUG
     virtual void List(FILE* out, PRInt32 aIndent) const;