611957 - HTMLCollection has length 2 but Iterator yields [2, undefined]

Status: RESOLVED INACTIVE

(Core :: XPConnect, defect)

Description

[Jonathan Protzenko [:protz]]

Attachment: Test extension that demonstrates the issue

Hi folks,

You thought you'd be all done after solving bug 571849? Hah! That's definitely not the case. Now look at this lovely piece of code:

          cn = iframe.contentDocument.body.firstElementChild.getElementsByClassName("message");
          dump("childNodes is non-empty: "+cn+"["+cn.length+"]\n"); //OK
          for each (let x in Iterator(cn)) {
            dump("x is "+x+"\n");
          }

The output is:

childNodes is non-empty: [object HTMLCollection][2]
x is 0,[object HTMLLIElement]
x is 1,[object HTMLLIElement]
x is 2,

True story! The HTMLCollection has length 2 and the Iterator() yields [2, undefined]! Funny, right?

I managed to come up with a testcase. The test basically involves calling getElementsByClassName to obtain an HTMLCollection with length 3, removing the last element, and doing the same call again.

I've reused the testcase from bug 571849 again, the relevant part now is:
          /* This demonstrates the issue */
          let cn = iframe.contentDocument.body.firstElementChild.getElementsByClassName("message");
          dump("childNodes is non-empty: "+cn+"["+cn.length+"]\n"); //OK
          for each (let x in Iterator(cn)) {
            dump("x is "+x+"\n");
          }
          cn[cn.length-1].parentNode.removeChild(cn[cn.length-1]);
          cn = iframe.contentDocument.body.firstElementChild.getElementsByClassName("message");
          dump("childNodes is non-empty: "+cn+"["+cn.length+"]\n"); //OK
          for each (let x in Iterator(cn)) {
            dump("x is "+x+"\n");
          }

the iframe's content page (i.e. test.html )is just a <ul> with three <li class="message"> in it.

Steps to reproduce:
- install the .xpi (attachment #1 [details] [diff] [review])
- enable dump in the console
- from the Error console, run open("chrome://testcase5/content/testcase.xul",
"", "chrome,width=500,height=500");
- click "add iframe"
- click "load some content"

I'll try to come up with a reduced test case, and I'd love to provide you with one that doesn't imply using an extension, but I feel like this is tricky (I already tried for bug 571849 without success).

Good luck in fixing this!

Comment 1

[Jonathan Protzenko [:protz]]

Oh and this should definitely be blocking, just like bug 571849.

Comment 2

[Boris Zbarsky [:bzbarsky]]

This isn't blocking, imo.  It's also not a regression, unlike bug 571849.

First of all, you only have one list object here.  So the only issue is that the property named "2" doesn't disappear from that list when you make the removeChild call.

I'll attach a trivial testcase that doesn't involve extensions, etc, that shows the issue.  It also shows it in Fx 3.6.  The only way to fix this is to implement virtual properties, which may or may not happen for 2.0.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 4

[Jonathan Protzenko [:protz]]

Boris, I'm doing a fresh call to getElementsByClassName after I removed the last element, so I expect the "2" property to not exist anymore in the fresh collection. Conversely, your testcase is using the same HTMLCollection!

Here's the relevant sample:

cn[cn.length-1].parentNode.removeChild(cn[cn.length-1]);
// Now I'm getting a fresh collection                vv
cn = iframe.contentDocument.body.firstElementChild.getElementsByClassName("message");
// And now I'm iterating on the fresh collection

Plus, this is a regression from 1.9.2. With all due respect, sir ;-), the output on 1.9.2 is

childNodes is non-empty: [object XPCNativeWrapper [object HTMLCollection]][3]
x is length,3
x is 0,[object XPCNativeWrapper [object HTMLLIElement]]
x is 1,[object XPCNativeWrapper [object HTMLLIElement]]
x is 2,[object XPCNativeWrapper [object HTMLLIElement]]
childNodes is non-empty: [object XPCNativeWrapper [object HTMLCollection]][2]
x is length,2
x is 0,[object XPCNativeWrapper [object HTMLLIElement]]
x is 1,[object XPCNativeWrapper [object HTMLLIElement]]

and the output on b7 is

childNodes is non-empty: [object HTMLCollection][3]
x is 0,[object HTMLLIElement]
x is 1,[object HTMLLIElement]
x is 2,[object HTMLLIElement]
// now iterating on a *fresh* HTMLCollection
childNodes is non-empty: [object HTMLCollection][2]
x is 0,[object HTMLLIElement]
x is 1,[object HTMLLIElement]
x is 2,

^^ weird thing here.

I think it has something to do with XPCNativeWrappers, which is why I need such a convoluted testcase.

Do you agree or am I still misunderstanding things? I'll try to find you on IRC.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Boris, I'm doing a fresh call to getElementsByClassName

Yes, but nothing says that this creates a new nodelist.  And in fact, it doesn't, if there's already a live nodelist for that classname; there's a cache mapping class names to live lists.  You can trivially test this using expando properties if you choose to do so.  Or heck, using ==.

1.9.2 did not have the name-to-list cache for getElementsByClassName (see bug 453929), so you did in fact get a different list on each call.  But had you used getElementsByTagName in 1.9.2, which does have a cache, you would have seen similar behavior.

The toString output difference is in fact odd.  Blake, shouldn't there be XPCNativeWrappers, or whatever they're called nowadays, here?  Or did toString() for those change to make them not visible anymore?

Comment 6

[Boris Zbarsky [:bzbarsky]]

That is, seems like there should be XRayWrappers here (though the rest of the behavior should be as observed by Jonathan).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, you're loading a chrome:// URI in the subframe.  So it doesn't get any wrapperization, since it's same-origin with the main document.  This is a change in how wrappers work that came along with the new wrappers being purely origin-based.

Comment 8

[Jonathan Protzenko [:protz]]

Boris, I haven't found a suitable bug to dupe this one to, so if you have the right one in mind, feel free to mark this one as a duplicate :)

Comment 9

[Johnny Stenback (:jst)]

Not a blocker.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I would think this got fixed by the proxy list bindings.  Is this still reproducible?

Comment 11

[Jonathan Protzenko [:protz]]

I disabled the workaround in my addon, I'll let you know if the bug seems to be gone!

Comment 12

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.