REGRESSION (r125251): wrapper lifetimes of SVGElementInstance are incorrect
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 18 Jan 2015 20:57:26 +0000 (20:57 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 18 Jan 2015 20:57:26 +0000 (20:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=132148

Reviewed by Anders Carlsson.

Test: svg/custom/use-instanceRoot-event-listeners.xhtml

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSDOMWindow::addEventListener): Updated for the new return type
of JSListener::create. For the event type, use JSString::toAtomicString instead of
calling JSString::value and then converting to an AtomicString.
(WebCore::JSDOMWindow::removeEventListener): Same changes as for addEventListener.

* bindings/js/JSEventListener.cpp:
(WebCore::forwardsEventListeners): Added. Helper to detect the special case needed
for SVGElementInstance. In the future, for better encapsulation, we could use virtual
functions, but for now hard coding this single class seems fine.
(WebCore::correspondingElementWrapper): Added. For use if forwardsEventListeners
returns true, to find out where event listeners will be forwarded.
(WebCore::createJSEventListenerForAttribute): Added. Replaces the old function
createJSAttributeEventListener, for SVGElementInstance attributes only.
(WebCore::createJSEventListenerForAdd): Added. Helper function to avoid repeated
generated code in the addElementListener bindings other than the DOMWindow one.

* bindings/js/JSEventListener.h:
(WebCore::JSEventListener::create): Changed to return a Ref instead of a PassRefPtr.
(WebCore::createJSEventListenerForAttribute): Renamed from createJSAttributeEventListener,
changed to return a RefPtr instead of a PassRefPtr and to take references rather than
pointers for non-null things.
(WebCore::createJSEventListenerForRemove): Added. Small wrapper that calls
createJSEventListenerForAdd since they are currently identical.

* bindings/scripts/CodeGeneratorJS.pm:
(GenerateAttributeEventListenerCall): Removed the special case for JSSVGElementInstance
and updated to call the new createJSEventListenerForAttribute. The special case for
SVGElementInstance is now in JSEventListener.h/cpp, which is nicer since we prefer to
keep the generated code simpler if possible.
(GenerateEventListenerCall): Removed the special case for JSSVGElementInstance. This
has been dead code since the explicit definition of add/removeEventListener was removed
from SVGElementInstance.idl, and was also a problem if someone were to use the
addEventListener function from EventTarget on an SVGElementInstance object. The function
needs to be generic at runtime. Use toAtomicString as in JSDOMWindow::addEventListener above.
Call the two new functions, createJSEventListenerForAdd and createJSEventListenerForRemove.
Those new functions properly handle SVGElementInstance.
(GenerateImplementation): Don't pass the class name to GenerateAttributeEventListenerCall
or GenerateEventListenerCall any more.
(GenerateConstructorDefinition): Use JSString::toAtomicString instead of calling
JSString::value and then converting to AtomicString.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@178633 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
Source/WebCore/bindings/js/JSEventListener.cpp
Source/WebCore/bindings/js/JSEventListener.h
Source/WebCore/bindings/scripts/CodeGeneratorJS.pm

index f93d8a0834f2a15ec8cc1e4951a6c1a92dcd9b54..ebd46a91d5ffe542a1284a83a171a7937d3f31bd 100644 (file)
@@ -1,3 +1,54 @@
+2015-01-18  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r125251): wrapper lifetimes of SVGElementInstance are incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=132148
+
+        Reviewed by Anders Carlsson.
+
+        Test: svg/custom/use-instanceRoot-event-listeners.xhtml
+
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::JSDOMWindow::addEventListener): Updated for the new return type
+        of JSListener::create. For the event type, use JSString::toAtomicString instead of
+        calling JSString::value and then converting to an AtomicString.
+        (WebCore::JSDOMWindow::removeEventListener): Same changes as for addEventListener.
+
+        * bindings/js/JSEventListener.cpp:
+        (WebCore::forwardsEventListeners): Added. Helper to detect the special case needed
+        for SVGElementInstance. In the future, for better encapsulation, we could use virtual
+        functions, but for now hard coding this single class seems fine.
+        (WebCore::correspondingElementWrapper): Added. For use if forwardsEventListeners
+        returns true, to find out where event listeners will be forwarded.
+        (WebCore::createJSEventListenerForAttribute): Added. Replaces the old function
+        createJSAttributeEventListener, for SVGElementInstance attributes only.
+        (WebCore::createJSEventListenerForAdd): Added. Helper function to avoid repeated
+        generated code in the addElementListener bindings other than the DOMWindow one.
+
+        * bindings/js/JSEventListener.h:
+        (WebCore::JSEventListener::create): Changed to return a Ref instead of a PassRefPtr.
+        (WebCore::createJSEventListenerForAttribute): Renamed from createJSAttributeEventListener,
+        changed to return a RefPtr instead of a PassRefPtr and to take references rather than
+        pointers for non-null things.
+        (WebCore::createJSEventListenerForRemove): Added. Small wrapper that calls
+        createJSEventListenerForAdd since they are currently identical.
+
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateAttributeEventListenerCall): Removed the special case for JSSVGElementInstance
+        and updated to call the new createJSEventListenerForAttribute. The special case for
+        SVGElementInstance is now in JSEventListener.h/cpp, which is nicer since we prefer to
+        keep the generated code simpler if possible.
+        (GenerateEventListenerCall): Removed the special case for JSSVGElementInstance. This
+        has been dead code since the explicit definition of add/removeEventListener was removed
+        from SVGElementInstance.idl, and was also a problem if someone were to use the
+        addEventListener function from EventTarget on an SVGElementInstance object. The function
+        needs to be generic at runtime. Use toAtomicString as in JSDOMWindow::addEventListener above.
+        Call the two new functions, createJSEventListenerForAdd and createJSEventListenerForRemove.
+        Those new functions properly handle SVGElementInstance.
+        (GenerateImplementation): Don't pass the class name to GenerateAttributeEventListenerCall
+        or GenerateEventListenerCall any more.
+        (GenerateConstructorDefinition): Use JSString::toAtomicString instead of calling
+        JSString::value and then converting to AtomicString.
+
 2015-01-17  Brian J. Burg  <burg@cs.washington.edu>
 
         Web Inspector: highlight data for overlay should use protocol type builders
