- Revision
- 178633
- Author
- [email protected]
- Date
- 2015-01-18 12:57:26 -0800 (Sun, 18 Jan 2015)
Log 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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (178632 => 178633)
--- trunk/Source/WebCore/ChangeLog 2015-01-18 18:39:59 UTC (rev 178632)
+++ trunk/Source/WebCore/ChangeLog 2015-01-18 20:57:26 UTC (rev 178633)
@@ -1,3 +1,54 @@
+2015-01-18 Darin Adler <[email protected]>
+
+ 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 <[email protected]>
Web Inspector: highlight data for overlay should use protocol type builders
Modified: trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp (178632 => 178633)
--- trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp 2015-01-18 18:39:59 UTC (rev 178632)
+++ trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp 2015-01-18 20:57:26 UTC (rev 178633)
@@ -652,7 +652,7 @@
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 @@
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();
}
Modified: trunk/Source/WebCore/bindings/js/JSEventListener.cpp (178632 => 178633)
--- trunk/Source/WebCore/bindings/js/JSEventListener.cpp 2015-01-18 18:39:59 UTC (rev 178632)
+++ trunk/Source/WebCore/bindings/js/JSEventListener.cpp 2015-01-18 20:57:26 UTC (rev 178633)
@@ -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 @@
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
Modified: trunk/Source/WebCore/bindings/js/JSEventListener.h (178632 => 178633)
--- trunk/Source/WebCore/bindings/js/JSEventListener.h 2015-01-18 18:39:59 UTC (rev 178632)
+++ trunk/Source/WebCore/bindings/js/JSEventListener.h 2015-01-18 20:57:26 UTC (rev 178633)
@@ -30,12 +30,13 @@
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 @@
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,16 +116,19 @@
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 nullptr;
+ return JSEventListener::create(asObject(listener), &wrapper, true, currentWorld(&state));
+ }
- return JSEventListener::create(asObject(listener), wrapper, true, currentWorld(exec));
+ inline Ref<JSEventListener> createJSEventListenerForRemove(JSC::ExecState& state, JSC::JSObject& listener, JSC::JSObject& wrapper)
+ {
+ return createJSEventListenerForAdd(state, listener, wrapper);
}
-
} // namespace WebCore
#endif // JSEventListener_h
Modified: trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (178632 => 178633)
--- trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm 2015-01-18 18:39:59 UTC (rev 178632)
+++ trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm 2015-01-18 20:57:26 UTC (rev 178633)
@@ -142,56 +142,31 @@
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 @@
$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 @@
# 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 @@
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());
