Drop V8RecursionScope dependency on ExecutionContext

Drop V8RecursionScope dependency on ExecutionContext

To support using V8RecursionScope in other places, drop the dependency on ExecutionContext in favor of just an v8::Isolate. This requires moving IDBPendingTransactionMonitor from a supplement on the context to per-isolate data.

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

[jsbell, 2014-09-09 00:08:12]

Now rebased on top of http://crrev.com/555633002 to make this easier to
understand.

[haraken, 2014-09-09 04:13:36]

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

[haraken, 2014-09-09 04:13:37]

One way to solve the issue would be to define:

void IDBPendingTransactionMonitor::dispose() {
  m_transactions.clear();
}

and call it somewhere before the main thread calls ThreadState::detach(). By
doing that we can guarantee that the persistent handles are gone at the point
where the ThreadState is detached.

What tests crash with this CL? I'm happy to help.

[jsbell, 2014-09-09 23:35:21]

Latest upload is terribly hacky but works, and at least it demonstrates one
possible direction.

> What tests crash with this CL? I'm happy to help.

Thanks!

Using patch set #3, running just webkit_unit_tests
--gtest_filter='IDBTransactionTest.EnsureLifetime' I hit a crash in:

#3 blink::ThreadState::checkThread()
#4 blink::ThreadLocalPersistents<>::Lock::Lock()
#5 blink::PersistentBase<>::~PersistentBase()
#6 blink::PersistentHeapCollectionBase<>::~PersistentHeapCollectionBase()
#7 blink::PersistentHeapVector<>::~PersistentHeapVector()
#8 blink::IDBPendingTransactionMonitor::~IDBPendingTransactionMonitor()
#9 WTF::OwnedPtrDeleter<>::deletePtr()
#10 WTF::OwnPtr<>::~OwnPtr()
#11 blink::V8PerIsolateData::~V8PerIsolateData()
#12 blink::V8PerIsolateData::dispose()
#13 blink::shutdown()

And I can verify that the PersistentHeapVector is empty when the destructor
runs; it's not that the *contents* are held, it's a thread-sensitive assertion
by the *collection* object itself.

Similarly, running any browser test with a Worker (e.g. content_browsertests
--gtest_filter='*Worker*' - no use of IDB!) I hit:

ASSERTION FAILED: !currentCount
../../third_party/WebKit/Source/platform/heap/ThreadState.cpp(455) : void
blink::ThreadState::cleanup()

.. coming out of:

#3 blink::ThreadState::cleanup()
#4 blink::ThreadState::detach()
#5 blink::WorkerThread::cleanup()
#6 WTF::FunctionWrapper<>::operator()()
#7 WTF::BoundFunctionImpl<>::operator()()
#8 WTF::Function<>::operator()()
#9 blink::Task::run()

Root cause is that IDBPendingTransactionMonitor (which has a
PersistentHeapVector<Member<X>>) still exists when ThreadState::detach() is
called, even though it is empty.

Patch set #4 addresses both of these by clearing out the V8PerIsolateData's
OwnPtr<IDBPendingTransactionMonitor> before ThreadState::detach is called - in
WebKit.cpp's shutdown() and in WorkerThread::cleanup(). It also reworks the
IDBPendingTransactionMonitor interface a bit to create it only if necessary,
which stops it being recreated during thread shutdown.

Patch set #5 noodles on that a bit, but is not a great improvement.

I have a feeling this is headed in the wrong direction, though. Does the
PersistentHeapVector itself need to be destroyed before the
ThreadState::detach(), or am I misunderstanding the problem?

[haraken, 2014-09-10 03:36:22]

