Oilpan: Prpare to move WorkerEventQueue to Oilpan heap.

Oilpan: Prpare to move WorkerEventQueue to Oilpan heap.

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

[tkent, 2014-05-30 09:24:48]

Please take a look at this.

[sof, 2014-05-30 10:23:29]

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
File Source/core/workers/WorkerEventQueue.h (right):

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
Source/core/workers/WorkerEventQueue.h:42: class WorkerEventQueue FINAL : public
NoBaseWillBeGarbageCollectedFinalized<WorkerEventQueue>, public EventQueue {
Does WorkerEventQueue need finalization?

[haraken, 2014-05-30 12:10:40]

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
File Source/core/workers/WorkerEventQueue.cpp (right):

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
Source/core/workers/WorkerEventQueue.cpp:50: close();

I'm not sure if you can remove close() from the destructor. If you don't call
it, task->cancel() is not called. Then the task is not unregistered from the
WorkerEventQueue, and thus it's possible that
m_eventQueue->removeEvent(m_event.get()) at line 81 is executed after the
WorkerEventQueue is destructed.

An ideal solution would be to move ExecutionContextTask (from which
EventDispatcherTask is derived) to the heap and use Members for
EventDispatcherTask::m_event and EventDispatcherTask::m_eventQueue. However, it
looks a bit hard to move ExecutionContextTask to the heap (probably only for me
who doesn't have much template knowledge).....

[tkent, 2014-05-30 13:47:57]

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
File Source/core/workers/WorkerEventQueue.cpp (right):

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
Source/core/workers/WorkerEventQueue.cpp:50: close();
On 2014/05/30 12:10:41, haraken wrote:
> 
> I'm not sure if you can remove close() from the destructor. If you don't call
> it, task->cancel() is not called. Then the task is not unregistered from the
> WorkerEventQueue, and thus it's possible that
> m_eventQueue->removeEvent(m_event.get()) at line 81 is executed after the
> WorkerEventQueue is destructed.
> 
> An ideal solution would be to move ExecutionContextTask (from which
> EventDispatcherTask is derived) to the heap and use Members for
> EventDispatcherTask::m_event and EventDispatcherTask::m_eventQueue. However,
it
> looks a bit hard to move ExecutionContextTask to the heap (probably only for
me
> who doesn't have much template knowledge).....

In this patch, I moved this |close()| callsite to WorkerGlobalScope::dispose
because we should not touch on-heap object in the destructor.

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
File Source/core/workers/WorkerEventQueue.h (right):

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
Source/core/workers/WorkerEventQueue.h:42: class WorkerEventQueue FINAL : public
NoBaseWillBeGarbageCollectedFinalized<WorkerEventQueue>, public EventQueue {
On 2014/05/30 10:23:29, sof wrote:
> Does WorkerEventQueue need finalization?

I don't think so.  I'll update the patch.

[haraken, 2014-05-30 14:00:56]

LGTM.

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
File Source/core/workers/WorkerEventQueue.cpp (right):

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
Source/core/workers/WorkerEventQueue.cpp:50: close();
On 2014/05/30 13:47:58, tkent wrote:
> On 2014/05/30 12:10:41, haraken wrote:
> > 
> > I'm not sure if you can remove close() from the destructor. If you don't
call
> > it, task->cancel() is not called. Then the task is not unregistered from the
> > WorkerEventQueue, and thus it's possible that
> > m_eventQueue->removeEvent(m_event.get()) at line 81 is executed after the
> > WorkerEventQueue is destructed.
> > 
> > An ideal solution would be to move ExecutionContextTask (from which
> > EventDispatcherTask is derived) to the heap and use Members for
> > EventDispatcherTask::m_event and EventDispatcherTask::m_eventQueue. However,
> it
> > looks a bit hard to move ExecutionContextTask to the heap (probably only for
> me
> > who doesn't have much template knowledge).....
> 
> In this patch, I moved this |close()| callsite to WorkerGlobalScope::dispose
> because we should not touch on-heap object in the destructor.

oh, I missed that. That makes sense.

[tkent, 2014-05-30 14:01:23]

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
File Source/core/workers/WorkerEventQueue.h (right):

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
Source/core/workers/WorkerEventQueue.h:42: class WorkerEventQueue FINAL : public
NoBaseWillBeGarbageCollectedFinalized<WorkerEventQueue>, public EventQueue {
On 2014/05/30 13:47:58, tkent wrote:
> On 2014/05/30 10:23:29, sof wrote:
> > Does WorkerEventQueue need finalization?
> 
> I don't think so.  I'll update the patch.
> 

I found finalization was required.
~EventQueue is defined, and other EventQueue subclasses need finalization.
blink_gc_plugin doesn't allow GarbageCollected though we don't need to call
~WorkerEventQueue actually.

[sof, 2014-05-30 15:05:41]

lgtm

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
File Source/core/workers/WorkerEventQueue.h (right):

https://codereview.chromium.org/303133005/diff/1/Source/core/workers/WorkerEv...
Source/core/workers/WorkerEventQueue.h:42: class WorkerEventQueue FINAL : public
NoBaseWillBeGarbageCollectedFinalized<WorkerEventQueue>, public EventQueue {
On 2014/05/30 14:01:23, tkent wrote:
> On 2014/05/30 13:47:58, tkent wrote:
> > On 2014/05/30 10:23:29, sof wrote:
> > > Does WorkerEventQueue need finalization?
> > 
> > I don't think so.  I'll update the patch.
> > 
> 
> I found finalization was required.
> ~EventQueue is defined, and other EventQueue subclasses need finalization.
> blink_gc_plugin doesn't allow GarbageCollected though we don't need to call
> ~WorkerEventQueue actually.

thanks, it might become possible at some point later.

[tkent, 2014-05-30 15:26:07]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:26:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/303133005/40001

[commit-bot: I haz the power, 2014-05-30 16:19:40]

Change committed as 175148