278472 - [FIX]Documents created by moving nodes to a blank document are corrupt

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[William J. Edney]

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

Comment 1

[William J. Edney]

Attachment: Demonstration of a bogus document attempting to be transformed

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[William J. Edney]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Asa Dotzler [:asa]]

hoping BZ can help us here.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Slightly easier to review... sorta.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

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!

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 14

[Johnny Stenback (:jst)]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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 16

[Peter Van der Beken [:peterv]]

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|

Comment 17

[Asa Dotzler [:asa]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 19

[Boris Zbarsky [:bzbarsky]]

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...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 21

[Asa Dotzler [:asa]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: With minor merges to 1.8 branch

Comment 23

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 24

[Alex Vincent [:WeirdAl]]

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

Comment 25

[Nickolay_Ponomarev]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

See comments in bug 323745.