LayoutTests:
authorweinig <weinig@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Feb 2007 03:48:56 +0000 (03:48 +0000)
committerweinig <weinig@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Feb 2007 03:48:56 +0000 (03:48 +0000)
        Reviewed by Maciej.

        - Test for http://bugs.webkit.org/show_bug.cgi?id=12850
          Leaks >10k objects

        * fast/events/remove-event-listener-expected.txt: Added.
        * fast/events/remove-event-listener.html: Added.

WebCore:

        Reviewed by Maciej.

        - Patch for http://bugs.webkit.org/show_bug.cgi?id=12850
          Leaks >10k objects

        and

        - http://bugs.webkit.org/show_bug.cgi?id=12853
          add a EventListener leak counter

        Problem: RemoveEventListener leaks memory if the listener is not
        registered.
        Fix: Added Window::findJSEventListener function w/o creating a
        JSEventListener; Renamed getJSEventListener to findOrCreateJSEventListener;

        As an enhancement, added a leak counter for EventListeners.

        Added a test case, LayoutTests/fast/events/remove-event-listener.html.

        * WebCore/bindings/js/kjs_dom.cpp:
        * WebCore/bindings/js/kjs_window.h:
        * WebCore/bindings/js/kjs_window.cpp:
        * WebCore/bindings/js/kjs_event.cpp: Add a leak counter.
        * WebCore/bindings/js/JSXMLHttpRequest.cpp:
        * LayoutTests/fast/events/remove-event-listener.html:

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

LayoutTests/ChangeLog
LayoutTests/fast/events/remove-event-listener-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/remove-event-listener.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/bindings/js/JSXMLHttpRequest.cpp
WebCore/bindings/js/kjs_dom.cpp
WebCore/bindings/js/kjs_events.cpp
WebCore/bindings/js/kjs_window.cpp
WebCore/bindings/js/kjs_window.h

index 4260ee6..d6a5e7b 100644 (file)
@@ -1,3 +1,13 @@
+2007-02-22  Ian Eng  <ian.eng.webkit@gmail.com>
+
+        Reviewed by Maciej.
+
+        - Test for http://bugs.webkit.org/show_bug.cgi?id=12850
+          Leaks >10k objects
+
+        * fast/events/remove-event-listener-expected.txt: Added.
+        * fast/events/remove-event-listener.html: Added.
+
 2007-02-22  Justin Garcia  <justin.garcia@apple.com>
 
         Reviewed by darin
diff --git a/LayoutTests/fast/events/remove-event-listener-expected.txt b/LayoutTests/fast/events/remove-event-listener-expected.txt
new file mode 100644 (file)
index 0000000..fcf69f9
--- /dev/null
@@ -0,0 +1 @@
+This page executes JavaScript that removes a non-existing event listener. It should not cause memory leaks.
diff --git a/LayoutTests/fast/events/remove-event-listener.html b/LayoutTests/fast/events/remove-event-listener.html
new file mode 100644 (file)
index 0000000..5e82910
--- /dev/null
@@ -0,0 +1,13 @@
+<html>
+<body>
+<script>
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+
+    // following code should leak the function and window/document objects.
+    function leaked(e) { var a = window; }
+    document.removeEventListener("mousemove", leaked, true);
+</script>
+This page executes JavaScript that removes a non-existing event listener. It should not cause memory leaks.
+</body>
+</html>
index ed8d491..f2ab5a2 100644 (file)
@@ -1,3 +1,31 @@
+2007-02-22  Ian Eng  <ian.eng.webkit@gmail.com>
+
+        Reviewed by Maciej.
+
+        - Patch for http://bugs.webkit.org/show_bug.cgi?id=12850
+          Leaks >10k objects
+
+        and
+
+        - http://bugs.webkit.org/show_bug.cgi?id=12853
+          add a EventListener leak counter
+
+        Problem: RemoveEventListener leaks memory if the listener is not
+        registered.
+        Fix: Added Window::findJSEventListener function w/o creating a 
+        JSEventListener; Renamed getJSEventListener to findOrCreateJSEventListener;
+
+        As an enhancement, added a leak counter for EventListeners.
+
+        Added a test case, LayoutTests/fast/events/remove-event-listener.html.
+
+        * WebCore/bindings/js/kjs_dom.cpp:
+        * WebCore/bindings/js/kjs_window.h:
+        * WebCore/bindings/js/kjs_window.cpp:
+        * WebCore/bindings/js/kjs_event.cpp: Add a leak counter.
+        * WebCore/bindings/js/JSXMLHttpRequest.cpp:
+        * LayoutTests/fast/events/remove-event-listener.html:
+
 2007-02-22  Anders Carlsson  <acarlsson@apple.com>
 
         Reviewed by Geoff.
