Bug 12850 – Event listener leaks seen @ gmail.com

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Feng Qian]

You need a gmail account. In KJS::Collector::collect(), before exiting the function, log heap.numLiveObjects. After visiting GMail, close the window, open a new one, there are still >10k live objects.

I looked further, there are protected objects (FunctionImp) that cannot be reclaimed. These protected FunctionImp also causes other objects to leak.

[Feng Qian]

Created attachment 13311 [details]
fix RemoveEventListener

I think the problem is in kjs_window.cpp, Window::getJSEventListener, which creates a JSEventListener object if it does not exist. The constructor of JSEventListener makes JSValue GC-protected.

In Window::RemoveEventListener and DOMEventTargetNode::RemoveEventListener, both calls getJSEventListener to check if a listener exists. This leaks both JSEventListener and the JavaScript event listener object.

I added a new function Window::hasJSEventListener for checking if a JSEventListener exists for a JS event listener object without creating a new one.

Before the change, run-safari and visit Gmail, then exist. Safari reports:
LEAK: 2102 Node
LEAK: 158688 KJS::Node

After applying the patch, no message reported.

[Maciej Stachowiak]

Comment on attachment 13311 [details]
fix RemoveEventListener

The change itself looks good. Thanks, this will is a really valuable fix! Technicalities:

- Needs a ChangeLog entry
- Needs a test case
- Per the coding style guidelines, the * should go next to the type name, although it is hard to tell from the current surrounding code.

r- for now for the technicalities.

[Maciej Stachowiak]

May also fix http://bugs.webkit.org/show_bug.cgi?id=12659

[Feng Qian]

Created attachment 13324 [details]
test case

It leaks several objects.

[Feng Qian]

Created attachment 13325 [details]
revised patch

Renamed hasJSEventListener to findJSEventListener.
Fixed some style issues and added an entry in ChangeLog.

[Feng Qian]

