Inspector gets confused as to whether a node has children

VERIFIED FIXED in mozilla1.9.1b3

Status

()

Core
XPConnect
VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: WeirdAl, Assigned: mrbkap)

Tracking

({testcase, verified1.9.1})

Trunk
mozilla1.9.1b3
x86
Windows XP
testcase, verified1.9.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 361105 [details]
testcase document

Steps to reproduce:
(1) Open the grid-navigable.xul attachment in DOM Inspector.
(2) In the DOM Nodes panel on the left, navigate to window > gridbox > stack > second anonymous grid > columns.
(3) Switch to JS Object Viewer in the right-hand panel.

Expected results:  DOM Nodes panel shows the columns element as having children, and is expandable.  JS Object Viewer panel shows the childNodes property as having children and is also expandable.

Actual results:  DOM Nodes panel shows the columns element as having no children.  JS Object Viewer panel shows the childNodes property as having children, but you cannot expand the child rows beneath it.
(Reporter)

Comment 1

9 years ago
Created attachment 361106 [details] [diff] [review]
patch for JS Object Viewer

I don't have a patch for the DOM Nodes viewer yet.
Attachment #361106 - Flags: review?(sdwilsh)

Updated

9 years ago
Blocks: 477844
Comment on attachment 361106 [details] [diff] [review]
patch for JS Object Viewer

r=sdwilsh, if you update the javadoc comment above the method, and add a comment stating why you are forcing everything to a string.
Attachment #361106 - Flags: superreview?(neil)
Attachment #361106 - Flags: review?(sdwilsh)
Attachment #361106 - Flags: review+

Comment 3

9 years ago
I don't think this is right. For some reason, the enumerator for an XPCSafeJSObjectWrapper for childNodes is returning the properties as numbers, not strings. For example, evaluate these in the JS console (opening the JS console from a blank browser tab for consistency):

>for (var prop in XPCSafeJSObjectWrapper(top.opener.content.document.childNodes)) dump(prop + "(" + (typeof prop) + ")\n")
0(number)
1(number)
length(string)
item(string)

>for (var prop in XPCNativeWrapper(top.opener.content.document.childNodes)) dump(prop + "(" + (typeof prop) + ")\n")
0(string)
1(string)

>for (var prop in (top.opener.content.document.childNodes)) dump(prop + "(" + (typeof prop) + ")\n")
0(string)
1(string)
length(string)
item(string)

Comment 4

9 years ago
Comment on attachment 361106 [details] [diff] [review]
patch for JS Object Viewer

This is actually the wrong fix for two reasons. The underlying bug is that XPConnect is enumerating numeric properties, although ES3 requires strings. Even if numeric properties were expected, we would want to stringise them during enumeration and not once every comparison.
Attachment #361106 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 5

9 years ago
I got this one.
Status: NEW → ASSIGNED
Component: DOM Inspector → XPConnect
Product: Other Applications → Core
QA Contact: dom-inspector → xpconnect
Target Milestone: --- → mozilla1.9.1b3
(Assignee)

Comment 6

9 years ago
Created attachment 361874 [details] [diff] [review]
Fix
Assignee: nobody → mrbkap
Attachment #361874 - Flags: superreview?(jst)
Attachment #361874 - Flags: review?(jst)
(Assignee)

Updated

9 years ago
Attachment #361106 - Attachment is obsolete: true

Updated

9 years ago
Attachment #361874 - Flags: superreview?(jst)
Attachment #361874 - Flags: superreview+
Attachment #361874 - Flags: review?(jst)
Attachment #361874 - Flags: review+
(Assignee)

Comment 7

9 years ago
This doesn't block, but it's a potential compatibility problem. I'll request approval for the 1.9 branch after I get this on trunk.
Flags: wanted1.9.1?
(Assignee)

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/48f66b2b3c61
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

9 years ago
I wonder what an automated test for this would look like.  The testcase in comment 0 is clearly too big, and requires DOM Inspector to actually test.
(Assignee)

Comment 10

9 years ago
Created attachment 364280 [details] [diff] [review]
Mochitest

This could be done with an xpcshell "simple" test, but since we already have the test_wrapper.html file, this will do.
Attachment #364280 - Flags: review?(jst)
(Assignee)

Updated

9 years ago
Attachment #361874 - Flags: approval1.9.1?

Updated

9 years ago
Attachment #364280 - Flags: superreview+
Attachment #364280 - Flags: review?(jst)
Attachment #364280 - Flags: review+
(Assignee)

Comment 11

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2357b4fd2560
Flags: in-testsuite+
Comment on attachment 361874 [details] [diff] [review]
Fix

a191=beltzner, please make sure to also land the test
Attachment #361874 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 13

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e0d8d4951c82 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f63833f5f97d
Keywords: fixed1.9.1
verified FIXED on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090721
Minefield/3.6a1pre ID:20090721044139

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090720
Shiretoko/3.5.1pre ID:20090720042942
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
(Reporter)

Comment 15

7 years ago
mrbkap: I think the Mochitest you landed for this bug (comment 10) is actually not testing the number correctly.

js> var x = {0: 3};
js> var y = "";
js> for (var prop in x) { y += (typeof prop) + "\n"; }
string

js> y
string

In other words, the property names are strings before you pass them into XPCSafeJSObjectWrapper(...).
(Assignee)

Comment 16

7 years ago
Alex, your test doesn't work the way you think it does -- for-in string-izes for you. Internally, we keep the properties as their original types and only convert when they become user-visible.
(Assignee)

Updated

2 years ago
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.