Oilpan: Prepare to move SQLTransactionBackendSync and SQLTransactionSync to Oilpan heap.

Oilpan: Prepare to move SQLTransactionBackendSync and SQLTransactionSync to Oilpan heap.

We need to rollback a transaction if SQLTransactionBackendSync is dying and its
transaction is in progress. However Oilpan architecture doesn't allow us to
touch associated DatabaseSync in ~SQLTransactionBackendSync.

This CL adds a PersistentHeapHashMap<WeakMember<T>, U> to
DatabaseSync to manage SQLTransactionSync, and it makes possible
to call SQLTransactionSync::rollbackIfInProgress before the destruction.

Note:
- m_database->executionContext() can be null in some functions because on-heap
  objects can live after dissociation of ExecutionContext.
- Change the argument of DatabaseSync::rollbackTransaction to a reference
  because PassRefPtr is unnecessary here and the argument is always non-null.

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

[zerny-chromium, 2014-03-14 10:42:24]

Hi Kent.

Erik and I have discussed the CL locally and suggest using something like:

#if ENABLE(OILPAN)
class WeakRollbackObserver {
public:
    WeakRollbackObserver() : m_sync(0) { }

    ~WeakRollbackObserver() { m_sync->willFinalize(); }

    static void observe(SQLTransactionBackendSync* sync) {
        observers().find(sync)->value.m_sync = sync;
    }

private:
    typedef HeapHashMap<WeakMember<SQLTransactionBackendSync>,
WeakRollbackObserver> ObserverMap;

    static ObserverMap& observers()
    {
        DEFINE_STATIC_LOCAL(Persistent<ObserverMap>, observerMap, ());
        return *observerMap;
    }

    SQLTransactionBackendSync* m_sync;
};
#endif

And call WeakRollbackObserver::observe(this) in SQLTransactionBackendSync.

The above avoids creating a persistent handle per observer (just one for the
hash set is needed). I'm not sure about how this fits in with db threads. Maybe
the observerMap should be allocated one per DatabaseSync object instead and
registering done on the db instance handed to the transactions constructor.

Regarding weak processing, in the above, this works by using the built-in weak
processing of hash maps. When a weak key is no longer alive the destructor of
the mapped value (WeakRollbackObserver) will be called.

I suggest renaming willFinalize to a more descriptive name, maybe
rollbackUnfinishedTransaction or similar.

[tkent, 2014-03-17 02:30:44]

On 2014/03/14 10:42:24, zerny-chromium wrote:
> Hi Kent.
> 
> Erik and I have discussed the CL locally and suggest using something like:
> 
> #if ENABLE(OILPAN)
> class WeakRollbackObserver {
> public:
>     WeakRollbackObserver() : m_sync(0) { }
> 
>     ~WeakRollbackObserver() { m_sync->willFinalize(); }
> 
>     static void observe(SQLTransactionBackendSync* sync) {
>         observers().find(sync)->value.m_sync = sync;
>     }
> 
> private:
>     typedef HeapHashMap<WeakMember<SQLTransactionBackendSync>,
> WeakRollbackObserver> ObserverMap;
> 
>     static ObserverMap& observers()
>     {
>         DEFINE_STATIC_LOCAL(Persistent<ObserverMap>, observerMap, ());
>         return *observerMap;
>     }
> 
>     SQLTransactionBackendSync* m_sync;
> };
> #endif
> 
> And call WeakRollbackObserver::observe(this) in SQLTransactionBackendSync.
> 
> The above avoids creating a persistent handle per observer (just one for the
> hash set is needed). I'm not sure about how this fits in with db threads.
Maybe
> the observerMap should be allocated one per DatabaseSync object instead and
> registering done on the db instance handed to the transactions constructor.
> 
> Regarding weak processing, in the above, this works by using the built-in weak
> processing of hash maps. When a weak key is no longer alive the destructor of
> the mapped value (WeakRollbackObserver) will be called.
> 
> I suggest renaming willFinalize to a more descriptive name, maybe
> rollbackUnfinishedTransaction or similar.

Thank you for the comment.  I didn't know HeapHashMap<WeakMember<T>, ...> worked
so, and thought it's a good idea.
However, I have no good idea how to clear the Persistent handle to the
HeapHashMap before thread cleanup. The following code just works, but we should
not apply this because of tight dependency to HashMap implementation.  Do you
have any other idea?

