Bug 772869. Make getOwnPropertyNames work correctly for WebIDL proxy bindings. r=peterv,ms2ger

content/base/src/nsContentList.cpp

@@ -22,6 +22,7 @@
 #include "mozilla/dom/HTMLCollectionBinding.h"
 #include "mozilla/dom/NodeListBinding.h"
 #include "mozilla/Likely.h"
+#include "nsGenericHTMLElement.h"
 
 // Form related includes
 #include "nsIDOMHTMLFormElement.h"
@@ -600,6 +601,42 @@ nsContentList::NamedItem(const nsAString& aName, bool aDoFlush)
   return nullptr;
 }
 
+void
+nsContentList::GetSupportedNames(nsTArray<nsString>& aNames)
+{
+  BringSelfUpToDate(true);
+
+  nsAutoTArray<nsIAtom*, 8> atoms;
+  for (uint32_t i = 0; i < mElements.Length(); ++i) {
+    nsIContent *content = mElements.ElementAt(i);
+    nsGenericHTMLElement* el = nsGenericHTMLElement::FromContent(content);
+    if (el) {
+      // XXXbz should we be checking for particular tags here?  How
+      // stable is this part of the spec?
+      // Note: nsINode::HasName means the name is exposed on the document,
+      // which is false for options, so we don't check it here.
+      const nsAttrValue* val = el->GetParsedAttr(nsGkAtoms::name);
+      if (val && val->Type() == nsAttrValue::eAtom) {
+        nsIAtom* name = val->GetAtomValue();
+        if (!atoms.Contains(name)) {
+          atoms.AppendElement(name);
+        }
+      }
+    }
+    if (content->HasID()) {
+      nsIAtom* id = content->GetID();
+      if (!atoms.Contains(id)) {
+        atoms.AppendElement(id);
+      }
+    }
+  }
+
+  aNames.SetCapacity(atoms.Length());
+  for (uint32_t i = 0; i < atoms.Length(); ++i) {
+    aNames.AppendElement(nsDependentAtomString(atoms[i]));
+  }
+}
+
 int32_t
 nsContentList::IndexOf(nsIContent *aContent, bool aDoFlush)
 {

content/base/src/nsContentList.h

@@ -278,6 +278,7 @@ public:
   virtual nsGenericElement* GetElementAt(uint32_t index);
   virtual JSObject* NamedItem(JSContext* cx, const nsAString& name,
                               mozilla::ErrorResult& error);
+  virtual void GetSupportedNames(nsTArray<nsString>& aNames);
 
   // nsContentList public methods
   NS_HIDDEN_(uint32_t) Length(bool aDoFlush);

content/base/src/nsDOMLists.h

@@ -33,6 +33,11 @@ public:
     mNames.Clear();
   }
 
+  void CopyList(nsTArray<nsString>& aNames)
+  {
+    aNames = mNames;
+  }
+
 private:
   nsTArray<nsString> mNames;
 };

content/html/content/public/nsIHTMLCollection.h

@@ -64,6 +64,8 @@ public:
     found = !!namedItem;
     return namedItem;
   }
+
+  virtual void GetSupportedNames(nsTArray<nsString>& aNames) = 0;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(nsIHTMLCollection, NS_IHTMLCOLLECTION_IID)

content/html/content/src/HTMLPropertiesCollection.cpp

@@ -348,6 +348,13 @@ HTMLPropertiesCollection::CrawlSubtree(Element* aElement)
   }                    
 }
 
+void
+HTMLPropertiesCollection::GetSupportedNames(nsTArray<nsString>& aNames)
+{
+  EnsureFresh();
+  mNames->CopyList(aNames);
+}
+
 PropertyNodeList::PropertyNodeList(HTMLPropertiesCollection* aCollection,
                                    nsIContent* aParent, const nsAString& aName)
   : mName(aName),

content/html/content/src/HTMLPropertiesCollection.h

@@ -77,6 +77,7 @@ public:
     EnsureFresh();
     return mNames;
   }
+  virtual void GetSupportedNames(nsTArray<nsString>& aNames);
 
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_NSIDOMHTMLPROPERTIESCOLLECTION

content/html/content/src/nsHTMLFormElement.cpp

@@ -100,6 +100,7 @@ public:
 
   virtual JSObject* NamedItem(JSContext* cx, const nsAString& name,
                               mozilla::ErrorResult& error);
+  virtual void GetSupportedNames(nsTArray<nsString>& aNames);
 
   nsresult AddElementToTable(nsGenericHTMLFormElement* aChild,
                              const nsAString& aName);