On 2014/09/09 23:35:21, jsbell wrote:
> Latest upload is terribly hacky but works, and at least it demonstrates one
> possible direction.
> 
> > What tests crash with this CL? I'm happy to help.
> 
> Thanks!
> 
> Using patch set #3, running just webkit_unit_tests
> --gtest_filter='IDBTransactionTest.EnsureLifetime' I hit a crash in:
> 
> #3 blink::ThreadState::checkThread()
> #4 blink::ThreadLocalPersistents<>::Lock::Lock()
> #5 blink::PersistentBase<>::~PersistentBase()
> #6 blink::PersistentHeapCollectionBase<>::~PersistentHeapCollectionBase()
> #7 blink::PersistentHeapVector<>::~PersistentHeapVector()
> #8 blink::IDBPendingTransactionMonitor::~IDBPendingTransactionMonitor()
> #9 WTF::OwnedPtrDeleter<>::deletePtr()
> #10 WTF::OwnPtr<>::~OwnPtr()
> #11 blink::V8PerIsolateData::~V8PerIsolateData()
> #12 blink::V8PerIsolateData::dispose()
> #13 blink::shutdown()
> 
> And I can verify that the PersistentHeapVector is empty when the destructor
> runs; it's not that the *contents* are held, it's a thread-sensitive assertion
> by the *collection* object itself.
> 
> Similarly, running any browser test with a Worker (e.g. content_browsertests
> --gtest_filter='*Worker*' - no use of IDB!) I hit:
> 
> ASSERTION FAILED: !currentCount
> ../../third_party/WebKit/Source/platform/heap/ThreadState.cpp(455) : void
> blink::ThreadState::cleanup()
> 
> .. coming out of:
> 
> #3 blink::ThreadState::cleanup()
> #4 blink::ThreadState::detach()
> #5 blink::WorkerThread::cleanup()
> #6 WTF::FunctionWrapper<>::operator()()
> #7 WTF::BoundFunctionImpl<>::operator()()
> #8 WTF::Function<>::operator()()
> #9 blink::Task::run()
> 
> Root cause is that IDBPendingTransactionMonitor (which has a
> PersistentHeapVector<Member<X>>) still exists when ThreadState::detach() is
> called, even though it is empty.
> 
> Patch set #4 addresses both of these by clearing out the V8PerIsolateData's
> OwnPtr<IDBPendingTransactionMonitor> before ThreadState::detach is called - in
> WebKit.cpp's shutdown() and in WorkerThread::cleanup(). It also reworks the
> IDBPendingTransactionMonitor interface a bit to create it only if necessary,
> which stops it being recreated during thread shutdown.
> 
> Patch set #5 noodles on that a bit, but is not a great improvement.
> 
> I have a feeling this is headed in the wrong direction, though. Does the
> PersistentHeapVector itself need to be destroyed before the
> ThreadState::detach(), or am I misunderstanding the problem?

I think you're heading for a right direction.

As you mentioned, we need to make sure that all persistent handles are gone
before the ThreadState is detached.

I think a slightly better fix would be:

- Rename V8PerIsolateData::dispose to V8PerIsolateData::destroy
- Introduce V8PerIsolateData::willBeDestroyed.
- Clear the OwnPtr<IDBPendingTransactionMonitor> in the
V8PerIsolateData::willBeDestroyed
- Call V8PerIsolateData::willBeDestroyed before calling ThreadState::detach.

[Mads Ager (chromium), 2014-09-10 07:21:26]

On 2014/09/09 23:35:21, jsbell wrote:
> Latest upload is terribly hacky but works, and at least it demonstrates one
> possible direction.
> 
> > What tests crash with this CL? I'm happy to help.
> 
> Thanks!
> 
> Using patch set #3, running just webkit_unit_tests
> --gtest_filter='IDBTransactionTest.EnsureLifetime' I hit a crash in:
> 
> #3 blink::ThreadState::checkThread()
> #4 blink::ThreadLocalPersistents<>::Lock::Lock()
> #5 blink::PersistentBase<>::~PersistentBase()
> #6 blink::PersistentHeapCollectionBase<>::~PersistentHeapCollectionBase()
> #7 blink::PersistentHeapVector<>::~PersistentHeapVector()
> #8 blink::IDBPendingTransactionMonitor::~IDBPendingTransactionMonitor()
> #9 WTF::OwnedPtrDeleter<>::deletePtr()
> #10 WTF::OwnPtr<>::~OwnPtr()
> #11 blink::V8PerIsolateData::~V8PerIsolateData()
> #12 blink::V8PerIsolateData::dispose()
> #13 blink::shutdown()
> 
> And I can verify that the PersistentHeapVector is empty when the destructor
> runs; it's not that the *contents* are held, it's a thread-sensitive assertion
> by the *collection* object itself.

What this says is that in this test the persistent handle is deallocated on a
different thread than the one that allocated it. That is not allowed because
there is no thread synchronization around persistent handles. Does this test end
up creating the IDBPendingTransactionMonitor object on one thread and deleting
it on another? I would think that this shouldn't happen? If you do need to
transfer ownership of a persistent between threads you need to use
CrossThreadPersistent. However, those should be avoided as much as possible
because it usually means that you are passing object pointers between threads
and therefore you have to be very careful that the CrossThreadPersistent cannot
outlive the thread that owns the object pointed to. 