template <typename T> class DyingTransactionRollbacker {
public:
    static void observe(T& target)
    {
        CrossThreadPersistent<HeapObserverMap>& map(observers());
        typename HeapObserverMap::AddResult result = map->add(&target, nullptr);
        if (result.isNewEntry)
            result.storedValue->value = adoptPtr(new
DyingTransactionRollbacker(map, target));
        else
            ASSERT(result.storedValue->value);
    }

    ~DyingTransactionRollbacker()
    {
        m_target.rollbackIfInProgress();
        // FIXME: This is very implementation-dependent.
        if (m_owner->size() == 1)
            m_owner = nullptr;
    }

private:
    typedef HeapHashMap<WeakMember<T>, OwnPtr<DyingTransactionRollbacker<T> > >
HeapObserverMap;

    DyingTransactionRollbacker(CrossThreadPersistent<HeapObserverMap>& owner, T&
target) : m_owner(owner), m_target(target) { }
    static CrossThreadPersistent<HeapObserverMap>& observers()
    {
        // Need to use CrossThreadPersistent because ThreadSpecific<T> is
        // destructed in another thread.
       
AtomicallyInitializedStatic(ThreadSpecific<CrossThreadPersistent<HeapObserverMap>
>*, tlsHandle = new ThreadSpecific<CrossThreadPersistent<HeapObserverMap> >());
        CrossThreadPersistent<HeapObserverMap>& map(**tlsHandle);
        if (!map)
            map = new HeapObserverMap();
        return map;
    }

    CrossThreadPersistent<HeapObserverMap>& m_owner;
    T& m_target;
};

[zerny-chromium, 2014-03-17 09:21:23]

I think we can allocate the set as part of the database object and avoid the
threading issues. An example CL is here:
https://codereview.chromium.org/201623003.

One thing to note is that we still need the Persistent to the HashSet in order
to ensure that the weak processing happens even if the entire group of
transactions + database die together. The root will ensure that we get a weak
callback and after that the group will be reclaimed (and the Persistent will die
with the DB object). What do you think?

[tkent, 2014-03-17 12:54:28]

On 2014/03/17 09:21:23, zerny-chromium wrote:
> I think we can allocate the set as part of the database object and avoid the
> threading issues. An example CL is here:
> https://codereview.chromium.org/201623003.
> 
> One thing to note is that we still need the Persistent to the HashSet in order
> to ensure that the weak processing happens even if the entire group of
> transactions + database die together. The root will ensure that we get a weak
> callback and after that the group will be reclaimed (and the Persistent will
die
> with the DB object). What do you think?

If we allow to use Persistent members in an on-heap class, making an observer
object a persistent member of the observation target class like [1] is simpler
and self-contained.

[1]
https://codereview.chromium.org/198563002/diff/1/Source/heap/HeapTest.cpp#new...

[zerny-chromium, 2014-03-17 13:08:02]

> If we allow to use Persistent members in an on-heap class, making an observer
> object a persistent member of the observation target class like [1] is simpler
> and self-contained.

But that means having a persistent per transaction instead of one per
database/thread. Alternatively you can pull the persistent our of the database
object as a static map per thread which is all we need. It just does not work to
have a single map shared by all threads.

[tkent, 2014-03-18 06:31:35]

On 2014/03/17 13:08:02, zerny-chromium wrote:
> > If we allow to use Persistent members in an on-heap class, making an
observer
> > object a persistent member of the observation target class like [1] is
simpler
> > and self-contained.
> 
> But that means having a persistent per transaction instead of one per
> database/thread. Alternatively you can pull the persistent our of the database
> object as a static map per thread which is all we need. It just does not work
to
> have a single map shared by all threads.

ok, reducing the number of persistent handles makes sense. I updated the patch.

[Mads Ager (chromium), 2014-03-18 07:38:32]

LGTM!

Thanks for iterating on this. I like the final result. :)

[zerny-chromium, 2014-03-18 07:40:20]

LGTM too!

[tkent, 2014-03-18 07:47:12]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-18 07:47:14]

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

[Vyacheslav Egorov (Google), 2014-03-18 07:54:54]

