Bug 952365 - Add a TreatNonObjectAsNull annotation for WebIDL callback functions and use it for event handlers, since web sites depend on assigning non-callable objects to them in some cases. r=peterv, a=lsblakk
authorBoris Zbarsky <bzbarsky@mit.edu>
Mon, 13 Jan 2014 15:12:57 -0500
changeset 174806 28995f30f8bbc47c2ae1822aa64b0beed8b67653
parent 174805 237a6c25de3926c42d508a4da6dc63c444305d64
child 174807 2c72a842fb1816f8aeaf97108e92cb5319d50dfd
push id3224
push userlsblakk@mozilla.com
push dateTue, 04 Feb 2014 01:06:49 +0000
treeherdermozilla-beta@60c04d0987f1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv, lsblakk
bugs952365
milestone28.0a2
Bug 952365 - Add a TreatNonObjectAsNull annotation for WebIDL callback functions and use it for event handlers, since web sites depend on assigning non-callable objects to them in some cases. r=peterv, a=lsblakk
dom/bindings/CallbackFunction.h
dom/bindings/Codegen.py
dom/bindings/parser/WebIDL.py
dom/bindings/parser/tests/test_treatNonCallableAsNull.py
dom/bindings/test/mochitest.ini
dom/bindings/test/test_treat_non_object_as_null.html
dom/imptests/html/html/webappapis/scripting/events/test_event-handler-spec-example.html
dom/webidl/EventHandler.webidl
--- a/dom/bindings/CallbackFunction.h
+++ b/dom/bindings/CallbackFunction.h
@@ -23,17 +23,16 @@ namespace mozilla {
 namespace dom {
 
 class CallbackFunction : public CallbackObject
 {
 public:
   explicit CallbackFunction(JSObject* aCallable)
     : CallbackObject(aCallable)
   {
-    MOZ_ASSERT(JS_ObjectIsCallable(nullptr, mCallback));
   }
 
   JS::Handle<JSObject*> Callable() const
   {
     return Callback();
   }
 
   bool HasGrayCallable() const
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -2849,18 +2849,19 @@ def getJSToNativeConversionInfo(type, de
     value is out of range.
 
     If isClamp is true, we're converting an integer and clamping if the
     value is out of range.
 
     If lenientFloatCode is not None, it should be used in cases when
     we're a non-finite float that's not unrestricted.
 
-    If allowTreatNonCallableAsNull is true, then [TreatNonCallableAsNull]
-    extended attributes on nullable callback functions will be honored.
+    If allowTreatNonCallableAsNull is true, then [TreatNonCallableAsNull] and
+    [TreatNonObjectAsNull] extended attributes on nullable callback functions
+    will be honored.
 
     If isCallbackReturnValue is "JSImpl" or "Callback", then the declType may be
     adjusted to make it easier to return from a callback.  Since that type is
     never directly observable by any consumers of the callback code, this is OK.
     Furthermore, if isCallbackReturnValue is "JSImpl", that affects the behavior
     of the FailureFatalCastableObjectUnwrapper conversion; this is used for
     implementing auto-wrapping of JS-implemented return values from a
     JS-implemented interface.
@@ -3736,16 +3737,18 @@ for (uint32_t i = 0; i < length; ++i) {
                                           (enumLoc, enumName,
                                            getEnumValueName(defaultValue.value))))
         return JSToNativeConversionInfo(template, declType=CGGeneric(declType),
                                         dealWithOptional=isOptional)
 
     if type.isCallback():
         assert not isEnforceRange and not isClamp
         assert not type.treatNonCallableAsNull() or type.nullable()
+        assert not type.treatNonObjectAsNull() or type.nullable()
+        assert not type.treatNonObjectAsNull() or not type.treatNonCallableAsNull()
 
         name = type.unroll().identifier.name
         if type.nullable():
             declType = CGGeneric("nsRefPtr<%s>" % name);
         else:
             declType = CGGeneric("OwningNonNull<%s>" % name)
         conversion = (
             "  ${declName} = new %s(&${val}.toObject());\n" % name)
@@ -3758,16 +3761,27 @@ for (uint32_t i = 0; i < length; ++i) {
                 assert(isinstance(defaultValue, IDLNullValue))
                 haveCallable = "${haveValue} && " + haveCallable
             template = (
                 ("if (%s) {\n" % haveCallable) +
                 conversion +
                 "} else {\n"
                 "  ${declName} = nullptr;\n"
                 "}")
+        elif allowTreatNonCallableAsNull and type.treatNonObjectAsNull():
+            if not isDefinitelyObject:
+                haveObject = "${val}.isObject()"
+                if defaultValue is not None:
+                    assert(isinstance(defaultValue, IDLNullValue))
+                    haveObject = "${haveValue} && " + haveObject
+                template = CGIfElseWrapper(haveObject,
+                                           CGGeneric(conversion),
+                                           CGGeneric("${declName} = nullptr;")).define()
+            else:
+                template = conversion
         else:
             template = wrapObjectTemplate(
                 "if (JS_ObjectIsCallable(cx, &${val}.toObject())) {\n" +
                 conversion +
                 "} else {\n"
                 "%s"
                 "}" % CGIndenter(onFailureNotCallable(failureCode)).define(),
                 type,
@@ -10641,16 +10655,17 @@ def isJSImplementedDescriptor(descriptor
     return (isinstance(descriptorProvider, Descriptor) and
             descriptorProvider.interface.isJSImplemented())
 
 class CGCallback(CGClass):
     def __init__(self, idlObject, descriptorProvider, baseName, methods,
                  getters=[], setters=[]):
         self.baseName = baseName
         self._deps = idlObject.getDeps()
+        self.idlObject = idlObject
         name = idlObject.identifier.name
         if isJSImplementedDescriptor(descriptorProvider):
             name = jsImplName(name)
         # For our public methods that needThisHandling we want most of the
         # same args and the same return type as what CallbackMember
         # generates.  So we want to take advantage of all its
         # CGNativeMember infrastructure, but that infrastructure can't deal
         # with templates and most especially template arguments.  So just
@@ -10662,24 +10677,32 @@ class CGCallback(CGClass):
             else:
                 realMethods.extend(self.getMethodImpls(method))
         CGClass.__init__(self, name,
                          bases=[ClassBase(baseName)],
                          constructors=self.getConstructors(),
                          methods=realMethods+getters+setters)
 
     def getConstructors(self):
+        if (not self.idlObject.isInterface() and
+            not self.idlObject._treatNonObjectAsNull):
+            body = "MOZ_ASSERT(JS_ObjectIsCallable(nullptr, mCallback));"
+        else:
+            # Not much we can assert about it, other than not being null, and
+            # CallbackObject does that already.
+            body = ""
         return [ClassConstructor(
             [Argument("JSObject*", "aCallback")],
             bodyInHeader=True,
             visibility="public",
             explicit=True,
             baseConstructors=[
                 "%s(aCallback)" % self.baseName
-                ])]
+                ],
+            body=body)]
 
     def getMethodImpls(self, method):
         assert method.needThisHandling
         args = list(method.args)
         # Strip out the JSContext*/JSObject* args
         # that got added.
         assert args[0].name == "cx" and args[0].argType == "JSContext*"
         assert args[1].name == "aThisObj" and args[1].argType == "JS::Handle<JSObject*>"
@@ -10733,16 +10756,17 @@ class CGCallback(CGClass):
                             body=bodyWithoutThis),
                 method]
 
     def deps(self):
         return self._deps
 
 class CGCallbackFunction(CGCallback):
     def __init__(self, callback, descriptorProvider):
+        self.callback = callback
         CGCallback.__init__(self, callback, descriptorProvider,
                             "CallbackFunction",
                             methods=[CallCallback(callback, descriptorProvider)])
 
     def getConstructors(self):
         return CGCallback.getConstructors(self) + [
             ClassConstructor(
             [Argument("CallbackFunction*", "aOther")],
@@ -11031,42 +11055,49 @@ class CallbackMethod(CallbackMember):
                                 needThisHandling, rethrowContentException)
     def getRvalDecl(self):
         return "JS::Rooted<JS::Value> rval(cx, JS::UndefinedValue());\n"
 
     def getCall(self):
         replacements = {
             "errorReturn" : self.getDefaultRetval(),
             "thisObj": self.getThisObj(),
-            "getCallable": self.getCallableDecl()
+            "getCallable": self.getCallableDecl(),
+            "callGuard": self.getCallGuard()
             }
         if self.argCount > 0:
             replacements["argv"] = "argv.begin()"
             replacements["argc"] = "argc"
         else:
             replacements["argv"] = "nullptr"
             replacements["argc"] = "0"
         return string.Template("${getCallable}"
-                "if (!JS_CallFunctionValue(cx, ${thisObj}, callable,\n"
+                "if (${callGuard}!JS_CallFunctionValue(cx, ${thisObj}, callable,\n"
                 "                          ${argc}, ${argv}, rval.address())) {\n"
                 "  aRv.Throw(NS_ERROR_UNEXPECTED);\n"
                 "  return${errorReturn};\n"
                 "}\n").substitute(replacements)
 
 class CallCallback(CallbackMethod):
     def __init__(self, callback, descriptorProvider):
+        self.callback = callback
         CallbackMethod.__init__(self, callback.signatures()[0], "Call",
                                 descriptorProvider, needThisHandling=True)
 
     def getThisObj(self):
         return "aThisObj"
 
     def getCallableDecl(self):
         return "JS::Rooted<JS::Value> callable(cx, JS::ObjectValue(*mCallback));\n"
 
+    def getCallGuard(self):
+        if self.callback._treatNonObjectAsNull:
+            return "JS_ObjectIsCallable(cx, mCallback) && "
+        return ""
+
 class CallbackOperationBase(CallbackMethod):
     """
     Common class for implementing various callback operations.
     """
     def __init__(self, signature, jsName, nativeName, descriptor, singleOperation, rethrowContentException=False):
         self.singleOperation = singleOperation
         self.methodName = jsName
         CallbackMethod.__init__(self, signature, nativeName, descriptor, singleOperation, rethrowContentException)
@@ -11095,16 +11126,19 @@ class CallbackOperationBase(CallbackMeth
             'bool isCallable = JS_ObjectIsCallable(cx, mCallback);\n'
             'JS::Rooted<JS::Value> callable(cx);\n'
             'if (isCallable) {\n'
             '  callable = JS::ObjectValue(*mCallback);\n'
             '} else {\n'
             '%s'
             '}\n' % CGIndenter(CGGeneric(getCallableFromProp)).define())
 
+    def getCallGuard(self):
+        return ""
+
 class CallbackOperation(CallbackOperationBase):
     """
     Codegen actual WebIDL operations on callback interfaces.
     """
     def __init__(self, method, signature, descriptor):
         self.ensureASCIIName(method)
         jsName = method.identifier.name
         CallbackOperationBase.__init__(self, signature,
--- a/dom/bindings/parser/WebIDL.py
+++ b/dom/bindings/parser/WebIDL.py
@@ -871,16 +871,19 @@ class IDLInterface(IDLObjectWithScope):
         self._extendedAttrDict = {}
         for attr in attrs:
             identifier = attr.identifier()
 
             # Special cased attrs
             if identifier == "TreatNonCallableAsNull":
                 raise WebIDLError("TreatNonCallableAsNull cannot be specified on interfaces",
                                   [attr.location, self.location])
+            if identifier == "TreatNonObjectAsNull":
+                raise WebIDLError("TreatNonObjectAsNull cannot be specified on interfaces",
+                                  [attr.location, self.location])
             elif identifier == "NoInterfaceObject":
                 if not attr.noArguments():
                     raise WebIDLError("[NoInterfaceObject] must take no arguments",
                                       [attr.location])
 
                 if self.ctor():
                     raise WebIDLError("Constructor and NoInterfaceObject are incompatible",
                                       [self.location])
@@ -1406,16 +1409,20 @@ class IDLType(IDLObject):
 
     def tag(self):
         assert False # Override me!
 
     def treatNonCallableAsNull(self):
         assert self.tag() == IDLType.Tags.callback
         return self.nullable() and self.inner._treatNonCallableAsNull
 
+    def treatNonObjectAsNull(self):
+        assert self.tag() == IDLType.Tags.callback
+        return self.nullable() and self.inner._treatNonObjectAsNull
+
     def addExtendedAttributes(self, attrs):
         assert len(attrs) == 0
 
     def resolveType(self, parentScope):
         pass
 
     def unroll(self):
         return self
@@ -2685,20 +2692,17 @@ class IDLAttribute(IDLInterfaceMember):
             raise WebIDLError("Cached attributes and attributes stored in "
                               "slots must be constant or pure, since the "
                               "getter won't always be called.",
                               [self.location])
         pass
 
     def handleExtendedAttribute(self, attr):
         identifier = attr.identifier()
-        if identifier == "TreatNonCallableAsNull":
-            raise WebIDLError("TreatNonCallableAsNull cannot be specified on attributes",
-                              [attr.location, self.location])
-        elif identifier == "SetterThrows" and self.readonly:
+        if identifier == "SetterThrows" and self.readonly:
             raise WebIDLError("Readonly attributes must not be flagged as "
                               "[SetterThrows]",
                               [self.location])
         elif (((identifier == "Throws" or identifier == "GetterThrows") and
                self.getExtendedAttribute("StoreInSlot")) or
               (identifier == "StoreInSlot" and
                (self.getExtendedAttribute("Throws") or
                 self.getExtendedAttribute("GetterThrows")))):
@@ -2922,16 +2926,17 @@ class IDLCallbackType(IDLType, IDLObject
 
         IDLObjectWithScope.__init__(self, location, parentScope, identifier)
 
         for (returnType, arguments) in self.signatures():
             for argument in arguments:
                 argument.resolve(self)
 
         self._treatNonCallableAsNull = False
+        self._treatNonObjectAsNull = False
 
     def isCallback(self):
         return True
 
     def signatures(self):
         return [(self._returnType, self._arguments)]
 
     def tag(self):
@@ -2967,18 +2972,23 @@ class IDLCallbackType(IDLType, IDLObject
         return (other.isPrimitive() or other.isString() or other.isEnum() or
                 other.isNonCallbackInterface() or other.isDate())
 
     def addExtendedAttributes(self, attrs):
         unhandledAttrs = []
         for attr in attrs:
             if attr.identifier() == "TreatNonCallableAsNull":
                 self._treatNonCallableAsNull = True
+            elif attr.identifier() == "TreatNonObjectAsNull":
+                self._treatNonObjectAsNull = True
             else:
                 unhandledAttrs.append(attr)
+        if self._treatNonCallableAsNull and self._treatNonObjectAsNull:
+            raise WebIDLError("Cannot specify both [TreatNonCallableAsNull] "
+                              "and [TreatNonObjectAsNull]", [self.location])
         if len(unhandledAttrs) != 0:
             IDLType.addExtendedAttributes(self, unhandledAttrs)
 
     def _getDependentObjects(self):
         return set([self._returnType] + self._arguments)
 
 class IDLMethodOverload:
     """
--- a/dom/bindings/parser/tests/test_treatNonCallableAsNull.py
+++ b/dom/bindings/parser/tests/test_treatNonCallableAsNull.py
@@ -49,8 +49,23 @@ def WebIDLTest(parser, harness):
             };
         """)
 
         results = parser.finish()
     except:
         threw = True
 
     harness.ok(threw, "Should have thrown.")
+
+    parser = parser.reset()
+
+    threw = False
+    try:
+        parser.parse("""
+            [TreatNonCallableAsNull, TreatNonObjectAsNull]
+            callback Function = any(any... arguments);
+        """)
+
+        results = parser.finish()
+    except:
+        threw = True
+
+    harness.ok(threw, "Should have thrown.")
--- a/dom/bindings/test/mochitest.ini
+++ b/dom/bindings/test/mochitest.ini
@@ -26,9 +26,10 @@ support-files =
 [test_integers.html]
 [test_interfaceToString.html]
 [test_exceptions_from_jsimplemented.html]
 [test_lenientThis.html]
 [test_lookupGetter.html]
 [test_namedNoIndexed.html]
 [test_queryInterface.html]
 [test_sequence_wrapping.html]
+[test_treat_non_object_as_null.html]
 [test_traceProtos.html]
new file mode 100644
--- /dev/null
+++ b/dom/bindings/test/test_treat_non_object_as_null.html
@@ -0,0 +1,39 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=952365
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 952365</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 952365 **/
+
+    var onvolumechange;
+    var x = {};
+    
+    (function() {
+       onvolumechange = x;
+       ise(onvolumechange, x,
+           "Should preserve an object value when assigning to event handler");
+       // Test that we don't try to actually call the non-callable object
+       window.dispatchEvent(new Event("volumechange"));
+       onvolumechange = 5;
+       ise(onvolumechange, null,
+           "Non-object values should become null when assigning to event handler");
+    })();
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=952365">Mozilla Bug 952365</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>
--- a/dom/imptests/html/html/webappapis/scripting/events/test_event-handler-spec-example.html
+++ b/dom/imptests/html/html/webappapis/scripting/events/test_event-handler-spec-example.html
@@ -1,22 +1,65 @@
 <!DOCTYPE html>
 <title>Event handler invocation order</title>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <div id="log"></div>
-<button type="button" id="test">Start test</button>
 <script>
-var t = async_test("Event handler listeners should be registered when they are first set.");
-var i = 0;
-var uncalled = "t.step(function() { assert_unreached('First event handler.') })"
-var button = document.getElementById('test');
-button.onclick = {};
-button.addEventListener('click', t.step_func(function () { assert_equals(++i, 1) }), false);
-button.setAttribute('onclick', uncalled); // event handler listener is registered here
-button.addEventListener('click', t.step_func(function () { assert_equals(++i, 3) }), false);
-button.onclick = t.step_func(function () { assert_equals(++i, 2); });
-button.addEventListener('click', t.step_func(function () { assert_equals(++i, 4) }), false);
-button.click()
-assert_equals(button.getAttribute("onclick"), uncalled)
-assert_equals(i, 4);
-t.done()
+var objects = [{}, function() {}, new Number(42), new String()];
+var primitives = [42, null, undefined, ""];
+objects.forEach(function(object) {
+  var t = async_test("Event handler listeners should be registered when they " +
+                     "are first set to a object value (" +
+                     format_value(object) + ").");
+  t.step(function() {
+    var i = 0;
+    var uncalled = "t.step(function() { assert_unreached('First event handler.') })"
+    var button = document.createElement('button');
+    button.onclick = object; // event handler listener is registered here
+    button.addEventListener('click', t.step_func(function () { assert_equals(++i, 2) }), false);
+    button.setAttribute('onclick', uncalled);
+    button.addEventListener('click', t.step_func(function () { assert_equals(++i, 3) }), false);
+    button.onclick = t.step_func(function () { assert_equals(++i, 1); });
+    button.addEventListener('click', t.step_func(function () { assert_equals(++i, 4) }), false);
+    button.click()
+    assert_equals(button.getAttribute("onclick"), uncalled)
+    assert_equals(i, 4);
+    t.done()
+  });
+});
+primitives.forEach(function(primitive) {
+  var t = async_test("Event handler listeners should be registered when they " +
+                     "are first set to a object value (" +
+                     format_value(primitive) + ").");
+  t.step(function() {
+    var i = 0;
+    var uncalled = "t.step(function() { assert_unreached('First event handler.') })"
+    var button = document.createElement('button');
+    button.onclick = primitive;
+    button.addEventListener('click', t.step_func(function () { assert_equals(++i, 1) }), false);
+    button.setAttribute('onclick', uncalled); // event handler listener is registered here
+    button.addEventListener('click', t.step_func(function () { assert_equals(++i, 3) }), false);
+    button.onclick = t.step_func(function () { assert_equals(++i, 2); });
+    button.addEventListener('click', t.step_func(function () { assert_equals(++i, 4) }), false);
+    button.click()
+    assert_equals(button.getAttribute("onclick"), uncalled)
+    assert_equals(i, 4);
+    t.done()
+  });
+});
+var t = async_test("Event handler listeners should be registered when they " +
+                   "are first set to an object value.");
+t.step(function() {
+  var i = 0;
+  var uncalled = "t.step(function() { assert_unreached('First event handler.') })"
+  var button = document.createElement('button');
+  button.addEventListener('click', t.step_func(function () { assert_equals(++i, 1) }), false);
+  button.setAttribute('onclick', uncalled); // event handler listener is registered here
+  button.addEventListener('click', t.step_func(function () { assert_equals(++i, 3) }), false);
+  button.onclick = t.step_func(function () { assert_equals(++i, 2); });
+  button.addEventListener('click', t.step_func(function () { assert_equals(++i, 4) }), false);
+  button.click()
+  assert_equals(button.getAttribute("onclick"), uncalled)
+  assert_equals(i, 4);
+  t.done()
+});
 </script>
--- a/dom/webidl/EventHandler.webidl
+++ b/dom/webidl/EventHandler.webidl
@@ -5,27 +5,27 @@
  *
  * The origin of this IDL file is
  * http://www.whatwg.org/specs/web-apps/current-work/#eventhandler
  *
  * © Copyright 2004-2011 Apple Computer, Inc., Mozilla Foundation, and
  * Opera Software ASA. You are granted a license to use, reproduce
  * and create derivative works of this document.
  */
-[TreatNonCallableAsNull]
+[TreatNonObjectAsNull]
 callback EventHandlerNonNull = any (Event event);
 typedef EventHandlerNonNull? EventHandler;
 
-[TreatNonCallableAsNull]
+[TreatNonObjectAsNull]
 // https://www.w3.org/Bugs/Public/show_bug.cgi?id=23489
 //callback OnBeforeUnloadEventHandlerNonNull = DOMString (Event event);
 callback OnBeforeUnloadEventHandlerNonNull = DOMString? (Event event);
 typedef OnBeforeUnloadEventHandlerNonNull? OnBeforeUnloadEventHandler;
 
-[TreatNonCallableAsNull]
+[TreatNonObjectAsNull]
 callback OnErrorEventHandlerNonNull = boolean ((Event or DOMString) event, optional DOMString source, optional unsigned long lineno, optional unsigned long column);
 typedef OnErrorEventHandlerNonNull? OnErrorEventHandler;
 
 [NoInterfaceObject]
 interface GlobalEventHandlers {
            attribute EventHandler onabort;
            attribute EventHandler onblur;
 // We think the spec is wrong here. See OnErrorEventHandlerForNodes/Window