@@ -2540,3 +2541,22 @@ nsFormControlList::NamedItem(JSContext* cx, const nsAString& name,
   }
   return &v.toObject();
 }
+
+static PLDHashOperator
+CollectNames(const nsAString& aName,
+             nsISupports* /* unused */,
+             void* aClosure)
+{
+  static_cast<nsTArray<nsString>*>(aClosure)->AppendElement(aName);
+  return PL_DHASH_NEXT;
+}
+
+void
+nsFormControlList::GetSupportedNames(nsTArray<nsString>& aNames)
+{
+  FlushPendingNotifications();
+  // Just enumerate mNameLookupTable.  This won't guarantee order, but
+  // that's OK, because the HTML5 spec doesn't define an order for
+  // this enumeration.
+  mNameLookupTable.EnumerateRead(CollectNames, &aNames);
+}

content/html/content/src/nsHTMLSelectElement.cpp

@@ -2196,6 +2196,37 @@ nsHTMLOptionCollection::NamedItem(JSContext* cx, const nsAString& name,
   return &v.toObject();
 }
 
+void
+nsHTMLOptionCollection::GetSupportedNames(nsTArray<nsString>& aNames)
+{
+  nsAutoTArray<nsIAtom*, 8> atoms;
+  for (uint32_t i = 0; i < mElements.Length(); ++i) {
+    nsHTMLOptionElement *content = mElements.ElementAt(i);
+    if (content) {
+      // Note: HasName means the names is exposed on the document,
+      // which is false for options, so we don't check it here.
+      const nsAttrValue* val = content->GetParsedAttr(nsGkAtoms::name);
+      if (val && val->Type() == nsAttrValue::eAtom) {
+        nsIAtom* name = val->GetAtomValue();
+        if (!atoms.Contains(name)) {
+          atoms.AppendElement(name);
+        }
+      }
+      if (content->HasID()) {
+        nsIAtom* id = content->GetID();
+        if (!atoms.Contains(id)) {
+          atoms.AppendElement(id);
+        }
+      }
+    }
+  }
+
+  aNames.SetCapacity(atoms.Length());
+  for (uint32_t i = 0; i < atoms.Length(); ++i) {
+    aNames.AppendElement(nsDependentAtomString(atoms[i]));
+  }
+}
+
 NS_IMETHODIMP
 nsHTMLOptionCollection::GetSelect(nsIDOMHTMLSelectElement **aReturn)
 {

content/html/content/src/nsHTMLSelectElement.h

@@ -141,6 +141,7 @@ public:
   {
     aError = SetOption(aIndex, aOption);
   }
+  virtual void GetSupportedNames(nsTArray<nsString>& aNames);
 
 private:
   /** The list of options (holds strong references).  This is infallible, so

content/html/content/src/nsHTMLTableElement.cpp

@@ -52,6 +52,7 @@ public:
 
   virtual JSObject* NamedItem(JSContext* cx, const nsAString& name,
                               ErrorResult& error);
+  virtual void GetSupportedNames(nsTArray<nsString>& aNames);
 
   NS_IMETHOD    ParentDestroyed();
 
@@ -273,6 +274,24 @@ TableRowsCollection::NamedItem(JSContext* cx, const nsAString& name,
   return nullptr;
 }
 
+void
+TableRowsCollection::GetSupportedNames(nsTArray<nsString>& aNames)
+{
+  DO_FOR_EACH_ROWGROUP(
+    nsTArray<nsString> names;
+    nsCOMPtr<nsIHTMLCollection> coll = do_QueryInterface(rows);
+    if (coll) {
+      coll->GetSupportedNames(names);
+      for (uint32_t i = 0; i < names.Length(); ++i) {
+        if (!aNames.Contains(names[i])) {
+          aNames.AppendElement(names[i]);
+        }
+      }
+    }
+  );
+}
+
+
 NS_IMETHODIMP 
 TableRowsCollection::NamedItem(const nsAString& aName,
                                nsIDOMNode** aReturn)

content/html/content/test/Makefile.in

@@ -329,6 +329,10 @@ MOCHITEST_FILES = \
 		test_iframe_sandbox_workers.html \
 		file_iframe_sandbox_g_if1.html \
 		file_iframe_sandbox_worker.js \
+		test_named_options.html \
+		test_htmlcollection.html \
+		test_formelements.html \
+		test_rowscollection.html \
 		$(NULL)
 
 MOCHITEST_BROWSER_FILES = \

content/html/content/test/test_formelements.html

@@ -0,0 +1,60 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=772869
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 772869</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=772869">Mozilla Bug 772869</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <form id="f">
+    <input name="x">
+    <input type="image" name="a">
+    <input type="file" name="y">
+    <input type="submit" name="z">
+    <input id="w">
+    <input name="w">
+  </form>
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 772869 **/
+var x = $("f").elements;
+x.something = "another";
+names = [];
+for (var name in x) {
+  names.push(name);
+}
+is(names.length, 14, "Should have 14 items");
+// Now sort entries 5 through 8, for comparison purposes.  We don't sort the
+// whole array, because we want to make sure the ordering between the parts
+// is correct
+temp = names.slice(5, 9);
+temp.sort();
+names.splice.bind(names, 5, 4).apply(null, temp);
+is(names.length, 14, "Should have still have 14 items");
+is(names[0], "0", "Entry 1")
+is(names[1], "1", "Entry 2")
+is(names[2], "2", "Entry 3")
+is(names[3], "3", "Entry 4")
+is(names[4], "4", "Entry 5")
+is(names[5], "w", "Entry 6")
+is(names[6], "x", "Entry 7")
+is(names[7], "y", "Entry 8")
+is(names[8], "z", "Entry 9")
+is(names[9], "something", "Entry 10")
+is(names[10], "item", "Entry 11")
+is(names[11], "namedItem", "Entry 12")
+is(names[12], "iterator", "Entry 13")
+is(names[13], "length", "Entry 14")
+</script>
+</pre>
+</body>
+</html>

content/html/content/test/test_htmlcollection.html

@@ -0,0 +1,48 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=772869
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 772869</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=772869">Mozilla Bug 772869</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <a class="foo" name="x"></a>
+  <span class="foo" id="y"></span>
+  <span class="foo" name="x"></span>
+  <form class="foo" name="z" id="w"></form>
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 772869 **/
+var x = document.getElementsByClassName("foo");
+x.something = "another";
+names = [];
+for (var name in x) {
+  names.push(name);
+}
+is(names.length, 13, "Should have 13 items");
+is(names[0], "0", "Entry 1")
+is(names[1], "1", "Entry 2")
+is(names[2], "2", "Entry 3")
+is(names[3], "3", "Entry 4")
+is(names[4], "x", "Entry 5")
+is(names[5], "y", "Entry 6")
+is(names[6], "z", "Entry 7")
+is(names[7], "w", "Entry 8")
+is(names[8], "something", "Entry 9")
+is(names[9], "item", "Entry 10")
+is(names[10], "namedItem", "Entry 11")
+is(names[11], "iterator", "Entry 12")
+is(names[12], "length", "Entry 13")
+</script>
+</pre>
+</body>
+</html>

content/html/content/test/test_named_options.html

@@ -0,0 +1,52 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=772869
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 772869</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=772869">Mozilla Bug 772869</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <select id="s">
+    <option name="x"></option>
+    <option name="y" id="z"></option>
+    <option name="z" id="x"></option>
+    <option id="w"></option>
+  </select>
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 772869 **/
+var opt = $("s").options;
+opt.loopy = "something"
+var names = Object.getOwnPropertyNames(opt);
+is(names.length, 9, "Should have eight entries");
+is(names[0], "0", "Entry 1")
+is(names[1], "1", "Entry 2")
+is(names[2], "2", "Entry 3")
+is(names[3], "3", "Entry 4")
+is(names[4], "x", "Entry 5")
+is(names[5], "y", "Entry 6")
+is(names[6], "z", "Entry 7")
+is(names[7], "w", "Entry 8")
+is(names[8], "loopy", "Entry 9")
+
+var names2 = [];
+for (var name in opt) {
+  names2.push(name);
+}
+for (var i = 0; i < names.length; ++i) {
+  is(names2[i], names[i], "Correct entry at " + i);
+}
+
+</script>
+</pre>
+</body>
+</html>

content/html/content/test/test_rowscollection.html

@@ -0,0 +1,60 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=772869
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 772869</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=772869">Mozilla Bug 772869</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <table id="f">
+    <thead>
+      <tr id="x"></tr>
+    </thead>
+    <tfoot>
+      <tr id="z"></tr>
+      <tr id="w"></tr>
+    </tfoot>
+    <tr id="x"></tr>
+    <tr id="y"></tr>
+    <tbody>
+      <tr id="z"></tr>
+    </tbody>
+  </table>
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 772869 **/
+var x = $("f").rows;
+x.something = "another";
+names = [];
+for (var name in x) {
+  names.push(name);
+}
+is(names.length, 15, "Should have 15 items");
+is(names[0], "0", "Entry 1")
+is(names[1], "1", "Entry 2")
+is(names[2], "2", "Entry 3")
+is(names[3], "3", "Entry 4")
+is(names[4], "4", "Entry 5")
+is(names[5], "5", "Entry 6")
+is(names[6], "x", "Entry 7")
+is(names[7], "y", "Entry 8")
+is(names[8], "z", "Entry 9")
+is(names[9], "w", "Entry 10")
+is(names[10], "something", "Entry 11")
+is(names[11], "item", "Entry 12")
+is(names[12], "namedItem", "Entry 13")
+is(names[13], "iterator", "Entry 14")
+is(names[14], "length", "Entry 15")
+</script>
+</pre>
+</body>
+</html>

dom/bindings/Codegen.py

@@ -5372,6 +5372,7 @@ class CGDOMJSProxyHandler_getOwnPropertyNames(ClassMethod):
         ClassMethod.__init__(self, "getOwnPropertyNames", "bool", args)
         self.descriptor = descriptor
     def getBody(self):
+        # Per spec, we do indices, then named props, then everything else
         indexedGetter = self.descriptor.operations['IndexedGetter']
         if indexedGetter:
             addIndices = """uint32_t length = UnwrapProxy(proxy)->Length();
@@ -5386,13 +5387,27 @@ for (int32_t i = 0; i < int32_t(length); ++i) {
         else:
             addIndices = ""
 
-        return addIndices + """JSObject* expando;
+        supportsNames = (self.descriptor.operations['NamedGetter'] or
+                         self.descriptor.operations['NamedSetter'] or
+                         self.descriptor.operations['NamedCreator'] or
+                         self.descriptor.operations['NamedDeleter'])
+        if supportsNames:
+            addNames = """nsTArray<nsString> names;
+UnwrapProxy(proxy)->GetSupportedNames(names);
+if (!AppendNamedPropertyIds(cx, proxy, names, props)) {
+  return false;
+}
+
+"""
+        else:
+            addNames = ""
+
+        return addIndices + addNames + """JSObject* expando;
 if (!xpc::WrapperFactory::IsXrayWrapper(proxy) && (expando = DOMProxyHandler::GetExpandoObject(proxy)) &&
     !js::GetPropertyNames(cx, expando, JSITER_OWNONLY | JSITER_HIDDEN, &props)) {
   return false;
 }
 
-// FIXME: https://bugzilla.mozilla.org/show_bug.cgi?id=772869 Add named items
 return true;"""
 
 class CGDOMJSProxyHandler_hasOwn(ClassMethod):

dom/bindings/DOMJSProxyHandler.cpp

@@ -215,6 +215,32 @@ DOMProxyHandler::obj_toString(JSContext* cx, const char* className)
   return str;
 }
 
