Bug 28156 – [V8] Fix memory leak in node event listeners. See http://crbug.com/17400.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vitaly Repeshko]

[V8] Fix http://crbug.com/17400.

[Vitaly Repeshko]

Created attachment 34482 [details]
patch

[Dimitri Glazkov (Google)]

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 @@
> [phone]  Vitaly Repeshko  <[e-mail]>
> +
> +        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;

[Vitaly Repeshko]

Created attachment 34507 [details]
updated patch

[Vitaly Repeshko]

(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 @@
> > [phone]  Vitaly Repeshko  <[e-mail]>
> > +
> > +        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.

[Dimitri Glazkov (Google)]

Comment on attachment 34507 [details]
updated patch

r=me

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/47001.