Bug 25009 - Upstream changes to WorkerContextExecutionProxy for V8 bindings in order to use V8EventListenerList as container.
Summary: Upstream changes to WorkerContextExecutionProxy for V8 bindings in order to u...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on: 25004
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-02 11:59 PDT by Jian Li
Modified: 2009-04-03 15:25 PDT (History)
1 user (show)

See Also:


Attachments
Proposed Patch (4.63 KB, patch)
2009-04-02 12:04 PDT, Jian Li
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-04-02 11:59:07 PDT
Upstream changes to WorkerContextExecutionProxy for V8 bindings in order to use V8EventListenerList as container.
Comment 1 Jian Li 2009-04-02 12:04:59 PDT
Created attachment 29203 [details]
Proposed Patch

Please land this patch after 25004 is landed and brought downstream.
Comment 2 Darin Fisher (:fishd, Google) 2009-04-02 13:15:53 PDT
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?
Comment 3 Jian Li 2009-04-02 13:32:13 PDT
(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?

Comment 4 Jian Li 2009-04-02 14:01:10 PDT
(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.
Comment 5 Dmitry Titov 2009-04-03 15:25:56 PDT
Landed: http://trac.webkit.org/changeset/42215