+bool
+DOMProxyHandler::AppendNamedPropertyIds(JSContext* cx, JSObject* proxy,
+                                        nsTArray<nsString>& names,
+                                        JS::AutoIdVector& props)
+{
+  for (uint32_t i = 0; i < names.Length(); ++i) {
+    JS::Value v;
+    if (!xpc::NonVoidStringToJsval(cx, names[i], &v)) {
+      return false;
+    }
+
+    jsid id;
+    if (!JS_ValueToId(cx, v, &id)) {
+      return false;
+    }
+
+    if (!HasPropertyOnPrototype(cx, proxy, this, id)) {
+      if (!props.append(id)) {
+        return false;
+      }
+    }
+  }
+
+  return true;
+}
+
 int32_t
 IdToInt32(JSContext* cx, jsid id)
 {

dom/bindings/DOMJSProxyHandler.h

@@ -56,6 +56,12 @@ public:
 
 protected:
   static JSString* obj_toString(JSContext* cx, const char* className);
+
+  // Append the property names in "names" that don't live on our proto
+  // chain to "props"
+  bool AppendNamedPropertyIds(JSContext* cx, JSObject* proxy,
+                              nsTArray<nsString>& names,
+                              JS::AutoIdVector& props);
 };
 
 extern jsid s_length_id;

