Last Comment Bug 278472 - [FIX]Documents created by moving nodes to a blank document are corrupt
: [FIX]Documents created by moving nodes to a blank document are corrupt
Status: RESOLVED FIXED
[sg:fix]
: fixed1.8
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.8beta5
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Hixie (not reading bugmail)
:
Mentors:
Depends on: 310650 310653
Blocks: 127051 307444
  Show dependency treegraph
 
Reported: 2005-01-14 17:51 PST by William J. Edney
Modified: 2006-01-17 09:40 PST (History)
8 users (show)
asa: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Demonstration of a bogus document attempting to be transformed (6.25 KB, text/html)
2005-01-14 17:52 PST, William J. Edney
no flags Details
Proposed patch (57.70 KB, patch)
2005-09-24 00:44 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Same as diff -w (57.06 KB, patch)
2005-09-24 00:45 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
jst: superreview+
Details | Diff | Splinter Review
Updated to comments (57.62 KB, patch)
2005-09-30 13:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
asa: approval1.8b5+
Details | Diff | Splinter Review
With minor merges to 1.8 branch (57.50 KB, patch)
2005-09-30 19:02 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review

Description William J. Edney 2005-01-14 17:51:10 PST
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 2005-01-14 17:52:30 PST
Created attachment 171315 [details]
Demonstration of a bogus document attempting to be transformed
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2005-02-01 16:24:28 PST
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 2005-02-01 17:25:32 PST
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 2005-02-01 17:41:00 PST
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 [:bz] (still a bit busy) 2005-02-01 18:48:28 PST
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 [:bz] (still a bit busy) 2005-09-07 22:06:09 PDT
If we need bug 307444 fixed for 1.8, we need this too...
Comment 7 Asa Dotzler [:asa] 2005-09-21 11:46:22 PDT
hoping BZ can help us here.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2005-09-24 00:44:38 PDT
Created attachment 197244 [details] [diff] [review]
Proposed patch
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2005-09-24 00:45:34 PDT
Created attachment 197245 [details] [diff] [review]
Same as diff -w

Slightly easier to review... sorta.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2005-09-24 00:46:23 PDT
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 2005-09-26 12:05:55 PDT
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 [:bz] (still a bit busy) 2005-09-26 14:12:15 PDT
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 2005-09-28 11:07:50 PDT
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, jst@mozilla.com) 2005-09-29 17:19:18 PDT
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 [:bz] (still a bit busy) 2005-09-29 21:00:04 PDT
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] 2005-09-30 08:39:38 PDT
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] 2005-09-30 11:53:16 PDT
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 [:bz] (still a bit busy) 2005-09-30 13:03:45 PDT
Created attachment 198034 [details] [diff] [review]
Updated to comments
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2005-09-30 13:34:14 PDT
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 [:bz] (still a bit busy) 2005-09-30 14:34:21 PDT
Fixed on trunk.
Comment 21 Asa Dotzler [:asa] 2005-09-30 16:52:31 PDT
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 [:bz] (still a bit busy) 2005-09-30 19:02:59 PDT
Created attachment 198097 [details] [diff] [review]
With minor merges to 1.8 branch
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2005-09-30 21:09:18 PDT
Fixed on branch.
Comment 24 Alex Vincent [:WeirdAl] 2005-09-30 23:48:24 PDT
Please note bug 310653, which is an assertion bug caused by bz's checkin.
Comment 25 Nickolay_Ponomarev 2006-01-17 09:07:02 PST
Is it possible that this bug caused a branch-only regression, bug 323745?
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2006-01-17 09:40:07 PST
See comments in bug 323745.

Note You need to log in before you can comment on or make changes to this bug.