Bug 1043690 - Part 2: Change the codegen for DOM proxies to ignore named props when looking up property descriptors on [[Set]]. r=efaust, a=lmandel
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 01 Aug 2014 23:37:14 -0400
changeset 208258 06542873b0dc
parent 208257 8375886783f2
child 208259 7b29fabbf26a
push id3792
push userryanvm@gmail.com
push date2014-08-07 18:27 +0000
treeherdermozilla-beta@7b29fabbf26a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust, lmandel
bugs1043690
milestone32.0
Bug 1043690 - Part 2: Change the codegen for DOM proxies to ignore named props when looking up property descriptors on [[Set]]. r=efaust, a=lmandel
dom/base/WindowNamedPropertiesHandler.cpp
dom/base/WindowNamedPropertiesHandler.h
dom/bindings/Codegen.py
dom/bindings/DOMJSProxyHandler.cpp
dom/bindings/DOMJSProxyHandler.h
dom/bindings/test/mochitest.ini
dom/bindings/test/test_setWithNamedGetterNoNamedSetter.html
--- a/dom/base/WindowNamedPropertiesHandler.cpp
+++ b/dom/base/WindowNamedPropertiesHandler.cpp
@@ -78,20 +78,21 @@ GetWindowFromGlobal(JSObject* aGlobal)
   }
   XPCWrappedNative* wrapper = XPCWrappedNative::Get(aGlobal);
   nsCOMPtr<nsPIDOMWindow> piWin = do_QueryWrappedNative(wrapper);
   MOZ_ASSERT(piWin);
   return static_cast<nsGlobalWindow*>(piWin.get());
 }
 
 bool
-WindowNamedPropertiesHandler::getOwnPropertyDescriptor(JSContext* aCx,
-                                                       JS::Handle<JSObject*> aProxy,
-                                                       JS::Handle<jsid> aId,
-                                                       JS::MutableHandle<JSPropertyDescriptor> aDesc)
+WindowNamedPropertiesHandler::getOwnPropDescriptor(JSContext* aCx,
+                                                   JS::Handle<JSObject*> aProxy,
+                                                   JS::Handle<jsid> aId,
+                                                   bool /* unused */,
+                                                   JS::MutableHandle<JSPropertyDescriptor> aDesc)
 {
   // Note: The infallibleInit call below depends on this check.
   if (!JSID_IS_STRING(aId)) {
     // Nothing to do if we're resolving a non-string property.
     return true;
   }
 
   JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, aProxy));
--- a/dom/base/WindowNamedPropertiesHandler.h
+++ b/dom/base/WindowNamedPropertiesHandler.h
@@ -23,19 +23,20 @@ public:
   preventExtensions(JSContext* aCx, JS::Handle<JSObject*> aProxy) MOZ_OVERRIDE
   {
     // Throw a TypeError, per WebIDL.
     JS_ReportErrorNumber(aCx, js_GetErrorMessage, nullptr,
                          JSMSG_CANT_CHANGE_EXTENSIBILITY);
     return false;
   }
   virtual bool
-  getOwnPropertyDescriptor(JSContext* aCx, JS::Handle<JSObject*> aProxy,
-                           JS::Handle<jsid> aId,
-                           JS::MutableHandle<JSPropertyDescriptor> aDesc) MOZ_OVERRIDE;
+  getOwnPropDescriptor(JSContext* aCx, JS::Handle<JSObject*> aProxy,
+                       JS::Handle<jsid> aId,
+                       bool /* unused */,
+                       JS::MutableHandle<JSPropertyDescriptor> aDesc) MOZ_OVERRIDE;
   virtual bool
   defineProperty(JSContext* aCx, JS::Handle<JSObject*> aProxy,
                  JS::Handle<jsid> aId,
                  JS::MutableHandle<JSPropertyDescriptor> aDesc) MOZ_OVERRIDE;
   virtual bool
   ownPropNames(JSContext* aCx, JS::Handle<JSObject*> aProxy, unsigned flags,
                JS::AutoIdVector& aProps) MOZ_OVERRIDE;
   virtual bool
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -9361,23 +9361,24 @@ class CGProxyUnwrap(CGAbstractMethod):
               obj = js::UncheckedUnwrap(obj);
             }
             MOZ_ASSERT(IsProxy(obj));
             return static_cast<${type}*>(js::GetProxyPrivate(obj).toPrivate());
             """,
             type=self.descriptor.nativeType)
 
 
-class CGDOMJSProxyHandler_getOwnPropertyDescriptor(ClassMethod):
+class CGDOMJSProxyHandler_getOwnPropDescriptor(ClassMethod):
     def __init__(self, descriptor):
         args = [Argument('JSContext*', 'cx'),
                 Argument('JS::Handle<JSObject*>', 'proxy'),
                 Argument('JS::Handle<jsid>', 'id'),
+                Argument('bool', 'ignoreNamedProps'),
                 Argument('JS::MutableHandle<JSPropertyDescriptor>', 'desc')]
-        ClassMethod.__init__(self, "getOwnPropertyDescriptor", "bool", args,
+        ClassMethod.__init__(self, "getOwnPropDescriptor", "bool", args,
                              virtual=True, override=True)
         self.descriptor = descriptor
 
     def getBody(self):
         indexedGetter = self.descriptor.operations['IndexedGetter']
         indexedSetter = self.descriptor.operations['IndexedSetter']
 
         if self.descriptor.supportsIndexedProperties():
@@ -9438,16 +9439,17 @@ class CGDOMJSProxyHandler_getOwnProperty
             fillDescriptor = (
                 "FillPropertyDescriptor(desc, proxy, %s, %s);\n"
                 "return true;\n" % (readonly, enumerable))
             templateValues = {'jsvalRef': 'desc.value()', 'jsvalHandle': 'desc.value()',
                               'obj': 'proxy', 'successCode': fillDescriptor}
             condition = "!HasPropertyOnPrototype(cx, proxy, id)"
             if self.descriptor.interface.getExtendedAttribute('OverrideBuiltins'):
                 condition = "(!isXray || %s)" % condition
+            condition = "!ignoreNamedProps && " + condition
             if self.descriptor.supportsIndexedProperties():
                 condition = "!IsArrayIndex(index) && " + condition
             namedGet = (CGIfWrapper(CGProxyNamedGetter(self.descriptor, templateValues),
                                     condition).define() +
                         "\n")
         else:
             namedGet = ""
 
@@ -10083,17 +10085,17 @@ class CGDOMJSProxyHandler_getInstance(Cl
             return &instance;
             """)
 
 
 class CGDOMJSProxyHandler(CGClass):
     def __init__(self, descriptor):
         assert (descriptor.supportsIndexedProperties() or
                 descriptor.supportsNamedProperties())