lgtm

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransactionBackendSync.cpp (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionBackendSync.cpp:246: // This can be
called during GC. Do not allocate new on-heap objects.
I don't think comments like this scale as long you call more than one function
inside because they should also essentially have the same comment.

Sadly I don't see any good way to handle this.

We can consider annotating functions that can be called during GC with a special
attribute and checking their bodies transitively but that would soon turn into a
mess probably.

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionBackendSync.cpp:251: if
(m_sqliteTransaction) {
from my reading of the code this whole if can be just replaced with
m_sqliteTransaction.clear() because destructor does rollback.

SQLiteTransaction::~SQLiteTransaction()
{
    if (m_inProgress)
        rollback();
}

[commit-bot: I haz the power, 2014-03-18 08:08:45]

Change committed as 169428

[haraken, 2014-03-18 09:59:08]

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseSync.cpp (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseSync.cpp:200: m_observers.add(&transaction,
adoptPtr(new TransactionObserver(transaction)));

Who removes an item from m_observers? I cannot find any code that does the
removal. Is it intentional?

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseSync.h (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseSync.h:92:
PersistentHeapHashMap<WeakMember<SQLTransactionSync>,
OwnPtr<TransactionObserver> > m_observers;

I'd like to have a comment about why this needs to be persistent.

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransactionBackendSync.h (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionBackendSync.h:55:
~SQLTransactionBackendSync();

This could be a virtual destructor (although not being a virtual destructor
won't cause any issue at the moment.)

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionBackendSync.h:56: void trace(Visitor*);

Shall we make this virtual and override it in SQLTransactionSync?

[zerny-chromium, 2014-03-18 10:09:08]

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseSync.cpp (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseSync.cpp:200: m_observers.add(&transaction,
adoptPtr(new TransactionObserver(transaction)));
On 2014/03/18 09:59:09, haraken wrote:
> 
> Who removes an item from m_observers? I cannot find any code that does the
> removal. Is it intentional?

They are removed as part of the weak processing of HeapHashMap. This is nice
because the HashMap can do a more efficient job without the need for callbacks.

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseSync.h (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseSync.h:92:
PersistentHeapHashMap<WeakMember<SQLTransactionSync>,
OwnPtr<TransactionObserver> > m_observers;
On 2014/03/18 09:59:09, haraken wrote:
> 
> I'd like to have a comment about why this needs to be persistent.

I think the bug report provides sufficient documentation here.

[haraken, 2014-03-18 11:31:33]

Now I understand how the persistent weak hash map is working :) LGTM.

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseSync.cpp (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseSync.cpp:200: m_observers.add(&transaction,
adoptPtr(new TransactionObserver(transaction)));
On 2014/03/18 10:09:09, zerny-chromium wrote:
> On 2014/03/18 09:59:09, haraken wrote:
> > 
> > Who removes an item from m_observers? I cannot find any code that does the
> > removal. Is it intentional?
> 
> They are removed as part of the weak processing of HeapHashMap. This is nice
> because the HashMap can do a more efficient job without the need for
callbacks.

Makes sense.

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseSync.h (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseSync.h:92:
PersistentHeapHashMap<WeakMember<SQLTransactionSync>,
OwnPtr<TransactionObserver> > m_observers;
On 2014/03/18 10:09:09, zerny-chromium wrote:
> On 2014/03/18 09:59:09, haraken wrote:
> > 
> > I'd like to have a comment about why this needs to be persistent.
> 
> I think the bug report provides sufficient documentation here.

I want to have a comment about the threading issue of this specific case. I
couldn't get it until mads explained it in the issue tracker :)

[tkent, 2014-03-19 04:58:38]

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseSync.h (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseSync.h:92:
PersistentHeapHashMap<WeakMember<SQLTransactionSync>,
OwnPtr<TransactionObserver> > m_observers;
On 2014/03/18 11:31:33, haraken wrote:
> On 2014/03/18 10:09:09, zerny-chromium wrote:
> > On 2014/03/18 09:59:09, haraken wrote:
> > > 
> > > I'd like to have a comment about why this needs to be persistent.
> > 
> > I think the bug report provides sufficient documentation here.
> 
> I want to have a comment about the threading issue of this specific case. I
> couldn't get it until mads explained it in the issue tracker :)

I'll add a comment.

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/SQLTransactionBackendSync.h (right):

https://codereview.chromium.org/198793003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/SQLTransactionBackendSync.h:56: void trace(Visitor*);
On 2014/03/18 09:59:09, haraken wrote:
> 
> Shall we make this virtual and override it in SQLTransactionSync?

I object to add such override.  It would look very silly according to common
sense of C++.
blink_gc_plugin can catch missing trace even if there are no overrides, right?

http://www.chromium.org/developers/blink-gc-plugin-errors