Changeset 178633 in webkit


Ignore:
Timestamp:
Jan 18, 2015 12:57:26 PM (9 years ago)
Author:
Darin Adler
Message:

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.

Location:
trunk/Source/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r178631 r178633  
     12015-01-18  Darin Adler  <darin@apple.com>
     2
     3        REGRESSION (r125251): wrapper lifetimes of SVGElementInstance are incorrect
     4        https://bugs.webkit.org/show_bug.cgi?id=132148
     5
     6        Reviewed by Anders Carlsson.
     7
     8        Test: svg/custom/use-instanceRoot-event-listeners.xhtml
     9
     10        * bindings/js/JSDOMWindowCustom.cpp:
     11        (WebCore::JSDOMWindow::addEventListener): Updated for the new return type
     12        of JSListener::create. For the event type, use JSString::toAtomicString instead of
     13        calling JSString::value and then converting to an AtomicString.
     14        (WebCore::JSDOMWindow::removeEventListener): Same changes as for addEventListener.
     15
     16        * bindings/js/JSEventListener.cpp:
     17        (WebCore::forwardsEventListeners): Added. Helper to detect the special case needed
     18        for SVGElementInstance. In the future, for better encapsulation, we could use virtual
     19        functions, but for now hard coding this single class seems fine.
     20        (WebCore::correspondingElementWrapper): Added. For use if forwardsEventListeners
     21        returns true, to find out where event listeners will be forwarded.
     22        (WebCore::createJSEventListenerForAttribute): Added. Replaces the old function
     23        createJSAttributeEventListener, for SVGElementInstance attributes only.
     24        (WebCore::createJSEventListenerForAdd): Added. Helper function to avoid repeated
     25        generated code in the addElementListener bindings other than the DOMWindow one.
     26
     27        * bindings/js/JSEventListener.h:
     28        (WebCore::JSEventListener::create): Changed to return a Ref instead of a PassRefPtr.
     29        (WebCore::createJSEventListenerForAttribute): Renamed from createJSAttributeEventListener,
     30        changed to return a RefPtr instead of a PassRefPtr and to take references rather than
     31        pointers for non-null things.
     32        (WebCore::createJSEventListenerForRemove): Added. Small wrapper that calls
     33        createJSEventListenerForAdd since they are currently identical.
     34
     35        * bindings/scripts/CodeGeneratorJS.pm:
     36        (GenerateAttributeEventListenerCall): Removed the special case for JSSVGElementInstance
     37        and updated to call the new createJSEventListenerForAttribute. The special case for
     38        SVGElementInstance is now in JSEventListener.h/cpp, which is nicer since we prefer to
     39        keep the generated code simpler if possible.
     40        (GenerateEventListenerCall): Removed the special case for JSSVGElementInstance. This
     41        has been dead code since the explicit definition of add/removeEventListener was removed
     42        from SVGElementInstance.idl, and was also a problem if someone were to use the
     43        addEventListener function from EventTarget on an SVGElementInstance object. The function
     44        needs to be generic at runtime. Use toAtomicString as in JSDOMWindow::addEventListener above.
     45        Call the two new functions, createJSEventListenerForAdd and createJSEventListenerForRemove.
     46        Those new functions properly handle SVGElementInstance.
     47        (GenerateImplementation): Don't pass the class name to GenerateAttributeEventListenerCall
     48        or GenerateEventListenerCall any more.
     49        (GenerateConstructorDefinition): Use JSString::toAtomicString instead of calling
     50        JSString::value and then converting to AtomicString.
     51
    1522015-01-17  Brian J. Burg  <burg@cs.washington.edu>
    253

  • trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp

    r178310 r178633  
    653653        return jsUndefined();
    654654
    655     impl().addEventListener(exec->argument(0).toString(exec)->value(exec), JSEventListener::create(asObject(listener), this, false, globalObject()->world()), exec->argument(2).toBoolean(exec));
     655    impl().addEventListener(exec->argument(0).toString(exec)->toAtomicString(exec), JSEventListener::create(asObject(listener), this, false, globalObject()->world()), exec->argument(2).toBoolean(exec));
    656656    return jsUndefined();
    657657}
    … …  
    667667        return jsUndefined();
    668668
    669     impl().removeEventListener(exec->argument(0).toString(exec)->value(exec), JSEventListener::create(asObject(listener), this, false, globalObject()->world()).get(), exec->argument(2).toBoolean(exec));
     669    impl().removeEventListener(exec->argument(0).toString(exec)->toAtomicString(exec), JSEventListener::create(asObject(listener), this, false, globalObject()->world()).ptr(), exec->argument(2).toBoolean(exec));
    670670    return jsUndefined();
    671671}

  • trunk/Source/WebCore/bindings/js/JSEventListener.cpp

    r174225 r178633  
    2828#include "JSMainThreadExecState.h"
    2929#include "JSMainThreadExecStateInstrumentation.h"
     30#include "JSSVGElementInstance.h"
    3031#include "ScriptController.h"
    3132#include "WorkerGlobalScope.h"
    … …  
    164165}
    165166
     167// SVGElementInstance forwards listeners to its corresponding element, so the listeners are
     168// protected by the wrapper of the corresponding element, not the element instance's wrapper.
     169
     170bool forwardsEventListeners(JSC::JSObject& object)
     171{
     172    if (object.classInfo() == JSSVGElementInstance::info())
     173        return true;
     174    ASSERT(!object.inherits(JSSVGElementInstance::info()));
     175    return false;
     176}
     177
     178static JSC::JSObject& correspondingElementWrapper(JSC::ExecState& state, JSC::JSObject& wrapper)
     179{
     180    JSSVGElementInstance& castedWrapper = *jsCast<JSSVGElementInstance*>(&wrapper);
     181    return *asObject(toJS(&state, castedWrapper.globalObject(), *castedWrapper.impl().correspondingElement()));
     182}
     183
     184RefPtr<JSEventListener> createJSEventListenerForAttribute(JSC::ExecState& state, JSC::JSValue listener, JSSVGElementInstance& wrapper)
     185{
     186    return createJSEventListenerForAttribute(state, listener, correspondingElementWrapper(state, wrapper));
     187}
     188
     189Ref<JSEventListener> createJSEventListenerForAdd(JSC::ExecState& state, JSC::JSObject& listener, JSC::JSObject& wrapper)
     190{
     191    JSC::JSObject& actualWrapper = forwardsEventListeners(wrapper) ? correspondingElementWrapper(state, wrapper) : wrapper;
     192    ASSERT(!forwardsEventListeners(actualWrapper));
     193    return JSEventListener::create(&listener, &actualWrapper, false, currentWorld(&state));
     194}
     195
    166196} // namespace WebCore

  • trunk/Source/WebCore/bindings/js/JSEventListener.h

    r165694 r178633  
    3131
    3232    class JSDOMGlobalObject;
     33    class JSSVGElementInstance;
    3334
    3435    class JSEventListener : public EventListener {
    3536    public:
    36         static PassRefPtr<JSEventListener> create(JSC::JSObject* listener, JSC::JSObject* wrapper, bool isAttribute, DOMWrapperWorld& world)
     37        static Ref<JSEventListener> create(JSC::JSObject* listener, JSC::JSObject* wrapper, bool isAttribute, DOMWrapperWorld& world)
    3738        {
    38             return adoptRef(new JSEventListener(listener, wrapper, isAttribute, world));
     39            return adoptRef(*new JSEventListener(listener, wrapper, isAttribute, world));
    3940        }
    4041
    … …  
    7677    };
    7778
     79    // For "onXXX" event attributes.
     80    RefPtr<JSEventListener> createJSEventListenerForAttribute(JSC::ExecState&, JSC::JSValue listener, JSC::JSObject& wrapper);
     81    RefPtr<JSEventListener> createJSEventListenerForAttribute(JSC::ExecState&, JSC::JSValue listener, JSSVGElementInstance& wrapper);
     82
     83    Ref<JSEventListener> createJSEventListenerForAdd(JSC::ExecState&, JSC::JSObject& listener, JSC::JSObject& wrapper);
     84    Ref<JSEventListener> createJSEventListenerForRemove(JSC::ExecState&, JSC::JSObject& listener, JSC::JSObject& wrapper);
     85
     86    bool forwardsEventListeners(JSC::JSObject& wrapper);
     87
    7888    inline JSC::JSObject* JSEventListener::jsFunction(ScriptExecutionContext* scriptExecutionContext) const
    7989    {
    … …  
    107117    }
    108118
    109     // Creates a JS EventListener for an "onXXX" event attribute.
    110     inline PassRefPtr<JSEventListener> createJSAttributeEventListener(JSC::ExecState* exec, JSC::JSValue listener, JSC::JSObject* wrapper)
     119    inline RefPtr<JSEventListener> createJSEventListenerForAttribute(JSC::ExecState& state, JSC::JSValue listener, JSC::JSObject& wrapper)
    111120    {
     121        ASSERT(!forwardsEventListeners(wrapper));
    112122        if (!listener.isObject())
    113             return 0;
    114 
    115         return JSEventListener::create(asObject(listener), wrapper, true, currentWorld(exec));
     123            return nullptr;
     124        return JSEventListener::create(asObject(listener), &wrapper, true, currentWorld(&state));
    116125    }
    117126
     127    inline Ref<JSEventListener> createJSEventListenerForRemove(JSC::ExecState& state, JSC::JSObject& listener, JSC::JSObject& wrapper)
     128    {
     129        return createJSEventListenerForAdd(state, listener, wrapper);
     130    }
    118131
    119132} // namespace WebCore

  • trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm

    r178411 r178633  
    143143sub GenerateAttributeEventListenerCall
    144144{
    145     my $className = shift;
    146145    my $implSetterFunctionName = shift;
    147146    my $windowEventListener = shift;
    … …  
    150149    my @GenerateEventListenerImpl = ();
    151150
    152     if ($className eq "JSSVGElementInstance") {
    153         # SVGElementInstances have to create JSEventListeners with the wrapper equal to the correspondingElement
    154         $wrapperObject = "asObject(correspondingElementWrapper)";
    155 
    156         push(@GenerateEventListenerImpl, <<END);
    157     JSValue correspondingElementWrapper = toJS(exec, castedThis->globalObject(), impl.correspondingElement());
    158     if (correspondingElementWrapper.isObject())
    159 END
    160 
    161         # Add leading whitespace to format the impl.set... line correctly
    162         push(@GenerateEventListenerImpl, "    ");
    163     }
    164 
    165     push(@GenerateEventListenerImpl, "    impl.set$implSetterFunctionName(createJSAttributeEventListener(exec, value, $wrapperObject));\n");
     151    push(@GenerateEventListenerImpl, "    impl.set$implSetterFunctionName(createJSEventListenerForAttribute(*exec, value, *$wrapperObject));\n");
    166152    return @GenerateEventListenerImpl;
    167153}
    … …  
    169155sub GenerateEventListenerCall
    170156{
    171     my $className = shift;
    172157    my $functionName = shift;
    173     my $passRefPtrHandling = ($functionName eq "add") ? "" : ".get()";
     158    my $suffix = ucfirst $functionName;
     159    my $passRefPtrHandling = ($functionName eq "add") ? "" : ".ptr()";
    174160
    175161    $implIncludes{"JSEventListener.h"} = 1;
    176162
    177163    my @GenerateEventListenerImpl = ();
    178     my $wrapperObject = "castedThis";
    179     if ($className eq "JSSVGElementInstance") {
    180         # SVGElementInstances have to create JSEventListeners with the wrapper equal to the correspondingElement
    181         $wrapperObject = "asObject(correspondingElementWrapper)";
    182 
    183         push(@GenerateEventListenerImpl, <<END);
    184     JSValue correspondingElementWrapper = toJS(exec, castedThis->globalObject(), impl.correspondingElement());
    185     if (!correspondingElementWrapper.isObject())
    186         return JSValue::encode(jsUndefined());
    187 END
    188     }
    189164
    190165    push(@GenerateEventListenerImpl, <<END);
    191166    JSValue listener = exec->argument(1);
    192     if (!listener.isObject())
     167    if (UNLIKELY(!listener.isObject()))
    193168        return JSValue::encode(jsUndefined());
    194     impl.${functionName}EventListener(exec->argument(0).toString(exec)->value(exec), JSEventListener::create(asObject(listener), $wrapperObject, false, currentWorld(exec))$passRefPtrHandling, exec->argument(2).toBoolean(exec));
     169    impl.${functionName}EventListener(exec->argument(0).toString(exec)->toAtomicString(exec), createJSEventListenerFor$suffix(*exec, *asObject(listener), *castedThis)$passRefPtrHandling, exec->argument(2).toBoolean(exec));
    195170    return JSValue::encode(jsUndefined());
    196171END
    … …  
    25982573                        push(@implContent, "    impl.set$implSetterFunctionName(createJSErrorHandler(exec, value, castedThis));\n");
    25992574                    } else {
    2600                         push(@implContent, GenerateAttributeEventListenerCall($className, $implSetterFunctionName, $windowEventListener));
     2575                        push(@implContent, GenerateAttributeEventListenerCall($implSetterFunctionName, $windowEventListener));
    26012576                    }
    26022577                } elsif ($attribute->signature->type =~ /Constructor$/) {
    … …  
    28382813                    # For compatibility with legacy content, the EventListener calls are generated without GenerateArgumentsCountCheck.
    28392814                    if ($function->signature->name eq "addEventListener") {
    2840                         push(@implContent, GenerateEventListenerCall($className, "add"));
     2815                        push(@implContent, GenerateEventListenerCall("add"));
    28412816                    } elsif ($function->signature->name eq "removeEventListener") {
    2842                         push(@implContent, GenerateEventListenerCall($className, "remove"));
     2817                        push(@implContent, GenerateEventListenerCall("remove"));
    28432818                    } else {
    28442819                        GenerateArgumentsCountCheck(\@implContent, $function, $interface);
    … …  
    45224497        return throwVMError(exec, createReferenceError(exec, "Constructor associated execution context is unavailable"));
    45234498
    4524     AtomicString eventType = exec->argument(0).toString(exec)->value(exec);
     4499    AtomicString eventType = exec->argument(0).toString(exec)->toAtomicString(exec);
    45254500    if (UNLIKELY(exec->hadException()))
    45264501        return JSValue::encode(jsUndefined());
Note: See TracChangeset for help on using the changeset viewer.