Oilpan: Move ThreadableWebSocketChannelClientWrapper to Oilpan's heap

Oilpan: Move ThreadableWebSocketChannelClientWrapper to Oilpan's heap

This is a preparation for moving WebSocketChannelClient to the heap.

This CL adds a template specialization to CrossThreadCopier.h so that ThreadableWebSocketChannelClientWrapper* can be passed to createCallbackTask().

BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174956

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175782

[haraken, 2014-05-07 07:45:16]

PTAL with some questions.

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Th...
File Source/modules/websockets/ThreadableWebSocketChannelClientWrapper.h
(right):

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Th...
Source/modules/websockets/ThreadableWebSocketChannelClientWrapper.h:86:
WebSocketChannelClient* m_client;

This raw pointer is safe because it's explicitly cleared in clearClient(). I'll
move the client to the heap in the next CL.

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:391:
m_loaderProxy.postTaskToWorkerGlobalScope(createCallbackTask(&workerGlobalScopeDidConnect,
m_workerClientWrapper.get(), m_mainWebSocketChannel->subprotocol(),
m_mainWebSocketChannel->extensions()));

If we don't want to add .get() here, we need to define the following template
specialization to CrossThreadCopier.h:

  template<> struct CrossThreadCopierBase<false, false,
Persistent<ThreadableWebSocketChannelClientWrapper> >

I don't think this is safe because Persistent is not thread-safe.

So, instead I added .get() and defined the following template specialization to
CrossThreadCopier.h:

  template<> struct CrossThreadCopierBase<false, false,
ThreadableWebSocketChannelClientWrapper*>

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.h (right):

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:125: const
RefPtrWillBePersistent<ThreadableWebSocketChannelClientWrapper>
m_workerClientWrapper;

This doesn't need to be RefPtrWillBeCrossThreadPersistent, because the Peer is
created and used in the main thread.

yoshino-san: Is my understanding correct?

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:168: const
RefPtrWillBePersistent<ThreadableWebSocketChannelClientWrapper>
m_workerClientWrapper;

This doesn't need to be RefPtrWillBeCrossThreadPersistent, because the Bridge is
created and used in the worker thread.

yoshino-san: Is my understanding correct?

