Bug 348156: Fix leaks by relying on cycle collector rather than calling UnbindFromTree on all nodes. r/sr=jst
authorjonas@sicking.cc
Thu, 29 Nov 2007 00:41:25 -0800
changeset 8440 669715ceed7a7b526d793f1d33b310595f7aeef6
parent 8439 f505ab28638e774fb018f9ec862afb74008944b3
child 8441 5b06c1dd0cf0ae18937be323450057f920ce683f
push idunknown
push userunknown
push dateunknown
bugs348156
milestone1.9b2pre
Bug 348156: Fix leaks by relying on cycle collector rather than calling UnbindFromTree on all nodes. r/sr=jst
content/base/public/nsIContent.h
content/base/src/nsDocument.cpp
content/base/src/nsGenericDOMDataNode.cpp
content/base/src/nsGenericDOMDataNode.h
content/base/src/nsGenericElement.cpp
content/base/src/nsGenericElement.h
content/html/content/src/nsGenericHTMLElement.cpp
content/html/content/src/nsGenericHTMLElement.h
content/html/content/src/nsHTMLFormElement.cpp
content/xtf/src/nsXTFElementWrapper.cpp
content/xtf/src/nsXTFElementWrapper.h
content/xul/content/src/nsXULElement.cpp
content/xul/content/src/nsXULElement.h
--- a/content/base/public/nsIContent.h
+++ b/content/base/public/nsIContent.h
@@ -57,18 +57,18 @@ class nsICSSStyleRule;
 class nsRuleWalker;
 class nsAttrValue;
 class nsAttrName;
 class nsTextFragment;
 class nsIDocShell;
 
 // IID for the nsIContent interface
 #define NS_ICONTENT_IID       \
-{ 0x36b375cb, 0xf01e, 0x4c18, \
-  { 0xbf, 0x9e, 0xba, 0xad, 0x77, 0x1d, 0xce, 0x22 } }
+{ 0xe0c5d967, 0x2c15, 0x4097, \
+  { 0xb0, 0xdc, 0x75, 0xa3, 0xa7, 0xfc, 0xcd, 0x1a } }
 
 // hack to make egcs / gcc 2.95.2 happy
 class nsIContent_base : public nsINode {
 public:
 #ifdef MOZILLA_INTERNAL_API
   // If you're using the external API, the only thing you can know about
   // nsIContent is that it exists with an IID
 
@@ -787,16 +787,22 @@ public:
 
   /**
    * Should be called when the node can become editable or when it can stop
    * being editable (for example when its contentEditable attribute changes,
    * when it is moved into an editable parent, ...).
    */
   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
    * file stream. Use aIndent as the base indent during formatting.
    */
   virtual void List(FILE* out = stdout, PRInt32 aIndent = 0) const = 0;
 
   /**
--- a/content/base/src/nsDocument.cpp
+++ b/content/base/src/nsDocument.cpp
@@ -1054,16 +1054,21 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(
 
   if (tmp->mSubDocuments && tmp->mSubDocuments->ops) {
     PL_DHashTableEnumerate(tmp->mSubDocuments, SubDocTraverser, &cb);
   }
 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) {
     tmp->mChildren.ChildAt(indx)->UnbindFromTree();
     tmp->mChildren.RemoveChildAt(indx);
   }
 
   NS_IMPL_CYCLE_COLLECTION_UNLINK_USERDATA
@@ -2052,17 +2057,20 @@ nsDocument::GetPrimaryShell() const
 }
 
 PR_STATIC_CALLBACK(void)
 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)
 SubDocInitEntry(PLDHashTable *table, PLDHashEntryHdr *entry, const void *key)
 {
   SubDocMapEntry *e =
     const_cast<SubDocMapEntry *>
               (static_cast<const SubDocMapEntry *>(entry));
@@ -2085,18 +2093,16 @@ nsDocument::SetSubDocumentFor(nsIContent
 
     if (mSubDocuments) {
       SubDocMapEntry *entry =
         static_cast<SubDocMapEntry*>
                    (PL_DHashTableOperate(mSubDocuments, aContent,
                                             PL_DHASH_LOOKUP));
 
       if (PL_DHASH_ENTRY_IS_BUSY(entry)) {
-        entry->mSubDocument->SetParentDocument(nsnull);
-
         PL_DHashTableRawRemove(mSubDocuments, entry);
       }
     }
   } else {
     if (!mSubDocuments) {
       // Create a new hashtable
 
       static PLDHashTableOps hash_table_ops =
@@ -5575,36 +5581,26 @@ 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) {
+    mChildren.ChildAt(i)->DestroyContent();
+  }
+
   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/nsGenericDOMDataNode.cpp
+++ b/content/base/src/nsGenericDOMDataNode.cpp
@@ -806,16 +806,20 @@ nsGenericDOMDataNode::GetBindingParent()
 }
 
 PRBool
 nsGenericDOMDataNode::IsNodeOfType(PRUint32 aFlags) const
 {
   return !(aFlags & ~(eCONTENT | eDATA_NODE));
 }
 
+void
+nsGenericDOMDataNode::DestroyContent()
+{
+}
 
 #ifdef DEBUG
 void
 nsGenericDOMDataNode::List(FILE* out, PRInt32 aIndent) const
 {
 }
 
 void
--- a/content/base/src/nsGenericDOMDataNode.h
+++ b/content/base/src/nsGenericDOMDataNode.h
@@ -220,16 +220,17 @@ public:
   nsresult SetText(const nsAString& aStr, PRBool aNotify)
   {
     return SetText(aStr.BeginReading(), aStr.Length(), aNotify);
   }
   virtual nsresult AppendText(const PRUnichar* aBuffer, PRUint32 aLength,
                               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;
 #endif
 
   virtual nsIContent *GetBindingParent() const;
   virtual PRBool IsNodeOfType(PRUint32 aFlags) const;
 
--- a/content/base/src/nsGenericElement.cpp
+++ b/content/base/src/nsGenericElement.cpp
@@ -2852,16 +2852,32 @@ nsGenericElement::GetPrimaryFrameFor(nsI
   nsIPresShell *presShell = aDocument->GetPrimaryShell();
   if (!presShell) {
     return nsnull;
   }
 
   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
 
 /*
  * This helper function checks if aChild is the same as aNode or if
  * aChild is one of aNode's ancestors. -- jst@citec.fi
  */
@@ -3335,26 +3351,29 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ns
   NS_IMPL_CYCLE_COLLECTION_UNLINK_USERDATA
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
 
   // Unlink child content (and unbind our subtree).
   {
     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);    
     }
   }  
 
   // 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/nsGenericElement.h
