Bug 29825 – [V8] Refactored V8 event listeners.

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

[Vitaly Repeshko]

[V8] Refactored V8 event listeners.

[Vitaly Repeshko]

Created attachment 40260 [details]
patch: WIP

Dimitri, please have a look. This is still WIP (testing lazy listeners) and depends on http://code.google.com/p/v8/source/detail?r=2980 (should be in V8 trunk this week), but I think it's almost done.

[Ojan Vafai]

drive-by comment: Is there any way to test for leaks so this doesn't regress?

[Sam Weinig]

If this is still a WIP, this should not be marked for review.  One question though, why are the attribute event listeners marked as OnProto?  This does not match the JSC bindings.  Are there tests that indicate it should be this way?

[Vitaly Repeshko]

(In reply to comment #2)
> drive-by comment: Is there any way to test for leaks so this doesn't regress?

Yes, for fixed leaks I'll extend src/webkit/tools/test_shell/listener_leak_test.cc.

[Vitaly Repeshko]

(In reply to comment #3)
> If this is still a WIP, this should not be marked for review.  One question
> though, why are the attribute event listeners marked as OnProto?  This does not
> match the JSC bindings.  Are there tests that indicate it should be this way?

They are marked with v8onproto attribute because old custom V8 code used to look up the holder of the property in prototype chain. I tried to copy the old behavior. I'll double-check if this is in fact needed. If JSC doesn't need this, chances are V8 doesn't need either.

[Dimitri Glazkov (Google)]

Comment on attachment 40260 [details]
patch: WIP

+1 on investigating v8onproto.

[Vitaly Repeshko]

Created attachment 40360 [details]
updated patch

Notification does not require v8onproto. Removed.

Remaining memory leaks in lazy listeners are unrelated to handles and are caused by Function cache for functions created by FunctionTemplate. I'm going to fix usages of FunctionTemplate in V8 bindings in a separate patch.

[Dimitri Glazkov (Google)]

Comment on attachment 40360 [details]
updated patch

This is super-awesome. I only have style nits.

I would really like to use enums for isAttribute params in getEventListener. That should clarify the call sites.

> +    V8Proxy* proxy = V8Proxy::retrieve(worker->scriptExecutionContext());
> +    if (proxy) {
> +        return findOnly ? V8EventListenerList::findWrapper(value, isAttribute) : V8EventListenerList::findOrCreateWrapper<V8EventListener>(proxy->frame(), value, isAttribute);
> +    }

Don't need braces here.

> +    if (proxy) {
> +        return findOnly ? V8EventListenerList::findWrapper(value, isAttribute) : V8EventListenerList::findOrCreateWrapper<V8EventListener>(proxy->frame(), value, isAttribute);
> +    }

Ditto.

> +v8::Handle<v8::String> V8HiddenPropertyName::listener()
> +{
> +    static v8::Persistent<v8::String>* string = createString("WebCore::V8HiddenPropertyName::listener");
> +

no line break here.

> +    return *string;
> +}
> +
> +v8::Handle<v8::String> V8HiddenPropertyName::attributeListener()
> +{
> +    static v8::Persistent<v8::String>* string = createString("WebCore::V8HiddenPropertyName::attributeListener");

Ditto.

> +void V8Proxy::disconnectEventListeners()
> +{
> +}
> +

Why is this here? Can we just get rid of it?

[Vitaly Repeshko]

Created attachment 40386 [details]
updated patch v2

[Dimitri Glazkov (Google)]

Comment on attachment 40386 [details]
updated patch v2

r=me.

[WebKit Commit Bot]

Comment on attachment 40386 [details]
updated patch v2

Clearing flags on attachment: 40386

Committed r48978: <http://trac.webkit.org/changeset/48978>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

See http://code.google.com/p/chromium/issues/detail?id=23597 I guess this caused some sort of regression.  I'm happy to roll this out if that's what's desired.

bugzilla-tool rollout 48978

will prepare the rollout.

bugzilla-tool rollout 48978 --complete-rollout --force-clean

would do the full rollout including testing to make sure it builds/passes tests and updating the bug.

[Dimitri Glazkov (Google)]

Let's give vitalyr a chance to fix this. Sadness that it broke.

[Vitaly Repeshko]

(In reply to comment #14)
> Let's give vitalyr a chance to fix this. Sadness that it broke.

Reverting this patch doesn't fix the breakage. Please see discussion in http://code.google.com/p/chromium/issues/detail?id=23597.