(In reply to comment #3)
> May also fix http://bugs.webkit.org/show_bug.cgi?id=12659
> 

I cannot find significant number of leaked JS object when visiting both sites even without fixing this bug. It might be fixed by an early patch that clearing helper object properties of window object.

[Feng Qian]

Created attachment 13326 [details]
fixed wrong path names in ChangeLog

[Feng Qian]

*** Bug 12853 has been marked as a duplicate of this bug. ***

[Feng Qian]

Created attachment 13327 [details]
revised patch again

added findJSUnprotectedEventListener and fixed use in JSXMLHttpRequest.cpp;
added a leak counter for EventListeners.

[Darin Adler]

Comment on attachment 13327 [details]
revised patch again

Great fix!

Is there no way to add a test for this? We really want a test for each bug fix if at all possible.

+            if (listener) {
                 node->removeEventListener(args[0]->toString(exec), listener,args[2]->toBoolean(exec));
+            }

Please don't add braces. Our coding style guidelines call for no braces on a single-line if statement. Also, adding those braces makes the patch longer for no good reason.

+JSEventListener *Window::findJSEventListener(JSValue* val, bool html) {

Braces that open a function call go on a separate line per our guidelines. The * goes next to the type name as per our style guidelines.

+  JSObject *object = static_cast<JSObject *>(val);

The * goes next to the type name as per our style guidelines.

-  case Window::RemoveEventListener:
+  case Window::RemoveEventListener: {
         if (!window->isSafeScript(exec))
             return jsUndefined();
-        if (JSEventListener *listener = Window::retrieveActive(exec)->getJSEventListener(args[1]))
+        JSEventListener *listener = Window::retrieveActive(exec)->findJSEventListener(args[1]);
+        if (listener) {
             if (Document *doc = frame->document())
                 doc->removeWindowEventListener(AtomicString(args[0]->toString(exec)), listener, args[2]->toBoolean(exec));
+        }
         return jsUndefined();
+  }

Why the gratuitous style change here? By putting the variable inside the if we avoid the need for braces around the entire case statement. There's no need to tweak the style while fixing this bug; I think it just makes the patch bigger and obscures the point of what you're fixing.

+    JSEventListener *findJSEventListener(JSValue*, bool html = false);

* goes next to the type name.

+    // JS Object.
+    JSUnprotectedEventListener *findJSUnprotectedEventListener(JSValue*, bool html = false);

Please put a blank lines between the comment and the declaration here as with the other functions.

[Feng Qian]

(In reply to comment #10)
> (From update of attachment 13327 [details] [edit])
> Great fix!
>

Thanks for your quick review. Here are some of my arguments:
 
> Is there no way to add a test for this? We really want a test for each bug fix
> if at all possible.

I attached a test case to in the bug report.
http://bugs.webkit.org/attachment.cgi?id=13324&action=view


> 
> +            if (listener) {
>                  node->removeEventListener(args[0]->toString(exec),
> listener,args[2]->toBoolean(exec));
> +            }
> 
> Please don't add braces. Our coding style guidelines call for no braces on a
> single-line if statement. Also, adding those braces makes the patch longer for
> no good reason.
> 
> +JSEventListener *Window::findJSEventListener(JSValue* val, bool html) {
> 
> Braces that open a function call go on a separate line per our guidelines. The
> * goes next to the type name as per our style guidelines.

I followed local style in the file. The local style also uses 2 spaces for indention.

> 
> +  JSObject *object = static_cast<JSObject *>(val);
> 
> The * goes next to the type name as per our style guidelines.
> 
> -  case Window::RemoveEventListener:
> +  case Window::RemoveEventListener: {
>          if (!window->isSafeScript(exec))
>              return jsUndefined();
> -        if (JSEventListener *listener =
> Window::retrieveActive(exec)->getJSEventListener(args[1]))
> +        JSEventListener *listener =
> Window::retrieveActive(exec)->findJSEventListener(args[1]);
> +        if (listener) {
>              if (Document *doc = frame->document())
>                 
> doc->removeWindowEventListener(AtomicString(args[0]->toString(exec)), listener,
> args[2]->toBoolean(exec));
> +        }
>          return jsUndefined();
> +  }
> 
> Why the gratuitous style change here? By putting the variable inside the if we
> avoid the need for braces around the entire case statement. There's no need to
> tweak the style while fixing this bug; I think it just makes the patch bigger
> and obscures the point of what you're fixing.
> 
> +    JSEventListener *findJSEventListener(JSValue*, bool html = false);
> 
> * goes next to the type name.
> 
> +    // JS Object.
> +    JSUnprotectedEventListener *findJSUnprotectedEventListener(JSValue*, bool
> html = false);
> 
> Please put a blank lines between the comment and the declaration here as with
> the other functions.
> 

[Feng Qian]

Created attachment 13328 [details]
revised patch again again

In new functions I added, I follow style guide: 4-space indention and * following type name. 

In existing functions, I follow the local rules.

[Maciej Stachowiak]

Comment on attachment 13328 [details]
revised patch again again

The right way to add a test case is to included it in the patch, in the LayoutTests directory. In this case, LayoutTests/fast/events would be the appropriate directory. This is so the person committing doesn't have to do extra work applying the patch.

Also: I think the comments on the findJSEventListener and related methods are excessive. Can we get these down to one or two lines instead of having whole paragraphs in the header?

Better yet, how about renaming the methods to make behavior more clear from the name? getJSEventListener could be renamed to findOrCreateJSEventListener for instance.

r- for lack of test case. 

Please also consider my feedback about the large comments. We tend to avoid those in WebKit because they can make it harder to read the code. Code-wise though, the patch is fine to land once a test case is added to the patch.

[Feng Qian]

Created attachment 13336 [details]
patch again again again

Your comments are well taken.

[Maciej Stachowiak]

Comment on attachment 13336 [details]
patch again again again

r=me

[Sam Weinig]

Landed in r19820.