Bug 707564 part 3. Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties. r=smaug
authorBoris Zbarsky <bzbarsky@mit.edu>
Sat, 03 Aug 2013 23:38:54 -0400
changeset 153603 db968c9c8831693b7d4980339c97419ef4063d4f
parent 153602 5e13fb5ddfae54463a1e4f4d16bf02258ab481fc
child 153604 54e8477f44152ccab35d4f3c00ed9843700be9a4
push id2859
push userakeybl@mozilla.com
push dateMon, 16 Sep 2013 19:14:59 +0000
treeherdermozilla-beta@87d3c51cd2bf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs707564
milestone25.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 707564 part 3. Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties. r=smaug
content/base/src/nsObjectLoadingContent.cpp
content/base/src/nsObjectLoadingContent.h
dom/base/Navigator.cpp
dom/base/Navigator.h
dom/bindings/Codegen.py
dom/bindings/test/Makefile.in
dom/bindings/test/test_bug707564.html
--- a/content/base/src/nsObjectLoadingContent.cpp
+++ b/content/base/src/nsObjectLoadingContent.cpp
@@ -3384,16 +3384,29 @@ nsObjectLoadingContent::DoNewResolve(JSC
   nsRefPtr<nsNPAPIPluginInstance> pi;
   nsresult rv = ScriptRequestPluginInstance(aCx, getter_AddRefs(pi));
   if (NS_FAILED(rv)) {
     return mozilla::dom::Throw<true>(aCx, rv);
   }
   return true;
 }
 
+void
+nsObjectLoadingContent::GetOwnPropertyNames(JSContext* aCx,
+                                            nsTArray<nsString>& /* unused */,
+                                            ErrorResult& aRv)
+{
+  // Just like DoNewResolve, just make sure we're instantiated.  That will do
+  // the work our Enumerate hook needs to do, and we don't want to return these
+  // property names from Xrays anyway.
+  nsRefPtr<nsNPAPIPluginInstance> pi;
+  aRv = ScriptRequestPluginInstance(aCx, getter_AddRefs(pi));
+}
+
+
 // SetupProtoChainRunner implementation
 nsObjectLoadingContent::SetupProtoChainRunner::SetupProtoChainRunner(
     nsIScriptContext* scriptContext,
     nsObjectLoadingContent* aContent)
   : mContext(scriptContext)
   , mContent(aContent)
 {
 }
--- a/content/base/src/nsObjectLoadingContent.h
+++ b/content/base/src/nsObjectLoadingContent.h
@@ -146,16 +146,19 @@ class nsObjectLoadingContent : public ns
 
     // Remove plugin from protochain
     void TeardownProtoChain();
 
     // Helper for WebIDL newResolve
     bool DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObject,
                       JS::Handle<jsid> aId,
                       JS::MutableHandle<JS::Value> aValue);
+    // Helper for WebIDL enumeration
+    void GetOwnPropertyNames(JSContext* aCx, nsTArray<nsString>& /* unused */,
+                             mozilla::ErrorResult& aRv);
 
     // WebIDL API
     nsIDocument* GetContentDocument();
     void GetActualType(nsAString& aType) const
     {
       CopyUTF8toUTF16(mContentType, aType);
     }
     uint32_t DisplayedType() const
