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

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: William J. Edney, Assigned: bz)

Tracking

({fixed1.8})

Trunk
mozilla1.8beta5
fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
Created attachment 171315 [details]
Demonstration of a bogus document attempting to be transformed
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
(Reporter)

Comment 4

13 years ago
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?
(Assignee)

Updated

12 years ago
Blocks: 307444
If we need bug 307444 fixed for 1.8, we need this too...
Flags: blocking1.8b5?
(Assignee)

Updated

12 years ago
Blocks: 307280

Updated

12 years ago
Whiteboard: [sg:fix]

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+

Comment 7

12 years ago
hoping BZ can help us here.
Assignee: general → bzbarsky

Updated

12 years ago
No longer blocks: 307280
Created attachment 197244 [details] [diff] [review]
Proposed patch
Created attachment 197245 [details] [diff] [review]
Same as diff -w

Slightly easier to review... sorta.
Attachment #197245 - Flags: superreview?(jst)
Attachment #197245 - Flags: review?(peterv)
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?
(Assignee)

Updated

12 years ago
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: Documents created by moving nodes to a blank document are corrupt → [FIX]Documents created by moving nodes to a blank document are corrupt
Target Milestone: --- → mozilla1.8beta5
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
Attachment #197245 - Flags: superreview?(jst) → superreview+
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|
Attachment #197245 - Flags: review?(peterv) → review+

Comment 17

12 years ago
Who can land this on the trunk? BZ, can you help us with a risk analysis for the
branch? This looks kinda big. 
Created attachment 198034 [details] [diff] [review]
Updated to comments
Attachment #197244 - Attachment is obsolete: true
Attachment #197245 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Blocks: 127051
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...
Attachment #198034 - Flags: approval1.8b5?
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 21

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

Updated

12 years ago
Attachment #198034 - Flags: approval1.8b5? → approval1.8b5+
Created attachment 198097 [details] [diff] [review]
With minor merges to 1.8 branch
Fixed on branch.
Keywords: fixed1.8
Please note bug 310653, which is an assertion bug caused by bz's checkin.
Depends on: 310650, 310653

Comment 25

12 years ago
Is it possible that this bug caused a branch-only regression, bug 323745?
See comments in bug 323745.
You need to log in before you can comment on or make changes to this bug.