index 25c159364b454b4f6b051e5e927a67f135e762e2..02608fa6e4e2f571e4b371e6819f89323a892a9a 100644 (file)
@@ -652,7 +652,7 @@ JSValue JSDOMWindow::addEventListener(ExecState* exec)
     if (!listener.isObject())
         return jsUndefined();
 
-    impl().addEventListener(exec->argument(0).toString(exec)->value(exec), JSEventListener::create(asObject(listener), this, false, globalObject()->world()), exec->argument(2).toBoolean(exec));
+    impl().addEventListener(exec->argument(0).toString(exec)->toAtomicString(exec), JSEventListener::create(asObject(listener), this, false, globalObject()->world()), exec->argument(2).toBoolean(exec));
     return jsUndefined();
 }
 
@@ -666,7 +666,7 @@ JSValue JSDOMWindow::removeEventListener(ExecState* exec)
     if (!listener.isObject())
         return jsUndefined();
 
-    impl().removeEventListener(exec->argument(0).toString(exec)->value(exec), JSEventListener::create(asObject(listener), this, false, globalObject()->world()).get(), exec->argument(2).toBoolean(exec));
+    impl().removeEventListener(exec->argument(0).toString(exec)->toAtomicString(exec), JSEventListener::create(asObject(listener), this, false, globalObject()->world()).ptr(), exec->argument(2).toBoolean(exec));
     return jsUndefined();
 }
 
index e9c9c6eb190e3d98a2f8c3af2b468c2d5048fbb8..a44994fe1e9c3063b867dbb59966d9f5b9a9e239 100644 (file)
@@ -27,6 +27,7 @@
 #include "JSEventTarget.h"
 #include "JSMainThreadExecState.h"
 #include "JSMainThreadExecStateInstrumentation.h"
+#include "JSSVGElementInstance.h"
 #include "ScriptController.h"
 #include "WorkerGlobalScope.h"
 #include <runtime/ExceptionHelpers.h>
@@ -163,4 +164,33 @@ bool JSEventListener::operator==(const EventListener& listener)
     return false;
 }
 
+// SVGElementInstance forwards listeners to its corresponding element, so the listeners are
+// protected by the wrapper of the corresponding element, not the element instance's wrapper.
+
+bool forwardsEventListeners(JSC::JSObject& object)
+{
+    if (object.classInfo() == JSSVGElementInstance::info())
+        return true;
+    ASSERT(!object.inherits(JSSVGElementInstance::info()));
+    return false;
+}
+
+static JSC::JSObject& correspondingElementWrapper(JSC::ExecState& state, JSC::JSObject& wrapper)
+{
+    JSSVGElementInstance& castedWrapper = *jsCast<JSSVGElementInstance*>(&wrapper);
+    return *asObject(toJS(&state, castedWrapper.globalObject(), *castedWrapper.impl().correspondingElement()));
+}
+
+RefPtr<JSEventListener> createJSEventListenerForAttribute(JSC::ExecState& state, JSC::JSValue listener, JSSVGElementInstance& wrapper)
+{
+    return createJSEventListenerForAttribute(state, listener, correspondingElementWrapper(state, wrapper));
+}
+
+Ref<JSEventListener> createJSEventListenerForAdd(JSC::ExecState& state, JSC::JSObject& listener, JSC::JSObject& wrapper)
+{
+    JSC::JSObject& actualWrapper = forwardsEventListeners(wrapper) ? correspondingElementWrapper(state, wrapper) : wrapper;
+    ASSERT(!forwardsEventListeners(actualWrapper));
+    return JSEventListener::create(&listener, &actualWrapper, false, currentWorld(&state));
+}
+
 } // namespace WebCore
