Bug 1245554. Window's named properties object should not claim to have duplicates of a given property name if it has multiple iframes with that name. r=peterv

The web platform test was pretty buggy in a few ways:

1)  It claimed that "bar" should be non-writable and enumerable.  This
explicitly contradicts the spec:
http://heycam.github.io/webidl/#named-properties-object-getownproperty step 3
substep 8 sets [[Writable]] to true, and
https://html.spec.whatwg.org/multipage/browsers.html#named-access-on-the-window-object
explicitly says these properties are unenumerable.

2)  It claimed that "constructor" should be exposed on the named properties
object.  But the named property visibility algorithm obviously returns false
for that property name, so it should not be exposed.

dom/base/WindowNamedPropertiesHandler.cpp

@@ -9,6 +9,7 @@
 #include "mozilla/dom/WindowBinding.h"
 #include "nsContentUtils.h"
 #include "nsDOMClassInfo.h"
+#include "nsDOMWindowList.h"
 #include "nsGlobalWindow.h"
 #include "nsHTMLDocument.h"
 #include "nsJSUtils.h"
@@ -175,14 +176,28 @@ WindowNamedPropertiesHandler::ownPropNames(JSContext* aCx,
   // Grab the DOM window.
   nsGlobalWindow* win = xpc::WindowOrNull(JS_GetGlobalForObject(aCx, aProxy));
   nsTArray<nsString> names;
-  win->GetSupportedNames(names);
-  // Filter out the ones we wouldn't expose from getOwnPropertyDescriptor.
-  // We iterate backwards so we can remove things from the list easily.
-  for (size_t i = names.Length(); i > 0; ) {
-    --i; // Now we're pointing at the next name we want to look at
-    nsCOMPtr<nsPIDOMWindowOuter> childWin = win->GetChildWindow(names[i]);
-    if (!childWin || !ShouldExposeChildWindow(names[i], childWin)) {
-      names.RemoveElementAt(i);
+  // The names live on the outer window, which might be null
+  nsGlobalWindow* outer = win->GetOuterWindowInternal();
+  if (outer) {
+    nsDOMWindowList* childWindows = outer->GetWindowList();
+    uint32_t length = childWindows->GetLength();
+    for (uint32_t i = 0; i < length; ++i) {
+      nsCOMPtr<nsIDocShellTreeItem> item =
+        childWindows->GetDocShellTreeItemAt(i);
+      // This is a bit silly, since we could presumably just do
+      // item->GetWindow().  But it's not obvious whether this does the same
+      // thing as GetChildWindow() with the item's name (due to the complexity
+      // of FindChildWithName).  Since GetChildWindow is what we use in
+      // getOwnPropDescriptor, let's try to be consistent.
+      nsString name;
+      item->GetName(name);
+      if (!names.Contains(name)) {
+        // Make sure we really would expose it from getOwnPropDescriptor.
+        nsCOMPtr<nsPIDOMWindowOuter> childWin = win->GetChildWindow(name);
+        if (childWin && ShouldExposeChildWindow(name, childWin)) {
+          names.AppendElement(name);
+        }
+      }
     }
   }
   if (!AppendNamedPropertyIds(aCx, aProxy, names, false, aProps)) {

dom/base/nsGlobalWindow.cpp

@@ -4186,23 +4186,6 @@ nsGlobalWindow::IndexedGetter(uint32_t aIndex)
   MOZ_CRASH();
 }
 
-void
-nsGlobalWindow::GetSupportedNames(nsTArray<nsString>& aNames)
-{
-  FORWARD_TO_OUTER_VOID(GetSupportedNames, (aNames));
-
-  nsDOMWindowList* windows = GetWindowList();
-  if (windows) {
-    uint32_t length = windows->GetLength();
-    nsString* name = aNames.AppendElements(length);
-    for (uint32_t i = 0; i < length; ++i, ++name) {
-      nsCOMPtr<nsIDocShellTreeItem> item =
-        windows->GetDocShellTreeItemAt(i);
-      item->GetName(*name);
-    }
-  }
-}
-
 bool
 nsGlobalWindow::DoResolve(JSContext* aCx, JS::Handle<JSObject*> aObj,
                           JS::Handle<jsid> aId,

dom/base/nsGlobalWindow.h

@@ -504,8 +504,6 @@ public:
   already_AddRefed<nsPIDOMWindowOuter> IndexedGetterOuter(uint32_t aIndex);
   already_AddRefed<nsPIDOMWindowOuter> IndexedGetter(uint32_t aIndex);
 
-  void GetSupportedNames(nsTArray<nsString>& aNames);
-
   static bool IsPrivilegedChromeWindow(JSContext* /* unused */, JSObject* aObj);
 
   static bool IsShowModalDialogEnabled(JSContext* /* unused */ = nullptr,
@@ -1604,9 +1602,11 @@ protected:
                     const RefPtr<mozilla::dom::StorageEvent>& aEvent,
                     mozilla::ErrorResult& aRv);
 
+public:
   // Outer windows only.
   nsDOMWindowList* GetWindowList();
 
+protected:
   // Helper for getComputedStyle and getDefaultComputedStyle
   already_AddRefed<nsICSSDeclaration>
     GetComputedStyleHelperOuter(mozilla::dom::Element& aElt,

dom/imptests/failures/html/html/browsers/the-window-object/test_window-named-properties.html.json

@@ -1,4 +1,2 @@
 {
-  "Static name on the prototype": true,
-  "constructor": true
 }

dom/imptests/html/html/browsers/the-window-object/test_window-named-properties.html

@@ -31,7 +31,7 @@ test(function() {
   assert_true("bar" in gsp, "bar in gsp");
   assert_true(gsp.hasOwnProperty("bar"), "gsp.hasOwnProperty(\"bar\")");
   assert_data_propdesc(Object.getOwnPropertyDescriptor(gsp, "bar"),
-                       false, true, true);
+                       true, false, true);
 }, "Static name on the prototype");
 test(function() {
   assert_equals(window.constructor, Window);
@@ -45,9 +45,8 @@ test(function() {
 
   var gsp = Object.getPrototypeOf(proto);
   assert_true("constructor" in gsp, "constructor in gsp");
-  assert_true(gsp.hasOwnProperty("constructor"), "gsp.hasOwnProperty(\"constructor\")");
-  assert_data_propdesc(Object.getOwnPropertyDescriptor(gsp, "constructor"),
-                       false, true, true);
+  assert_false(gsp.hasOwnProperty("constructor"), "gsp.hasOwnProperty(\"constructor\")");
+  assert_equals(Object.getOwnPropertyDescriptor(gsp, "constructor"), undefined)
 }, "constructor");
 var t = async_test("Dynamic name")
 var t2 = async_test("Ghost name")

testing/web-platform/meta/html/browsers/the-window-object/window-named-properties.html.ini

@@ -1,8 +0,0 @@
-[window-named-properties.html]
-  type: testharness
-  [Static name on the prototype]
-    expected: FAIL
-
-  [constructor]
-    expected: FAIL
-

testing/web-platform/tests/html/browsers/the-window-object/window-named-properties.html

@@ -10,6 +10,8 @@
 <script src="/resources/testharnessreport.js"></script>
 <div id=log></div>
 <iframe name="bar"></iframe>
+<iframe name="baz"></iframe>
+<iframe name="baz"></iframe>
 <iframe name="constructor"></iframe>
 <script>
 function assert_data_propdesc(pd, Writable, Enumerable, Configurable) {
@@ -31,7 +33,7 @@ test(function() {
   assert_true("bar" in gsp, "bar in gsp");
   assert_true(gsp.hasOwnProperty("bar"), "gsp.hasOwnProperty(\"bar\")");
   assert_data_propdesc(Object.getOwnPropertyDescriptor(gsp, "bar"),
-                       false, true, true);
+                       true, false, true);
 }, "Static name on the prototype");
 test(function() {
   assert_equals(window.constructor, Window);
@@ -45,10 +47,15 @@ test(function() {
 
   var gsp = Object.getPrototypeOf(proto);
   assert_true("constructor" in gsp, "constructor in gsp");
-  assert_true(gsp.hasOwnProperty("constructor"), "gsp.hasOwnProperty(\"constructor\")");
-  assert_data_propdesc(Object.getOwnPropertyDescriptor(gsp, "constructor"),
-                       false, true, true);
+  assert_false(gsp.hasOwnProperty("constructor"), "gsp.hasOwnProperty(\"constructor\")");
+  assert_equals(Object.getOwnPropertyDescriptor(gsp, "constructor"), undefined);
 }, "constructor");
+test(function() {
+  var gsp = Object.getPrototypeOf(Object.getPrototypeOf(window));
+  var names = Object.getOwnPropertyNames(gsp);
+  assert_equals(names.filter((name) => name == "baz").length, 1);
+
+}, "duplicate property names")
 var t = async_test("Dynamic name")
 var t2 = async_test("Ghost name")
 t.step(function() {