Closed Bug 197427 Opened 17 years ago Closed 16 years ago

Node.replaceChild, insertBefore broken in XUL documents

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: WeirdAl, Unassigned)

References

Details

(Keywords: dom1, helpwanted, testcase)

Attachments

(3 files, 2 obsolete files)

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.
Attached file replaceChild testcase
Attached file 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?
Care to fix?  Just comparing nsXULElement methods to nsGenericElement should
show what the differences are....
Keywords: helpwanted
OS: Windows 98 → All
Hardware: PC → All
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
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
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.
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.
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.)
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.
Attachment #136530 - Attachment is obsolete: true
Attachment #136538 - Flags: review?(caillon)
> I wonder why it didn't take

Because you need to rebuild layout/build to relink the layout library.
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 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.
Attachment #136538 - Flags: review?(caillon) → review?(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.
Attachment #136538 - Flags: review?(peterv) → review-
> 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..
Yeah, an assertion would be better, so remove the null check you added in
ReplaceChild too.
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.
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?
(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)

Attachment #140131 - Flags: superreview?(dbaron)
Attachment #140131 - Flags: review?(bugmail)
Attachment #136538 - Attachment is obsolete: true
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.
Attachment #140131 - Flags: review?(bugmail) → review+
Agreed.
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!!
Attachment #140131 - Flags: superreview?(dbaron) → superreview+
Fix checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
This caused bug 233191 and bug 233137 thus far....
And maybe bug 234116 and the regression of bug 210910.
This bug might be still at large and haunting. I have just posted a bug #246691
describing the problem.
Component: DOM: Core → DOM: Core & HTML
QA Contact: desale → general
You need to log in before you can comment on or make changes to this bug.