Bug 30648 – [V8] Change event listeners to not hold context.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dimitri Glazkov (Google)]

This change largely just aligns V8 bindings with http://trac.webkit.org/changeset/48884.

In the process, most of bug 29077's work is demolished, because we no longer need to persist v8::Context in listeners. I left it for isolated worlds, but only because I don't know much about them.

[Dimitri Glazkov (Google)]

Created attachment 41620 [details]
Change event listeners to not hold context, v1.

[Adam Barth]

Comment on attachment 41620 [details]
Change event listeners to not hold context, v1.

In v8::Local<v8::Context> V8Proxy::context() we seem to have lost this code:

if (frame != V8Proxy::retrieveFrame(context->get()))

Can you add it back before landing this?  I know there isn't a test that explains this code.  The behavior is "wrong" but missing the code is worse.

Also, this patch as a subtle bug with isolated worlds and event handlers and I must be tested properly.  I think because we're testing it with JS events, not native events.  I think we should land this patch anyway and deal with that issue separately (if it isn't a figment of my imagination).

Other than that *this patch is awesome*.  In my ideal world, the bindings would have little or no knowledge of Frame.

[Dimitri Glazkov (Google)]

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

[Christian Plesner Hansen]

> -    v8::Handle<v8::Context> v8Context = v8::Local<v8::Context>::New(m_context->get());
> +    v8::Local<v8::Context> v8Context = toV8Context(context);

I'm not too familiar with this code so I may be wrong, but doesn't this switch event execution from occurring in the context where the event was defined to the current context?

> -    context()->Global()->ForceSet(v8::String::New("document"), documentWrapper, static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
> +    m_context->Global()->ForceSet(v8::String::New("document"), documentWrapper, static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
>  }

As a general principle you should not do operations that involve allocation directly on persistent handles, since persistent handles may be disposed during gc.  This change (and the other similar changes) may be safe but there are other places where we do extra work to wrap contexts in Locals to avoid this issue so it does seem a little suspect.

[Adam Barth]

> As a general principle you should not do operations that involve allocation
> directly on persistent handles, since persistent handles may be disposed during
> gc.

How can the handle be disposed if we're holding it persistently?  Isn't the point of a persistent handle that we don't want it to be GCed?

[Christian Plesner Hansen]

> How can the handle be disposed if we're holding it persistently?  Isn't the
> point of a persistent handle that we don't want it to be GCed?

The object stored in the handle will be kept alive but if the handle itself is weak the weak callback may cause it to be disposed.