197427 - Node.replaceChild, insertBefore broken in XUL documents

Status: RESOLVED FIXED

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

Description

[Alex Vincent [:WeirdAl]]

I am having difficulty with the replaceChild and insertBefore methods of Node
objects in XUL documents.  In the case of replaceChild, the refChild node is not
removed from the document.  In the case of insertBefore, when the refChild and
newChild nodes are the same, the refChild node is removed from the DOM, or at
least "hidden" (for some cases I am able to get a lost node back, but lose other
nodes in the process).

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4a) Gecko/20030314
Testcases coming up.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: replaceChild testcase

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: insertBefore testcase

ML document versions of these testcases work perfectly.

Is there a way we can run the DOM TS in XUL, perhaps discover other violations
of DOM Core?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Care to fix?  Just comparing nsXULElement methods to nsGenericElement should
show what the differences are....

Comment 4

[Alex Vincent [:WeirdAl]]

http://lxr.mozilla.org/seamonkey/source/content/xul/content/src/nsXULElement.cpp#961
versus
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#2438

and
http://lxr.mozilla.org/seamonkey/source/content/xul/content/src/nsXULElement.cpp#1044
versus
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#2661

Comment 5

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 6

[Karsten Düsterloh]

In the case of 
  old_return = replaceChild(new, old)
