121495 - nsXULElement::GetChildNodes returns a non-live nodelist

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The NodeList that's given by the childNodes property of an XUL element is not
live.  In particular, its length does not change as children are added
to/removed from the element.

Something like this will go into an infinite loop, for instance:

var children = node.childNodes;
while (children.length != 0) {
  node.removeChild(children.item(0));
}

since children.length is never updated.

Comment 1

[Sivakiran Tummala]

changing priority to P3

Comment 2

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Preliminary patch

This does the C++ part.  It doesn't address the issue of JS that depends on the
bogus behavior so far....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Oops. Same, without stray changes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Sweep through JS in tree

A few of these were not strictly needed, but were just better code than what
was there.

I hate how we now have 2 or 3 copies of every single friggin' JS file, all with
the same exact errors in them... :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146158 [details] [diff] [review]
Oops.  Same, without stray changes.

jst, would you review the core change?	The changes to consumers will be in
separate patches.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146163 [details] [diff] [review]
Sweep through JS in tree

Neil, jst, would you review the JS parts of this change?

Comment 8

[[e-mail]]

Comment on attachment 146163 [details] [diff] [review]
Sweep through JS in tree

>   // We need to leave the <label> to flex the buttons to the left
>   // so, don't remove the last child at position length - 1
>-  for (var i = childNodesLength - 2; i >= 0; i--) {
>-    toolbar.removeChild(childNodes.item(i));
>+  while (childNodes.length > 1) {
>+    // Remove back to front so there's less moving about.
>+    toolbar.removeChild(childNodes.item(childNodes.length - 2));
>   }
I don't see what was wrong with the reverse loop approach, indeed I was
wondering why you didn't use it in cases like the one below:
>     var kids = this.mTree.childNodes;
>     for (var i = 0 ; i < kids.length; ++i)
>-      if (kids[i].localName != "treechildren")
>+      if (kids[i].localName != "treechildren") {
>         this.mTree.removeChild(kids[i]);
>+        --i;  // Adjust, since the list adjusted itself
>+      }
I'm not exactly a good commenter but I'm not convinced of the understandability
of that comment... reversing the loop might give you a chance to write a more
meaningful comment :-)

>Index: extensions/p3p/resources/content/p3pSummary.js
As this change is in a function commented as a workaround I'm going to assume
that it just needs to work as it should get superceded...

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I don't see what was wrong with the reverse loop approach

It tempts people to loop with a counter in general, which is a bad idea.  ;)

> reversing the loop might give you a chance to write a more meaningful comment

Yeah, good point.  I'll reverse those loops.

Thanks for the review, Neil!

Comment 10

[[e-mail]]

(In reply to comment #9)
>>I don't see what was wrong with the reverse loop approach
>It tempts people to loop with a counter in general, which is a bad idea. ;)
Well just comment why the reverse loop happens to work as per those more
meaningful comments you're going to write :-)

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 146158 [details] [diff] [review]
Oops.  Same, without stray changes.

Sweet! r+sr=jst

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 146163 [details] [diff] [review]
Sweep through JS in tree

sr=jst

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to all comments and merged to tip

Comment 14

[Boris Zbarsky [:bzbarsky]]

Patch checked in.  No effect on Ts/Tp/Txul.

Comment 15

[Scott MacGregor]

this caused bug 320117