https://codereview.chromium.org/267323004/diff/1/Source/platform/CrossThreadC...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/267323004/diff/1/Source/platform/CrossThreadC...
Source/platform/CrossThreadCopier.h:109: template<> struct
CrossThreadCopierBase<false, false, ThreadableWebSocketChannelClientWrapper*> {

I must admit that I'm not understanding what I'm doing.

I added this template specialization to make the CL compilable and pass all
tests, but I'm not quite sure if this is right.

template experts: I'd be happy if you could give me any advice.

[tkent, 2014-05-07 08:10:41]

https://codereview.chromium.org/267323004/diff/1/Source/platform/CrossThreadC...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/267323004/diff/1/Source/platform/CrossThreadC...
Source/platform/CrossThreadCopier.h:109: template<> struct
CrossThreadCopierBase<false, false, ThreadableWebSocketChannelClientWrapper*> {
On 2014/05/07 07:45:16, haraken wrote:
> 
> I must admit that I'm not understanding what I'm doing.
> 
> I added this template specialization to make the CL compilable and pass all
> tests, but I'm not quite sure if this is right.
> 
> template experts: I'd be happy if you could give me any advice.

The generated code would be ok.  We don't need special handling on copying a
pointer to an on-heap object over threads.
However, referring to a class in a modules here is a kind of layering violation.
We should have a specialization for IsSubclassOfTemplate<T, GarbageCollected>.

Also, this change is not enough. We need a specialization for
CrossThreadTaskTraits so that m_parameterN in CrossTheradTaskM becomes
CrossThreadPersistent.

[tyoshino (SeeGerritForStatus), 2014-05-07 08:32:52]

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:391:
m_loaderProxy.postTaskToWorkerGlobalScope(createCallbackTask(&workerGlobalScopeDidConnect,
m_workerClientWrapper.get(), m_mainWebSocketChannel->subprotocol(),
m_mainWebSocketChannel->extensions()));
On 2014/05/07 07:45:16, haraken wrote:
> 
> If we don't want to add .get() here, we need to define the following template
> specialization to CrossThreadCopier.h:
> 
>   template<> struct CrossThreadCopierBase<false, false,
> Persistent<ThreadableWebSocketChannelClientWrapper> >
> 
> I don't think this is safe because Persistent is not thread-safe.
> 
> So, instead I added .get() and defined the following template specialization
to
> CrossThreadCopier.h:
> 
>   template<> struct CrossThreadCopierBase<false, false,
> ThreadableWebSocketChannelClientWrapper*>

Is the reason why this works that the task would contain a RawPtr and it keeps
the wrapper alive?

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.h (right):

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:125: const
RefPtrWillBePersistent<ThreadableWebSocketChannelClientWrapper>
m_workerClientWrapper;
On 2014/05/07 07:45:16, haraken wrote:
> 
> This doesn't need to be RefPtrWillBeCrossThreadPersistent, because the Peer is
> created and used in the main thread.

Right. The object pointed by m_workerClientWrapper is used only in the worker
thread but m_workerClientWrapper is created/destroyed only in the main thread.

> 
> yoshino-san: Is my understanding correct?

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.h:168: const
RefPtrWillBePersistent<ThreadableWebSocketChannelClientWrapper>
m_workerClientWrapper;
On 2014/05/07 07:45:16, haraken wrote:
> 
> This doesn't need to be RefPtrWillBeCrossThreadPersistent, because the Bridge
is
> created and used in the worker thread.

Right. This m_workerClientWrapper is created/used/destroyed only in the worker
thread.

> 
> yoshino-san: Is my understanding correct?

[zerny-chromium, 2014-05-07 09:50:00]

Except for the CrossThreadCopier specialization issue, lgtm.

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:391:
m_loaderProxy.postTaskToWorkerGlobalScope(createCallbackTask(&workerGlobalScopeDidConnect,
m_workerClientWrapper.get(), m_mainWebSocketChannel->subprotocol(),
m_mainWebSocketChannel->extensions()));
On 2014/05/07 08:32:52, tyoshino wrote:
> On 2014/05/07 07:45:16, haraken wrote:
> > 
> > If we don't want to add .get(), we need to define the following template
> > specialization to CrossThreadCopier.h:
> > 
> >   template<> struct CrossThreadCopierBase<false, false,
> > Persistent<ThreadableWebSocketChannelClientWrapper> >
> > 
> > I don't think this is safe because Persistent is not thread-safe.
> > 
> > So, instead I added .get() and defined the following template specialization
> to
> > CrossThreadCopier.h:
> > 
> >   template<> struct CrossThreadCopierBase<false, false,
> > ThreadableWebSocketChannelClientWrapper*>
> 
> Is the reason why this works that the task would contain a RawPtr and it keeps
> the wrapper alive?

Do you need the .get() calls (here and below) after adding the template
specialization? There is an implicit conversion from Persistent<T> to T* which I
think should be sufficient here.

[haraken, 2014-05-07 11:18:33]

Thanks for review! PTAL.

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:391:
m_loaderProxy.postTaskToWorkerGlobalScope(createCallbackTask(&workerGlobalScopeDidConnect,
m_workerClientWrapper.get(), m_mainWebSocketChannel->subprotocol(),
m_mainWebSocketChannel->extensions()));
On 2014/05/07 09:50:00, zerny-chromium wrote:
> On 2014/05/07 08:32:52, tyoshino wrote:
> > On 2014/05/07 07:45:16, haraken wrote:
> > > 
> > > If we don't want to add .get(), we need to define the following template
> > > specialization to CrossThreadCopier.h:
> > > 
> > >   template<> struct CrossThreadCopierBase<false, false,
> > > Persistent<ThreadableWebSocketChannelClientWrapper> >
> > > 
> > > I don't think this is safe because Persistent is not thread-safe.
> > > 
> > > So, instead I added .get() and defined the following template
specialization
> > to
> > > CrossThreadCopier.h:
> > > 
> > >   template<> struct CrossThreadCopierBase<false, false,
> > > ThreadableWebSocketChannelClientWrapper*>
> > 
> > Is the reason why this works that the task would contain a RawPtr and it
keeps
> > the wrapper alive?
> 
> Do you need the .get() calls (here and below) after adding the template
> specialization? There is an implicit conversion from Persistent<T> to T* which
I
> think should be sufficient here.

I couldn't make it workable (See my comment in CrossThreadCopier.h).

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:391:
m_loaderProxy.postTaskToWorkerGlobalScope(createCallbackTask(&workerGlobalScopeDidConnect,
m_workerClientWrapper.get(), m_mainWebSocketChannel->subprotocol(),
m_mainWebSocketChannel->extensions()));
On 2014/05/07 08:32:52, tyoshino wrote:
> On 2014/05/07 07:45:16, haraken wrote:
> > 
> > If we don't want to add .get() here, we need to define the following
template
> > specialization to CrossThreadCopier.h:
> > 
> >   template<> struct CrossThreadCopierBase<false, false,
> > Persistent<ThreadableWebSocketChannelClientWrapper> >
> > 
> > I don't think this is safe because Persistent is not thread-safe.
> > 
> > So, instead I added .get() and defined the following template specialization
> to
> > CrossThreadCopier.h:
> > 
> >   template<> struct CrossThreadCopierBase<false, false,
> > ThreadableWebSocketChannelClientWrapper*>
> 
> Is the reason why this works that the task would contain a RawPtr and it keeps
> the wrapper alive?

This works because the Peer object in the main thread is keeping the Persistent
handle to ThreadableWebSocketChannelClientWrapper while the worker is using the
ThreadableWebSocketChannelClientWrapper (using a raw pointer). In my
understanding, the Peer object is alive while the worker thread is using the raw
pointer.

Does it make sense?

https://codereview.chromium.org/267323004/diff/1/Source/platform/CrossThreadC...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/267323004/diff/1/Source/platform/CrossThreadC...
Source/platform/CrossThreadCopier.h:109: template<> struct
CrossThreadCopierBase<false, false, ThreadableWebSocketChannelClientWrapper*> {
On 2014/05/07 08:10:41, tkent wrote:
> On 2014/05/07 07:45:16, haraken wrote:
> > 
> > I must admit that I'm not understanding what I'm doing.
> > 
> > I added this template specialization to make the CL compilable and pass all
> > tests, but I'm not quite sure if this is right.
> > 
> > template experts: I'd be happy if you could give me any advice.
> 
> The generated code would be ok.  We don't need special handling on copying a
> pointer to an on-heap object over threads.
> However, referring to a class in a modules here is a kind of layering
violation.
> We should have a specialization for IsSubclassOfTemplate<T, GarbageCollected>.
> 
> Also, this change is not enough. We need a specialization for
> CrossThreadTaskTraits so that m_parameterN in CrossTheradTaskM becomes
> CrossThreadPersistent.

Done. Please take a look, I'm not familiar with templates :)

https://codereview.chromium.org/267323004/diff/20001/Source/platform/CrossThr...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/267323004/diff/20001/Source/platform/CrossThr...
Source/platform/CrossThreadCopier.h:145: WTF::IsSubclassOfTemplate<typename
WTF::RemovePointer<T>::Type, GarbageCollected>::value,

zerny@: In order to pass a Persistent to createCallbackTask, I need to add
something like:

WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type,
GarbageCollected>::value || WTF::IsSubclassOfTemplate<typename
WTF::RemoveTemplate<T, Persistent>::Type, GarbageCollected>::value

but this didn't compile :(

[zerny-chromium, 2014-05-07 11:56:38]

https://codereview.chromium.org/267323004/diff/20001/Source/platform/CrossThr...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/267323004/diff/20001/Source/platform/CrossThr...
Source/platform/CrossThreadCopier.h:145: WTF::IsSubclassOfTemplate<typename
WTF::RemovePointer<T>::Type, GarbageCollected>::value,
On 2014/05/07 11:18:33, haraken wrote:
> 
> zerny@: In order to pass a Persistent to createCallbackTask, I need to add
> something like:
> 
> WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type,
> GarbageCollected>::value || WTF::IsSubclassOfTemplate<typename
> WTF::RemoveTemplate<T, Persistent>::Type, GarbageCollected>::value
> 
> but this didn't compile :(

Something like that should work. Do you need to WebCore:: qualify Persistent?

[haraken, 2014-05-08 03:18:40]

On 2014/05/07 11:56:38, zerny-chromium wrote:
>
https://codereview.chromium.org/267323004/diff/20001/Source/platform/CrossThr...
> File Source/platform/CrossThreadCopier.h (right):
> 
>
https://codereview.chromium.org/267323004/diff/20001/Source/platform/CrossThr...
> Source/platform/CrossThreadCopier.h:145: WTF::IsSubclassOfTemplate<typename
> WTF::RemovePointer<T>::Type, GarbageCollected>::value,
> On 2014/05/07 11:18:33, haraken wrote:
> > 
> > zerny@: In order to pass a Persistent to createCallbackTask, I need to add
> > something like:
> > 
> > WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type,
> > GarbageCollected>::value || WTF::IsSubclassOfTemplate<typename
> > WTF::RemoveTemplate<T, Persistent>::Type, GarbageCollected>::value
> > 
> > but this didn't compile :(
> 
> Something like that should work. Do you need to WebCore:: qualify Persistent?

Unfortunately it doesn't work:

In file included from
../../third_party/WebKit/Source/platform/CrossThreadCopier.cpp:33:
../../third_party/WebKit/Source/platform/CrossThreadCopier.h:146:83: error:
template template argument has different template parameters than its
corresponding template template parameter
            || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T,
WebCore::Persistent>::Type, GarbageCollected>::value,
                                                                                
 ^
../../third_party/WebKit/Source/platform/heap/Handle.h:281:1: note: too many
template parameters in template template argument
template<typename T, typename RootsAccessor /* =
ThreadLocalPersistents<ThreadingTrait<T>::Affinity > */ >
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/wtf/TypeTraits.h:219:27: note: previous template
template parameter is here
    template <typename T, template <class V> class OuterTemplate> struct
RemoveTemplate {
                          ^~~~~~~~~~~~~~~~~~
In file included from
../../third_party/WebKit/Source/platform/CrossThreadCopier.cpp:33:
../../third_party/WebKit/Source/platform/CrossThreadCopier.h:146:96: error:
expected a qualified name after 'typename'
            || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T,
WebCore::Persistent>::Type, GarbageCollected>::value,
                                                                                
              ^
../../third_party/WebKit/Source/platform/CrossThreadCopier.h:146:96: error:
unknown type name 'Type'
../../third_party/WebKit/Source/platform/CrossThreadCopier.h:147:12: error:
expected class name
        T> {
           ^
../../third_party/WebKit/Source/platform/CrossThreadCopier.cpp:76:83: error: no
member named 'Type' in
'WebCore::CrossThreadCopier<WTF::PassRefPtr<WebCore::CopierThreadSafeRefCountedTest>
>'
                  CrossThreadCopier<PassRefPtr<CopierThreadSafeRefCountedTest>
>::Type


BTW, yoshino-san has a clean-up CL around this area, so I'll wait for the
clean-up before landing this CL.

[tyoshino (SeeGerritForStatus), 2014-05-08 06:29:55]

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/1/Source/modules/websockets/Wo...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:391:
m_loaderProxy.postTaskToWorkerGlobalScope(createCallbackTask(&workerGlobalScopeDidConnect,
m_workerClientWrapper.get(), m_mainWebSocketChannel->subprotocol(),
m_mainWebSocketChannel->extensions()));
On 2014/05/07 11:18:33, haraken wrote:
> On 2014/05/07 08:32:52, tyoshino wrote:
> > On 2014/05/07 07:45:16, haraken wrote:
> > > 
> > > If we don't want to add .get() here, we need to define the following
> template
> > > specialization to CrossThreadCopier.h:
> > > 
> > >   template<> struct CrossThreadCopierBase<false, false,
> > > Persistent<ThreadableWebSocketChannelClientWrapper> >
> > > 
> > > I don't think this is safe because Persistent is not thread-safe.
> > > 
> > > So, instead I added .get() and defined the following template
specialization
> > to
> > > CrossThreadCopier.h:
> > > 
> > >   template<> struct CrossThreadCopierBase<false, false,
> > > ThreadableWebSocketChannelClientWrapper*>
> > 
> > Is the reason why this works that the task would contain a RawPtr and it
keeps
> > the wrapper alive?
> 
> This works because the Peer object in the main thread is keeping the
Persistent
> handle to ThreadableWebSocketChannelClientWrapper while the worker is using
the
> ThreadableWebSocketChannelClientWrapper (using a raw pointer). In my
> understanding, the Peer object is alive while the worker thread is using the
raw
> pointer.
> 
> Does it make sense?

FTR: As discussed offline, the PassRefPtr in the task object was required to
prolong life of m_workerClientWrapper. haraken is trying to resolve this.

[tyoshino (SeeGerritForStatus), 2014-05-08 07:18:29]

On 2014/05/08 03:18:40, haraken wrote:
> BTW, yoshino-san has a clean-up CL around this area, so I'll wait for the
> clean-up before landing this CL.

Done
https://codereview.chromium.org/265713004/

[haraken, 2014-05-08 09:09:02]

hmm, I'm working with tasak@ but we cannot yet come up with templates that makes
the CL compilable.

I'd be happy if template experts give us any advice.

What we need to do is:

- createCallbackTask() accepts ThreadableWebSocketWrapperClient*.
- createCallbackTask() accepts Persistent<ThreadableWebSocketWrapperClient>.
- CrossThreadTask holds the client pointer using CrossThreadPersistent.

https://codereview.chromium.org/267323004/diff/80001/Source/platform/CrossThr...
File Source/platform/CrossThreadCopier.h (right):

https://codereview.chromium.org/267323004/diff/80001/Source/platform/CrossThr...
Source/platform/CrossThreadCopier.h:133: template<typename T> struct
CrossThreadCopierBase<false, false, true, T> {

This needs to match:

CrossThreadCopierBase<false, false, true, ThreadableWebSocketWrapperClient*>

and

CrossThreadCopierBase<false, false, true,
Persistent<ThreadableWebSocketWrapperClient>>

Then the copy() method needs to return
CrossThreadPersistent<ThreadableWebSocketWrapperClient>.

https://codereview.chromium.org/267323004/diff/80001/Source/platform/CrossThr...
Source/platform/CrossThreadCopier.h:149: || WTF::IsSubclassOfTemplate<typename
WTF::RemoveTemplate2<T, Persistent>::Type, GarbageCollected>::value,

This RemoveTemplate2 doesn't work. How can I get T from Persistent<T>?

[zerny-chromium, 2014-05-08 13:35:03]

On 2014/05/08 09:09:02, haraken wrote:
> hmm, I'm working with tasak@ but we cannot yet come up with templates that
makes
> the CL compilable.
> 
> I'd be happy if template experts give us any advice.
> 
> What we need to do is:
> 
> - createCallbackTask() accepts ThreadableWebSocketWrapperClient*.
> - createCallbackTask() accepts Persistent<ThreadableWebSocketWrapperClient>.
> - CrossThreadTask holds the client pointer using CrossThreadPersistent.
> 
>
https://codereview.chromium.org/267323004/diff/80001/Source/platform/CrossThr...
> File Source/platform/CrossThreadCopier.h (right):
> 
>
https://codereview.chromium.org/267323004/diff/80001/Source/platform/CrossThr...
> Source/platform/CrossThreadCopier.h:133: template<typename T> struct
> CrossThreadCopierBase<false, false, true, T> {
> 
> This needs to match:
> 
> CrossThreadCopierBase<false, false, true, ThreadableWebSocketWrapperClient*>
> 
> and
> 
> CrossThreadCopierBase<false, false, true,
> Persistent<ThreadableWebSocketWrapperClient>>
> 
> Then the copy() method needs to return
> CrossThreadPersistent<ThreadableWebSocketWrapperClient>.
> 
>
https://codereview.chromium.org/267323004/diff/80001/Source/platform/CrossThr...
> Source/platform/CrossThreadCopier.h:149: || WTF::IsSubclassOfTemplate<typename
> WTF::RemoveTemplate2<T, Persistent>::Type, GarbageCollected>::value,
> 
> This RemoveTemplate2 doesn't work. How can I get T from Persistent<T>?

This is because RemoveTemplate2 requires both arguments of the template to be
the same in order to match. Ie, it would match Persistent<T, T>. In this case we
only want to match on Persistent and grab the first argument:

template <typename T, template <class V, class W> class OuterTemplate> struct
MyRemoveTemplate2 {
    typedef T Type1;
    typedef T Type2;
};

template <typename T, typename S, template <class V, class W> class
OuterTemplate> struct MyRemoveTemplate2<OuterTemplate<T, S>, OuterTemplate> {
    typedef T Type1;
    typedef S Type2;
};

Then you can use: MyRemoveTemplate2<T, Persistent>::Type1 to get at the T in
Persistent<T>. Alternatively and more concrete you can use:

template <typename T> struct RemovePersistent {
    typedef T Type;
};
template <typename T> struct RemovePersistent<Persistent<T> > {
    typedef T Type;
};


I'm not sure why we can not .get() the argument and avoid special casing on
Persistent. That way, you just need to test RemovePointer<T> against
GarbageCollected.

[haraken, 2014-05-08 14:36:53]

Thanks Ian!

> I'm not sure why we can not .get() the argument and avoid special casing on
> Persistent. That way, you just need to test RemovePointer<T> against
> GarbageCollected.

Good point, this should work. Thanks!

(I'm having another template error and investigating :)

[haraken, 2014-05-09 08:08:22]

zerny@: Sorry for bothering you repeatedly. Let me ask one more question about
the template.

https://codereview.chromium.org/267323004/diff/100001/Source/core/dom/CrossTh...
File Source/core/dom/CrossThreadTask.h (right):

https://codereview.chromium.org/267323004/diff/100001/Source/core/dom/CrossTh...
Source/core/dom/CrossThreadTask.h:85: (*m_method)(context, m_parameter1);

This line hits the following compile error:

../../third_party/WebKit/Source/core/dom/CrossThreadTask.h:85:30: error: no
viable conversion from
'WebCore::CrossThreadPersistent<WebCore::ThreadableWebSocketChannelClientWrapper
*>' to 'WTF::RawPtr<WebCore::ThreadableWebSocketChannelClientWrapper>'  
        (*m_method)(context, m_parameter1);

The issue is that we don't have an implicit conversion from
CrossThreadPersistent to RawPtr, but how can I add it? My change to Handle.h
seems not enough.

[haraken, 2014-05-12 16:25:41]

Fixed the template issue.

PTAL.

[tkent, 2014-05-13 06:35:15]

lgtm

[tyoshino (SeeGerritForStatus), 2014-05-13 09:44:10]

lgtm

[haraken, 2014-05-26 09:34:38]

Unfortunately this CL hits the empty ASSERT error at the end of the worker
thread execution of the following tests:

http/tests/websocket/workers/worker-reload-repeated.html
http/tests/websocket/workers/worker-reload.html

Looking.

[Mads Ager (chromium), 2014-05-26 10:43:54]

On 2014/05/26 09:34:38, haraken wrote:
> Unfortunately this CL hits the empty ASSERT error at the end of the worker
> thread execution of the following tests:
> 
> http/tests/websocket/workers/worker-reload-repeated.html
> http/tests/websocket/workers/worker-reload.html
> 
> Looking.

Thanks Haraken. We should carefully investigate these since having a pointer
into the heap of a worker that is shutting down is guaranteed to lead to a
dangling pointer. That said, the relaxation of the assert only landed today.
Therefore, be aware of whether or not you have conservative GCs when these
asserts trigger...

[haraken, 2014-05-27 04:53:09]

On 2014/05/26 10:43:54, Mads Ager (chromium) wrote:
> On 2014/05/26 09:34:38, haraken wrote:
> > Unfortunately this CL hits the empty ASSERT error at the end of the worker
> > thread execution of the following tests:
> > 
> > http/tests/websocket/workers/worker-reload-repeated.html
> > http/tests/websocket/workers/worker-reload.html
> > 
> > Looking.
> 
> Thanks Haraken. We should carefully investigate these since having a pointer
> into the heap of a worker that is shutting down is guaranteed to lead to a
> dangling pointer. That said, the relaxation of the assert only landed today.
> Therefore, be aware of whether or not you have conservative GCs when these
> asserts trigger...

I found that:

- Your first fix to relax the empty ASSERT didn't fix the issue.

- Your second fix to more relax the empty ASSERT fixed the issue. This is just
because when the worker thread calls collectAllGarbage(), the main thread is not
in the start of the event loop, and thus the last GC that runs in
collectAllGArbage() is _always_ a conservative one.

I guess that your second fix is just "hiding" a real leak this CL is going to
introduce. (If so, it implies that the empty ASSERT after your second fix is not
helpful... :-/) I'm investigating.

[Mads Ager (chromium), 2014-05-27 08:17:26]

On Tue, May 27, 2014 at 6:53 AM, <haraken@chromium.org> wrote:

> On 2014/05/26 10:43:54, Mads Ager (chromium) wrote:
>
>> On 2014/05/26 09:34:38, haraken wrote:
>> > Unfortunately this CL hits the empty ASSERT error at the end of the
>> worker
>> > thread execution of the following tests:
>> >
>> > http/tests/websocket/workers/worker-reload-repeated.html
>> > http/tests/websocket/workers/worker-reload.html
>> >
>> > Looking.
>>
>
>  Thanks Haraken. We should carefully investigate these since having a
>> pointer
>> into the heap of a worker that is shutting down is guaranteed to lead to a
>> dangling pointer. That said, the relaxation of the assert only landed
>> today.
>> Therefore, be aware of whether or not you have conservative GCs when these
>> asserts trigger...
>>
>
> I found that:
>
> - Your first fix to relax the empty ASSERT didn't fix the issue.
>
> - Your second fix to more relax the empty ASSERT fixed the issue. This is
> just
> because when the worker thread calls collectAllGarbage(), the main thread
> is not
> in the start of the event loop, and thus the last GC that runs in
> collectAllGArbage() is _always_ a conservative one.
>
> I guess that your second fix is just "hiding" a real leak this CL is going
> to
> introduce. (If so, it implies that the empty ASSERT after your second fix
> is not
> helpful... :-/) I'm investigating.


Yes, unfortunately it is very likely that the assert will be mainly
disabled at this point. We should see what we can do in debug mode to keep
better track of what has been marked only because of conservative stack
scanning so that we can at least assert that no objects in the worker heap
was reachable from a non-stack pointer...

-- M

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2014-05-27 09:21:36]

PTAL. The patch set 10 passes the empty ASSERT check.

https://codereview.chromium.org/267323004/diff/120001/Source/core/dom/CrossTh...
File Source/core/dom/CrossThreadTask.h (right):

https://codereview.chromium.org/267323004/diff/120001/Source/core/dom/CrossTh...
Source/core/dom/CrossThreadTask.h:61: typedef CrossThreadPersistent<T>
ParamType;

This was the cause of the empty ASSERT error.

This template is expanded as follows:

class ExecutionContextTask {
  CrossThreadPersistent<ThreadableWebSocketChannelClientWrapper> param;
};

// This object is created in a worker thread.
class ThreadableWebSocketChannelClientWrapper : public
GarbageCollectedFinalized<ThreadableWebSocketChannelClientWrapper>{
  Vector<OwnPtr<ExecutionContextTask>> m_pendingTasks;
}

The problem happens in a case where the worker thread shuts down before flushing
m_pendingTasks. In that case, the worker thread calls collectAllGarbage() but it
cannot collect the ThreadableWebSocketChannelClientWrapper, because the
ThreadableWebSocketChannelClientWrapper and the CrossThreadPersistent are
referenced in a cycle.

I tried to explicitly clear m_pendingTasks before shutting down the worker
thread, but I found it's tricky. Also we won't want to add similar hacks to
other caller sites of ExecutionContextTask.

A better approach would be to move ExecutionContextTask to the heap and use a
Member instead of the CrossThreadPersistent. However, this requires a lot of
work.

As a short-term fix, I changed the CrossThreadPersistent to a RawPtr. This is
not safe in general but is OK in this particular case, because the tasks are
guaranteed to be executed in the same worker thread and thus
ThreadableWebSocketChannelClientWrapper is guaranteed to be alive while the
tasks are in the pending queue of the ThreadableWebSocketChannelClientWrapper.

What do you think?

[Mads Ager (chromium), 2014-05-27 10:40:55]

On 2014/05/27 09:21:36, haraken wrote:
> PTAL. The patch set 10 passes the empty ASSERT check.
> 
>
https://codereview.chromium.org/267323004/diff/120001/Source/core/dom/CrossTh...
> File Source/core/dom/CrossThreadTask.h (right):
> 
>
https://codereview.chromium.org/267323004/diff/120001/Source/core/dom/CrossTh...
> Source/core/dom/CrossThreadTask.h:61: typedef CrossThreadPersistent<T>
> ParamType;
> 
> This was the cause of the empty ASSERT error.
> 
> This template is expanded as follows:
> 
> class ExecutionContextTask {
>   CrossThreadPersistent<ThreadableWebSocketChannelClientWrapper> param;
> };
> 
> // This object is created in a worker thread.
> class ThreadableWebSocketChannelClientWrapper : public
> GarbageCollectedFinalized<ThreadableWebSocketChannelClientWrapper>{
>   Vector<OwnPtr<ExecutionContextTask>> m_pendingTasks;
> }
> 
> The problem happens in a case where the worker thread shuts down before
flushing
> m_pendingTasks. In that case, the worker thread calls collectAllGarbage() but
it
> cannot collect the ThreadableWebSocketChannelClientWrapper, because the
> ThreadableWebSocketChannelClientWrapper and the CrossThreadPersistent are
> referenced in a cycle.
> 
> I tried to explicitly clear m_pendingTasks before shutting down the worker
> thread, but I found it's tricky. Also we won't want to add similar hacks to
> other caller sites of ExecutionContextTask.
> 
> A better approach would be to move ExecutionContextTask to the heap and use a
> Member instead of the CrossThreadPersistent. However, this requires a lot of
> work.
> 
> As a short-term fix, I changed the CrossThreadPersistent to a RawPtr. This is
> not safe in general but is OK in this particular case, because the tasks are
> guaranteed to be executed in the same worker thread and thus
> ThreadableWebSocketChannelClientWrapper is guaranteed to be alive while the
> tasks are in the pending queue of the ThreadableWebSocketChannelClientWrapper.
> 
> What do you think?

Good detective work, yes, that's a cycle that you have to break. If you are sure
that the raw pointer is safe for now I would be fine with this as a temporary
fix while we work on getting ExecutionContextTask on the heap so we can use a
Member and trace it.

[haraken, 2014-05-27 11:27:19]

On 2014/05/27 10:40:55, Mads Ager (chromium) wrote:
> On 2014/05/27 09:21:36, haraken wrote:
> > PTAL. The patch set 10 passes the empty ASSERT check.
> > 
> >
>
https://codereview.chromium.org/267323004/diff/120001/Source/core/dom/CrossTh...
> > File Source/core/dom/CrossThreadTask.h (right):
> > 
> >
>
https://codereview.chromium.org/267323004/diff/120001/Source/core/dom/CrossTh...
> > Source/core/dom/CrossThreadTask.h:61: typedef CrossThreadPersistent<T>
> > ParamType;
> > 
> > This was the cause of the empty ASSERT error.
> > 
> > This template is expanded as follows:
> > 
> > class ExecutionContextTask {
> >   CrossThreadPersistent<ThreadableWebSocketChannelClientWrapper> param;
> > };
> > 
> > // This object is created in a worker thread.
> > class ThreadableWebSocketChannelClientWrapper : public
> > GarbageCollectedFinalized<ThreadableWebSocketChannelClientWrapper>{
> >   Vector<OwnPtr<ExecutionContextTask>> m_pendingTasks;
> > }
> > 
> > The problem happens in a case where the worker thread shuts down before
> flushing
> > m_pendingTasks. In that case, the worker thread calls collectAllGarbage()
but
> it
> > cannot collect the ThreadableWebSocketChannelClientWrapper, because the
> > ThreadableWebSocketChannelClientWrapper and the CrossThreadPersistent are
> > referenced in a cycle.
> > 
> > I tried to explicitly clear m_pendingTasks before shutting down the worker
> > thread, but I found it's tricky. Also we won't want to add similar hacks to
> > other caller sites of ExecutionContextTask.
> > 
> > A better approach would be to move ExecutionContextTask to the heap and use
a
> > Member instead of the CrossThreadPersistent. However, this requires a lot of
> > work.
> > 
> > As a short-term fix, I changed the CrossThreadPersistent to a RawPtr. This
is
> > not safe in general but is OK in this particular case, because the tasks are
> > guaranteed to be executed in the same worker thread and thus
> > ThreadableWebSocketChannelClientWrapper is guaranteed to be alive while the
> > tasks are in the pending queue of the
ThreadableWebSocketChannelClientWrapper.
> > 
> > What do you think?
> 
> Good detective work, yes, that's a cycle that you have to break. If you are
sure
> that the raw pointer is safe for now I would be fine with this as a temporary
> fix while we work on getting ExecutionContextTask on the heap so we can use a
> Member and trace it.

I'm sure that:

- the task is created and consumed by the same worker thread
- the ThreadableWebSocketChannelClient owns the task, so it's OK to make the
task hold a RawPtr back to the ThreadableWebSocketChannelClient

I added a FIXME to move ExecutionContextTask to the heap. (I'll try to work on
it in a follow-up but might take over the work to others if it requires a lot of
template knowledge.)

[tkent, 2014-05-27 12:21:49]

temporal raw pointers lgtm.

[haraken, 2014-05-27 12:26:08]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-27 12:26:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/267323004/180001

[commit-bot: I haz the power, 2014-05-27 14:20:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9065)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9251)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/9359)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12451)

[commit-bot: I haz the power, 2014-05-27 14:49:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-27 14:49:40]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/9384)

[haraken, 2014-05-27 15:01:44]

https://codereview.chromium.org/267323004/diff/180001/Source/core/dom/CrossTh...
File Source/core/dom/CrossThreadTask.h (right):

https://codereview.chromium.org/267323004/diff/180001/Source/core/dom/CrossTh...
Source/core/dom/CrossThreadTask.h:63: typedef RawPtr<T> ParamType;

In order to make the plugin happy, I need to add IGNORE_GC_PLUGIN macros to the
RawPtr. However, we don't want to add IGNORE_GC_PLUGIN to the following 36
parameters like this.

  IGNORE_GC_PLUGIN("...")
  PX m_parameterX;

Is there any way to avoid that?

(Also I noticed that it wouldn't be easy to move ExecutionContextTask to the
heap. If we move ExecutionContextTask to the heap, we need to implement a
trace() method that traces Members. However, there is no easy way to know what
parameters are Members. It might be possible to create an IsMember template, but
we don't want to write it for the 36 parameters...)

[zerny-chromium, 2014-05-28 06:03:19]

https://codereview.chromium.org/267323004/diff/180001/Source/core/dom/CrossTh...
File Source/core/dom/CrossThreadTask.h (right):

https://codereview.chromium.org/267323004/diff/180001/Source/core/dom/CrossTh...
Source/core/dom/CrossThreadTask.h:63: typedef RawPtr<T> ParamType;
On 2014/05/27 15:01:44, haraken wrote:
> 
> In order to make the plugin happy, I need to add IGNORE_GC_PLUGIN macros to
the
> RawPtr. However, we don't want to add IGNORE_GC_PLUGIN to the following 36
> parameters like this.
> 
>   IGNORE_GC_PLUGIN("...")
>   PX m_parameterX;
> 
> Is there any way to avoid that?

You can do an ignore on the classes:

  class GC_PLUGIN_IGNORE("bugnr") CrossThreadTask1;
  ...
  class GC_PLUGIN_IGNORE("bugnr") CrossThreadTask8;


> (Also I noticed that it wouldn't be easy to move ExecutionContextTask to the
> heap. If we move ExecutionContextTask to the heap, we need to implement a
> trace() method that traces Members. However, there is no easy way to know what
> parameters are Members. It might be possible to create an IsMember template,
but
> we don't want to write it for the 36 parameters...)

We could add a trace to the CrossThreadTaskTraits specialization so the member
version expands to a trace and the non-member to a noop.

template<typename T> struct CrossThreadTaskTraits<Member<T> > {
  typedef Member<T> ParamType;
  void trace(Visitor* v, ParamType p) { v->trace(p); }
};

and then unconditionally call the traits trace on all params.

[haraken, 2014-05-28 07:33:57]

On 2014/05/28 06:03:19, zerny-chromium wrote:
>
https://codereview.chromium.org/267323004/diff/180001/Source/core/dom/CrossTh...
> File Source/core/dom/CrossThreadTask.h (right):
> 
>
https://codereview.chromium.org/267323004/diff/180001/Source/core/dom/CrossTh...
> Source/core/dom/CrossThreadTask.h:63: typedef RawPtr<T> ParamType;
> On 2014/05/27 15:01:44, haraken wrote:
> > 
> > In order to make the plugin happy, I need to add IGNORE_GC_PLUGIN macros to
> the
> > RawPtr. However, we don't want to add IGNORE_GC_PLUGIN to the following 36
> > parameters like this.
> > 
> >   IGNORE_GC_PLUGIN("...")
> >   PX m_parameterX;
> > 
> > Is there any way to avoid that?
> 
> You can do an ignore on the classes:
> 
>   class GC_PLUGIN_IGNORE("bugnr") CrossThreadTask1;
>   ...
>   class GC_PLUGIN_IGNORE("bugnr") CrossThreadTask8;

It worked, thanks Ian!

[haraken, 2014-05-28 07:34:36]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-28 07:34:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/267323004/200001

[commit-bot: I haz the power, 2014-05-28 09:02:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9211)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9423)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/9838)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12926)

[commit-bot: I haz the power, 2014-05-28 09:27:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-28 09:27:40]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/9871)

[haraken, 2014-05-28 09:31:41]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-28 09:31:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/267323004/220001

[commit-bot: I haz the power, 2014-05-28 10:51:15]

Change committed as 174956

[haraken, 2014-06-02 01:39:43]

Reverted this CL because it broke worker-reload-repeated.html.

In short, I need to move the ExecutionContextTask to the heap before landing
this CL.

https://codereview.chromium.org/267323004/diff/220001/Source/modules/websocke...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/220001/Source/modules/websocke...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:488: if
(!waitForMethodCompletion(createCallbackTask(&Peer::initialize,
reference.release(), AllowCrossThreadAccess(&m_loaderProxy),
m_workerClientWrapper.get(), sourceURL, lineNumber, syncHelper.release()))) {

This line is the culprit of the crash. A problem happens in a case where the
worker thread requests Peer::initialize on the main thread and the worker thread
receives a shutdownEvent.

In that case, currently the worker thread returns immediately without waiting
for the completion of Peer::initialize on the main thread. Then the worker
thread immediately requests Peer::destroy on the main thread. As a result, the
main thread will execute Peer::initialize and Peer::destroy in a row (probably)
after the worker thread is shut down.

This is problematic because things can happen in the following order:

(1) On the worker thread, Bridge::waitForMethodCompletion receives a
shutdownEvent and returns immediately.
(2) On the worker thread, Bridge is destructed. At this point, Bridge's
destructor removes the last reference to
ThreadableWebSocketChannelClientWrapper.
(3) The worker thread runs a final GC before shutting down.
(4) On the main thread, Peer::initialize runs.
(5) Peer::initialize tries to access the ThreadableWebSocketChannelClientWrapper
passed from the worker thread, but
it crashes because the ThreadableWebSocketChannelClientWrapper is already
destructed in the final GC.

The core problem is that the ExecutionContextTask (which is a task created in
the worker thread and then consumed in the main thread) doesn't keep the
ThreadableWebSocketChannelClientWrapper alive. Currently the
ExecutionContextTask uses a RawPtr to the
ThreadableWebSocketChannelClientWrapper. This should be a Member. In order to
make it a Member, we need to move the ExecutionContextTask to the heap.

In other words, I was assuming that ExecutionContextTasks used in the WebSocket
code are always passed from a worker thread to the same worker thread (and thus
it's OK to use a RawPtr<ThreadableWebSocketChannelClientWrapper> in
ExecutionContextTask), but it was not true for the Peer::initialize task.

In short, I need to move the ExecutionContextTask to the heap before landing
this CL.

[haraken, 2014-06-06 00:52:50]

I found that we need a mechanism to allocate a GCed object that can outlive the
thread that allocated the object. Specifically, ExecutionContextTask needs to
outlive the thread that allocates the task.

I'll hold on this CL until the issue is resolved.

[haraken, 2014-06-09 02:11:20]

tyoshino@, yhirano@: I'm sorry for doing back & forth changes to WebSocket. I
reworked on this CL. PTAL.

The only diff from the previous CL you l-g-t-med is in
WorkerThreadableWebSocketChannel.cpp. In the latest CL, the worker thread waits
for the main thread to complete Peer::initialize when a shutdownEvent is sent to
the worker.

More background:

Oilpan does not support an object that outlives a thread that created the object
(because it will add a lot of complexity to the infrastructure). Thus we need to
make sure that on-heap objects don't outlive the creation thread.

Given the restriction, a problem happens in a case where the worker thread
requests Peer::initialize on the main thread and the worker thread receives a
shutdownEvent.

In that case, currently the worker thread returns immediately without waiting
for the completion of Peer::initialize on the main thread. Then the worker
thread immediately requests Peer::destroy on the main thread. As a result, the
main thread will execute Peer::initialize and Peer::destroy in a row (probably)
after the worker thread is shut down.

This is problematic because things can happen in the following order:

(1) On the worker thread, Bridge::waitForMethodCompletion receives a
shutdownEvent and returns immediately.
(2) On the worker thread, Bridge is destructed. At this point, Bridge's
destructor removes the last reference to ThreadableWebSocketChannelSyncHelper.
(3) The worker thread runs a final GC before shutting down.
(4) On the main thread, Peer::initialize runs.
(5) Peer::initialize tries to access the ThreadableWebSocketChannelSyncHelper
passed from the worker thread, but it crashes because the
ThreadableWebSocketChannelSyncHelper is already destructed in the final GC. This
is the object-outlives-creation-thread problem.

I tried a couple of approaches to avoid the problem, and it seems that the only
workable solution I can come up with is to make the worker thread wait for the
main thread to complete Peer::initialize. I guess this won't become a
performance issue because this will just affect a rare case where a
shutdownEvent is sent when the worker thread tries to request Peer::initialize.
Also the wait just happens on the worker thread, not the main thread. This will
just delay destructing the worker thread.

What do you think?

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:626: return
false;

yhirano@: This should be false, because this branch is hit when a shutdownEvent
is sent to the worker thread.

[yhirano, 2014-06-09 02:47:38]

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:626: return
false;
On 2014/06/09 02:11:20, haraken wrote:
> 
> yhirano@: This should be false, because this branch is hit when a
shutdownEvent
> is sent to the worker thread.

What you are doing here is waiting for the syncHelper event even if the shutdown
event is fired, right?
If so, can you add a comment?

[haraken, 2014-06-09 03:28:51]

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:626: return
false;
On 2014/06/09 02:47:38, yhirano wrote:
> On 2014/06/09 02:11:20, haraken wrote:
> > 
> > yhirano@: This should be false, because this branch is hit when a
> shutdownEvent
> > is sent to the worker thread.
> 
> What you are doing here is waiting for the syncHelper event even if the
shutdown
> event is fired, right?
> If so, can you add a comment?

Done.

[tyoshino (SeeGerritForStatus), 2014-06-09 04:33:39]

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:626: return
false;
On 2014/06/09 03:28:51, haraken wrote:
> On 2014/06/09 02:47:38, yhirano wrote:
> > On 2014/06/09 02:11:20, haraken wrote:
> > > 
> > > yhirano@: This should be false, because this branch is hit when a
> > shutdownEvent
> > > is sent to the worker thread.
> > 
> > What you are doing here is waiting for the syncHelper event even if the
> shutdown
> > event is fired, right?
> > If so, can you add a comment?
> 
> Done.

Can we ASSERT here that the event signaled is not a shutdownEvent?

[yhirano, 2014-06-09 04:39:11]

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
File Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp (right):

https://codereview.chromium.org/267323004/diff/240001/Source/modules/websocke...
Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp:626: return
false;
On 2014/06/09 03:28:51, haraken wrote:
> On 2014/06/09 02:47:38, yhirano wrote:
> > On 2014/06/09 02:11:20, haraken wrote:
> > > 
> > > yhirano@: This should be false, because this branch is hit when a
> > shutdownEvent
> > > is sent to the worker thread.
> > 
> > What you are doing here is waiting for the syncHelper event even if the
> shutdown
> > event is fired, right?
> > If so, can you add a comment?
> 
> Done.

Thanks. I have a question. If we don't ignore the SyncHelper event even when the
worker thread is terminated, can we not listen to the shutdown event and check
whether the thread is terminated after the SyncHelper event arrives with
m_workerGlobalScope->thread()->runLoop().terminated() ?

[haraken, 2014-06-09 05:32:40]

Thanks for reviews!

yoshino-san:
> Can we ASSERT here that the event signaled is not a shutdownEvent?

Done.

hirano-san:
> > > What you are doing here is waiting for the syncHelper event even if the
> > shutdown
> > > event is fired, right?
> > > If so, can you add a comment?
> > 
> > Done.
> 
> Thanks. I have a question. If we don't ignore the SyncHelper event even when
the
> worker thread is terminated, can we not listen to the shutdown event and check
> whether the thread is terminated after the SyncHelper event arrives with
> m_workerGlobalScope->thread()->runLoop().terminated() ?

The CL is not listening to the shutdownEvent when we're listening to the
SyncHelper event.

I added ASSERT(m_workerGlobalScope->thread()->runLoop().terminated()).

[haraken, 2014-06-09 05:41:26]

On 2014/06/09 05:32:40, haraken wrote:
> Thanks for reviews!
> 
> yoshino-san:
> > Can we ASSERT here that the event signaled is not a shutdownEvent?
> 
> Done.
> 
> hirano-san:
> > > > What you are doing here is waiting for the syncHelper event even if the
> > > shutdown
> > > > event is fired, right?
> > > > If so, can you add a comment?
> > > 
> > > Done.
> > 
> > Thanks. I have a question. If we don't ignore the SyncHelper event even when
> the
> > worker thread is terminated, can we not listen to the shutdown event and
check
> > whether the thread is terminated after the SyncHelper event arrives with
> > m_workerGlobalScope->thread()->runLoop().terminated() ?
> 
> The CL is not listening to the shutdownEvent when we're listening to the
> SyncHelper event.
> 
> I added ASSERT(m_workerGlobalScope->thread()->runLoop().terminated()).

Discussed offline. Refactored the CL as yhirano-san suggested.

PTAL.

[yhirano, 2014-06-09 05:44:10]

lgtm

[tyoshino (SeeGerritForStatus), 2014-06-09 07:51:52]

wow, what a simple solution :)

lgtm

[haraken, 2014-06-09 07:53:39]

Thanks for reviews! Landing.

[haraken, 2014-06-09 07:53:43]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-09 07:54:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/267323004/320001

[commit-bot: I haz the power, 2014-06-09 08:58:27]

Change committed as 175782