From 9d0c915718d3cff9622996c175543b64c55ad662 Mon Sep 17 00:00:00 2001 From: "jonas@sicking.cc" Date: Mon, 18 Jun 2007 15:27:27 -0700 Subject: [PATCH] Bug 348156: Don't rely on UnbindFromTree to break cycles since that puts us in an inconsistent state. r/sr=jst --- content/base/src/nsDocument.cpp | 19 ++++----------- content/base/src/nsGenericElement.cpp | 5 +++- content/base/src/nsNodeUtils.cpp | 24 +++++++++++++++++++ content/base/src/nsNodeUtils.h | 8 +++++++ .../html/content/src/nsHTMLFormElement.cpp | 17 +++++++------ content/xtf/src/nsXTFElementWrapper.cpp | 11 +++++++-- content/xtf/src/nsXTFElementWrapper.h | 2 ++ 7 files changed, 62 insertions(+), 24 deletions(-) diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp index 61e40db7edcc..f18550b5581d 100644 --- a/content/base/src/nsDocument.cpp +++ b/content/base/src/nsDocument.cpp @@ -5433,26 +5433,17 @@ nsDocument::Destroy() 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 diff --git a/content/base/src/nsGenericElement.cpp b/content/base/src/nsGenericElement.cpp index 53a2f2478245..5fe954f0b99e 100644 --- a/content/base/src/nsGenericElement.cpp +++ b/content/base/src/nsGenericElement.cpp @@ -3271,8 +3271,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 diff --git a/content/base/src/nsNodeUtils.cpp b/content/base/src/nsNodeUtils.cpp index 7d0a9cfe73f0..b3b43abae1aa 100755 --- a/content/base/src/nsNodeUtils.cpp +++ b/content/base/src/nsNodeUtils.cpp @@ -677,3 +677,27 @@ nsNodeUtils::UnlinkUserData(nsINode *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)); + } +} diff --git a/content/base/src/nsNodeUtils.h b/content/base/src/nsNodeUtils.h index c43a0f0e96ec..ac6756b0e555 100755 --- a/content/base/src/nsNodeUtils.h +++ b/content/base/src/nsNodeUtils.h @@ -258,6 +258,14 @@ public: */ 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); diff --git a/content/html/content/src/nsHTMLFormElement.cpp b/content/html/content/src/nsHTMLFormElement.cpp index 84230618c703..c1cd36bea8e2 100644 --- a/content/html/content/src/nsHTMLFormElement.cpp +++ b/content/html/content/src/nsHTMLFormElement.cpp @@ -363,7 +363,7 @@ protected: /** The request currently being submitted */ nsCOMPtr mSubmittingRequest; /** The web progress object we are currently listening to */ - nsCOMPtr mWebProgress; + nsWeakPtr mWebProgress; /** The default submit element -- WEAK */ nsIFormControl* mDefaultSubmitElement; @@ -1057,10 +1057,12 @@ nsHTMLFormElement::SubmitSubmission(nsIFormSubmission* aFormSubmission) 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 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(); } @@ -1632,10 +1634,11 @@ nsHTMLFormElement::ForgetCurrentSubmission() mNotifiedObservers = PR_FALSE; mIsSubmitting = PR_FALSE; mSubmittingRequest = nsnull; - if (mWebProgress) { - mWebProgress->RemoveProgressListener(this); - mWebProgress = nsnull; + nsCOMPtr webProgress = do_QueryReferent(mWebProgress); + if (webProgress) { + webProgress->RemoveProgressListener(this); } + mWebProgress = nsnull; } // nsIWebProgressListener diff --git a/content/xtf/src/nsXTFElementWrapper.cpp b/content/xtf/src/nsXTFElementWrapper.cpp index 4349350b8371..010ee36d39b8 100644 --- a/content/xtf/src/nsXTFElementWrapper.cpp +++ b/content/xtf/src/nsXTFElementWrapper.cpp @@ -104,8 +104,15 @@ nsXTFElementWrapper::Init() //---------------------------------------------------------------------- // 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) diff --git a/content/xtf/src/nsXTFElementWrapper.h b/content/xtf/src/nsXTFElementWrapper.h index b85fae777363..556d633d727f 100644 --- a/content/xtf/src/nsXTFElementWrapper.h +++ b/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