From ea1afb36a54362db3b608fd9188a6a2d4323e7c8 Mon Sep 17 00:00:00 2001 From: "jonas@sicking.cc" Date: Tue, 15 May 2007 18:13:47 -0700 Subject: [PATCH] Bug 53901: Don't set is-in-doc flag on cloned XUL nodes. r/sr=jst --- content/base/public/nsINode.h | 7 +- content/base/src/nsGenericDOMDataNode.cpp | 9 +- content/base/src/nsGenericElement.cpp | 76 ++++++---- content/base/src/nsNodeUtils.cpp | 10 +- content/xbl/src/nsXBLService.cpp | 8 +- content/xul/content/src/nsXULElement.cpp | 172 ---------------------- content/xul/content/src/nsXULElement.h | 14 +- dom/src/base/nsDOMClassInfo.cpp | 8 +- 8 files changed, 76 insertions(+), 228 deletions(-) diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h index 4c6bf05dcb7..141574ebf39 100644 --- a/content/base/public/nsINode.h +++ b/content/base/public/nsINode.h @@ -82,10 +82,9 @@ class nsNodeSupportsWeakRefTearoff; // NOTE: Should only be used on nsIContent nodes #define NODE_MAY_HAVE_FRAME 0x00000020U -// Whether the 'in doc' flag is faked to true for -// this node. This should only ever be set on XUL -// elements. -#define NODE_HAS_FAKED_INDOC 0x00000040U +// Forces the XBL code to treat this node as if it was +// in the document and therefor should get bindings attached. +#define NODE_FORCE_XBL_BINDINGS 0x00000040U // Whether a binding manager may have a pointer to this #define NODE_MAY_BE_IN_BINDING_MNGR 0x00000080U diff --git a/content/base/src/nsGenericDOMDataNode.cpp b/content/base/src/nsGenericDOMDataNode.cpp index 154a38a67c9..9741254b74c 100644 --- a/content/base/src/nsGenericDOMDataNode.cpp +++ b/content/base/src/nsGenericDOMDataNode.cpp @@ -547,14 +547,7 @@ nsGenericDOMDataNode::BindToTree(nsIDocument* aDocument, nsIContent* aParent, NS_PRECONDITION(aParent || aDocument, "Must have document if no parent!"); NS_PRECONDITION(HasSameOwnerDoc(NODE_FROM(aParent, aDocument)), "Must have the same owner document"); - // XXXbz XUL elements are confused about their current doc when they're - // cloned, so we don't assert if aParent is a XUL element and aDocument is - // null, even if aParent->GetCurrentDoc() is non-null - // NS_PRECONDITION(!aParent || aDocument == aParent->GetCurrentDoc(), - // "aDocument must be current doc of aParent"); - NS_PRECONDITION(!aParent || - (aParent->IsNodeOfType(eXUL) && aDocument == nsnull) || - aDocument == aParent->GetCurrentDoc(), + NS_PRECONDITION(!aParent || aDocument == aParent->GetCurrentDoc(), "aDocument must be current doc of aParent"); NS_PRECONDITION(!GetCurrentDoc() && !IsInDoc(), "Already have a document. Unbind first!"); diff --git a/content/base/src/nsGenericElement.cpp b/content/base/src/nsGenericElement.cpp index aa1e406b174..63f638c2e83 100644 --- a/content/base/src/nsGenericElement.cpp +++ b/content/base/src/nsGenericElement.cpp @@ -80,6 +80,7 @@ #include "nsMutationEvent.h" #include "nsNodeUtils.h" #include "nsDocument.h" +#include "nsXULElement.h" #include "nsBindingManager.h" #include "nsXBLBinding.h" @@ -1699,14 +1700,7 @@ nsGenericElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent, NS_PRECONDITION(aParent || aDocument, "Must have document if no parent!"); NS_PRECONDITION(HasSameOwnerDoc(NODE_FROM(aParent, aDocument)), "Must have the same owner document"); - // XXXbz XUL elements are confused about their current doc when they're - // cloned, so we don't assert if aParent is a XUL element and aDocument is - // null, even if aParent->GetCurrentDoc() is non-null - // NS_PRECONDITION(!aParent || aDocument == aParent->GetCurrentDoc(), - // "aDocument must be current doc of aParent"); - NS_PRECONDITION(!aParent || - (aParent->IsNodeOfType(eXUL) && aDocument == nsnull) || - aDocument == aParent->GetCurrentDoc(), + NS_PRECONDITION(!aParent || aDocument == aParent->GetCurrentDoc(), "aDocument must be current doc of aParent"); NS_PRECONDITION(!GetCurrentDoc(), "Already have a document. Unbind first!"); // Note that as we recurse into the kids, they'll have a non-null parent. So @@ -1718,7 +1712,10 @@ nsGenericElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent, (!aBindingParent && aParent && aParent->GetBindingParent() == GetBindingParent()), "Already have a binding parent. Unbind first!"); - NS_PRECONDITION(aBindingParent != this || IsNativeAnonymous(), + // XXXbz XUL's SetNativeAnonymous is all weird, so can't assert + // anything here + NS_PRECONDITION(IsNodeOfType(eXUL) || + aBindingParent != this || IsNativeAnonymous(), "Only native anonymous content should have itself as its " "own binding parent"); @@ -1727,19 +1724,29 @@ nsGenericElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent, } // First set the binding parent - if (aBindingParent) { - nsDOMSlots *slots = GetDOMSlots(); + nsXULElement* xulElem = nsXULElement::FromContent(this); + if (xulElem) { + xulElem->SetXULBindingParent(aBindingParent); + } + else { + if (aBindingParent) { + nsDOMSlots *slots = GetDOMSlots(); - if (!slots) { - return NS_ERROR_OUT_OF_MEMORY; + if (!slots) { + return NS_ERROR_OUT_OF_MEMORY; + } + + slots->mBindingParent = aBindingParent; // Weak, so no addref happens. } - - slots->mBindingParent = aBindingParent; // Weak, so no addref happens. } - // Now set the parent + // Now set the parent and set the "Force attach xbl" flag if needed. if (aParent) { mParentPtrBits = NS_REINTERPRET_CAST(PtrBits, aParent) | PARENT_BIT_PARENT_IS_CONTENT; + + if (aParent->HasFlag(NODE_FORCE_XBL_BINDINGS)) { + SetFlags(NODE_FORCE_XBL_BINDINGS); + } } else { mParentPtrBits = NS_REINTERPRET_CAST(PtrBits, aDocument); @@ -1760,12 +1767,18 @@ nsGenericElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent, // Being added to a document. mParentPtrBits |= PARENT_BIT_INDOCUMENT; + + // Unset this flag since we now really are in a document. + UnsetFlags(NODE_FORCE_XBL_BINDINGS); } // Now recurse into our kids nsresult rv; PRUint32 i; - for (i = 0; i < GetChildCount(); ++i) { + // Don't call GetChildCount() here since that'll make XUL generate + // template children, which we're not in a consistent enough state for. + // Additionally, there's not really a need to generate the children here. + for (i = 0; i < mAttrsAndChildren.ChildCount(); ++i) { // The child can remove itself from the parent in BindToTree. nsCOMPtr child = mAttrsAndChildren.ChildAt(i); rv = child->BindToTree(aDocument, this, aBindingParent, @@ -1808,15 +1821,26 @@ nsGenericElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) // Unset things in the reverse order from how we set them in BindToTree mParentPtrBits = aNullParent ? 0 : mParentPtrBits & ~PARENT_BIT_INDOCUMENT; + + // Unset this since that's what the old code effectively did. + UnsetFlags(NODE_FORCE_XBL_BINDINGS); - nsDOMSlots *slots = GetExistingDOMSlots(); - if (slots) { - slots->mBindingParent = nsnull; + nsXULElement* xulElem = nsXULElement::FromContent(this); + if (xulElem) { + xulElem->SetXULBindingParent(nsnull); + } + else { + nsDOMSlots *slots = GetExistingDOMSlots(); + if (slots) { + slots->mBindingParent = nsnull; + } } if (aDeep) { - // Do the kids - PRUint32 i, n = GetChildCount(); + // Do the kids. Don't call GetChildCount() here since that'll force + // XUL to generate template children, which there is no need for since + // all we're going to do is unbind them anyway. + PRUint32 i, n = mAttrsAndChildren.ChildCount(); for (i = 0; i < n; ++i) { // Note that we pass PR_FALSE for aNullParent here, since we don't want @@ -2902,7 +2926,6 @@ nsGenericElement::doReplaceOrInsertBefore(PRBool aReplace, } else { // Not inserting a fragment but rather a single node. - PRBool newContentIsXUL = newContent->IsNodeOfType(eXUL); // Remove the element from the old parent if one exists nsINode* oldParent = newContent->GetNodeParent(); @@ -2946,7 +2969,7 @@ nsGenericElement::doReplaceOrInsertBefore(PRBool aReplace, } } - if (!newContentIsXUL) { + if (!newContent->IsNodeOfType(eXUL)) { nsContentUtils::ReparentContentWrapper(newContent, aParent, container->GetOwnerDoc(), container->GetOwnerDoc()); @@ -3022,9 +3045,8 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_END NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsGenericElement) nsIDocument* currentDoc = tmp->GetCurrentDoc(); - if (currentDoc && !tmp->HasFlag(NODE_HAS_FAKED_INDOC) && - nsCCUncollectableMarker::InGeneration( - currentDoc->GetMarkedCCGeneration())) { + if (currentDoc && nsCCUncollectableMarker::InGeneration( + currentDoc->GetMarkedCCGeneration())) { return NS_OK; } diff --git a/content/base/src/nsNodeUtils.cpp b/content/base/src/nsNodeUtils.cpp index 59c962d6168..a47d8e89347 100755 --- a/content/base/src/nsNodeUtils.cpp +++ b/content/base/src/nsNodeUtils.cpp @@ -69,13 +69,6 @@ } \ prev = node; \ node = node->GetNodeParent(); \ - \ - if (!node && prev->IsNodeOfType(nsINode::eXUL)) { \ - /* XUL elements can have the in-document flag set, but \ - still be in an orphaned subtree. In this case we \ - need to notify the document */ \ - node = NS_STATIC_CAST(nsIContent*, prev)->GetCurrentDoc(); \ - } \ } while (node); \ PR_END_MACRO @@ -645,8 +638,7 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, PRBool aClone, PRBool aDeep, if (aClone && !aParent && aNode->IsNodeOfType(nsINode::eXUL)) { nsXULElement *xulElem = NS_STATIC_CAST(nsXULElement*, elem); if (!xulElem->mPrototype || xulElem->IsInDoc()) { - clone->mParentPtrBits |= nsINode::PARENT_BIT_INDOCUMENT; - clone->SetFlags(NODE_HAS_FAKED_INDOC); + clone->SetFlags(NODE_FORCE_XBL_BINDINGS); } } #endif diff --git a/content/xbl/src/nsXBLService.cpp b/content/xbl/src/nsXBLService.cpp index 38baa9b06c0..7b3d116b3d4 100644 --- a/content/xbl/src/nsXBLService.cpp +++ b/content/xbl/src/nsXBLService.cpp @@ -494,7 +494,13 @@ nsXBLService::LoadBindings(nsIContent* aContent, nsIURI* aURL, PRBool aAugmentFl nsresult rv; - nsCOMPtr document = aContent->GetOwnerDoc(); + nsCOMPtr document; + if (aContent->HasFlag(NODE_FORCE_XBL_BINDINGS)) { + document = aContent->GetOwnerDoc(); + } + else { + document = aContent->GetCurrentDoc(); + } // XXX document may be null if we're in the midst of paint suppression if (!document) diff --git a/content/xul/content/src/nsXULElement.cpp b/content/xul/content/src/nsXULElement.cpp index 170b48c9493..7b9f2033c31 100644 --- a/content/xul/content/src/nsXULElement.cpp +++ b/content/xul/content/src/nsXULElement.cpp @@ -248,16 +248,6 @@ nsXULElement::nsXULElement(nsINodeInfo* aNodeInfo) XUL_PROTOTYPE_ATTRIBUTE_METER(gNumElements); } -nsXULElement::~nsXULElement() -{ - //XXX UnbindFromTree is not called always before dtor. - //XXX Related to templates or overlays? - //XXXbz probably related to the cloning thing! - if (IsInDoc()) { - UnbindFromTree(); - } -} - nsXULElement::nsXULSlots::nsXULSlots(PtrBits aFlags) : nsXULElement::nsDOMSlots(aFlags) { @@ -795,168 +785,6 @@ nsXULElement::MaybeAddPopupListener(nsIAtom* aLocalName) // nsIContent interface // -nsresult -nsXULElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent, - nsIContent* aBindingParent, - PRBool aCompileEventHandlers) -{ - NS_PRECONDITION(aParent || aDocument, "Must have document if no parent!"); - NS_PRECONDITION(HasSameOwnerDoc(NODE_FROM(aParent, aDocument)), - "Must have the same owner document"); - // XXXbz XUL elements are confused about their current doc when they're - // cloned, so we don't assert if aParent is a XUL element and aDocument is - // null, even if aParent->GetCurrentDoc() is non-null - // NS_PRECONDITION(!aParent || aDocument == aParent->GetCurrentDoc(), - // "aDocument must be current doc of aParent"); - NS_PRECONDITION(!aParent || - (aParent->IsNodeOfType(eXUL) && aDocument == nsnull) || - aDocument == aParent->GetCurrentDoc(), - "aDocument must be current doc of aParent"); - // XXXbz we'd like to assert that GetCurrentDoc() is null, but the cloning - // mess makes that impossible. We can't even assert that aDocument == - // GetCurrentDoc() when GetCurrentDoc() is non-null, since we may be - // getting inserted into a different document. :( - // NS_PRECONDITION(!GetCurrentDoc(), - // "Already have a document. Unbind first!"); - - // Note that as we recurse into the kids, they'll have a non-null - // parent. So only assert if our parent is _changing_ while we - // have a parent. - NS_PRECONDITION(!GetParent() || aParent == GetParent(), - "Already have a parent. Unbind first!"); - NS_PRECONDITION(!GetBindingParent() || - aBindingParent == GetBindingParent() || - (!aBindingParent && aParent && - aParent->GetBindingParent() == GetBindingParent()), - "Already have a binding parent. Unbind first!"); - // XXXbz XUL's SetNativeAnonymous is all weird, so can't assert - // anything here - // NS_PRECONDITION(aBindingParent != this || IsNativeAnonymous(), - // "Only native anonymous content should have itself as its " - // "own binding parent"); - - if (!aBindingParent && aParent) { - aBindingParent = aParent->GetBindingParent(); - } - - // First set the binding parent - mBindingParent = aBindingParent; - - // Now set the parent - if (aParent) { - mParentPtrBits = NS_REINTERPRET_CAST(PtrBits, aParent) | PARENT_BIT_PARENT_IS_CONTENT; - } - else { - mParentPtrBits = NS_REINTERPRET_CAST(PtrBits, aDocument); - } - - // XXXbz sXBL/XBL2 issue! - - // Finally, set the document - if (aDocument) { - // Notify XBL- & nsIAnonymousContentCreator-generated - // anonymous content that the document is changing. XXXbz - // ordering issues here? Probably not, since - // ChangeDocumentFor is just pretty broken anyway.... Need to - // get it working. - - // XXXbz XBL doesn't handle this (asserts), and we don't really want - // to be doing this during parsing anyway... sort this out. - // aDocument->BindingManager()->ChangeDocumentFor(this, nsnull, - // aDocument); - - // Being added to a document. - mParentPtrBits |= PARENT_BIT_INDOCUMENT; - - // Unset this flag since we now really are in a document. - UnsetFlags(NODE_HAS_FAKED_INDOC); - } - - // Now recurse into our kids - nsresult rv; - PRUint32 i; - for (i = 0; i < GetChildCount(); ++i) { - // The child can remove itself from the parent in BindToTree. - nsCOMPtr child = mAttrsAndChildren.ChildAt(i); - rv = child->BindToTree(aDocument, this, aBindingParent, - aCompileEventHandlers); - NS_ENSURE_SUCCESS(rv, rv); - } - - nsNodeUtils::ParentChainChanged(this); - - // XXXbz script execution during binding can trigger some of these - // postcondition asserts.... But we do want that, since things will - // generally be quite broken when that happens. - // XXXbz we'd like to assert that we have the right GetCurrentDoc(), but - // we may be being bound to a null document while we already have a - // current doc, due to the cloneNode hack... So can't assert that yet. - // NS_POSTCONDITION(aDocument == GetCurrentDoc(), - // "Bound to wrong document"); - NS_POSTCONDITION(aParent == GetParent(), "Bound to wrong parent"); - NS_POSTCONDITION(aBindingParent == GetBindingParent(), - "Bound to wrong binding parent"); - - return NS_OK; -} - -void -nsXULElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) -{ - // XXXbz we'd like to assert that called didn't screw up aDeep, but I'm not - // sure we can.... - // NS_PRECONDITION(aDeep || (!GetCurrentDoc() && !GetBindingParent()), - // "Shallow unbind won't clear document and binding " - // "parent on kids!"); - // Make sure to unbind this node before doing the kids - nsIDocument *document = GetCurrentDoc(); - if (document) { - // Notify XBL- & nsIAnonymousContentCreator-generated - // anonymous content that the document is changing. - document->BindingManager()->ChangeDocumentFor(this, document, nsnull); - - document->ClearBoxObjectFor(this); - } - - // mControllers can own objects that are implemented - // in JavaScript (such as some implementations of - // nsIControllers. These objects prevent their global - // object's script object from being garbage collected, - // which means JS continues to hold an owning reference - // to the nsGlobalWindow, which owns the document, - // which owns this content. That's a cycle, so we break - // it here. (It might be better to break this by releasing - // mDocument in nsGlobalWindow::SetDocShell, but I'm not - // sure whether that would fix all possible cycles through - // mControllers.) - nsDOMSlots* slots = GetExistingDOMSlots(); - if (slots) { - NS_IF_RELEASE(slots->mControllers); - } - - // Unset things in the reverse order from how we set them in BindToTree - mParentPtrBits = aNullParent ? 0 : mParentPtrBits & ~PARENT_BIT_INDOCUMENT; - - mBindingParent = nsnull; - - if (aDeep) { - // Do the kids. Note that we don't want to GetChildCount(), because - // that will force content generation... if we never had to generate - // the content, we shouldn't force it now! - PRUint32 i, n = PeekChildCount(); - - for (i = 0; i < n; ++i) { - // Note that we pass PR_FALSE for aNullParent here, since we don't - // want the kids to forget us. We _do_ want them to forget their - // binding parent, though, since this only walks non-anonymous - // kids. - mAttrsAndChildren.ChildAt(i)->UnbindFromTree(PR_TRUE, PR_FALSE); - } - } - - nsNodeUtils::ParentChainChanged(this); -} - void nsXULElement::SetNativeAnonymous(PRBool aAnonymous) { diff --git a/content/xul/content/src/nsXULElement.h b/content/xul/content/src/nsXULElement.h index 5cf20babd3a..93447b57faf 100644 --- a/content/xul/content/src/nsXULElement.h +++ b/content/xul/content/src/nsXULElement.h @@ -486,11 +486,6 @@ public: PRBool aNotify); // nsIContent - virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent, - nsIContent* aBindingParent, - PRBool aCompileEventHandlers); - virtual void UnbindFromTree(PRBool aDeep = PR_TRUE, - PRBool aNullParent = PR_TRUE); virtual void SetNativeAnonymous(PRBool aAnonymous); virtual nsresult RemoveChildAt(PRUint32 aIndex, PRBool aNotify); virtual nsIAtom *GetIDAttributeName() const; @@ -563,13 +558,20 @@ public: virtual void RecompileScriptEventListeners(); + // This function should ONLY be used by BindToTree implementations. + // The function exists soly because XUL elements store the binding parent + // differently than nsGenericElement does. + void SetXULBindingParent(nsIContent* aBindingParent) + { + mBindingParent = aBindingParent; + } + protected: // XXX This can be removed when nsNodeUtils::CloneAndAdopt doesn't need // access to mPrototype anymore. friend class nsNodeUtils; nsXULElement(nsINodeInfo* aNodeInfo); - virtual ~nsXULElement(void); // Implementation methods nsresult EnsureContentsGenerated(void) const; diff --git a/dom/src/base/nsDOMClassInfo.cpp b/dom/src/base/nsDOMClassInfo.cpp index 53e61d64962..13e8bba3123 100644 --- a/dom/src/base/nsDOMClassInfo.cpp +++ b/dom/src/base/nsDOMClassInfo.cpp @@ -6839,7 +6839,13 @@ nsElementSH::PostCreate(nsIXPConnectWrappedNative *wrapper, JSContext *cx, nsCOMPtr content(do_QueryWrappedNative(wrapper)); NS_ENSURE_TRUE(content, NS_ERROR_UNEXPECTED); - nsCOMPtr doc = content->GetDocument(); + nsCOMPtr doc; + if (content->HasFlag(NODE_FORCE_XBL_BINDINGS)) { + doc = content->GetOwnerDoc(); + } + else { + doc = content->GetCurrentDoc(); + } if (!doc) { // There's no baseclass that cares about this call so we just