dom/bindings/test/TestBindingHeader.h

@@ -612,6 +612,7 @@ public:
   virtual nsISupports* GetParentObject();
 
   void NamedGetter(const nsAString&, bool&, nsAString&);
+  void GetSupportedNames(nsTArray<nsString>&);
 };
 
 class TestIndexedAndNamedGetterInterface : public nsISupports,
@@ -627,6 +628,7 @@ public:
   void NamedGetter(const nsAString&, bool&, nsAString&);
   void NamedItem(const nsAString&, nsAString&);
   uint32_t Length();
+  void GetSupportedNames(nsTArray<nsString>&);
 };
 
 class TestIndexedSetterInterface : public nsISupports,
@@ -652,6 +654,7 @@ public:
   virtual nsISupports* GetParentObject();
 
   void NamedSetter(const nsAString&, TestIndexedSetterInterface&);
+  void GetSupportedNames(nsTArray<nsString>&);
 };
 
 class TestIndexedAndNamedSetterInterface : public nsISupports,
@@ -666,6 +669,7 @@ public:
   void IndexedSetter(uint32_t, TestIndexedSetterInterface&);
   void NamedSetter(const nsAString&, TestIndexedSetterInterface&);
   void SetNamedItem(const nsAString&, TestIndexedSetterInterface&);
