Bug 28156 - [V8] Fix memory leak in node event listeners. See http://crbug.com/17400.
Summary: [V8] Fix memory leak in node event listeners. See http://crbug.com/17400.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-10 10:48 PDT by Vitaly Repeshko
Modified: 2009-08-10 13:37 PDT (History)
1 user (show)

See Also:


Attachments
patch (16.90 KB, patch)
2009-08-10 10:55 PDT, Vitaly Repeshko
dglazkov: review-
Details | Formatted Diff | Diff
updated patch (17.63 KB, patch)
2009-08-10 13:24 PDT, Vitaly Repeshko
dglazkov: review+
dglazkov: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.