> Similarly, running any browser test with a Worker (e.g. content_browsertests
> --gtest_filter='*Worker*' - no use of IDB!) I hit:
> 
> ASSERTION FAILED: !currentCount
> ../../third_party/WebKit/Source/platform/heap/ThreadState.cpp(455) : void
> blink::ThreadState::cleanup()
> 
> .. coming out of:
> 
> #3 blink::ThreadState::cleanup()
> #4 blink::ThreadState::detach()
> #5 blink::WorkerThread::cleanup()
> #6 WTF::FunctionWrapper<>::operator()()
> #7 WTF::BoundFunctionImpl<>::operator()()
> #8 WTF::Function<>::operator()()
> #9 blink::Task::run()
> 
> Root cause is that IDBPendingTransactionMonitor (which has a
> PersistentHeapVector<Member<X>>) still exists when ThreadState::detach() is
> called, even though it is empty.
> 
> Patch set #4 addresses both of these by clearing out the V8PerIsolateData's
> OwnPtr<IDBPendingTransactionMonitor> before ThreadState::detach is called - in
> WebKit.cpp's shutdown() and in WorkerThread::cleanup(). It also reworks the
> IDBPendingTransactionMonitor interface a bit to create it only if necessary,
> which stops it being recreated during thread shutdown.
> 
> Patch set #5 noodles on that a bit, but is not a great improvement.
> 
> I have a feeling this is headed in the wrong direction, though. Does the
> PersistentHeapVector itself need to be destroyed before the
> ThreadState::detach(), or am I misunderstanding the problem?

I think your suggestion and Haraken's suggestion makes sense. Let me add a bit
of context about the assert that you are hitting. :)

When a worker thread is shutting down, there should be no more pointers into its
heap because the heap pages will be torn down and any pointer into the heap will
be a potential use-after-free situation. We have some fairly complicated logic
in the oilpan infrastructure to catch such situations and to make accidental
cross-thread pointers lead to safe null-pointer crashes instead of
use-after-free if used after the worker thread dies.

The particular assertion that you are hitting here is that when a worker thread
shuts down a number of GCs should lead to having no persistent handles into that
heap. If it doesn't there will be an object in the heap that is leaked and will
never have its destructor called (which could mean that there are other leaking
system resources). In many situations (including the collection you have a
persistent handle to here) that is probably safe, but it seems undesirable and
hence we have the assert.

We could remove that assert again, but I find it useful to catch accidental
leaks, so if we can update the shutdown code to make sure to delete persistents
in V8PerIsolateData I would prefer that. We are doing similar things in the
shutdown sequence already: V8 is going down and therefore it will never use a
DOM wrapper again and therefore we delete all the persistent handles from
wrappers to DOM objects before running the shutdown sequence on the Blink heap.

I hope this context helps understand what is going on. The threading issue is
the most serious of these. The cleanup one is more about keeping helpful (but
not necessarily vital) assertions to catch leaks.

[jsbell, 2014-09-10 18:03:44]

On 2014/09/10 03:36:22, haraken wrote:
> I think a slightly better fix would be:
> 
> - Rename V8PerIsolateData::dispose to V8PerIsolateData::destroy
> - Introduce V8PerIsolateData::willBeDestroyed.
> - Clear the OwnPtr<IDBPendingTransactionMonitor> in the
> V8PerIsolateData::willBeDestroyed
> - Call V8PerIsolateData::willBeDestroyed before calling ThreadState::detach.

Thanks and sgtm - now that I know this is the right direction we can make
something clean like the above.

I'll need to split this further, e.g introducing VPerIsolateModuleData or
something, and landing the actual Microtask+V8RecursionScope change separately.

[jsbell, 2014-09-10 18:06:53]

On 2014/09/10 07:21:26, Mads Ager (chromium) wrote:
> 
> What this says is that in this test the persistent handle is deallocated on a
> different thread than the one that allocated it. That is not allowed because
> there is no thread synchronization around persistent handles. Does this test
end
> up creating the IDBPendingTransactionMonitor object on one thread and deleting
> it on another? I would think that this shouldn't happen?

Nope, everything has per-isolate (so implicitly, per-thread) ownership. It's
purely about the tear-down timing, where the PersistentHeapVector<> was being
destructed after ThreadState::detach() so the asserts were perfectly valid.
 
> I hope this context helps understand what is going on. The threading issue is
> the most serious of these. The cleanup one is more about keeping helpful (but
> not necessarily vital) assertions to catch leaks.

