Documents created by creating a blank document and then moving nodes from another document to it produce corrupt documents. The testcase has a large explanation and three separate demonstrations. Peter, copied you on this as I noticed this particularly when I tried to transform these wacky documents with the XSLT processor, but they're acting strange at a general level, which is why I logged this against the DOM component. Cheers, - Bill

Confirming, but I can serialize the "corrupt" documents fine with the serializer... so sounds like it's just XSLT that has issues with them. Does it expect a script global object or something, perhaps?

XSLT doesn't need a script global object afaik. Could this be a security issue. You might have problems transforming a document with an about:blank uri.

I can confirm that this is indeed a DOM problem of some sort. The reason I know is that I changed the $nodeMoveChildrenFromTo function to read: function $nodeMoveChildrenFromTo(fromNode, toNode) { var fromLength; var i; var theNode; fromLength = fromNode.childNodes.length; for (i = 0; i < fromLength; i++) { theNode = fromNode.removeChild(fromNode.childNodes[0]); toNode.appendChild(theNode); }; return; }; the problem went away. Also, note the explicit removeChild there. I found that that works best, even though the DOM spec says that an appendChild will do that implicitly. Weird... Cheers, - Bill

Ah, yes. The insertBefore method of nsDocument doesn't remove the new child from its old parent (which the equivalent method on elements does do). As a result, the code in that testcase was ending up with nodes which had a null mDocument when all was said and done... Note also that appending document fragments to a document is also broken. Perhaps more of the generic element code should be shared here, somehow?

If we need bug 307444 fixed for 1.8, we need this too...

hoping BZ can help us here.

Slightly easier to review... sorta.

Comment on attachment 197245 [details] [diff] [review] Same as diff -w This a little big and scary looking, but it should be reasoanbly safe, I think... as safe as any other patch I'm likely to be able to do for this bug in the short run. This fixes this bug and the crash in bug 307444. On trunk, I'd like to put some thought into making this code happier (eg eliminate the SetRootContent stuff on documents to make them more like nsIContent, and perhaps even have a common interface like nsIContentContainer that both nsIContent and nsIDocument would inherit from (with the impl living in nsGenericElement or nsContentUtils, or something). Let me know what you think of that idea while you're looking over this code?

I have long thought about an nsINode interface that would be parent to nsIContent and nsIDocument (and possibly even nsIAttribute). There are two problems though. First off, if we had some baseclass that implemented that interface we'd end up with one more vtable pointer once elements implement nsIContent. Second, it would probably be rare that we can actually return such an interface. In most cases that would probably just force consumers to QI to nsIContent/nsIDocument which would be bad perfwise. Not saying that it's a bad idea, i'd certainly like to see more codereuse and a faster replacement for nsIDOMNode... I just havn't figured out how yet :)

Yeah, that was really my problem... I was thinking we could put the impl in nsContentUtils and have people make one-line calls to it from there to avoid bloating the vtable (the methods would need to take the nsAttrAndChildArray as an arg or something)... Good to have you back, Jonas!

Yeah, that sounds like an ok solution. We could even have them as static methods in nsGenericElement if we prefer. I don't really care where they go :)

Comment on attachment 197245 [details] [diff] [review] Same as diff -w Looks good, and I agree, this is sortof big, but I don't see how we could get this much smaller or safer for 1.8 :( sr=jst

sicking, if you can review this in time for 1.8b5, please do? I mailed peterv, but I don't know what his schedule is like...

Comment on attachment 197245 [details] [diff] [review] Same as diff -w >Index: content/base/src/nsGenericElement.h >=================================================================== >+ * @param [out] aReturn the child we insert Not sure how the JavaDoc tools will deal with this, I suppose |@param aReturn [out]| would be safer. >Index: content/base/src/nsGenericElement.cpp >=================================================================== >+/* static */ >+nsresult >+nsGenericElement::doInsertBefore(nsIDOMNode* aNewChild, nsIDOMNode* aRefChild, >+ nsIContent* aParent, nsIDocument* aDocument, >+ nsAttrAndChildArray& aChildArray, >+ nsIDOMNode** aReturn) > if (nodeType == nsIDOMNode::DOCUMENT_FRAGMENT_NODE) { > nsCOMPtr<nsIDocumentFragment> doc_fragment(do_QueryInterface(newContent)); > NS_ENSURE_TRUE(doc_fragment, NS_ERROR_UNEXPECTED); > > doc_fragment->DisconnectChildren(); > > PRUint32 count = newContent->GetChildCount(); > >- PRUint32 old_count = GetChildCount(); >+ PRUint32 old_count = aParent ? aParent->GetChildCount() : >+ aDocument->GetChildCount(); container->GetChildCount? >Index: content/base/src/nsContentUtils.cpp >=================================================================== >- nsCOMPtr<nsISupports> new_parent; >+ nsISupports* new_parent; > > if (!aNewParent) { >- if (old_doc->GetRootContent() == aContent) { >- new_parent = old_doc; >- } >+ new_parent = aNewDocument; > } else { > new_parent = aNewParent; > } Nit: |nsISupports* new_parent = aNewParent ? aNewParent : aNewDocument|

Who can land this on the trunk? BZ, can you help us with a risk analysis for the branch? This looks kinda big.

Comment on attachment 198034 [details] [diff] [review] Updated to comments Requesting branch approval. The risk analysis is as follows: The patch affects the behavior of insertBefore and replaceChild methods, but I took great pains that the behavior of those methods when called on an element should not change with this patch in any way. I've reread this a few times with that in view and run some tests, and I'm pretty confident that I got this right. In particular, I was careful to change none of the security checks (we continue to do slightly different ones for elements and documents, as we did before). The behavior does change for documents (a much rarer circumstance), in several ways: 1) Children being inserted are now properly removed from their old parent instead of effectively appearing in two different DOMs at once. 2) We do better checking on child ordering; there were cases when we could construct DOMs that would later cause us to have issues and we no longer allow that. 3) We handle insertion of document fragments under a document. Given that the only behavior changes are for documents and that the existing code was pretty much wholly broken (and hence I must suspect unused, given the lack of bugs on it), I think it should be safe to take this on branch...

Fixed on trunk.

I've verified that this does indeed fix the testcase with Windows XP and a tinderbox trunk build that includes the fix.

Fixed on branch.

Please note bug 310653, which is an assertion bug caused by bz's checkin.

Is it possible that this bug caused a branch-only regression, bug 323745?

See comments in bug 323745.