Bug 1372371. Fix enumerability handling in the window resolve hook. r=qdot
authorBoris Zbarsky <bzbarsky@mit.edu>
Mon, 12 Jun 2017 22:17:03 -0400
changeset 363546 6977697a2dcf830929b1c4f9babbb006792de926
parent 363545 fcc48fdcf68ad5d3b36c9fb9a200199586265d10
child 363547 5c652308e5697c237b746e595d056ecde471c4da
push id91356
push userbzbarsky@mozilla.com
push dateTue, 13 Jun 2017 02:17:14 +0000
treeherdermozilla-inbound@6977697a2dcf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersqdot
bugs1372371
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1372371. Fix enumerability handling in the window resolve hook. r=qdot
dom/base/nsGlobalWindow.cpp
dom/base/nsGlobalWindow.h
dom/base/nsObjectLoadingContent.cpp
dom/base/nsObjectLoadingContent.h
dom/base/test/mochitest.ini
dom/base/test/test_window_keys.html
dom/base/test/test_window_own_props.html
dom/bindings/Codegen.py
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -1395,19 +1395,31 @@ nsOuterWindowProxy::set(JSContext *cx, J
 
   return js::Wrapper::set(cx, proxy, id, v, receiver, result);
 }
 
 bool
 nsOuterWindowProxy::getOwnEnumerablePropertyKeys(JSContext *cx, JS::Handle<JSObject*> proxy,
                                                  JS::AutoIdVector &props) const
 {
-  // BaseProxyHandler::keys seems to do what we want here: call
-  // ownPropertyKeys and then filter out the non-enumerable properties.
-  return js::BaseProxyHandler::getOwnEnumerablePropertyKeys(cx, proxy, props);
+  // Like ownPropertyKeys, our indexed stuff followed by our "normal" enumerable
+  // own property names.
+  //
+  // Note that this does not match current spec per
+  // https://github.com/whatwg/html/issues/2753 but as that issue says I believe
+  // the spec is wrong.
+  if (!AppendIndexedPropertyNames(cx, proxy, props)) {
+    return false;
+  }
+
+  JS::AutoIdVector innerProps(cx);
+  if (!js::Wrapper::getOwnEnumerablePropertyKeys(cx, proxy, innerProps)) {
+    return false;
+  }
+  return js::AppendUnique(cx, props, innerProps);
 }
 
 bool
 nsOuterWindowProxy::enumerate(JSContext *cx, JS::Handle<JSObject*> proxy,
                               JS::MutableHandle<JSObject*> objp) const
 {
   // BaseProxyHandler::enumerate seems to do what we want here: fall
   // back on the property names returned from js::GetPropertyKeys()
@@ -5134,18 +5146,32 @@ nsGlobalWindow::MayResolve(jsid aId)
   nsAutoString name;
   AssignJSFlatString(name, JSID_TO_FLAT_STRING(aId));
 
   return nameSpaceManager->LookupName(name);
 }
 
 void
 nsGlobalWindow::GetOwnPropertyNames(JSContext* aCx, JS::AutoIdVector& aNames,
-                                    ErrorResult& aRv)
-{
+                                    bool aEnumerableOnly, ErrorResult& aRv)
+{
+  if (aEnumerableOnly) {
+    // The names we would return from here get defined on the window via one of
+    // two codepaths.  The ones coming from the WebIDLGlobalNameHash will end up
+    // in the DefineConstructor function in BindingUtils, which always defines
+    // things as non-enumerable.  The ones coming from the script namespace
+    // manager get defined by nsDOMClassInfo::PostCreatePrototype calling
+    // ResolvePrototype and using the resulting descriptot to define the
+    // property.  ResolvePrototype always passes 0 to the FillPropertyDescriptor
+    // for the property attributes, so all those are non-enumerable as well.
+    //
+    // So in the aEnumerableOnly case we have nothing to do.
+    return;
+  }
+
   MOZ_ASSERT(IsInnerWindow());
   // "Components" is marked as enumerable but only resolved on demand :-/.
   //aNames.AppendElement(NS_LITERAL_STRING("Components"));
 
   nsScriptNameSpaceManager* nameSpaceManager = GetNameSpaceManager();
   if (nameSpaceManager) {
     JS::Rooted<JSObject*> wrapper(aCx, GetWrapper());
 
--- a/dom/base/nsGlobalWindow.h
+++ b/dom/base/nsGlobalWindow.h
@@ -482,17 +482,17 @@ public:
   bool DoResolve(JSContext* aCx, JS::Handle<JSObject*> aObj,
                  JS::Handle<jsid> aId,
                  JS::MutableHandle<JS::PropertyDescriptor> aDesc);
   // The return value is whether DoResolve might end up resolving the given id.
   // If in doubt, return true.
   static bool MayResolve(jsid aId);
 
   void GetOwnPropertyNames(JSContext* aCx, JS::AutoIdVector& aNames,
-                           mozilla::ErrorResult& aRv);
+                           bool aEnumerableOnly, mozilla::ErrorResult& aRv);
 
   // Object Management
   static already_AddRefed<nsGlobalWindow> Create(nsGlobalWindow *aOuterWindow);
 
   static nsGlobalWindow *FromSupports(nsISupports *supports)
   {
     // Make sure this matches the casts we do in QueryInterface().
     return (nsGlobalWindow *)(mozilla::dom::EventTarget *)supports;
--- a/dom/base/nsObjectLoadingContent.cpp
+++ b/dom/base/nsObjectLoadingContent.cpp
@@ -3897,16 +3897,17 @@ nsObjectLoadingContent::MayResolve(jsid 
 {
   // We can resolve anything, really.
   return true;
 }
 
 void
 nsObjectLoadingContent::GetOwnPropertyNames(JSContext* aCx,
                                             JS::AutoIdVector& /* unused */,
+                                            bool /* unused */,
                                             ErrorResult& aRv)
 {
   // Just like DoResolve, just make sure we're instantiated.  That will do
   // the work our Enumerate hook needs to do.  This purposefully does not fire
   // for xray resolves, see bug 967694
   RefPtr<nsNPAPIPluginInstance> pi;
   aRv = ScriptRequestPluginInstance(aCx, getter_AddRefs(pi));
 }
--- a/dom/base/nsObjectLoadingContent.h
+++ b/dom/base/nsObjectLoadingContent.h
@@ -177,17 +177,17 @@ class nsObjectLoadingContent : public ns
                    JS::Handle<jsid> aId,
                    JS::MutableHandle<JS::PropertyDescriptor> aDesc);
     // The return value is whether DoResolve might end up resolving the given
     // id.  If in doubt, return true.
     static bool MayResolve(jsid aId);
 
     // Helper for WebIDL enumeration
     void GetOwnPropertyNames(JSContext* aCx, JS::AutoIdVector& /* unused */,
-                             mozilla::ErrorResult& aRv);
+                             bool /* unused */, mozilla::ErrorResult& aRv);
 
     // WebIDL API
     nsIDocument* GetContentDocument(nsIPrincipal& aSubjectPrincipal);
     void GetActualType(nsAString& aType) const
     {
       CopyUTF8toUTF16(mContentType, aType);
     }
     uint32_t DisplayedType() const
--- a/dom/base/test/mochitest.ini
+++ b/dom/base/test/mochitest.ini
@@ -807,17 +807,19 @@ skip-if = toolkit == 'android'
 [test_window_constructor.html]
 [test_window_cross_origin_props.html]
 [test_window_define_nonconfigurable.html]
 [test_window_define_symbol.html]
 [test_window_element_enumeration.html]
 [test_window_enumeration.html]
 [test_window_extensible.html]
 [test_window_indexing.html]
+[test_window_keys.html]
 [test_window_named_frame_enumeration.html]
 [test_window_orientation.html]
 skip-if = true # bug 1312417
+[test_window_own_props.html]
 [test_window_proto.html]
 [test_writable-replaceable.html]
 [test_x-frame-options.html]
 [test_xbl_userdata.xhtml]
 [test_youtube_flash_embed.html]
 # Please keep alphabetical order.
new file mode 100644
--- /dev/null
+++ b/dom/base/test/test_window_keys.html
@@ -0,0 +1,28 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1372371
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1372371</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=1372371">Mozilla Bug 1372371</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <iframe></iframe>
+</div>
+<pre id="test">
+</pre>
+  <script type="application/javascript">
+
+  /** Test for Bug 1372371 **/
+  var list = Object.keys(window);
+  is(list.indexOf("WebSocket"), -1, "WebSocket should not be enumerable");
+  is(list.indexOf("0"), 0, "Indexed props should be enumerable");
+  </script>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/dom/base/test/test_window_own_props.html
@@ -0,0 +1,29 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1372371
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1372371</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=1372371">Mozilla Bug 1372371</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <iframe></iframe>
+</div>
+<pre id="test">
+</pre>
+  <script type="application/javascript">
+
+  /** Test for Bug 1372371 **/
+  var list = Object.getOwnPropertyNames(window);
+  // Pick an interface name we would not have resolved here yet.
+  isnot(list.indexOf("WebSocket"), -1, "WebSocket should be a property");
+  is(list.indexOf("0"), 0, "Indexed props should be properties");
+  </script>
+</body>
+</html>
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -8925,17 +8925,17 @@ class CGEnumerateHook(CGAbstractBindingM
         # Our "self" is actually the "obj" argument in this case, not the thisval.
         CGAbstractBindingMethod.__init__(
             self, descriptor, ENUMERATE_HOOK_NAME,
             args, getThisObj="", callArgs="")
 
     def generate_code(self):
         return CGGeneric(dedent("""
             binding_detail::FastErrorResult rv;
-            self->GetOwnPropertyNames(cx, properties, rv);
+            self->GetOwnPropertyNames(cx, properties, enumerableOnly, rv);
             if (rv.MaybeSetPendingException(cx)) {
               return false;
             }
             return true;
             """))
 
     def definition_body(self):
         if self.descriptor.isGlobal():
@@ -11191,17 +11191,18 @@ class CGEnumerateOwnPropertiesViaGetOwnP
         CGAbstractBindingMethod.__init__(self, descriptor,
                                          "EnumerateOwnPropertiesViaGetOwnPropertyNames",
                                          args, getThisObj="",
                                          callArgs="")
 
     def generate_code(self):
         return CGGeneric(dedent("""
             binding_detail::FastErrorResult rv;
-            self->GetOwnPropertyNames(cx, props, rv);
+            // This wants all own props, not just enumerable ones.
+            self->GetOwnPropertyNames(cx, props, false, rv);
             if (rv.MaybeSetPendingException(cx)) {
               return false;
             }
             return true;
             """))
 
 
 class CGPrototypeTraitsClass(CGClass):