--- a/dom/base/Navigator.cpp
+++ b/dom/base/Navigator.cpp
@@ -1451,17 +1451,17 @@ bool
 Navigator::DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObject,
                         JS::Handle<jsid> aId,
                         JS::MutableHandle<JS::Value> aValue)
 {
   if (!JSID_IS_STRING(aId)) {
     return true;
   }
 
-  nsScriptNameSpaceManager *nameSpaceManager =
+  nsScriptNameSpaceManager* nameSpaceManager =
     nsJSRuntime::GetNameSpaceManager();
   if (!nameSpaceManager) {
     return Throw<true>(aCx, NS_ERROR_NOT_INITIALIZED);
   }
 
   nsDependentJSString name(aId);
 
   const nsGlobalNameStruct* name_struct =
@@ -1545,16 +1545,39 @@ Navigator::DoNewResolve(JSContext* aCx, 
   if (!JS_WrapValue(aCx, prop_val.address())) {
     return Throw<true>(aCx, NS_ERROR_UNEXPECTED);
   }
 
   aValue.set(prop_val);
   return true;
 }
 
+static PLDHashOperator
+SaveNavigatorName(const nsAString& aName, void* aClosure)
+{
+  nsTArray<nsString>* arr = static_cast<nsTArray<nsString>*>(aClosure);
+  arr->AppendElement(aName);
+  return PL_DHASH_NEXT;
+}
+
+void
+Navigator::GetOwnPropertyNames(JSContext* aCx, nsTArray<nsString>& aNames,
+                               ErrorResult& aRv)
+{
+  nsScriptNameSpaceManager *nameSpaceManager =
+    nsJSRuntime::GetNameSpaceManager();
+  if (!nameSpaceManager) {
+    NS_ERROR("Can't get namespace manager.");
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return;
+  }
+
+  nameSpaceManager->EnumerateNavigatorNames(SaveNavigatorName, &aNames);
+}
+
 JSObject*
 Navigator::WrapObject(JSContext* cx, JS::Handle<JSObject*> scope)
 {
   return NavigatorBinding::Wrap(cx, scope, this);
 }
 
 /* static */
 bool
--- a/dom/base/Navigator.h
+++ b/dom/base/Navigator.h
@@ -238,16 +238,18 @@ public:
                        MozDOMGetUserMediaErrorCallback* aOnError,
                        ErrorResult& aRv);
   void MozGetUserMediaDevices(MozGetUserMediaDevicesSuccessCallback* aOnSuccess,
                               MozDOMGetUserMediaErrorCallback* aOnError,
                               ErrorResult& aRv);
 #endif // MOZ_MEDIA_NAVIGATOR
   bool DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObject,
                     JS::Handle<jsid> aId, JS::MutableHandle<JS::Value> aValue);
+  void GetOwnPropertyNames(JSContext* aCx, nsTArray<nsString>& aNames,
+                           ErrorResult& aRv);
 
   // WebIDL helper methods
   static bool HasBatterySupport(JSContext* /* unused*/, JSObject* /*unused */);
   static bool HasPowerSupport(JSContext* /* unused */, JSObject* aGlobal);
   static bool HasIdleSupport(JSContext* /* unused */, JSObject* aGlobal);
   static bool HasWakeLockSupport(JSContext* /* unused*/, JSObject* /*unused */);
   static bool HasDesktopNotificationSupport(JSContext* /* unused*/,
                                             JSObject* /*unused */)
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -18,16 +18,17 @@ AUTOGENERATED_WARNING_COMMENT = \
     "/* THIS FILE IS AUTOGENERATED - DO NOT EDIT */\n\n"
 ADDPROPERTY_HOOK_NAME = '_addProperty'
 FINALIZE_HOOK_NAME = '_finalize'
 TRACE_HOOK_NAME = '_trace'
 CONSTRUCT_HOOK_NAME = '_constructor'
 LEGACYCALLER_HOOK_NAME = '_legacycaller'
 HASINSTANCE_HOOK_NAME = '_hasInstance'
 NEWRESOLVE_HOOK_NAME = '_newResolve'
+ENUMERATE_HOOK_NAME= '_enumerate'
 ENUM_ENTRY_VARIABLE_NAME = 'strings'
 
 def replaceFileIfChanged(filename, newContents):
     """
     Read a copy of the old file, so that we don't touch it if it hasn't changed.
     Returns True if the file was updated, false otherwise.
     """
     oldFileContents = ""
@@ -168,43 +169,45 @@ class CGDOMJSClass(CGThing):
         return "extern DOMJSClass Class;\n"
     def define(self):
         traceHook = TRACE_HOOK_NAME if self.descriptor.customTrace else 'nullptr'
         callHook = LEGACYCALLER_HOOK_NAME if self.descriptor.operations["LegacyCaller"] else 'nullptr'
         classFlags = "JSCLASS_IS_DOMJSCLASS | JSCLASS_HAS_RESERVED_SLOTS(3)"
         if self.descriptor.interface.getExtendedAttribute("NeedNewResolve"):
             newResolveHook = "(JSResolveOp)" + NEWRESOLVE_HOOK_NAME
             classFlags += " | JSCLASS_NEW_RESOLVE"
