From da22cd5b718adabd7014e1e77ff0b257c7914227 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 5 Nov 2012 11:58:03 -0500 Subject: [PATCH] Bug 772869. Make getOwnPropertyNames work correctly for WebIDL proxy bindings. r=peterv,ms2ger --- content/base/src/nsContentList.cpp | 37 ++++++++++++ content/base/src/nsContentList.h | 1 + content/base/src/nsDOMLists.h | 5 ++ .../html/content/public/nsIHTMLCollection.h | 2 + .../content/src/HTMLPropertiesCollection.cpp | 7 +++ .../content/src/HTMLPropertiesCollection.h | 1 + .../html/content/src/nsHTMLFormElement.cpp | 20 +++++++ .../html/content/src/nsHTMLSelectElement.cpp | 31 ++++++++++ .../html/content/src/nsHTMLSelectElement.h | 1 + .../html/content/src/nsHTMLTableElement.cpp | 19 ++++++ content/html/content/test/Makefile.in | 4 ++ .../html/content/test/test_formelements.html | 60 +++++++++++++++++++ .../content/test/test_htmlcollection.html | 48 +++++++++++++++ .../html/content/test/test_named_options.html | 52 ++++++++++++++++ .../content/test/test_rowscollection.html | 60 +++++++++++++++++++ dom/bindings/Codegen.py | 19 +++++- dom/bindings/DOMJSProxyHandler.cpp | 26 ++++++++ dom/bindings/DOMJSProxyHandler.h | 6 ++ dom/bindings/test/TestBindingHeader.h | 8 +++ .../tests/submissions/Ms2ger/Makefile.in | 1 - .../Ms2ger/test_Element-children.html.json | 3 - .../submission/Opera/microdata/test_001.html | 6 ++ 22 files changed, 411 insertions(+), 6 deletions(-) create mode 100644 content/html/content/test/test_formelements.html create mode 100644 content/html/content/test/test_htmlcollection.html create mode 100644 content/html/content/test/test_named_options.html create mode 100644 content/html/content/test/test_rowscollection.html delete mode 100644 dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/test_Element-children.html.json diff --git a/content/base/src/nsContentList.cpp b/content/base/src/nsContentList.cpp index 896ab150dfb..0a62a3418fc 100644 --- a/content/base/src/nsContentList.cpp +++ b/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& aNames) +{ + BringSelfUpToDate(true); + + nsAutoTArray 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) { diff --git a/content/base/src/nsContentList.h b/content/base/src/nsContentList.h index 84fbec0e021..ef591d4d436 100644 --- a/content/base/src/nsContentList.h +++ b/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& aNames); // nsContentList public methods NS_HIDDEN_(uint32_t) Length(bool aDoFlush); diff --git a/content/base/src/nsDOMLists.h b/content/base/src/nsDOMLists.h index c16dd6d60e3..ef18c506003 100644 --- a/content/base/src/nsDOMLists.h +++ b/content/base/src/nsDOMLists.h @@ -33,6 +33,11 @@ public: mNames.Clear(); } + void CopyList(nsTArray& aNames) + { + aNames = mNames; + } + private: nsTArray mNames; }; diff --git a/content/html/content/public/nsIHTMLCollection.h b/content/html/content/public/nsIHTMLCollection.h index 280c94864e9..21a7fbac6c7 100644 --- a/content/html/content/public/nsIHTMLCollection.h +++ b/content/html/content/public/nsIHTMLCollection.h @@ -64,6 +64,8 @@ public: found = !!namedItem; return namedItem; } + + virtual void GetSupportedNames(nsTArray& aNames) = 0; }; NS_DEFINE_STATIC_IID_ACCESSOR(nsIHTMLCollection, NS_IHTMLCOLLECTION_IID) diff --git a/content/html/content/src/HTMLPropertiesCollection.cpp b/content/html/content/src/HTMLPropertiesCollection.cpp index ebc553641a5..05922afa0d3 100644 --- a/content/html/content/src/HTMLPropertiesCollection.cpp +++ b/content/html/content/src/HTMLPropertiesCollection.cpp @@ -348,6 +348,13 @@ HTMLPropertiesCollection::CrawlSubtree(Element* aElement) } } +void +HTMLPropertiesCollection::GetSupportedNames(nsTArray& aNames) +{ + EnsureFresh(); + mNames->CopyList(aNames); +} + PropertyNodeList::PropertyNodeList(HTMLPropertiesCollection* aCollection, nsIContent* aParent, const nsAString& aName) : mName(aName), diff --git a/content/html/content/src/HTMLPropertiesCollection.h b/content/html/content/src/HTMLPropertiesCollection.h index 06c05921ba1..e731e27ee7b 100644 --- a/content/html/content/src/HTMLPropertiesCollection.h +++ b/content/html/content/src/HTMLPropertiesCollection.h @@ -77,6 +77,7 @@ public: EnsureFresh(); return mNames; } + virtual void GetSupportedNames(nsTArray& aNames); NS_DECL_CYCLE_COLLECTING_ISUPPORTS NS_DECL_NSIDOMHTMLPROPERTIESCOLLECTION diff --git a/content/html/content/src/nsHTMLFormElement.cpp b/content/html/content/src/nsHTMLFormElement.cpp index ccb3df787ee..64260a0004f 100644 --- a/content/html/content/src/nsHTMLFormElement.cpp +++ b/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& 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*>(aClosure)->AppendElement(aName); + return PL_DHASH_NEXT; +} + +void +nsFormControlList::GetSupportedNames(nsTArray& 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); +} diff --git a/content/html/content/src/nsHTMLSelectElement.cpp b/content/html/content/src/nsHTMLSelectElement.cpp index 654a9c386fd..9daf5eb76d4 100644 --- a/content/html/content/src/nsHTMLSelectElement.cpp +++ b/content/html/content/src/nsHTMLSelectElement.cpp @@ -2196,6 +2196,37 @@ nsHTMLOptionCollection::NamedItem(JSContext* cx, const nsAString& name, return &v.toObject(); } +void +nsHTMLOptionCollection::GetSupportedNames(nsTArray& aNames) +{ + nsAutoTArray 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) { diff --git a/content/html/content/src/nsHTMLSelectElement.h b/content/html/content/src/nsHTMLSelectElement.h index 468a3db20dd..a72bd62ee6f 100644 --- a/content/html/content/src/nsHTMLSelectElement.h +++ b/content/html/content/src/nsHTMLSelectElement.h @@ -141,6 +141,7 @@ public: { aError = SetOption(aIndex, aOption); } + virtual void GetSupportedNames(nsTArray& aNames); private: /** The list of options (holds strong references). This is infallible, so diff --git a/content/html/content/src/nsHTMLTableElement.cpp b/content/html/content/src/nsHTMLTableElement.cpp index 12d76cfdaca..b6a1277787f 100644 --- a/content/html/content/src/nsHTMLTableElement.cpp +++ b/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& aNames); NS_IMETHOD ParentDestroyed(); @@ -273,6 +274,24 @@ TableRowsCollection::NamedItem(JSContext* cx, const nsAString& name, return nullptr; } +void +TableRowsCollection::GetSupportedNames(nsTArray& aNames) +{ + DO_FOR_EACH_ROWGROUP( + nsTArray names; + nsCOMPtr 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) diff --git a/content/html/content/test/Makefile.in b/content/html/content/test/Makefile.in index 5fe2565e444..d3af05d202d 100644 --- a/content/html/content/test/Makefile.in +++ b/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 = \ diff --git a/content/html/content/test/test_formelements.html b/content/html/content/test/test_formelements.html new file mode 100644 index 00000000000..16652ce91ad --- /dev/null +++ b/content/html/content/test/test_formelements.html @@ -0,0 +1,60 @@ + + + + + + Test for Bug 772869 + + + + +Mozilla Bug 772869 +

