633133 - Testing for named items in HTMLCollections using 'in' operator always returns false

Status: RESOLVED FIXED

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

Description

[aeneas]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11) Gecko/20100101 Firefox/4.0b11
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11) Gecko/20100101 Firefox/4.0b11

This functionality is of great importance:
The (elementId in nodeList) should return true but FX4B always returns "false", even if (DOM)element with requested Id exists in the nodeList!

Works superb in all major browsers, including latest releases of: Safari, Chrome, Opera, MSIE9.

Reproducible: Always

Steps to Reproduce:
1.Have a page with some ordinary nested elements:
<div id='mydiv'><p id='p1'>paragraph 1</p><p id='p2'>paragraph 2</p></div>

2. var mydiv=document.getElementById('mydiv'), mydivCol = mydiv.getElementsByTagName("*"), p1 = document.getElementById("p1"),p2 = document.getElementById("p2"); //etc... 
3. alert(p1.id in mydivCol); alert("p2" in mydivCol); alert(p2.id in mydivCol);
Actual Results:  
alert box content(s):
"false"
"false"
"false"

Expected Results:  
alert box content(s):
"true"
"true"
"true"

Comment 1

[Boris Zbarsky [:bzbarsky]]

This isn't a security issue.

Comment 2

[Boris Zbarsky [:bzbarsky]]

jst, is there a reason nsNamedArraySH doesn't have a resolve hook?  Seems like it should...

Comment 3

[aeneas]

I've reported it as DOM_Traversal bug. [?!!] Why do you say so?
(In reply to comment #1)
> This isn't a security issue.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Testcase

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> I've reported it as DOM_Traversal bug. [?!!] Why do you say so?
> (In reply to comment #1)
> > This isn't a security issue.

There were a message asking you if the bug was security sensitive [1] when you filed the bug and you very likely checked the checkbox.

[1] Something like: "Many users could be harmed by this security problem: it should be kept hidden from the public until it is resolved."

Comment 6

[aeneas]

Great, any news on: why does Firefox (concerning the "in" operator) return a false negative exclusively on nodeLists?
And are there any chances that FX will restore this operator native ability in near future?
(In reply to comment #5)
> (In reply to comment #3)
> > I've reported it as DOM_Traversal bug. [?!!] Why do you say so?
> > (In reply to comment #1)
> > > This isn't a security issue.
> 
> There were a message asking you if the bug was security sensitive [1] when you
> filed the bug and you very likely checked the checkbox.
> 
> [1] Something like: "Many users could be harmed by this security problem: it
> should be kept hidden from the public until it is resolved."

Comment 7

[aeneas]

(In reply to comment #4)
> Created attachment 512054 [details]
> Testcase

Big thanks for creating and providing that live testcase
Regards

Comment 8

[Boris Zbarsky [:bzbarsky]]

aeneas, please don't overquote.

The "why" is in comment 2.

The "chances" depend on the answer to the question in that comment.

Comment 9

[aeneas]

Acknowledged that. Thank you.
In fact I know there are no "chances" for this to be resolved any time soon, if at all. But thanks for your consideration. I've at done my part. 
Regards

Comment 10

[Johnny Stenback (:jst)]

(In reply to comment #2)
> jst, is there a reason nsNamedArraySH doesn't have a resolve hook?  Seems like
> it should...

Hmm, it's been a while, no obvious reasons come to mind. The one thing that I did think about was that the lack of a resolve hook means the scriptable helper never defines a property for a given name, we only find it in the getProperty hook, which could've had an impact on property retention after the underlying C++ element was removed from the DOM tree, but a quick test shows that that's certainly not the case right now, named properties do still stick to the JS object even after the underlying named element is removed from the DOM tree. 

Peterv, any good reason come to mind for you here?

Comment 11

[Peter Van der Beken [:peterv]]

I can't think of anything right now.

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This might fix it.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 512632 [details] [diff] [review]
Patch v1

- In nsNamedArraySH::NewResolve():

+      *_retval = ::JS_DefinePropertyById(cx, obj, id, JSVAL_VOID, nsnull,
+                                         nsnull, JSPROP_ENUMERATE);

nsGenericArraySH::NewEnumerate() uses the flags JSPROP_ENUMERATE | JSPROP_SHARED, do the same here.

- In test_bug633133.html:

+<div id="content" style="display: none">
+  <div id='foo'></div>
+  <div name='bar'></div>
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 633133 **/
+
+var divCollection = document.getElementsByTagName('div');
+var foo = divCollection.foo;
+var bar = divCollection.bar;

This already resolves "foo" and "bar" on divCollection...

+ok(foo.id in divCollection, "'foo' should be in the div collection");
+ok(bar.getAttribute('name') in divCollection, "'bar' should be in the div collection");

... which means that this tests that "foo" in divCollection works after it's already been resolved once. I think this test would be a better test of this fix if you test "foo" in divCollection before you touch any properties on divCollection. Same for "bar".

r=jst with those changes, assuming the test still passes :)

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

r=jst

And it still passes tests ;)

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Mounir's patch, expanded

This patch fixes another blocker. gal and I looked through the rest of nsDOMClassInfo for a similar pattern to nsNamedArraySH and found nsHTMLSelectElementSH, so I fixed that here too. We need to get this in for final. volkmar++ for finding and fixing this, so I didn't have to ;-)

Comment 17

[Johnny Stenback (:jst)]

This is a final hardblocker because of the duped bug (bug 635715) being a hardblocker, and being fixed by the patch in this bug.

Comment 18

[Andreas Gal :gal]

TM tree is open. Land it there.

Comment 19

[Mounir Lamouri (:mounir)]

Pushed in tracemonkey:
http://hg.mozilla.org/tracemonkey/rev/d7ea5c7f6cb7

Comment 20

[Mounir Lamouri (:mounir)]

I had to backout:
http://hg.mozilla.org/tracemonkey/rev/f84928566d5c

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - 'in' for HTMLCollection

r=jst

Comment 22

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - 'in' for HTMLSelectElement

r=jst

Comment 23

[Mounir Lamouri (:mounir)]

I sent both patches to try:
Part 1: http://tbpl.mozilla.org/?tree=MozillaTry&rev=6244d467acb9
Part 2: http://tbpl.mozilla.org/?tree=MozillaTry&rev=78d2f491d7ba

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 514836 [details] [diff] [review]
Part 1 - 'in' for HTMLCollection

Looks good. Thanks!

Comment 25

[Johnny Stenback (:jst)]

Both parts landed (with a change to default 'rv' to NS_OK instead of making all impls set the out param, to match other code in this neck of the woods, and per discussion with mrbkap and Mounir):

http://hg.mozilla.org/mozilla-central/rev/6c27993ee809
http://hg.mozilla.org/mozilla-central/rev/c2c9c3eb5c2c