Bug 25009 – Upstream changes to WorkerContextExecutionProxy for V8 bindings in order to use V8EventListenerList as container.

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

[Jian Li]

Upstream changes to WorkerContextExecutionProxy for V8 bindings in order to use V8EventListenerList as container.

[Jian Li]

Created attachment 29203 [details]
Proposed Patch

Please land this patch after 25004 is landed and brought downstream.

[Darin Fisher (:fishd, Google)]

Comment on attachment 29203 [details]
Proposed Patch

> +++ b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp
> @@ -62,6 +62,9 @@ WorkerContextExecutionProxy::WorkerContextExecutionProxy(WorkerContext* workerCo
>      : m_workerContext(workerContext)
>      , m_recursion(0)
>  {
> +    // Need to use lock since V8EventListenerList constructor creates HandleScope.
> +    v8::Locker locker;
> +    m_listeners.set(new V8EventListenerList("m_listeners"));

Why not put the v8::Locker closer to where the HandleScope is used?

As is, if someone changes V8EventListenerList to no longer use a HandleScope, it
won't be obvious that they should also fix this code to no longer use a locker.


> +    {
> +        // Need to use lock since V8EventListenerList::clear() creates HandleScope.
> +        v8::Locker locker;
> +        m_listeners->clear();
> +    }

Same issue here.  Maybe the locker should move into the clear method?


> +        // Need to use lock since V8EventListenerList::add() creates HandleScope.
> +        v8::Locker locker;
> +        m_listeners->add(newListener.get());
> +    }

ditto


> +    // Need to use lock since V8EventListenerList::remove() creates HandleScope.
> +    v8::Locker locker;
> +    m_listeners->remove(listener);

ditto

Or, perhaps the issue is that when V8EventListenerList is used on the main
thread of WebKit, we don't want to have it use v8::Locker?  If that is the
case, then perhaps it would be wise to create a wrapper class for
V8EventlistenerList that encapsulates the locker usage?  Or maybe that is
just plain overkill?

[Jian Li]

(In reply to comment #2)
> (From update of attachment 29203 [details] [review])
> > +++ b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp
> > @@ -62,6 +62,9 @@ WorkerContextExecutionProxy::WorkerContextExecutionProxy(WorkerContext* workerCo
> >      : m_workerContext(workerContext)
> >      , m_recursion(0)
> >  {
> > +    // Need to use lock since V8EventListenerList constructor creates HandleScope.
> > +    v8::Locker locker;
> > +    m_listeners.set(new V8EventListenerList("m_listeners"));
> 
> Why not put the v8::Locker closer to where the HandleScope is used?
> 
> As is, if someone changes V8EventListenerList to no longer use a HandleScope,
> it
> won't be obvious that they should also fix this code to no longer use a locker.
> 
> 
> > +    {
> > +        // Need to use lock since V8EventListenerList::clear() creates HandleScope.
> > +        v8::Locker locker;
> > +        m_listeners->clear();
> > +    }
> 
> Same issue here.  Maybe the locker should move into the clear method?
> 
> 
> > +        // Need to use lock since V8EventListenerList::add() creates HandleScope.
> > +        v8::Locker locker;
> > +        m_listeners->add(newListener.get());
> > +    }
> 
> ditto
> 
> 
> > +    // Need to use lock since V8EventListenerList::remove() creates HandleScope.
> > +    v8::Locker locker;
> > +    m_listeners->remove(listener);
> 
> ditto
> 
> Or, perhaps the issue is that when V8EventListenerList is used on the main
> thread of WebKit, we don't want to have it use v8::Locker?  If that is the
> case, then perhaps it would be wise to create a wrapper class for
> V8EventlistenerList that encapsulates the locker usage?  Or maybe that is
> just plain overkill?
> 
Yes, V8EventListenerList could be used on the main thread of WebKit that does not use any v8::Locker. I think I can add another constructor to V8EventListenerList that takes additional parameter to indicate if v8::Locker should be used. How do you think?

[Jian Li]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 29203 [details] [review] [review])
> > > +++ b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp
> > > @@ -62,6 +62,9 @@ WorkerContextExecutionProxy::WorkerContextExecutionProxy(WorkerContext* workerCo
> > >      : m_workerContext(workerContext)
> > >      , m_recursion(0)
> > >  {
> > > +    // Need to use lock since V8EventListenerList constructor creates HandleScope.
> > > +    v8::Locker locker;
> > > +    m_listeners.set(new V8EventListenerList("m_listeners"));
> > 
> > Why not put the v8::Locker closer to where the HandleScope is used?
> > 
> > As is, if someone changes V8EventListenerList to no longer use a HandleScope,
> > it
> > won't be obvious that they should also fix this code to no longer use a locker.
> > 
> > 
> > > +    {
> > > +        // Need to use lock since V8EventListenerList::clear() creates HandleScope.
> > > +        v8::Locker locker;
> > > +        m_listeners->clear();
> > > +    }
> > 
> > Same issue here.  Maybe the locker should move into the clear method?
> > 
> > 
> > > +        // Need to use lock since V8EventListenerList::add() creates HandleScope.
> > > +        v8::Locker locker;
> > > +        m_listeners->add(newListener.get());
> > > +    }
> > 
> > ditto
> > 
> > 
> > > +    // Need to use lock since V8EventListenerList::remove() creates HandleScope.
> > > +    v8::Locker locker;
> > > +    m_listeners->remove(listener);
> > 
> > ditto
> > 
> > Or, perhaps the issue is that when V8EventListenerList is used on the main
> > thread of WebKit, we don't want to have it use v8::Locker?  If that is the
> > case, then perhaps it would be wise to create a wrapper class for
> > V8EventlistenerList that encapsulates the locker usage?  Or maybe that is
> > just plain overkill?
> > 
> Yes, V8EventListenerList could be used on the main thread of WebKit that does
> not use any v8::Locker. I think I can add another constructor to
> V8EventListenerList that takes additional parameter to indicate if v8::Locker
> should be used. How do you think?
> 

After talking with Darin, we feel that doing subclassing or adding flag in V8EventListenerList is too much pain. So we stay with what it is now.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/42215