-        methods = [CGDOMJSProxyHandler_getOwnPropertyDescriptor(descriptor),
+        methods = [CGDOMJSProxyHandler_getOwnPropDescriptor(descriptor),
                    CGDOMJSProxyHandler_defineProperty(descriptor),
                    ClassUsingDeclaration("mozilla::dom::DOMProxyHandler",
                                          "defineProperty"),
                    CGDOMJSProxyHandler_ownPropNames(descriptor),
                    CGDOMJSProxyHandler_hasOwn(descriptor),
                    CGDOMJSProxyHandler_get(descriptor),
                    CGDOMJSProxyHandler_className(descriptor),
                    CGDOMJSProxyHandler_finalizeInBackground(descriptor),
--- a/dom/bindings/DOMJSProxyHandler.cpp
+++ b/dom/bindings/DOMJSProxyHandler.cpp
@@ -183,16 +183,26 @@ BaseDOMProxyHandler::getPropertyDescript
     desc.object().set(nullptr);
     return true;
   }
 
   return JS_GetPropertyDescriptorById(cx, proto, id, desc);
 }
 
 bool
+BaseDOMProxyHandler::getOwnPropertyDescriptor(JSContext* cx,
+                                              JS::Handle<JSObject*> proxy,
+                                              JS::Handle<jsid> id,
+                                              MutableHandle<JSPropertyDescriptor> desc)
+{
+  return getOwnPropDescriptor(cx, proxy, id, /* ignoreNamedProps = */ false,
+                              desc);
+}
+
+bool
 DOMProxyHandler::defineProperty(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,
                                 MutableHandle<JSPropertyDescriptor> desc, bool* defined)
 {
   if (desc.hasGetterObject() && desc.setter() == JS_StrictPropertyStub) {
     return JS_ReportErrorFlagsAndNumber(cx,
                                         JSREPORT_WARNING | JSREPORT_STRICT |
                                         JSREPORT_STRICT_MODE_ERROR,
                                         js_GetErrorMessage, nullptr,
@@ -220,17 +230,40 @@ DOMProxyHandler::set(JSContext *cx, Hand
              "Should not have a XrayWrapper here");
   bool done;
   if (!setCustom(cx, proxy, id, vp, &done)) {
     return false;
   }
   if (done) {
     return true;
   }
-  return mozilla::dom::BaseDOMProxyHandler::set(cx, proxy, receiver, id, strict, vp);
+
+  // Make sure to ignore our named properties when checking for own
+  // property descriptors for a set.
+  JS::Rooted<JSPropertyDescriptor> desc(cx);
+  if (!getOwnPropDescriptor(cx, proxy, id, /* ignoreNamedProps = */ true,
+                            &desc)) {
+    return false;
+  }
+  bool descIsOwn = desc.object() != nullptr;
+  if (!desc.object()) {
+    // Don't just use getPropertyDescriptor, unlike BaseProxyHandler::set,
+    // because that would call getOwnPropertyDescriptor on ourselves.  Instead,
+    // directly delegate to the proto, if any.
+    JS::Rooted<JSObject*> proto(cx);
+    if (!js::GetObjectProto(cx, proxy, &proto)) {
+      return false;
+    }
+    if (proto && !JS_GetPropertyDescriptorById(cx, proto, id, &desc)) {
+      return false;
+    }
+  }
+
+  return js::SetPropertyIgnoringNamedGetter(cx, this, proxy, receiver, id,
+                                            &desc, descIsOwn, strict, vp);
 }
 
 bool
 DOMProxyHandler::delete_(JSContext* cx, JS::Handle<JSObject*> proxy,
                          JS::Handle<jsid> id, bool* bp)
 {
   JS::Rooted<JSObject*> expando(cx);
   if (!xpc::WrapperFactory::IsXrayWrapper(proxy) && (expando = GetExpandoObject(proxy))) {
--- a/dom/bindings/DOMJSProxyHandler.h
+++ b/dom/bindings/DOMJSProxyHandler.h
@@ -46,16 +46,19 @@ public:
 
   // Implementations of traps that can be implemented in terms of
   // fundamental traps.
   bool enumerate(JSContext* cx, JS::Handle<JSObject*> proxy,
                  JS::AutoIdVector& props) MOZ_OVERRIDE;
   bool getPropertyDescriptor(JSContext* cx, JS::Handle<JSObject*> proxy,
                              JS::Handle<jsid> id,
                              JS::MutableHandle<JSPropertyDescriptor> desc) MOZ_OVERRIDE;
+  bool getOwnPropertyDescriptor(JSContext* cx, JS::Handle<JSObject*> proxy,
+                                JS::Handle<jsid> id,
+                                JS::MutableHandle<JSPropertyDescriptor> desc) MOZ_OVERRIDE;
 
   bool watch(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,
              JS::Handle<JSObject*> callable) MOZ_OVERRIDE;
   bool unwatch(JSContext* cx, JS::Handle<JSObject*> proxy,
                JS::Handle<jsid> id) MOZ_OVERRIDE;
   virtual bool getOwnPropertyNames(JSContext* cx, JS::Handle<JSObject*> proxy,
                                    JS::AutoIdVector &props) MOZ_OVERRIDE;
   // We override keys() and implement it directly instead of using the
@@ -67,16 +70,26 @@ public:
 
 protected:
   // Hook for subclasses to implement shared getOwnPropertyNames()/keys()
   // functionality.  The "flags" argument is either JSITER_OWNONLY (for keys())
   // or JSITER_OWNONLY | JSITER_HIDDEN (for getOwnPropertyNames()).
   virtual bool ownPropNames(JSContext* cx, JS::Handle<JSObject*> proxy,
                             unsigned flags,
                             JS::AutoIdVector& props) = 0;
+
+  // Hook for subclasses to allow set() to ignore named props while other things
+  // that look at property descriptors see them.  This is intentionally not
+  // named getOwnPropertyDescriptor to avoid subclasses that override it hiding
+  // our public getOwnPropertyDescriptor.
+  virtual bool getOwnPropDescriptor(JSContext* cx,
+                                    JS::Handle<JSObject*> proxy,
+                                    JS::Handle<jsid> id,
+                                    bool ignoreNamedProps,
+                                    JS::MutableHandle<JSPropertyDescriptor> desc) = 0;
 };
 
 class DOMProxyHandler : public BaseDOMProxyHandler
 {
 public:
   DOMProxyHandler()
     : BaseDOMProxyHandler(ProxyFamily())
   {
--- a/dom/bindings/test/mochitest.ini
+++ b/dom/bindings/test/mochitest.ini
@@ -34,11 +34,12 @@ skip-if = true
 skip-if = (toolkit == 'gonk' && debug) #debug-only failure; bug 926547
 [test_lenientThis.html]
 [test_lookupGetter.html]
 [test_namedNoIndexed.html]
 [test_named_getter_enumerability.html]
 [test_Object.prototype_props.html]
 [test_queryInterface.html]
 [test_sequence_wrapping.html]
+[test_setWithNamedGetterNoNamedSetter.html]
 [test_throwing_method_noDCE.html]
 [test_treat_non_object_as_null.html]
 [test_traceProtos.html]
new file mode 100644
--- /dev/null
+++ b/dom/bindings/test/test_setWithNamedGetterNoNamedSetter.html
@@ -0,0 +1,40 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1043690
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1043690</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=1043690">Mozilla Bug 1043690</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+<form>
+  <input name="action">
+</form>
+</div>
+  <script type="application/javascript">
+
+  /** Test for Bug 1043690 **/
+  var f = document.querySelector("form");
+  var i = document.querySelector("input");
+  ise(f.getAttribute("action"), null, "Should have no action attribute");
+  ise(f.action, i, "form.action should be the input");
+  f.action = "http://example.org";
+  ise(f.getAttribute("action"), "http://example.org",
+      "Should have an action attribute now");
+  ise(f.action, i, "form.action should still be the input");
+  i.remove();
+  ise(f.action, "http://example.org/",
+      "form.action should no longer be shadowed");
+
+
+  </script>
+<pre id="test">
+</pre>
+</body>
+</html>