DatabaseBackend objects should be destructed in the originator thread

DatabaseBackend objects should be destructed in the originator thread.

Before this CL, it was possible that DatabaseThread held the last
references of DatabaseBackend objects and it was released in the
database thread.
We must destruct objects in the originator thread in Oilpan.

Implementation:
DatabaseThread::recordDatabaseClosed doesn't remove a RefPtr<
DatabaseBackend>, and we release RefPtr<DatabaseBackend> objects
in ~DatabaseThread, which is executed in the main/worker thread.
Note that there are no ways to close a database explicitly, and
DatabaseBackend objects are never destructed before their
DatabaseContext is stopped. DatabaseThread::recordDatabaseClosed
is actually unnecessary.

DatabaseThread doesn't know DatabaseBackend open status any longer.
We remove DatabaseThread::isDatabaseOpen, and its existing
callsites use DatabaseBackend::open() instead.


BUG=347902
TEST=none; no visible behavior changes hopefully.
R=michaeln@chromium.org

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

[michaeln, 2014-03-03 19:42:33]

I'm not sure this accomplishes the behavior you want, what about
cleanupDatabaseThread()? Also, I'd like to point out that the inability to close
a database prior to page shutdown is more of a bug than a feature.

[michaeln, 2014-03-03 19:43:43]

also, cc'ing jochen is is pretty familiar with this too

[tkent, 2014-03-04 01:03:49]

On 2014/03/03 19:42:33, michaeln wrote:
> I'm not sure this accomplishes the behavior you want, what about
> cleanupDatabaseThread()?

The purpose of this CL is exactly to avoid DatabaseBackend destruction in
cleanupDatabaseThread. (*it).get()->close() in cleanupDatabaseThread calls
DatabaseThread::recordDatabaseClosed, which can remove the last reference of a
DatabaseBackend.

> Also, I'd like to point out that the inability to close
> a database prior to page shutdown is more of a bug than a feature.

I understand your feeling about this. If we fix this, we should do
 1. Do not hold any RefPtrs to Database{,Backend} in C++ code. Only JavaScript
wrappers should hold it.
 2. ~DatabaseBackend needs to close itself in the database thread, but it should
be destructed in the context thread.
  This smells of bad complexity.

I'd like to avoid to consume a lot of time to improve Web SQL Database, which is
an obsolete feature.

[michaeln, 2014-03-04 01:16:02]

On 2014/03/04 01:03:49, tkent wrote:
> On 2014/03/03 19:42:33, michaeln wrote:
> > I'm not sure this accomplishes the behavior you want, what about
> > cleanupDatabaseThread()?
> 
> The purpose of this CL is exactly to avoid DatabaseBackend destruction in
> cleanupDatabaseThread. (*it).get()->close() in cleanupDatabaseThread calls
> DatabaseThread::recordDatabaseClosed, which can remove the last reference of a
> DatabaseBackend.

Can you explain to me how this prevents DatabaseBackend destruction in
cleanupDatabaseThread? Please see the local var openSetCopy in
cleanupDatabaseThread. When that goes out of scope won't that release the last
ref> During cleanup, recordDataseClosed sees an empty collection in
m_openDatabaseSet. Am I missing something?

>   This smells of bad complexity.

Yes.

> I'd like to avoid to consume a lot of time to improve Web SQL Database, which
is
> an obsolete feature.

And Yes.

[tkent, 2014-03-04 01:19:23]

> Can you explain to me how this prevents DatabaseBackend destruction in
> cleanupDatabaseThread? Please see the local var openSetCopy in
> cleanupDatabaseThread. When that goes out of scope won't that release the last
> ref> During cleanup, recordDataseClosed sees an empty collection in
> m_openDatabaseSet. Am I missing something?

Oh, I overlooked DatabaseSet::swap. I need to remove it.

[tkent, 2014-03-04 01:29:22]

On 2014/03/04 01:19:23, tkent wrote:
> > Can you explain to me how this prevents DatabaseBackend destruction in
> > cleanupDatabaseThread? Please see the local var openSetCopy in
> > cleanupDatabaseThread. When that goes out of scope won't that release the
last
> > ref> During cleanup, recordDataseClosed sees an empty collection in
> > m_openDatabaseSet. Am I missing something?
> 
> Oh, I overlooked DatabaseSet::swap. I need to remove it.

Done. I have updated the patch.

[michaeln, 2014-03-04 01:42:52]

Ok, now the patch makes more sense to me :)

lgtm pending green'ness

[tkent, 2014-03-04 01:46:31]

Thanks!

[tkent, 2014-03-04 03:09:45]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-04 03:10:29]

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

[tkent, 2014-03-04 05:26:42]

Committed patchset #2 manually as r168362 (presubmit successful).

[tkent, 2014-03-05 04:58:37]

I realized this change was unnecessary. Reverting.

[tkent, 2014-03-05 04:59:03]

A revert of this CL has been created in
https://codereview.chromium.org/178103007/ by tkent@chromium.org.

The reason for reverting is: Unnecessary change
.