Yep, thanks!

[jsbell, 2014-09-10 20:23:37]

jsbell@chromium.org changed reviewers:
+ tasak@chromium.org

[jsbell, 2014-09-10 21:41:45]

Prospective chain of patches is:

1. https://codereview.chromium.org/555633002 - ExecutionContext->ScriptState in
IDBTransaction ctor
2. https://codereview.chromium.org/429453010 (this patch) - drop
ExecutionContext dependency from V8RecursionScope
3. https://codereview.chromium.org/555163005 - wrap Microtask execution with
V8RecursionScope

This patch has a few potential issues:

* I used the pattern of `SomeObject::doSomething(isolate)` over
`SomeObject::from(isolate)->doSomething()`. I don't have strong feelings, and am
happy to switch. We seem to use both patterns in nearby code.

* I left a dependency from V8PerIsolateData on modules/; splitting out
V8PerIsolateModuleData (or whatever) looked like it would require either a new
GinEmbedder constant (yuck!) or an interface held in V8PerIsolateData and
static_cast<> by ModuleProxy. tasak@, do you have a vision for how we should
handle module-specific per-isolate data? Should we land this and then refactor,
or do it at the same time?

* Maybe using V8RecursionScope for this is the wrong idea entirely, and we
should use a TaskObserver instead, registered after the rest? I think that's a
newish concept, so I'm not sure it's appropriate? See CL #3 for context.

Thanks in advance for your feedback!

[haraken, 2014-09-11 01:18:54]

LGTM with comments!

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8PerIsolateData.cpp:126: // prior to thread detach.

thread detach => calling ThreadState::detach

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8PerIsolateData.cpp:136: void* data =
isolate->GetData(gin::kEmbedderBlink);

We can use from(isolate) here as well.

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8RecursionScope.h (right):

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8RecursionScope.h:62: V8RecursionScope(v8::Isolate*
isolate)

Add explicit.

https://codereview.chromium.org/429453010/diff/140001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/429453010/diff/140001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:298: m_workerGlobalScope->dispose();

I think you can call willBeDestroyed here (between line 298 and 299).

https://codereview.chromium.org/429453010/diff/140001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:300: 

I think you can call willBeDestroyed here.

https://codereview.chromium.org/429453010/diff/140001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:340:
V8PerIsolateData::willBeDestroyed(workerGlobalScope->script()->isolate());

Can we call this in WorkerThread::cleanup()? We want to call
V8PerIsolateData::willBeDestroyed at the very end of the thread execution but
before starting detaching the thread.

https://codereview.chromium.org/429453010/diff/140001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBPendingTransactionMonitor.cpp (right):

https://codereview.chromium.org/429453010/diff/140001/Source/modules/indexedd...
Source/modules/indexeddb/IDBPendingTransactionMonitor.cpp:52:
perIsolateData->idbPendingTransactionMonitor()->addNewTransaction(transaction);

Slightly better:

1) Introduce V8PerIsolateData::ensureIDBPendingTransactionMonitor
2) Call
perIsolateData->ensureIDBPendingTransactionMonitor()->addNewTransaction(...)

[haraken, 2014-09-11 01:21:04]

> Prospective chain of patches is:
> 
> 1. https://codereview.chromium.org/555633002 - ExecutionContext->ScriptState
in
> IDBTransaction ctor
> 2. https://codereview.chromium.org/429453010 (this patch) - drop
> ExecutionContext dependency from V8RecursionScope

The 1 and 2 look good to me.

> 3. https://codereview.chromium.org/555163005 - wrap Microtask execution with
> V8RecursionScope

I don't think I'm eligible to judge 3 :) I'd like to defer the review to adamk@.

> This patch has a few potential issues:
> 
> * I used the pattern of `SomeObject::doSomething(isolate)` over
> `SomeObject::from(isolate)->doSomething()`. I don't have strong feelings, and
am
> happy to switch. We seem to use both patterns in nearby code.

This is fine!

> * I left a dependency from V8PerIsolateData on modules/; splitting out
> V8PerIsolateModuleData (or whatever) looked like it would require either a new
> GinEmbedder constant (yuck!) or an interface held in V8PerIsolateData and
> static_cast<> by ModuleProxy. tasak@, do you have a vision for how we should
> handle module-specific per-isolate data? Should we land this and then
refactor,
> or do it at the same time?

