Closed Bug 633133 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- final+

People

(Reporter: aeneasdardanus, Assigned: mounir)

References

Details

(Keywords: testcase, Whiteboard: [hardblocker])

Attachments

(3 files, 3 obsolete files)

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"
This isn't a security issue.
Group: core-security
Component: DOM: Traversal-Range → DOM: Core & HTML
QA Contact: traversal-range → general
jst, is there a reason nsNamedArraySH doesn't have a resolve hook?  Seems like it should...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Bad example: the in operator - returns false → Testing for named items in HTMLCollections using 'in' operator always returns false
I've reported it as DOM_Traversal bug. [?!!] Why do you say so?
(In reply to comment #1)
> This isn't a security issue.
Attached file Testcase
Severity: major → normal
Keywords: testcase
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(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."
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."
(In reply to comment #4)
> Created attachment 512054 [details]
> Testcase

Big thanks for creating and providing that live testcase
Regards
aeneas, please don't overquote.

The "why" is in comment 2.

The "chances" depend on the answer to the question in that comment.
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
(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?
I can't think of anything right now.
Attached patch Patch v1 (obsolete) — Splinter Review
This might fix it.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #512632 - Flags: review?(jst)
Whiteboard: [post-2.0][needs review]
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 :)
Attachment #512632 - Flags: review?(jst) → review+
Attached patch Patch v1.1 (obsolete) — Splinter Review
r=jst

And it still passes tests ;)
Attachment #512632 - Attachment is obsolete: true
Whiteboard: [post-2.0][needs review] → [can land][post-2.0]
Whiteboard: [can land][post-2.0] → [can land]
Duplicate of this bug: 635715
blocking2.0: --- → final+
Whiteboard: [can land] → [can land][hardblocker]
Attached patch Mounir's patch, expanded (obsolete) — Splinter Review
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 ;-)
Attachment #514684 - Flags: review?(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.
Attachment #514684 - Flags: review?(jst) → review+
TM tree is open. Land it there.
Whiteboard: [can land][hardblocker] → [can land][hardblocker][has patch]
Pushed in tracemonkey:
http://hg.mozilla.org/tracemonkey/rev/d7ea5c7f6cb7
Whiteboard: [can land][hardblocker][has patch] → [hardblocker][has patch][fixed-in-tracemonkey]
I had to backout:
http://hg.mozilla.org/tracemonkey/rev/f84928566d5c
Whiteboard: [hardblocker][has patch][fixed-in-tracemonkey] → [hardblocker][has patch]
Whiteboard: [hardblocker][has patch] → [hardblocker]
r=jst
Attachment #513584 - Attachment is obsolete: true
Attachment #514684 - Attachment is obsolete: true
Attachment #514836 - Flags: review?(mrbkap)
Whiteboard: [hardblocker] → [hardblocker][has patch][needs review][needs try]
Comment on attachment 514836 [details] [diff] [review]
Part 1 - 'in' for HTMLCollection

Looks good. Thanks!
Attachment #514836 - Flags: review?(mrbkap) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Whiteboard: [hardblocker][has patch][needs review][needs try] → [hardblocker]
Target Milestone: --- → mozilla2.0
Depends on: 637019
Duplicate of this bug: 642330
Duplicate of this bug: 644111
Duplicate of this bug: 644039
You need to log in before you can comment on or make changes to this bug.