index ac47fc60179ba84bc954af65abc646e305fabbd3..b932fce396ed9fd00bbea2d41feb98439572f073 100644 (file)
 namespace WebCore {
 
     class JSDOMGlobalObject;
+    class JSSVGElementInstance;
 
     class JSEventListener : public EventListener {
     public:
-        static PassRefPtr<JSEventListener> create(JSC::JSObject* listener, JSC::JSObject* wrapper, bool isAttribute, DOMWrapperWorld& world)
+        static Ref<JSEventListener> create(JSC::JSObject* listener, JSC::JSObject* wrapper, bool isAttribute, DOMWrapperWorld& world)
         {
-            return adoptRef(new JSEventListener(listener, wrapper, isAttribute, world));
+            return adoptRef(*new JSEventListener(listener, wrapper, isAttribute, world));
         }
 
         static const JSEventListener* cast(const EventListener* listener)
@@ -75,6 +76,15 @@ namespace WebCore {
         RefPtr<DOMWrapperWorld> m_isolatedWorld;
     };
 
+    // For "onXXX" event attributes.
+    RefPtr<JSEventListener> createJSEventListenerForAttribute(JSC::ExecState&, JSC::JSValue listener, JSC::JSObject& wrapper);
+    RefPtr<JSEventListener> createJSEventListenerForAttribute(JSC::ExecState&, JSC::JSValue listener, JSSVGElementInstance& wrapper);
+
+    Ref<JSEventListener> createJSEventListenerForAdd(JSC::ExecState&, JSC::JSObject& listener, JSC::JSObject& wrapper);
+    Ref<JSEventListener> createJSEventListenerForRemove(JSC::ExecState&, JSC::JSObject& listener, JSC::JSObject& wrapper);
+
+    bool forwardsEventListeners(JSC::JSObject& wrapper);
+
     inline JSC::JSObject* JSEventListener::jsFunction(ScriptExecutionContext* scriptExecutionContext) const
     {
         // initializeJSFunction can trigger code that deletes this event listener
@@ -106,15 +116,18 @@ namespace WebCore {
         return m_jsFunction.get();
     }
 
-    // Creates a JS EventListener for an "onXXX" event attribute.
-    inline PassRefPtr<JSEventListener> createJSAttributeEventListener(JSC::ExecState* exec, JSC::JSValue listener, JSC::JSObject* wrapper)
+    inline RefPtr<JSEventListener> createJSEventListenerForAttribute(JSC::ExecState& state, JSC::JSValue listener, JSC::JSObject& wrapper)
     {
+        ASSERT(!forwardsEventListeners(wrapper));
         if (!listener.isObject())
-            return 0;
-
-        return JSEventListener::create(asObject(listener), wrapper, true, currentWorld(exec));
+            return nullptr;
+        return JSEventListener::create(asObject(listener), &wrapper, true, currentWorld(&state));
     }
 
+    inline Ref<JSEventListener> createJSEventListenerForRemove(JSC::ExecState& state, JSC::JSObject& listener, JSC::JSObject& wrapper)
+    {
+        return createJSEventListenerForAdd(state, listener, wrapper);
+    }
 
 } // namespace WebCore
 
index 37311d6cb77edf993e47413806f93ea74b88bd50..42974d4598123efcadf723ce395bcc661c0e0973 100644 (file)
@@ -142,56 +142,31 @@ sub GenerateInterface
 
 sub GenerateAttributeEventListenerCall
 {
-    my $className = shift;
     my $implSetterFunctionName = shift;
     my $windowEventListener = shift;
 
     my $wrapperObject = $windowEventListener ? "globalObject" : "castedThis";
     my @GenerateEventListenerImpl = ();
 
-    if ($className eq "JSSVGElementInstance") {
-        # SVGElementInstances have to create JSEventListeners with the wrapper equal to the correspondingElement
-        $wrapperObject = "asObject(correspondingElementWrapper)";
-
-        push(@GenerateEventListenerImpl, <<END);
-    JSValue correspondingElementWrapper = toJS(exec, castedThis->globalObject(), impl.correspondingElement());
-    if (correspondingElementWrapper.isObject())
-END
-
-        # Add leading whitespace to format the impl.set... line correctly
-        push(@GenerateEventListenerImpl, "    ");
-    }
-
-    push(@GenerateEventListenerImpl, "    impl.set$implSetterFunctionName(createJSAttributeEventListener(exec, value, $wrapperObject));\n");
+    push(@GenerateEventListenerImpl, "    impl.set$implSetterFunctionName(createJSEventListenerForAttribute(*exec, value, *$wrapperObject));\n");
     return @GenerateEventListenerImpl;
 }
 
 sub GenerateEventListenerCall
 {
-    my $className = shift;
     my $functionName = shift;
-    my $passRefPtrHandling = ($functionName eq "add") ? "" : ".get()";
+    my $suffix = ucfirst $functionName;
+    my $passRefPtrHandling = ($functionName eq "add") ? "" : ".ptr()";
 
     $implIncludes{"JSEventListener.h"} = 1;
 
     my @GenerateEventListenerImpl = ();
-    my $wrapperObject = "castedThis";
-    if ($className eq "JSSVGElementInstance") {
-        # SVGElementInstances have to create JSEventListeners with the wrapper equal to the correspondingElement
-        $wrapperObject = "asObject(correspondingElementWrapper)";
-
-        push(@GenerateEventListenerImpl, <<END);
-    JSValue correspondingElementWrapper = toJS(exec, castedThis->globalObject(), impl.correspondingElement());
-    if (!correspondingElementWrapper.isObject())
-        return JSValue::encode(jsUndefined());
-END
-    }
 
     push(@GenerateEventListenerImpl, <<END);
     JSValue listener = exec->argument(1);
-    if (!listener.isObject())
+    if (UNLIKELY(!listener.isObject()))
         return JSValue::encode(jsUndefined());
-    impl.${functionName}EventListener(exec->argument(0).toString(exec)->value(exec), JSEventListener::create(asObject(listener), $wrapperObject, false, currentWorld(exec))$passRefPtrHandling, exec->argument(2).toBoolean(exec));
+    impl.${functionName}EventListener(exec->argument(0).toString(exec)->toAtomicString(exec), createJSEventListenerFor$suffix(*exec, *asObject(listener), *castedThis)$passRefPtrHandling, exec->argument(2).toBoolean(exec));
     return JSValue::encode(jsUndefined());
 END
     return @GenerateEventListenerImpl;
@@ -2597,7 +2572,7 @@ sub GenerateImplementation
                         $implIncludes{"JSErrorHandler.h"} = 1;
                         push(@implContent, "    impl.set$implSetterFunctionName(createJSErrorHandler(exec, value, castedThis));\n");
                     } else {
-                        push(@implContent, GenerateAttributeEventListenerCall($className, $implSetterFunctionName, $windowEventListener));
+                        push(@implContent, GenerateAttributeEventListenerCall($implSetterFunctionName, $windowEventListener));
                     }
                 } elsif ($attribute->signature->type =~ /Constructor$/) {
                     my $constructorType = $attribute->signature->type;
@@ -2837,9 +2812,9 @@ sub GenerateImplementation
 
                     # For compatibility with legacy content, the EventListener calls are generated without GenerateArgumentsCountCheck.
                     if ($function->signature->name eq "addEventListener") {
-                        push(@implContent, GenerateEventListenerCall($className, "add"));
+                        push(@implContent, GenerateEventListenerCall("add"));
                     } elsif ($function->signature->name eq "removeEventListener") {
-                        push(@implContent, GenerateEventListenerCall($className, "remove"));
+                        push(@implContent, GenerateEventListenerCall("remove"));
                     } else {
                         GenerateArgumentsCountCheck(\@implContent, $function, $interface);
 
@@ -4521,7 +4496,7 @@ EncodedJSValue JSC_HOST_CALL ${constructorClassName}::construct${className}(Exec
     if (!executionContext)
         return throwVMError(exec, createReferenceError(exec, "Constructor associated execution context is unavailable"));
 
-    AtomicString eventType = exec->argument(0).toString(exec)->value(exec);
+    AtomicString eventType = exec->argument(0).toString(exec)->toAtomicString(exec);
     if (UNLIKELY(exec->hadException()))
         return JSValue::encode(jsUndefined());