+  void GetSupportedNames(nsTArray<nsString>&);
 };
 
 class TestIndexedAndNamedGetterAndSetterInterface : public TestIndexedSetterInterface
@@ -680,6 +684,7 @@ public:
   void NamedSetter(const nsAString&, const nsAString&);
   void Stringify(nsAString&);
   uint32_t Length();
+  void GetSupportedNames(nsTArray<nsString>&);
 };
 
 class TestCppKeywordNamedMethodsInterface : public nsISupports,
@@ -736,6 +741,7 @@ public:
   virtual nsISupports* GetParentObject();
 
   void NamedDeleter(const nsAString&, bool&);
+  void GetSupportedNames(nsTArray<nsString>&);
 };
 
 class TestNamedDeleterWithRetvalInterface : public nsISupports,
@@ -751,6 +757,7 @@ public:
   bool NamedDeleter(const nsAString&) MOZ_DELETE;
   bool DelNamedItem(const nsAString&);
   bool DelNamedItem(const nsAString&, bool&) MOZ_DELETE;
+  void GetSupportedNames(nsTArray<nsString>&);
 };
 
 class TestIndexedAndNamedDeleterInterface : public nsISupports,
@@ -768,6 +775,7 @@ public:
   void NamedDeleter(const nsAString&) MOZ_DELETE;
   void DelNamedItem(const nsAString&);
   void DelNamedItem(const nsAString&, bool&) MOZ_DELETE;
+  void GetSupportedNames(nsTArray<nsString>&);
 };
 
 } // namespace dom

dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/Makefile.in

@@ -16,7 +16,6 @@ MOCHITEST_FILES := \
   test_DOMImplementation-createDocument.html.json \
   test_Document-createElementNS.html.json \
   test_Document-getElementsByTagName.html.json \
-  test_Element-children.html.json \
   test_Event-constructors.html.json \
   test_Event-defaultPrevented.html.json \
   test_EventTarget-dispatchEvent.html.json \

dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/test_Element-children.html.json

@@ -1,3 +0,0 @@
-{
-  "HTMLCollection edge cases 1": true
-}

dom/imptests/html/tests/submission/Opera/microdata/test_001.html

@@ -1528,6 +1528,12 @@ test(function () {
 	assert_equals( testEl.properties.item(2), testEl.childNodes[1].childNodes[0], 'item(2)' );
 	assert_equals( testEl.properties.item(3), testEl.childNodes[2], 'item(3)' );
 }, 'properties.item must give each property in tree order');
+test(function () {
+	var testEl = makeEl('div',{itemscope:'itemscope'},'<div itemprop="foo"></div><div itemprop="bar"><div itemprop="foo"></div></div><div itemprop="baz qux"></div>');
+	testEl.properties.something = "another";
+	var names = Object.getOwnPropertyNames(testEl.properties);
+	assert_array_equals( names, ["0", "1", "2", "3", "foo", "bar", "baz", "qux", "something"] );
+}, 'properties.item must have the right property names on it when enumerated');
 test(function () {
 	var testEl = makeEl('div',{itemscope:'itemscope'},'<div itemprop="foo"></div><div itemprop="bar"><div itemprop="foo"></div></div><div itemprop="baz qux"></div>');
 	assert_equals( testEl.properties.item(4), null, 'positive index' );