Oilpan: Remove raw pointer to InProcessWorkerBase from WorkerMessagingProxy

Oilpan: Remove raw pointer to InProcessWorkerBase from WorkerMessagingProxy

BUG=509911

[keishi, 2015-07-29 21:15:15]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org, yutak@chromium.org

[keishi, 2015-07-29 21:15:16]

WorkerMessagingProxy deletes itself from
WorkerMessagingProxy::workerThreadTerminated.
When I don't do this, WorkerMessagingProxy gets finalized when exiting a
SafePoint but it tries to reenter a SafePoint and I got the crash below.
I used a keepAlive to delay the finalizer after worker thread termination.

STDERR: ASSERTION FAILED: !state->sweepForbidden()
STDERR: ../../third_party/WebKit/Source/platform/heap/SafePoint.cpp(101) : void
blink::SafePointBarrier::enterSafePoint(blink::ThreadState *)
STDERR: 1   0x10dac6948
blink::SafePointBarrier::enterSafePoint(blink::ThreadState*)
STDERR: 2   0x10dacce53
blink::ThreadState::enterSafePoint(blink::ThreadState::StackState, void*)
STDERR: 3   0x10d67eb27
blink::SafePointScope::SafePointScope(blink::ThreadState::StackState,
blink::ThreadState*)
STDERR: 4   0x10d67e9c1
blink::SafePointScope::SafePointScope(blink::ThreadState::StackState,
blink::ThreadState*)
STDERR: 5   0x10d68d307 blink::WebThreadSupportingGC::~WebThreadSupportingGC()
STDERR: 6   0x10d68d383 blink::WebThreadSupportingGC::~WebThreadSupportingGC()
STDERR: 7   0x124010d65
WTF::OwnedPtrDeleter<blink::WebThreadSupportingGC>::deletePtr(blink::WebThreadSupportingGC*)
STDERR: 8   0x124010cfa WTF::OwnPtr<blink::WebThreadSupportingGC>::~OwnPtr()
STDERR: 9   0x124010653 WTF::OwnPtr<blink::WebThreadSupportingGC>::~OwnPtr()
STDERR: 10  0x124010261 blink::DedicatedWorkerThread::~DedicatedWorkerThread()
STDERR: 11  0x1240102b3 blink::DedicatedWorkerThread::~DedicatedWorkerThread()
STDERR: 12  0x12401030a blink::DedicatedWorkerThread::~DedicatedWorkerThread()
STDERR: 13  0x12400ff8a WTF::RefCounted<blink::WorkerThread>::deref()
STDERR: 14  0x124034205 void
WTF::derefIfNotNull<blink::WorkerThread>(blink::WorkerThread*)
STDERR: 15  0x1240341a6 WTF::RefPtr<blink::WorkerThread>::~RefPtr()
STDERR: 16  0x124031fa3 WTF::RefPtr<blink::WorkerThread>::~RefPtr()
STDERR: 17  0x12403000f blink::WorkerMessagingProxy::~WorkerMessagingProxy()
STDERR: 18  0x12400f956
blink::DedicatedWorkerMessagingProxy::~DedicatedWorkerMessagingProxy()
STDERR: 19  0x12400f9a3
blink::DedicatedWorkerMessagingProxy::~DedicatedWorkerMessagingProxy()
STDERR: 20  0x10ee15bb6
blink::GarbageCollectedFinalized<blink::WorkerGlobalScopeProxy>::finalizeGarbageCollectedObject()
STDERR: 21  0x10ee15b66 blink::FinalizerTraitImpl<blink::WorkerGlobalScopeProxy,
true>::finalize(void*)
STDERR: 22  0x10ee15b13
blink::FinalizerTrait<blink::WorkerGlobalScopeProxy>::finalize(void*)
STDERR: 23  0x10dab0fdb blink::HeapObjectHeader::finalize(unsigned char*,
unsigned long)
STDERR: 24  0x10dab5edd blink::NormalPage::sweep()
STDERR: 25  0x10dab20c4 blink::BaseHeap::sweepUnsweptPage()
STDERR: 26  0x10dab24a1 blink::BaseHeap::completeSweep()
STDERR: 27  0x10dac7e2f blink::ThreadState::completeSweep()
STDERR: 28  0x10dacc595 blink::ThreadState::preSweep()
STDERR: 29  0x10daccf7c
blink::ThreadState::leaveSafePoint(blink::SafePointAwareMutexLocker*)
STDERR: 30  0x10d67ebdc blink::SafePointScope::~SafePointScope()
STDERR: 31  0x10d67ea03 blink::SafePointScope::~SafePointScope()