+ +
+
+
+ + diff --git a/content/html/content/test/test_htmlcollection.html b/content/html/content/test/test_htmlcollection.html new file mode 100644 index 00000000000..715877d2d21 --- /dev/null +++ b/content/html/content/test/test_htmlcollection.html @@ -0,0 +1,48 @@ + + + + + + Test for Bug 772869 + + + + +Mozilla Bug 772869 +

+ +
+
+
+ + diff --git a/content/html/content/test/test_named_options.html b/content/html/content/test/test_named_options.html new file mode 100644 index 00000000000..8753da9d69a --- /dev/null +++ b/content/html/content/test/test_named_options.html @@ -0,0 +1,52 @@ + + + + + + Test for Bug 772869 + + + + +Mozilla Bug 772869 +

+ +
+
+
+ + diff --git a/content/html/content/test/test_rowscollection.html b/content/html/content/test/test_rowscollection.html new file mode 100644 index 00000000000..2eed607cb3b --- /dev/null +++ b/content/html/content/test/test_rowscollection.html @@ -0,0 +1,60 @@ + + + + + + Test for Bug 772869 + + + + +Mozilla Bug 772869 +

+ +
+
+
+ + diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py index ba7e20779c8..17fd1cf47b3 100644 --- a/dom/bindings/Codegen.py +++ b/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 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): diff --git a/dom/bindings/DOMJSProxyHandler.cpp b/dom/bindings/DOMJSProxyHandler.cpp index a22e9208972..c6f21cfd67e 100644 --- a/dom/bindings/DOMJSProxyHandler.cpp +++ b/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& 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) { diff --git a/dom/bindings/DOMJSProxyHandler.h b/dom/bindings/DOMJSProxyHandler.h index 94e0954d784..3d5226418f9 100644 --- a/dom/bindings/DOMJSProxyHandler.h +++ b/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& names, + JS::AutoIdVector& props); }; extern jsid s_length_id; diff --git a/dom/bindings/test/TestBindingHeader.h b/dom/bindings/test/TestBindingHeader.h index 40b2ad4b727..cdd47b3dfa2 100644 --- a/dom/bindings/test/TestBindingHeader.h +++ b/dom/bindings/test/TestBindingHeader.h @@ -612,6 +612,7 @@ public: virtual nsISupports* GetParentObject(); void NamedGetter(const nsAString&, bool&, nsAString&); + void GetSupportedNames(nsTArray&); }; 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&); }; class TestIndexedSetterInterface : public nsISupports, @@ -652,6 +654,7 @@ public: virtual nsISupports* GetParentObject(); void NamedSetter(const nsAString&, TestIndexedSetterInterface&); + void GetSupportedNames(nsTArray&); }; 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&); }; class TestIndexedAndNamedGetterAndSetterInterface : public TestIndexedSetterInterface @@ -680,6 +684,7 @@ public: void NamedSetter(const nsAString&, const nsAString&); void Stringify(nsAString&); uint32_t Length(); + void GetSupportedNames(nsTArray&); }; class TestCppKeywordNamedMethodsInterface : public nsISupports, @@ -736,6 +741,7 @@ public: virtual nsISupports* GetParentObject(); void NamedDeleter(const nsAString&, bool&); + void GetSupportedNames(nsTArray&); }; 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&); }; 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&); }; } // namespace dom diff --git a/dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/Makefile.in b/dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/Makefile.in index a6f4c1725e9..3f8a623134a 100644 --- a/dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/Makefile.in +++ b/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 \ diff --git a/dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/test_Element-children.html.json b/dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/test_Element-children.html.json deleted file mode 100644 index f2017e74651..00000000000 --- a/dom/imptests/failures/webapps/DOMCore/tests/submissions/Ms2ger/test_Element-children.html.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "HTMLCollection edge cases 1": true -} diff --git a/dom/imptests/html/tests/submission/Opera/microdata/test_001.html b/dom/imptests/html/tests/submission/Opera/microdata/test_001.html index cf583293be2..13c8868462f 100644 --- a/dom/imptests/html/tests/submission/Opera/microdata/test_001.html +++ b/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'},'
'); + 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'},'
'); assert_equals( testEl.properties.item(4), null, 'positive index' );