+            enumerateHook = ENUMERATE_HOOK_NAME
         else:
             newResolveHook = "JS_ResolveStub"
+            enumerateHook = "JS_EnumerateStub"
         return """
 DOMJSClass Class = {
   { "%s",
     %s,
     %s, /* addProperty */
     JS_DeletePropertyStub, /* delProperty */
     JS_PropertyStub,       /* getProperty */
     JS_StrictPropertyStub, /* setProperty */
-    JS_EnumerateStub,
-    %s,
+    %s, /* enumerate */
+    %s, /* resolve */
     JS_ConvertStub,
     %s, /* finalize */
     nullptr,               /* checkAccess */
     %s, /* call */
     nullptr,               /* hasInstance */
     nullptr,               /* construct */
     %s, /* trace */
     JSCLASS_NO_INTERNAL_MEMBERS
   },
 %s
 };
 """ % (self.descriptor.interface.identifier.name,
        classFlags,
        ADDPROPERTY_HOOK_NAME if self.descriptor.concrete and not self.descriptor.workers and self.descriptor.wrapperCache else 'JS_PropertyStub',
-       newResolveHook, FINALIZE_HOOK_NAME, callHook, traceHook,
+       enumerateHook, newResolveHook, FINALIZE_HOOK_NAME, callHook, traceHook,
        CGIndenter(CGGeneric(DOMClass(self.descriptor))).define())
 
 def PrototypeIDAndDepth(descriptor):
     prototypeID = "prototypes::id::"
     if descriptor.interface.hasInterfacePrototypeObject():
         prototypeID += descriptor.interface.identifier.name
         if descriptor.workers:
             prototypeID += "_workers"
@@ -5318,45 +5321,70 @@ class CGLegacyCallHook(CGAbstractBinding
         return CGMethodCall(nativeName, False, self.descriptor,
                             self._legacycaller)
 
 class CGNewResolveHook(CGAbstractBindingMethod):
     """
     NewResolve hook for our object
     """
     def __init__(self, descriptor):
-        self._needNewResolve = descriptor.interface.getExtendedAttribute("NeedNewResolve")
+        assert descriptor.interface.getExtendedAttribute("NeedNewResolve")
         args = [Argument('JSContext*', 'cx'), Argument('JS::Handle<JSObject*>', 'obj'),
                 Argument('JS::Handle<jsid>', 'id'), Argument('unsigned', 'flags'),
                 Argument('JS::MutableHandle<JSObject*>', 'objp')]
-        # Our "self" is actually the callee in this case, not the thisval.
+        # Our "self" is actually the "obj" argument in this case, not the thisval.
         CGAbstractBindingMethod.__init__(
             self, descriptor, NEWRESOLVE_HOOK_NAME,
             args, getThisObj="", callArgs="")
 
-    def define(self):
-        if not self._needNewResolve:
-            return ""
-        return CGAbstractBindingMethod.define(self)
-
     def generate_code(self):
         return CGIndenter(CGGeneric(
                 "JS::Rooted<JS::Value> value(cx);\n"
                 "if (!self->DoNewResolve(cx, obj, id, &value)) {\n"
                 "  return false;\n"
                 "}\n"
                 "if (value.isUndefined()) {\n"
                 "  return true;\n"
                 "}\n"
                 "if (!JS_DefinePropertyById(cx, obj, id, value, nullptr, nullptr, JSPROP_ENUMERATE)) {\n"
                 "  return false;\n"
                 "}\n"
                 "objp.set(obj);\n"
                 "return true;"))
 
+class CGEnumerateHook(CGAbstractBindingMethod):
+    """
+    Enumerate hook for our object
+    """
+    def __init__(self, descriptor):
+        assert descriptor.interface.getExtendedAttribute("NeedNewResolve")
+        args = [Argument('JSContext*', 'cx'),
+                Argument('JS::Handle<JSObject*>', 'obj')]
+        # 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 CGIndenter(CGGeneric(
+                "nsAutoTArray<nsString, 8> names;\n"
+                "ErrorResult rv;\n"
+                "self->GetOwnPropertyNames(cx, names, rv);\n"
+                "rv.WouldReportJSException();\n"
+                "if (rv.Failed()) {\n"
+                '  return ThrowMethodFailedWithDetails<true>(cx, rv, "%s", "enumerate");\n'
+                "}\n"
+                "JS::Rooted<JS::Value> dummy(cx);\n"
+                "for (uint32_t i = 0; i < names.Length(); ++i) {\n"
+                "  if (!JS_LookupUCProperty(cx, obj, names[i].get(), names[i].Length(), dummy.address())) {\n"
+                "    return false;\n"
+                "  }\n"
+                "}\n"
+                "return true;"))
+
 class CppKeywords():
     """
     A class for checking if method names declared in webidl
     are not in conflict with C++ keywords.
     """
     keywords = frozenset(['alignas', 'alignof', 'and', 'and_eq', 'asm', 'auto', 'bitand', 'bitor', 'bool',
     'break', 'case', 'catch', 'char', 'char16_t', 'char32_t', 'class', 'compl', 'const', 'constexpr',
     'const_cast', 'continue', 'decltype', 'default', 'delete', 'do', 'double', 'dynamic_cast', 'else', 'enum',
@@ -7879,17 +7907,19 @@ class CGDescriptor(CGThing):
                                                descriptor.interface.ctor()))
             cgThings.append(CGClassHasInstanceHook(descriptor))
             cgThings.append(CGInterfaceObjectJSClass(descriptor, properties))
             if descriptor.needsConstructHookHolder():
                 cgThings.append(CGClassConstructHookHolder(descriptor))
             cgThings.append(CGNamedConstructors(descriptor))
 
         cgThings.append(CGLegacyCallHook(descriptor))