The binding componentization is not yet complete, so I think we can worry about
the problem in a separate CL. I'm fine with landing this CL as is.

> * Maybe using V8RecursionScope for this is the wrong idea entirely, and we
> should use a TaskObserver instead, registered after the rest? I think that's a
> newish concept, so I'm not sure it's appropriate? See CL #3 for context.

Defer to adamk :)

[jsbell, 2014-09-11 22:14:52]

Thanks!

What do you think about the placement of the willBeDestroyed call now?

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8PerIsolateData.cpp:126: // prior to thread detach.
On 2014/09/11 01:18:53, haraken wrote:
> 
> thread detach => calling ThreadState::detach

Done.

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8PerIsolateData.cpp:136: void* data =
isolate->GetData(gin::kEmbedderBlink);
On 2014/09/11 01:18:53, haraken wrote:
> 
> We can use from(isolate) here as well.

Done.

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8RecursionScope.h (right):

https://codereview.chromium.org/429453010/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8RecursionScope.h:62: V8RecursionScope(v8::Isolate*
isolate)
On 2014/09/11 01:18:53, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/429453010/diff/140001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/429453010/diff/140001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:298: m_workerGlobalScope->dispose();
On 2014/09/11 01:18:54, haraken wrote:
> 
> I think you can call willBeDestroyed here (between line 298 and 299).

Agreed, doing it here in cleanup() would be better and make worker thread
shutdown more similar to the main thread. The complication is that the path to
the isolate (WorkerGlobalScope -> WorkerScriptController -> v8::Isolate) is
broken by the WorkerThreadShutdownFinishTask which runs before cleanup() - line
343 calls workerGlobalScope->clearScript();

I considered passing the isolate through in the closure, but how about (latest
patch) moving the call into WorkerScriptController itself, since that is
responsible for initializing the V8PerContextData in the first place?

https://codereview.chromium.org/429453010/diff/140001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBPendingTransactionMonitor.cpp (right):

https://codereview.chromium.org/429453010/diff/140001/Source/modules/indexedd...
Source/modules/indexeddb/IDBPendingTransactionMonitor.cpp:52:
perIsolateData->idbPendingTransactionMonitor()->addNewTransaction(transaction);
On 2014/09/11 01:18:54, haraken wrote:
> 
> Slightly better:
> 
> 1) Introduce V8PerIsolateData::ensureIDBPendingTransactionMonitor
> 2) Call
> perIsolateData->ensureIDBPendingTransactionMonitor()->addNewTransaction(...)

Done.

[jsbell, 2014-09-11 22:20:07]

Oops, collides with https://codereview.chromium.org/559363002

[haraken, 2014-09-11 23:21:19]

Still LGTM

https://codereview.chromium.org/429453010/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/429453010/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/WorkerScriptController.cpp:153:
V8PerIsolateData::willBeDestroyed(m_isolate);

Nit: I'd put this after line 156. Conceptually PerIsolateData's cleanup should
be done at the very end of other cleanups (i.e., after disposePerContextData).

[jsbell, 2014-09-12 22:52:06]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:53:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/429453010/200001

[commit-bot: I haz the power, 2014-09-12 23:01:34]

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

[commit-bot: I haz the power, 2014-09-12 23:01:35]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[jsbell, 2014-09-12 23:27:48]

https://codereview.chromium.org/429453010/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/429453010/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/WorkerScriptController.cpp:153:
V8PerIsolateData::willBeDestroyed(m_isolate);
On 2014/09/11 23:21:18, haraken wrote:
> 
> Nit: I'd put this after line 156. Conceptually PerIsolateData's cleanup should
> be done at the very end of other cleanups (i.e., after disposePerContextData).

Done.

[jsbell, 2014-09-12 23:28:16]

adamk@ - can you rs as Source/web OWNER?

[adamk, 2014-09-12 23:32:04]

On 2014/09/12 at 23:28:16, jsbell wrote:
> adamk@ - can you rs as Source/web OWNER?

rslgtm for Source/web

[adamk, 2014-09-12 23:40:40]

On 2014/09/12 at 23:32:04, adamk wrote:
> On 2014/09/12 at 23:28:16, jsbell wrote:
> > adamk@ - can you rs as Source/web OWNER?
> 
> rslgtm for Source/web

I mean to say, lgtm

[jsbell, 2014-09-12 23:41:03]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-09-12 23:41:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/429453010/200001

[commit-bot: I haz the power, 2014-09-13 00:00:49]

Committed patchset #11 (id:200001) as 181946