index 76c139c..5abb934 100644 (file)
@@ -139,10 +139,10 @@ void JSXMLHttpRequest::putValueProperty(ExecState* exec, int token, JSValue* val
 {
     switch (token) {
         case Onreadystatechange:
-            m_impl->setOnReadyStateChangeListener(Window::retrieveActive(exec)->getJSUnprotectedEventListener(value, true));
+            m_impl->setOnReadyStateChangeListener(Window::retrieveActive(exec)->findOrCreateJSUnprotectedEventListener(value, true));
             break;
         case Onload:
-            m_impl->setOnLoadListener(Window::retrieveActive(exec)->getJSUnprotectedEventListener(value, true));
+            m_impl->setOnLoadListener(Window::retrieveActive(exec)->findOrCreateJSUnprotectedEventListener(value, true));
             break;
     }
 }
@@ -272,13 +272,13 @@ JSValue* JSXMLHttpRequestPrototypeFunction::callAsFunction(ExecState* exec, JSOb
             return jsUndefined();
         
         case JSXMLHttpRequest::AddEventListener: {
-            JSUnprotectedEventListener* listener = Window::retrieveActive(exec)->getJSUnprotectedEventListener(args[1], true);
+            JSUnprotectedEventListener* listener = Window::retrieveActive(exec)->findOrCreateJSUnprotectedEventListener(args[1], true);
             if (listener)
                 request->m_impl->addEventListener(args[0]->toString(exec), listener, args[2]->toBoolean(exec));
             return jsUndefined();
         }
         case JSXMLHttpRequest::RemoveEventListener: {
-            JSUnprotectedEventListener* listener = Window::retrieveActive(exec)->getJSUnprotectedEventListener(args[1], true);
+            JSUnprotectedEventListener* listener = Window::retrieveActive(exec)->findJSUnprotectedEventListener(args[1], true);
             if (listener)
                 request->m_impl->removeEventListener(args[0]->toString(exec), listener, args[2]->toBoolean(exec));
             return jsUndefined();
index 94f97bf..bbf3541 100644 (file)
@@ -623,7 +623,7 @@ void DOMEventTargetNode::putValueProperty(ExecState* exec, int token, JSValue* v
 
 void DOMEventTargetNode::setListener(ExecState* exec, const AtomicString &eventType, JSValue* func) const
 {
-    EventTargetNodeCast(impl())->setHTMLEventListener(eventType, Window::retrieveActive(exec)->getJSEventListener(func, true));
+    EventTargetNodeCast(impl())->setHTMLEventListener(eventType, Window::retrieveActive(exec)->findOrCreateJSEventListener(func, true));
 }
 
 JSValue* DOMEventTargetNode::getListener(const AtomicString &eventType) const
@@ -661,14 +661,14 @@ JSValue* DOMEventTargetNodePrototypeFunction::callAsFunction(ExecState* exec, JS
     EventTargetNode* node = static_cast<EventTargetNode*>(DOMNode->impl());
     switch (id) {
         case DOMEventTargetNode::AddEventListener: {
-            JSEventListener *listener = Window::retrieveActive(exec)->getJSEventListener(args[1]);
+            JSEventListener *listener = Window::retrieveActive(exec)->findOrCreateJSEventListener(args[1]);
             if (listener)
                 node->addEventListener(args[0]->toString(exec), listener,args[2]->toBoolean(exec));
             return jsUndefined();
         }
         case DOMEventTargetNode::RemoveEventListener: {
-            JSEventListener *listener = Window::retrieveActive(exec)->getJSEventListener(args[1]);
-            if (listener)
+            JSEventListener *listener = Window::retrieveActive(exec)->findJSEventListener(args[1]);
+            if (listener) 
                 node->removeEventListener(args[0]->toString(exec), listener,args[2]->toBoolean(exec));
             return jsUndefined();
         }
index 35a6fc2..281fada 100644 (file)
@@ -201,6 +201,24 @@ void JSUnprotectedEventListener::mark()
         listener->mark();
 }
 
+#ifndef NDEBUG
+#ifndef LOG_CHANNEL_PREFIX
+#define LOG_CHANNEL_PREFIX Log
+#endif
+WTFLogChannel LogWebCoreEventListenerLeaks = { 0x00000000, "", WTFLogChannelOn };
+
+struct EventListenerCounter {
+    static unsigned count;
+    ~EventListenerCounter()
+    {
+        if (count)
+            LOG(WebCoreEventListenerLeaks, "LEAK: %u EventListeners\n", count);
+    }
+};
+unsigned EventListenerCounter::count = 0;
+static EventListenerCounter eventListenerCounter;
+#endif
+
 // -------------------------------------------------------------------------
 
 JSEventListener::JSEventListener(JSObject* _listener, Window* _win, bool _html)
@@ -213,6 +231,9 @@ JSEventListener::JSEventListener(JSObject* _listener, Window* _win, bool _html)
             ? _win->jsHTMLEventListeners : _win->jsEventListeners;
         listeners.set(_listener, this);
     }
+#ifndef NDEBUG
+    ++eventListenerCounter.count;
+#endif
 }
 
 JSEventListener::~JSEventListener()
@@ -222,6 +243,9 @@ JSEventListener::~JSEventListener()
             ? win->jsHTMLEventListeners : win->jsEventListeners;
         listeners.remove(listener);
     }
+#ifndef NDEBUG
+    --eventListenerCounter.count;
+#endif
 }
 
 JSObject* JSEventListener::listenerObj() const
index 26b6342..261cb69 100644 (file)
@@ -1280,7 +1280,7 @@ void Window::setListener(ExecState *exec, const AtomicString &eventType, JSValue
   if (!doc)
     return;
 
-  doc->setHTMLWindowEventListener(eventType, getJSEventListener(func,true));
+  doc->setHTMLWindowEventListener(eventType, findOrCreateJSEventListener(func,true));
 }
 
 JSValue *Window::getListener(ExecState *exec, const AtomicString &eventType) const
@@ -1298,30 +1298,48 @@ JSValue *Window::getListener(ExecState *exec, const AtomicString &eventType) con
     return jsNull();
 }
 
