Oilpan: Move database closing task in ~DatabaseBackendSync to a pre-finalization hook with HeapHash…

Oilpan: Move database closing task in ~DatabaseBackendSync to a pre-finalization hook with HeapHashMap.

This is a preparation to move DatabaseContext class to Oilpan heap.

We need to access a DatabaseContext during closing a database. So, we can't
close the database in ~DatabaseBackendSync if we make DatabaseContext class
GarbageCollected.

We have a HashSet in DatabaseContext to close opening DatabaseBackendSync on
context shutdown.
 - If a DatabaseBackendSync is alive until context shutdown, DatabaseContext can
   close it as ever.

 - If a DatabaseBackendSync is garbage-collected before context shutdown, we
   need to detect it is about to finalized and need to close it. We change the
   HashSet to HeapHashMap<WeakMember<DatabaseBackendBase>,
   OwnPtr<DatabaseCloser>>.  It realizes pre-finalization hook.

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

[Mads Ager (chromium), 2014-03-27 09:28:17]

LGTM

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

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:208: for
(PersistentHeapHashMap<WeakMember<DatabaseBackendBase>, OwnPtr<DatabaseCloser>
>::iterator i = m_openSyncDatabases.begin(); i != m_openSyncDatabases.end();
++i) {
Is the iteration needed here? If you just clear the map that should call the
destructors on all the values and that should close the databases. So I think we
just need m_openSyncDatabases.clear() here?

[haraken, 2014-03-27 09:30:12]

Let me ask a couple of questions to help me understand.

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

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:193:
m_database.closeImmediately();

Just to confirm: ~DatabaseCloser() is called in the finalization phase, not in
the weak callback. Thus it is OK for closeImmediately() to allocate on-heap
objects. Right?

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:193:
m_database.closeImmediately();

Just help me understand: Why is it OK to touch m_database in ~DatabaseCloser()?

I think it's safe because m_database is in a persistent hash map, but I don't
fully understand how it's safe. I want to understand the order in which the
following things happen:

- The weak callback for DatabaseBackendBase.
- ~DatabaseBackendBase()
- ~DatabaseCloser()
- didCloseDatabase()

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:211: m_openSyncDatabases.clear();

Just to confirm: Before this CL, closeImmediately() was called in
stopSyncDatabases(). After this CL, closeImmediately() won't be called until the
next GC. Is this OK?

[Mads Ager (chromium), 2014-03-27 09:41:11]

Good point that allocation is not allowed in the weak processing. I didn't see
allocation but we should double check. Quick answers to the overall questions
below.

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

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:193:
m_database.closeImmediately();
On 2014/03/27 09:30:13, haraken wrote:
> 
> Just help me understand: Why is it OK to touch m_database in
~DatabaseCloser()?
> 
> I think it's safe because m_database is in a persistent hash map, but I don't
> fully understand how it's safe. I want to understand the order in which the
> following things happen:
> 
> - The weak callback for DatabaseBackendBase.
> - ~DatabaseBackendBase()
> - ~DatabaseCloser()
> - didCloseDatabase()

It is safe because it is run during weak processing where none of the objects
have been destructed yet.

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:193:
m_database.closeImmediately();
On 2014/03/27 09:30:13, haraken wrote:
> 
> Just to confirm: ~DatabaseCloser() is called in the finalization phase, not in
> the weak callback. Thus it is OK for closeImmediately() to allocate on-heap
> objects. Right?

It is called during weak processing so allocation is not allowed. That is a good
point. It does not look like it does allocate, but I might be missing something?
Do you see anything that will allocate in closeImmediately? That would
definitely be a problem.

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:211: m_openSyncDatabases.clear();
On 2014/03/27 09:30:13, haraken wrote:
> 
> Just to confirm: Before this CL, closeImmediately() was called in
> stopSyncDatabases(). After this CL, closeImmediately() won't be called until
the
> next GC. Is this OK?

When you clear the set, that calls the destructors on the OwnPtrs which will
close the databases. So this closes the databases right here in the same way
that it did before.

[tkent, 2014-03-27 09:41:52]

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

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:208: for
(PersistentHeapHashMap<WeakMember<DatabaseBackendBase>, OwnPtr<DatabaseCloser>
>::iterator i = m_openSyncDatabases.begin(); i != m_openSyncDatabases.end();
++i) {
On 2014/03/27 09:28:17, Mads Ager (chromium) wrote:
> Is the iteration needed here? If you just clear the map that should call the
> destructors on all the values and that should close the databases. So I think
we
> just need m_openSyncDatabases.clear() here?

I thought HeapHashMap used HeapAllocator, so deallocateTable() in HashTable.h,
which is called in HashMap::clear(), did nothing.  We need to kick
~DatabaseCloser immediately.

[tkent, 2014-03-27 09:44:48]

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

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:193:
m_database.closeImmediately();
On 2014/03/27 09:41:11, Mads Ager (chromium) wrote:
> On 2014/03/27 09:30:13, haraken wrote:
> > 
> > Just to confirm: ~DatabaseCloser() is called in the finalization phase, not
in
> > the weak callback. Thus it is OK for closeImmediately() to allocate on-heap
> > objects. Right?
> 
> It is called during weak processing so allocation is not allowed. That is a
good
> point. It does not look like it does allocate, but I might be missing
something?
> Do you see anything that will allocate in closeImmediately? That would
> definitely be a problem.

closeImmediately won't allocate from oilpan heap AFAIK.

[Mads Ager (chromium), 2014-03-27 09:45:42]

On 2014/03/27 09:41:52, tkent wrote:
>
https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
> File Source/modules/webdatabase/DatabaseContext.cpp (right):
> 
>
https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
> Source/modules/webdatabase/DatabaseContext.cpp:208: for
> (PersistentHeapHashMap<WeakMember<DatabaseBackendBase>, OwnPtr<DatabaseCloser>
> >::iterator i = m_openSyncDatabases.begin(); i != m_openSyncDatabases.end();
> ++i) {
> On 2014/03/27 09:28:17, Mads Ager (chromium) wrote:
> > Is the iteration needed here? If you just clear the map that should call the
> > destructors on all the values and that should close the databases. So I
think
> we
> > just need m_openSyncDatabases.clear() here?
> 
> I thought HeapHashMap used HeapAllocator, so deallocateTable() in HashTable.h,
> which is called in HashMap::clear(), did nothing.  We need to kick
> ~DatabaseCloser immediately.

All HashMaps call the destructor of value objects when calling clear. The bug
that Erik mentioned at our sync meeting today is that when a HeapHashMap dies
with elements in it the destructors are not called. When things are explicitly
removed or the set is cleared, destructors are called.

[Mads Ager (chromium), 2014-03-27 09:59:27]

On 2014/03/27 09:45:42, Mads Ager (chromium) wrote:
> On 2014/03/27 09:41:52, tkent wrote:
> >
>
https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
> > File Source/modules/webdatabase/DatabaseContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
> > Source/modules/webdatabase/DatabaseContext.cpp:208: for
> > (PersistentHeapHashMap<WeakMember<DatabaseBackendBase>,
OwnPtr<DatabaseCloser>
> > >::iterator i = m_openSyncDatabases.begin(); i != m_openSyncDatabases.end();
> > ++i) {
> > On 2014/03/27 09:28:17, Mads Ager (chromium) wrote:
> > > Is the iteration needed here? If you just clear the map that should call
the
> > > destructors on all the values and that should close the databases. So I
> think
> > we
> > > just need m_openSyncDatabases.clear() here?
> > 
> > I thought HeapHashMap used HeapAllocator, so deallocateTable() in
HashTable.h,
> > which is called in HashMap::clear(), did nothing.  We need to kick
> > ~DatabaseCloser immediately.
> 
> All HashMaps call the destructor of value objects when calling clear. The bug
> that Erik mentioned at our sync meeting today is that when a HeapHashMap dies
> with elements in it the destructors are not called. When things are explicitly
> removed or the set is cleared, destructors are called.

And I'm wrong... :(

This is Erik's bug as well. So you do need this.

[Mads Ager (chromium), 2014-03-27 10:01:05]

On 2014/03/27 09:59:27, Mads Ager (chromium) wrote:
> On 2014/03/27 09:45:42, Mads Ager (chromium) wrote:
> > On 2014/03/27 09:41:52, tkent wrote:
> > >
> >
>
https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
> > > File Source/modules/webdatabase/DatabaseContext.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
> > > Source/modules/webdatabase/DatabaseContext.cpp:208: for
> > > (PersistentHeapHashMap<WeakMember<DatabaseBackendBase>,
> OwnPtr<DatabaseCloser>
> > > >::iterator i = m_openSyncDatabases.begin(); i !=
m_openSyncDatabases.end();
> > > ++i) {
> > > On 2014/03/27 09:28:17, Mads Ager (chromium) wrote:
> > > > Is the iteration needed here? If you just clear the map that should call
> the
> > > > destructors on all the values and that should close the databases. So I
> > think
> > > we
> > > > just need m_openSyncDatabases.clear() here?
> > > 
> > > I thought HeapHashMap used HeapAllocator, so deallocateTable() in
> HashTable.h,
> > > which is called in HashMap::clear(), did nothing.  We need to kick
> > > ~DatabaseCloser immediately.
> > 
> > All HashMaps call the destructor of value objects when calling clear. The
bug
> > that Erik mentioned at our sync meeting today is that when a HeapHashMap
dies
> > with elements in it the destructors are not called. When things are
explicitly
> > removed or the set is cleared, destructors are called.
> 
> And I'm wrong... :(
> 
> This is Erik's bug as well. So you do need this.

class F {
public:
    F() {}
    ~F() { RELEASE_ASSERT(false); }
};

TEST(HeapTest, Flaf)
{
    HeapHashMap<F*, OwnPtr<F> > map;
    F* f = new F();
    map.add(f, adoptPtr(f));
    map.clear();
}

Should crash but doesn't. Could you add a FIXME: Oilpan: that we should be able
to remove this code and just use clear.

[tkent, 2014-03-27 10:31:02]

On 2014/03/27 10:01:05, Mads Ager (chromium) wrote:
> Should crash but doesn't. Could you add a FIXME: Oilpan: that we should be
able
> to remove this code and just use clear.

ok, I have added a FIXME comment.

[tkent, 2014-03-27 10:31:07]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-27 10:31:18]

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

[haraken, 2014-03-27 10:45:37]

LGTM

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

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:193:
m_database.closeImmediately();
On 2014/03/27 09:44:48, tkent wrote:
> On 2014/03/27 09:41:11, Mads Ager (chromium) wrote:
> > On 2014/03/27 09:30:13, haraken wrote:
> > > 
> > > Just to confirm: ~DatabaseCloser() is called in the finalization phase,
not
> in
> > > the weak callback. Thus it is OK for closeImmediately() to allocate
on-heap
> > > objects. Right?
> > 
> > It is called during weak processing so allocation is not allowed. That is a
> good
> > point. It does not look like it does allocate, but I might be missing
> something?
> > Do you see anything that will allocate in closeImmediately? That would
> > definitely be a problem.
> 
> closeImmediately won't allocate from oilpan heap AFAIK.

Yeah, I don't think closeImmediately allocates. Since closeImmediately is
complicated, I just wanted to make sure.

Things happen in the following order, right?

(1) The last reference to DatabaseBackendBase is gone.
(2) A GC is triggered.
(3) A weak processing for the DatabaseBackendBase starts.
(4) ~DatabaseCloser() is called.
(5) closeImmediately() is called.
(6) didCloseDatabase() is called.
(7) The weak processing is done.
(8) The GC finishes.
(9) A finalization phase starts.
(10) ~DatabaseBackendBase() is called.

This makes sense to me.

[tkent, 2014-03-27 10:54:27]

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

https://codereview.chromium.org/214163004/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseContext.cpp:193:
m_database.closeImmediately();
On 2014/03/27 10:45:37, haraken wrote:
> Things happen in the following order, right?
> 
> (1) The last reference to DatabaseBackendBase is gone.
> (2) A GC is triggered.
> (3) A weak processing for the DatabaseBackendBase starts.
> (4) ~DatabaseCloser() is called.
> (5) closeImmediately() is called.
> (6) didCloseDatabase() is called.
> (7) The weak processing is done.
> (8) The GC finishes.
> (9) A finalization phase starts.
> (10) ~DatabaseBackendBase() is called.

That's right.

[commit-bot: I haz the power, 2014-03-27 10:57:22]

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

[commit-bot: I haz the power, 2014-03-27 10:57:23]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[tkent, 2014-03-27 11:02:23]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-27 11:02:31]

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

[commit-bot: I haz the power, 2014-03-27 11:08:32]

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

[commit-bot: I haz the power, 2014-03-27 11:08:33]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[tkent, 2014-03-27 11:14:19]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-27 11:14:31]

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

[wibling-chromium, 2014-03-27 11:29:56]

lgtm

[commit-bot: I haz the power, 2014-03-27 11:31:45]

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

[commit-bot: I haz the power, 2014-03-27 11:31:46]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[tkent, 2014-03-27 11:46:31]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-27 11:46:36]

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

[commit-bot: I haz the power, 2014-03-27 12:57:55]

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

[commit-bot: I haz the power, 2014-03-27 12:57:56]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[tkent, 2014-03-27 13:00:07]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-27 13:00:14]

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

[commit-bot: I haz the power, 2014-03-27 14:07:11]

Change committed as 170180