where the new node is already one of the children, the behaviour is pretty
unexpected. :(

Before the call:
childNode[x] = old
childNode[y] = new
old_return   = void

After the above call to replaceChild:
childNode[x] = new
childNode[y] = new
old_return   = new

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, the XUL code here is just pretty broken all around.  We really need to
merge more of nsXULElement with nsGenericElement or at least do a copy-and-paste
patch.  The helpwanted is for the latter, of course.

Comment 8

[Alex Vincent [:WeirdAl]]

Attachment: First attempt at a patch (still fails testcases)

This is a patch, basically a copy & paste of what I think is relevant code from
nsGenericElement.cpp to nsXULElement.cpp.  It doesn't seem to have done the
trick.

Comment 9

[Alex Vincent [:WeirdAl]]

Check that; after rebuilding the entire Mozilla suite, it passes the second
testcase.  That was my goal.

I wonder why it didn't take when I rebuilt the directory nsXULElement.cpp was
in, but did after the complete rebuild.  (That should prove educational, if
someone can fill me in.)

Comment 10

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1 (copies code from nsGenericElement.cpp into nsXULElement.cpp)

After rebuilding mozilla/content/xul and mozilla/layout with this patch,
Mozilla passes both testcases I placed on this bug ages ago.

I copied code which was already in nsGenericElement into nsXULElement.	That
includes isSelfOrAncestor, which was in nsGenericElement but not nsXULElement.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I wonder why it didn't take

Because you need to rebuild layout/build to relink the layout library.

Comment 12

[Alex Vincent [:WeirdAl]]

caillon asked me to run the patch through jst's reviewer simulacrum.  Two errors
found, both nsresult res where it should be nsresult rv.  Will fix.

Would someone e-mail me privately and tell me what res should be used for, and
what rv should be used for?  I might as well learn now.

Comment 13

[Alex Vincent [:WeirdAl]]

Comment on attachment 136538 [details] [diff] [review]
patch, v1 (copies code from nsGenericElement.cpp into nsXULElement.cpp)

Thank goodness the code from nsGenericElement.cpp appears to not have changed
since.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 136538 [details] [diff] [review]
patch, v1 (copies code from nsGenericElement.cpp into nsXULElement.cpp)

I've commented on a bunch of issues that apply to several spots further in the
patch, please correct them in the other spots too. Correct these and I'll take
another look.

> Index: content/xul/content/src/nsXULElement.cpp
> ===================================================================

> +static PRBool
> +isSelfOrAncestor(nsIContent *aNode, nsIContent *aChild)

This should be at least shared with nsGenericElement (make it a public static
function in nsGenericElement)

> +
>  
>  NS_IMETHODIMP
>  nsXULElement::InsertBefore(nsIDOMNode* aNewChild, nsIDOMNode* aRefChild,
>                             nsIDOMNode** aReturn)
>  {

You should add a null-check for aReturn.

You need to set *aReturn to null before returning errors.

>      NS_PRECONDITION(aNewChild != nsnull, "null ptr");
>      if (! aNewChild)
>          return NS_ERROR_NULL_POINTER;

> +        
> +    nsCOMPtr<nsIContent> refContent;
> +    nsresult res = NS_OK;

rv, not res
(also in other spots)

> +    PRInt32 refPos = 0;
> +
> +    if (aRefChild) {
> +      refContent = do_QueryInterface(aRefChild, &res);


Inconsistent indentation. The file uses four spaces, so should you.
(also in other spots)

> +
> +      if (NS_FAILED(res)) {

This could be

      refContent = do_QueryInterface(aRefChild);
      if (!refContent) {

(also in other spots)

> +    res = nsContentUtils::CheckSameOrigin(this, aNewChild);
> +    if (NS_FAILED(res))
> +        return res;

I think you can drop this, verify with caillon or jst (since you call
CanCallerAccess later on).

> +    switch (nodeType) {
> +    case nsIDOMNode::ELEMENT_NODE :

Indent the case's and remove the space before the :
(also in other spots)

> +      doc_fragment->DropChildReferences();
> +    } else {

move the else to a new line.
(also in other spots)

> +            if (refContent == GetChildAt(refPos - 1)) {
> +              refPos--;

--refPos
(also in other spots)

>      }
>  
>      NS_ADDREF(aNewChild);
>      *aReturn = aNewChild;

      NS_ADDREF(*aReturn = aNewChild);

> -    return NS_OK;
> +    return res;

returning NS_OK seems fine here.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> You should add a null-check for aReturn.

Er... why?  That's an out param.  An assert that it's non-null is about all it
deserves..

Comment 16

[Peter Van der Beken [:peterv]]

Yeah, an assertion would be better, so remove the null check you added in
ReplaceChild too.

Comment 17

[Johnny Stenback (:jst)]

What we really need here is to share the code in nsGenericElement.cpp. Break
that out to static class methods, or global non-class methods, and make it
independent of nsGenericElement and use it from both nsGenericElement and
nsXULElement. Duplication all this code is not desireable, and I think it can be
avoided.

Comment 18

[Alex Vincent [:WeirdAl]]

Re comment 17, this goes beyond my C++ skills.  I'm an amateur, and still
learning.  Would someone else like to work on the patch and get it right?

Comment 19

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

(In reply to comment #17)
> What we really need here is to share the code in nsGenericElement.cpp.

That is covered by bug 198533 and is nontrivial to fix. IMHO making both classes
look the same is a step in the right direction since it'll ease making stuff
inherit from each other in the future. (I'm doing the same thing during my
attributes work)

Comment 20

[Johnny Stenback (:jst)]

Attachment: Share insertBefore(), appendChild(), replaceChild(), and removeChild() code between all element classes. Wow, what a concept!

Um, no, that's doable. Here's a patch.

Comment 21

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

Comment on attachment 140131 [details] [diff] [review]
Share insertBefore(), appendChild(), replaceChild(), and removeChild() code between all element classes. Wow, what a concept!

r=me

Though when we make nsXULElement inherit nsGenericElement we should IMHO back
this change out again, memberfunctions are IMHO cleanerlooking then static
functions with a |this| argument.

Comment 22

[Johnny Stenback (:jst)]

Agreed.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140131 [details] [diff] [review]
Share insertBefore(), appendChild(), replaceChild(), and removeChild() code between all element classes. Wow, what a concept!

sr=bzbarsky.  Hallelujah!!

Comment 24

[Johnny Stenback (:jst)]

Fix checked in.

Comment 25

[Boris Zbarsky [:bzbarsky]]

This caused bug 233191 and bug 233137 thus far....

Comment 26

[Jesse Ruderman]

And maybe bug 234116 and the regression of bug 210910.

Comment 27

[Michal Ziemski]

This bug might be still at large and haunting. I have just posted a bug #246691
describing the problem.