-JSEventListener *Window::getJSEventListener(JSValue *val, bool html)
+JSEventListener* Window::findJSEventListener(JSValue* val, bool html)
 {
+    if (!val->isObject())
+        return 0;
+    JSObject* object = static_cast<JSObject*>(val);
+    ListenersMap& listeners = html ? jsHTMLEventListeners : jsEventListeners;
+    return listeners.get(object);
+}
+
+JSEventListener *Window::findOrCreateJSEventListener(JSValue *val, bool html)
+{
+  JSEventListener *listener = findJSEventListener(val, html);
+  if (listener)
+    return listener;
+
   if (!val->isObject())
     return 0;
   JSObject *object = static_cast<JSObject *>(val);
 
-  ListenersMap& listeners = html ? jsHTMLEventListeners : jsEventListeners;
-  if (JSEventListener* listener = listeners.get(object))
-    return listener;
-
   // Note that the JSEventListener constructor adds it to our jsEventListeners list
   return new JSEventListener(object, this, html);
 }
 
-JSUnprotectedEventListener *Window::getJSUnprotectedEventListener(JSValue *val, bool html)
+JSUnprotectedEventListener* Window::findJSUnprotectedEventListener(JSValue* val, bool html)
 {
-  if (!val->isObject())
-    return 0;
-  JSObject* object = static_cast<JSObject *>(val);
+    if (!val->isObject())
+        return 0;
+    JSObject* object = static_cast<JSObject*>(val);
+    UnprotectedListenersMap& listeners = html ? jsUnprotectedHTMLEventListeners : jsUnprotectedEventListeners;
+    return listeners.get(object);
+}
 
