Oilpan: Prepare to move DatabaseThread and SQLTransactionCoordinator to Oilpan heap.

Oilpan: Prepare to move DatabaseThread and SQLTransactionCoordinator to Oilpan heap.

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

[tkent, 2014-03-19 09:39:43]

please review this.

[haraken, 2014-03-19 09:42:45]

LGTM

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseThread.h (right):

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseThread.h:91: DatabaseTaskSynchronizer*
m_cleanupSync;

This raw pointer looks safe.

[Mads Ager (chromium), 2014-03-19 09:45:26]

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseContext.h (right):

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.h:78:
RefPtrWillBePersistent<DatabaseThread> m_databaseThread;
This Persistent is in a ThreadSafeRefCounted class. That means that the last
deref can be from a different thread than the one that allocated this object.
Don't we need a CrossThreadPersistent here so it is safe to destruct it on the
thread that does the last deref?

[tkent, 2014-03-19 23:43:53]

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseContext.h (right):

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.h:78:
RefPtrWillBePersistent<DatabaseThread> m_databaseThread;
On 2014/03/19 09:45:26, Mads Ager (chromium) wrote:
> This Persistent is in a ThreadSafeRefCounted class. That means that the last
> deref can be from a different thread than the one that allocated this object.
> Don't we need a CrossThreadPersistent here so it is safe to destruct it on the
> thread that does the last deref?