+++ b/content/base/src/nsGenericElement.h
@@ -429,16 +429,17 @@ public:
   virtual already_AddRefed<nsIURI> GetBaseURI() const;
   virtual PRBool IsLink(nsIURI** aURI) const;
   virtual void SetMayHaveFrame(PRBool aMayHaveFrame);
   virtual PRBool MayHaveFrame() const;
 
   virtual PRUint32 GetScriptTypeID() const;
   virtual nsresult SetScriptTypeID(PRUint32 aLang);
 
+  virtual void DestroyContent();
 #ifdef DEBUG
   virtual void List(FILE* out, PRInt32 aIndent) const
   {
     List(out, aIndent, EmptyCString());
   }
   virtual void DumpContent(FILE* out, PRInt32 aIndent, PRBool aDumpAll) const;
   void List(FILE* out, PRInt32 aIndent, const nsCString& aPrefix) const;
   void ListAttributes(FILE* out) const;
--- a/content/html/content/src/nsGenericHTMLElement.cpp
+++ b/content/html/content/src/nsGenericHTMLElement.cpp
@@ -3061,16 +3061,27 @@ nsGenericHTMLFrameElement::SetAttr(PRInt
   if (NS_SUCCEEDED(rv) && aNameSpaceID == kNameSpaceID_None &&
       aName == nsGkAtoms::src) {
     return LoadSrc();
   }
 
   return rv;
 }
 
+void
+nsGenericHTMLFrameElement::DestroyContent()
+{
+  if (mFrameLoader) {
+    mFrameLoader->Destroy();
+    mFrameLoader = nsnull;
+  }
+
+  nsGenericHTMLElement::DestroyContent();
+}
+
 //----------------------------------------------------------------------
 
 void
 nsGenericHTMLElement::SetElementFocus(PRBool aDoFocus)
 {
   nsCOMPtr<nsPresContext> presContext = GetPresContext();
   if (!presContext)
     return;
--- a/content/html/content/src/nsGenericHTMLElement.h
+++ b/content/html/content/src/nsGenericHTMLElement.h
@@ -928,16 +928,17 @@ public:
   nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                    const nsAString& aValue, PRBool aNotify)
   {
     return SetAttr(aNameSpaceID, aName, nsnull, aValue, aNotify);
   }
   virtual nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                            nsIAtom* aPrefix, const nsAString& aValue,
                            PRBool aNotify);
+  virtual void DestroyContent();
 
   // nsIDOMNSHTMLElement 
   NS_IMETHOD GetTabIndex(PRInt32 *aTabIndex);
   NS_IMETHOD SetTabIndex(PRInt32 aTabIndex);
 
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED_NO_UNLINK(nsGenericHTMLFrameElement,
                                                      nsGenericHTMLElement)
 
--- 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 */
@@ -1098,20 +1098,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;
@@ -1718,20 +1720,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);
--- a/content/xul/content/src/nsXULElement.cpp
+++ b/content/xul/content/src/nsXULElement.cpp
@@ -1466,16 +1466,26 @@ nsXULElement::GetAttrCount() const
                 break;
             }
         }
     }
 
     return count;
 }
 
+void
+nsXULElement::DestroyContent()
+{
+    nsDOMSlots* slots = GetExistingDOMSlots();
+    if (slots) {
+      NS_IF_RELEASE(slots->mControllers);
+    }
+
+    nsGenericElement::DestroyContent();
+}
 
 #ifdef DEBUG
 void
 nsXULElement::List(FILE* out, PRInt32 aIndent) const
 {
     nsCString prefix("<XUL");
     if (HasSlots()) {
       prefix.Append('*');
--- a/content/xul/content/src/nsXULElement.h
+++ b/content/xul/content/src/nsXULElement.h
@@ -563,16 +563,17 @@ public:
     virtual PRInt32 FindAttrValueIn(PRInt32 aNameSpaceID,
                                     nsIAtom* aName,
                                     AttrValuesArray* aValues,
                                     nsCaseTreatment aCaseSensitive) const;
     virtual nsresult UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                                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;
     virtual void DumpContent(FILE* out, PRInt32 aIndent,PRBool aDumpAll) const
     {
     }
 #endif