-        cgThings.append(CGNewResolveHook(descriptor))
+        if descriptor.interface.getExtendedAttribute("NeedNewResolve"):
+            cgThings.append(CGNewResolveHook(descriptor))
+            cgThings.append(CGEnumerateHook(descriptor))
 
         if descriptor.interface.hasInterfacePrototypeObject():
             cgThings.append(CGPrototypeJSClass(descriptor, properties))
 
         cgThings.append(CGCreateInterfaceObjectsMethod(descriptor, properties))
         if descriptor.interface.hasInterfacePrototypeObject():
             cgThings.append(CGGetProtoObjectMethod(descriptor))
         if descriptor.interface.hasInterfaceObject():
--- a/dom/bindings/test/Makefile.in
+++ b/dom/bindings/test/Makefile.in
@@ -70,16 +70,17 @@ MOCHITEST_FILES := \
   test_queryInterface.html \
   test_exceptionThrowing.html \
   test_bug852846.html \
   test_bug862092.html \
   test_bug560072.html \
   test_lenientThis.html \
   test_ByteString.html \
   test_exception_messages.html \
+  test_bug707564.html \
   $(NULL)
 
 MOCHITEST_CHROME_FILES = \
   test_bug775543.html \
   $(NULL)
 
 ifdef GNU_CC
 CXXFLAGS += -Wno-uninitialized
new file mode 100644
--- /dev/null
+++ b/dom/bindings/test/test_bug707564.html
@@ -0,0 +1,43 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=707564
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 707564</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+
+  /** Test for Bug 707564 **/
+  SimpleTest.waitForExplicitFinish();
+
+  addLoadEvent(function() {
+    var props = Object.getOwnPropertyNames(frames[0].navigator);
+    isnot(props.indexOf("mozApps"), -1,
+          "Should enumerate a mozApps property on navigator");
+
+    // Now enumerate a different navigator object
+    var found = false;
+    for (var name in frames[1].navigator) {
+      if (name == "mozApps") {
+        found = true;
+      }
+    }
+    ok(found, "Should enumerate a mozApps property on navigator via for...in");
+    SimpleTest.finish();
+  });
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=707564">Mozilla Bug 707564</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+<iframe></iframe>
+<iframe></iframe>
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>