Good point.
DatabaseThread doesn't need to be ThreadSafeRefCounted.  Only this RefPtr holds
a reference count of a DatabaseThread.  So, this CL is safe.
A DatabaseThread object is transferred from a context thread to a database
thread by m_thread->postTask(new Task(WTF::bind(function, this)) in
DatabaseThread.cpp. Ideally this task should have CrossThreadPersistent. However
a raw pointer is ok because DatabaseContext::m_databaseThread has a strong
reference until the database thread is dead.

We might need to add ParamStrorageTraits<RawPtr<T>> to wtf/Functional.h in the
future though it is unnecessary for DatabaseThread.

[tkent, 2014-03-19 23:43:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-19 23:44:14]

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

[commit-bot: I haz the power, 2014-03-19 23:45:15]

Change committed as 169594

[Mads Ager (chromium), 2014-03-20 06:24:15]

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseContext.h (right):

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.h:78:
RefPtrWillBePersistent<DatabaseThread> m_databaseThread;
On 2014/03/19 23:43:53, tkent wrote:
> On 2014/03/19 09:45:26, Mads Ager (chromium) wrote:
> > This Persistent is in a ThreadSafeRefCounted class. That means that the last
> > deref can be from a different thread than the one that allocated this
object.
> > Don't we need a CrossThreadPersistent here so it is safe to destruct it on
the
> > thread that does the last deref?
> 
> Good point.
> DatabaseThread doesn't need to be ThreadSafeRefCounted.  Only this RefPtr
holds
> a reference count of a DatabaseThread.  So, this CL is safe.
> A DatabaseThread object is transferred from a context thread to a database
> thread by m_thread->postTask(new Task(WTF::bind(function, this)) in
> DatabaseThread.cpp. Ideally this task should have CrossThreadPersistent.
However
> a raw pointer is ok because DatabaseContext::m_databaseThread has a strong
> reference until the database thread is dead.
> 
> We might need to add ParamStrorageTraits<RawPtr<T>> to wtf/Functional.h in the
> future though it is unnecessary for DatabaseThread.

Thanks for that part of the explanation Kent!

The thing that worries me is not DataBaseThread. Having that in a Persistent
seems fine. The thing that worries me is that DatabaseContext is
ThreadSafeRefCounted. That means that in principle it could be created on one
thread and destructed on another thread which would lead to multi-threaded
access to a Persistent chain which can lead to a corrupted persistent chain. It
therefore seems like this Persistent should be a CrossThreadPersistent so that
it can safely be created on one thread and destructed on another.

Is it guaranteed that the DatabaseContext is destructed on the same thread that
created it?

(In any case, this will be solved when we move DatabaseContext into the heap and
use a Member here instead of the CrossThreadPersistent.)

[Mads Ager (chromium), 2014-03-20 06:26:03]

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseContext.h (right):

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.h:78:
RefPtrWillBePersistent<DatabaseThread> m_databaseThread;
On 2014/03/20 06:24:16, Mads Ager (chromium) wrote:
> On 2014/03/19 23:43:53, tkent wrote:
> > On 2014/03/19 09:45:26, Mads Ager (chromium) wrote:
> > > This Persistent is in a ThreadSafeRefCounted class. That means that the
last
> > > deref can be from a different thread than the one that allocated this
> object.
> > > Don't we need a CrossThreadPersistent here so it is safe to destruct it on
> the
> > > thread that does the last deref?
> > 
> > Good point.
> > DatabaseThread doesn't need to be ThreadSafeRefCounted.  Only this RefPtr
> holds
> > a reference count of a DatabaseThread.  So, this CL is safe.
> > A DatabaseThread object is transferred from a context thread to a database
> > thread by m_thread->postTask(new Task(WTF::bind(function, this)) in
> > DatabaseThread.cpp. Ideally this task should have CrossThreadPersistent.
> However
> > a raw pointer is ok because DatabaseContext::m_databaseThread has a strong
> > reference until the database thread is dead.
> > 
> > We might need to add ParamStrorageTraits<RawPtr<T>> to wtf/Functional.h in
the
> > future though it is unnecessary for DatabaseThread.
> 
> Thanks for that part of the explanation Kent!
> 
> The thing that worries me is not DataBaseThread. Having that in a Persistent
> seems fine. The thing that worries me is that DatabaseContext is
> ThreadSafeRefCounted. That means that in principle it could be created on one
> thread and destructed on another thread which would lead to multi-threaded
> access to a Persistent chain which can lead to a corrupted persistent chain.
It
> therefore seems like this Persistent should be a CrossThreadPersistent so that
> it can safely be created on one thread and destructed on another.
> 
> Is it guaranteed that the DatabaseContext is destructed on the same thread
that
> created it?

It probably is. I guess this is the execution context for the database thread
that is destructed at the very end of database thread execution?

[tkent, 2014-03-20 07:50:09]

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/DatabaseContext.h (right):

https://codereview.chromium.org/201083003/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.h:78:
RefPtrWillBePersistent<DatabaseThread> m_databaseThread;
On 2014/03/20 06:26:03, Mads Ager (chromium) wrote:
> On 2014/03/20 06:24:16, Mads Ager (chromium) wrote:
> > On 2014/03/19 23:43:53, tkent wrote:
> > > On 2014/03/19 09:45:26, Mads Ager (chromium) wrote:
> > > > This Persistent is in a ThreadSafeRefCounted class. That means that the
> last
> > > > deref can be from a different thread than the one that allocated this
> > object.
> > > > Don't we need a CrossThreadPersistent here so it is safe to destruct it
on
> > the
> > > > thread that does the last deref?
> > > 
> > > Good point.
> > > DatabaseThread doesn't need to be ThreadSafeRefCounted.  Only this RefPtr
> > holds
> > > a reference count of a DatabaseThread.  So, this CL is safe.
> > > A DatabaseThread object is transferred from a context thread to a database
> > > thread by m_thread->postTask(new Task(WTF::bind(function, this)) in
> > > DatabaseThread.cpp. Ideally this task should have CrossThreadPersistent.
> > However
> > > a raw pointer is ok because DatabaseContext::m_databaseThread has a strong
> > > reference until the database thread is dead.
> > > 
> > > We might need to add ParamStrorageTraits<RawPtr<T>> to wtf/Functional.h in
> the
> > > future though it is unnecessary for DatabaseThread.
> > 
> > Thanks for that part of the explanation Kent!
> > 
> > The thing that worries me is not DataBaseThread. Having that in a Persistent
> > seems fine. The thing that worries me is that DatabaseContext is
> > ThreadSafeRefCounted. That means that in principle it could be created on
one
> > thread and destructed on another thread which would lead to multi-threaded
> > access to a Persistent chain which can lead to a corrupted persistent chain.
> It
> > therefore seems like this Persistent should be a CrossThreadPersistent so
that
> > it can safely be created on one thread and destructed on another.
> > 
> > Is it guaranteed that the DatabaseContext is destructed on the same thread
> that
> > created it?
> 
> It probably is. I guess this is the execution context for the database thread
> that is destructed at the very end of database thread execution?
> 

> Is it guaranteed that the DatabaseContext is destructed on the same thread
that
created it?

Yes.
A DatabaseContext object can be referred by objects in a context thread and
objects in a database thread, and the DatabaseContext object is always outlives
the corresponding database thread. So, m_databasethread is always cleared in the
context thread.