[haraken, 2015-07-30 11:46:32]

Moving WorkerGlobalScopeProxy to the heap will make things complicated... Maybe
a better fix would be:

- Keep WorkerGlobalScopeProxy off-heap.
- Introduce WeakPersistent.
- Use a WeakPersistent from the WorkerGlobalScopeProxy to InProcessWorkerBase?

https://codereview.chromium.org/1257653007/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1257653007/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerMessagingProxy.cpp:309: workerObjectDestroyed();

It is not nice to call workerObjectDestroyed() in weak processing, since the
weak processing is not called if the WorkerMessagingProxy and the
InProcessWorkerBase die at the same time.

I think we should call workerObjectDestroyed() in the pre-finalizer of
InProcessWorkerBase. Then workerObjectDestroyed() will be unconditionally called
before the InProcessWorkerBase gets destructed.

https://codereview.chromium.org/1257653007/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerMessagingProxy.h (right):

https://codereview.chromium.org/1257653007/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerMessagingProxy.h:121:
OwnPtr<Persistent<WorkerMessagingProxy>> m_keepAlive;

Once https://codereview.chromium.org/1233173002/ is landed, we can use the
SelfKeepAlive handle.

[haraken, 2015-07-30 14:22:38]

On 2015/07/30 11:46:32, haraken wrote:
> Moving WorkerGlobalScopeProxy to the heap will make things complicated...
Maybe
> a better fix would be:
> 
> - Keep WorkerGlobalScopeProxy off-heap.
> - Introduce WeakPersistent.
> - Use a WeakPersistent from the WorkerGlobalScopeProxy to InProcessWorkerBase?

WeakPersistent was landed.

[keishi, 2015-08-13 06:52:25]

On 2015/07/30 14:22:38, haraken wrote:
> On 2015/07/30 11:46:32, haraken wrote:
> > Moving WorkerGlobalScopeProxy to the heap will make things complicated...
> Maybe
> > a better fix would be:
> > 
> > - Keep WorkerGlobalScopeProxy off-heap.
> > - Introduce WeakPersistent.
> > - Use a WeakPersistent from the WorkerGlobalScopeProxy to
InProcessWorkerBase?
> 
> WeakPersistent was landed.

Done. WorkerGlobalScopeProxy is guaranteed to outlives the worker so I think it
is fine for the ~InProcessWorkerBase to call
m_contextProxy->workerObjectDestroyed().

[haraken, 2015-08-13 07:47:43]

On 2015/08/13 06:52:25, keishi wrote:
> On 2015/07/30 14:22:38, haraken wrote:
> > On 2015/07/30 11:46:32, haraken wrote:
> > > Moving WorkerGlobalScopeProxy to the heap will make things complicated...
> > Maybe
> > > a better fix would be:
> > > 
> > > - Keep WorkerGlobalScopeProxy off-heap.
> > > - Introduce WeakPersistent.
> > > - Use a WeakPersistent from the WorkerGlobalScopeProxy to
> InProcessWorkerBase?
> > 
> > WeakPersistent was landed.
> 
> Done. WorkerGlobalScopeProxy is guaranteed to outlives the worker so I think
it
> is fine for the ~InProcessWorkerBase to call
> m_contextProxy->workerObjectDestroyed().

Sounds reasonable.

But sorry, this is going to be addressed by peria-san moving AbstractWorker to
the heap. So let's land https://codereview.chromium.org/1212643004/ instead.