Bug 28156

Summary: [V8] Fix memory leak in node event listeners. See http://crbug.com/17400.
Product: WebKit Reporter: Vitaly Repeshko <vitalyr>
Component: WebCore Misc.Assignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch
dglazkov: review-
updated patch dglazkov: review+, dglazkov: commit-queue+

Description Vitaly Repeshko 2009-08-10 10:48:47 PDT
[V8] Fix http://crbug.com/17400.
Comment 1 Vitaly Repeshko 2009-08-10 10:55:02 PDT
Created attachment 34482 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2009-08-10 11:18:26 PDT
Comment on attachment 34482 [details]
patch

Just a few style nits, LG overall:

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index ded636d..0b05faa 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,32 @@
> +2009-08-10  Vitaly Repeshko  <vitalyr@quad.spb.corp.google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [V8] Fix memory leak in node event listeners. See http://crbug.com/17400.
> +
> +	https://bugs.webkit.org/show_bug.cgi?id=28156

Indent w/same spacing as summary, no need for an extra line break between URL and summary.

> +        * bindings/scripts/CodeGeneratorV8.pm:
> +        * bindings/v8/V8AbstractEventListener.h:
> +        (WebCore::V8AbstractEventListener::isObjectListener):
> +        * bindings/v8/V8DOMWrapper.cpp:
> +        (WebCore::V8DOMWrapper::getTemplate):
> +        * bindings/v8/V8ObjectEventListener.cpp:
> +        * bindings/v8/V8ObjectEventListener.h:
> +        (WebCore::V8ObjectEventListener::isObjectListener):
> +        * bindings/v8/V8Utilities.cpp:
> +        (WebCore::removeHiddenDependency):
> +        * bindings/v8/custom/V8CustomBinding.h:
> +        * bindings/v8/custom/V8ElementCustom.cpp:
> +        * bindings/v8/custom/V8NodeCustom.cpp:
> +        (WebCore::toEventType):
> +        (WebCore::getEventListener):
> +        (WebCore::ACCESSOR_SETTER):
> +        (WebCore::ACCESSOR_GETTER):
> +        (WebCore::CALLBACK_FUNC_DECL):
> +        * bindings/v8/custom/V8XMLHttpRequestCustom.cpp:
> +        (WebCore::getEventListener):

Please provide brief explanation of what changes were made for each method (or file, if the methods aren't listed). For example:

* bindings/scripts/CodeGeneratorV8.pm: Changed event handler to live at Node, rather than Element.

> +END
> +    if (IsNodeSubType($dataNode)) {
> +        push(@implContent, <<END);
> +  instance->SetInternalFieldCount(V8Custom::kNodeMinimumInternalFieldCount);

Is spacing correct here?

> +END
> +    } else {
> +        push(@implContent, <<END);
> +  instance->SetInternalFieldCount(V8Custom::kDefaultWrapperInternalFieldCount);

Ditto.

> +    return PassRefPtr<EventListener>();

return 0;
Comment 3 Vitaly Repeshko 2009-08-10 13:24:32 PDT
Created attachment 34507 [details]
updated patch
Comment 4 Vitaly Repeshko 2009-08-10 13:25:50 PDT
(In reply to comment #2)
> (From update of attachment 34482 [details])
> Just a few style nits, LG overall:
> 
> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> > index ded636d..0b05faa 100644
> > --- a/WebCore/ChangeLog
> > +++ b/WebCore/ChangeLog
> > @@ -1,3 +1,32 @@
> > +2009-08-10  Vitaly Repeshko  <vitalyr@quad.spb.corp.google.com>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        [V8] Fix memory leak in node event listeners. See http://crbug.com/17400.
> > +
> > +	https://bugs.webkit.org/show_bug.cgi?id=28156
> 
> Indent w/same spacing as summary, no need for an extra line break between URL
> and summary.
> 
> > +        * bindings/scripts/CodeGeneratorV8.pm:
> > +        * bindings/v8/V8AbstractEventListener.h:
> > +        (WebCore::V8AbstractEventListener::isObjectListener):
> > +        * bindings/v8/V8DOMWrapper.cpp:
> > +        (WebCore::V8DOMWrapper::getTemplate):
> > +        * bindings/v8/V8ObjectEventListener.cpp:
> > +        * bindings/v8/V8ObjectEventListener.h:
> > +        (WebCore::V8ObjectEventListener::isObjectListener):
> > +        * bindings/v8/V8Utilities.cpp:
> > +        (WebCore::removeHiddenDependency):
> > +        * bindings/v8/custom/V8CustomBinding.h:
> > +        * bindings/v8/custom/V8ElementCustom.cpp:
> > +        * bindings/v8/custom/V8NodeCustom.cpp:
> > +        (WebCore::toEventType):
> > +        (WebCore::getEventListener):
> > +        (WebCore::ACCESSOR_SETTER):
> > +        (WebCore::ACCESSOR_GETTER):
> > +        (WebCore::CALLBACK_FUNC_DECL):
> > +        * bindings/v8/custom/V8XMLHttpRequestCustom.cpp:
> > +        (WebCore::getEventListener):
> 
> Please provide brief explanation of what changes were made for each method (or
> file, if the methods aren't listed). For example:
> 
> * bindings/scripts/CodeGeneratorV8.pm: Changed event handler to live at Node,
> rather than Element.

Done.

> > +END
> > +    if (IsNodeSubType($dataNode)) {
> > +        push(@implContent, <<END);
> > +  instance->SetInternalFieldCount(V8Custom::kNodeMinimumInternalFieldCount);
> 
> Is spacing correct here?
> 
> > +END
> > +    } else {
> > +        push(@implContent, <<END);
> > +  instance->SetInternalFieldCount(V8Custom::kDefaultWrapperInternalFieldCount);
> 
> Ditto.

Seems okay to me. Here we have separate indentation for Perl and generated code.

> > +    return PassRefPtr<EventListener>();
> 
> return 0;

Done.
Comment 5 Dimitri Glazkov (Google) 2009-08-10 13:27:51 PDT
Comment on attachment 34507 [details]
updated patch

r=me
Comment 6 Dimitri Glazkov (Google) 2009-08-10 13:37:42 PDT
Landed as http://trac.webkit.org/changeset/47001.