-  UnprotectedListenersMap& listeners = html ? jsUnprotectedHTMLEventListeners : jsUnprotectedEventListeners;
-  if (JSUnprotectedEventListener* listener = listeners.get(object))
+JSUnprotectedEventListener *Window::findOrCreateJSUnprotectedEventListener(JSValue *val, bool html)
+{
+  JSUnprotectedEventListener *listener = findJSUnprotectedEventListener(val, html);
+  if (listener)
     return listener;
 
+  if (!val->isObject())
+    return 0;
+  JSObject *object = static_cast<JSObject *>(val);
+
   // The JSUnprotectedEventListener constructor adds it to our jsUnprotectedEventListeners map.
   return new JSUnprotectedEventListener(object, this, html);
 }
@@ -1787,14 +1805,14 @@ JSValue *WindowFunc::callAsFunction(ExecState *exec, JSObject *thisObj, const Li
   case Window::AddEventListener:
         if (!window->isSafeScript(exec))
             return jsUndefined();
-        if (JSEventListener *listener = Window::retrieveActive(exec)->getJSEventListener(args[1]))
+        if (JSEventListener *listener = Window::retrieveActive(exec)->findOrCreateJSEventListener(args[1]))
             if (Document *doc = frame->document())
                 doc->addWindowEventListener(AtomicString(args[0]->toString(exec)), listener, args[2]->toBoolean(exec));
         return jsUndefined();
   case Window::RemoveEventListener:
         if (!window->isSafeScript(exec))
             return jsUndefined();
-        if (JSEventListener *listener = Window::retrieveActive(exec)->getJSEventListener(args[1]))
+        if (JSEventListener *listener = Window::retrieveActive(exec)->findJSEventListener(args[1]))
             if (Document *doc = frame->document())
                 doc->removeWindowEventListener(AtomicString(args[0]->toString(exec)), listener, args[2]->toBoolean(exec));
         return jsUndefined();
index 460d2d1..51195b6 100644 (file)
@@ -135,8 +135,19 @@ namespace KJS {
     BarInfo *scrollbars(ExecState*) const;
     BarInfo *statusbar(ExecState*) const;
     BarInfo *toolbar(ExecState*) const;
-    JSEventListener *getJSEventListener(JSValue*, bool html = false);
-    JSUnprotectedEventListener *getJSUnprotectedEventListener(JSValue*, bool html = false);
+
+    // Finds a wrapper of a JS EventListener, returns 0 if no existing one.
+    JSEventListener* findJSEventListener(JSValue*, bool html = false);
+
+    // Finds or creates a wrapper of a JS EventListener. JS EventListener object is GC-protected.
+    JSEventListener *findOrCreateJSEventListener(JSValue*, bool html = false);
+
+    // Finds a wrapper of a GC-unprotected JS EventListener, returns 0 if no existing one.
+    JSUnprotectedEventListener* findJSUnprotectedEventListener(JSValue*, bool html = false);
+
+    // Finds or creates a wrapper of a JS EventListener. JS EventListener object is *NOT* GC-protected.
+    JSUnprotectedEventListener *findOrCreateJSUnprotectedEventListener(JSValue*, bool html = false);
+
     void clear();
     virtual UString toString(ExecState *) const;