Tentative fix of a crash in Oilpan GC after a database thread termination.

Tentative fix of a crash in Oilpan GC after a database thread termination.

Make sure DatabaseThread::m_openDatabaseSet has no backing store
when a database thread is terminated.
m_openDatabaseSet outlives a database thread, and its backing store
is allocated in the database thread.  So, we should unlink the
backing store before the thread termination.

* DatabaseThread::cleanupDatabaseThread
 It's possible m_openDatabaseSet.size() is zero and
 m_openDatabaseSet still has backing store in the following scenario:
  1. A web page opens a database.  m_openDatabaseSet has it.
  2. Chromium calls DatabaseTracker::closeDatabasesImmediately()
  3. It calls m_openDatabaseSet.remove().

* DatabaseThread::recordDatabaseOpen
  Don't add a Database object after termination request.
  I think this can't happen. But we change it just in case.

This CL has no tests. It's very hard to make an automated test for the
scenario.

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

[tkent, 2014-10-15 07:44:31]

Patchset #1 (id:1) has been deleted

[tkent, 2014-10-15 08:14:09]

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

[tkent, 2014-10-15 08:14:10]

Please review this.

[haraken, 2014-10-15 08:20:17]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-10-15 08:20:18]

LGTM

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

https://codereview.chromium.org/657023003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseThread.cpp:119: m_thread->postTask(new
Task(WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted, this)));

Just help me understand: Why do we need to post this task? Can't we just call
cleanupDatabaseThreadCompleted() here?

https://codereview.chromium.org/657023003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseThread.cpp:144:
m_openDatabaseSet.remove(database);

Don't you need to add a MutexLocker and check !m_terminationRequested here as
well?

[tkent, 2014-10-15 08:41:22]

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

https://codereview.chromium.org/657023003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseThread.cpp:119: m_thread->postTask(new
Task(WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted, this)));
On 2014/10/15 08:20:18, haraken wrote:
> 
> Just help me understand: Why do we need to post this task? Can't we just call
> cleanupDatabaseThreadCompleted() here?

I don't remember the reason.  Anyway, it's unrelated to this CL.

https://codereview.chromium.org/657023003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseThread.cpp:144:
m_openDatabaseSet.remove(database);
On 2014/10/15 08:20:18, haraken wrote:
> 
> Don't you need to add a MutexLocker and check !m_terminationRequested here as
> well?

No. This function is supposed to be called after a termination request.

[tkent, 2014-10-15 08:43:48]

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

https://codereview.chromium.org/657023003/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseThread.cpp:144:
m_openDatabaseSet.remove(database);
On 2014/10/15 08:41:22, tkent wrote:
> On 2014/10/15 08:20:18, haraken wrote:
> > 
> > Don't you need to add a MutexLocker and check !m_terminationRequested here
as
> > well?
> 
> No. This function is supposed to be called after a termination request.

This HeapHashSet::remove() is harmless even if m_terminationRequested is true,
and mutex lock is  not a light operation.

[tkent, 2014-10-15 08:58:29]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-10-15 08:59:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657023003/20001

[commit-bot: I haz the power, 2014-10-15 09:02:44]

Committed patchset #1 (id:20001) as 183730