DatabaseBackend should not post a task to an already terminated database thread

DatabaseBackend should not post a task to an already terminated database thread

Currently some methods of DatabaseBackend post a task to a database thread without checking DatabaseThread::terminationRequested(). Consequently, the task can be posted to an already terminated database thread.

This is problematic but by accident isn't currently causing any real issue (for some reason). This begins to cause an issue when I destruct DatabaseThread::m_thread promptly after the database thread processed a cleanup task (See https://codereview.chromium.org/589363002).

This CL adds the necessary DatabaseThread::terminationRequested()s before DatabaseBackend posts a task to the database thread.

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

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

[haraken, 2014-09-24 01:04:28]

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

[haraken, 2014-09-24 01:04:34]

PTAL

[haraken, 2014-09-24 01:06:28]

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

https://codereview.chromium.org/596083005/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseThread.cpp:155:
ASSERT(!terminationRequested());

This is a stronger (more straightforward) way to verify that the database thread
is not yet terminated at this point.

[tkent, 2014-09-24 02:14:04]

lgtm

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

https://codereview.chromium.org/596083005/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseBackend.cpp:263: if
(!databaseContext()->databaseThread() ||
databaseContext()->databaseThread()->terminationRequested())
We should have DatabaseContext::databaseThreadIsAvailable() or something.

[haraken, 2014-09-24 02:23:18]

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

https://codereview.chromium.org/596083005/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/DatabaseBackend.cpp:263: if
(!databaseContext()->databaseThread() ||
databaseContext()->databaseThread()->terminationRequested())
On 2014/09/24 02:14:04, tkent (high review load) wrote:
> We should have DatabaseContext::databaseThreadIsAvailable() or something.

Done.

[haraken, 2014-09-24 02:23:36]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-24 02:25:01]

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

[commit-bot: I haz the power, 2014-09-24 03:37:04]

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

[commit-bot: I haz the power, 2014-09-24 03:37:05]

Failed to apply patch for Source/modules/webdatabase/DatabaseBackend.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/modules/webdatabase/DatabaseBackend.cpp
  |diff --git a/Source/modules/webdatabase/DatabaseBackend.cpp
b/Source/modules/webdatabase/DatabaseBackend.cpp
  |index
e90628b9a6cf9072cf94b336d2857175e0b1ea31..17988706cc1a89a26b726df25e77a75b720f1c90
100644
  |--- a/Source/modules/webdatabase/DatabaseBackend.cpp
  |+++ b/Source/modules/webdatabase/DatabaseBackend.cpp
  --------------------------
  No file to patch.  Skipping patch.
  5 out of 5 hunks ignored

Patch:       Source/modules/webdatabase/DatabaseBackend.cpp
Index: Source/modules/webdatabase/DatabaseBackend.cpp
diff --git a/Source/modules/webdatabase/DatabaseBackend.cpp
b/Source/modules/webdatabase/DatabaseBackend.cpp
index
e90628b9a6cf9072cf94b336d2857175e0b1ea31..17988706cc1a89a26b726df25e77a75b720f1c90
100644
--- a/Source/modules/webdatabase/DatabaseBackend.cpp
+++ b/Source/modules/webdatabase/DatabaseBackend.cpp
@@ -260,7 +260,7 @@ void DatabaseBackend::trace(Visitor* visitor)
 bool DatabaseBackend::openAndVerifyVersion(bool setVersionInNewDatabase,
DatabaseError& error, String& errorMessage)
 {
     TaskSynchronizer synchronizer;
-    if (!databaseContext()->databaseThread() ||
databaseContext()->databaseThread()->terminationRequested(&synchronizer))
+    if (!databaseContext()->databaseThreadAvailable())
         return false;
 
     DatabaseTracker::tracker().prepareToOpenDatabase(this);
@@ -331,7 +331,7 @@ void DatabaseBackend::scheduleTransaction()
     if (m_isTransactionQueueEnabled && !m_transactionQueue.isEmpty())
         transaction = m_transactionQueue.takeFirst();
 
-    if (transaction && databaseContext()->databaseThread()) {
+    if (transaction && databaseContext()->databaseThreadAvailable()) {
         OwnPtr<DatabaseTransactionTask> task =
DatabaseTransactionTask::create(transaction);
         WTF_LOG(StorageAPI, "Scheduling DatabaseTransactionTask %p for
transaction %p\n", task.get(), task->transaction());
         m_transactionInProgress = true;
@@ -342,7 +342,7 @@ void DatabaseBackend::scheduleTransaction()
 
 void DatabaseBackend::scheduleTransactionStep(SQLTransactionBackend*
transaction)
 {
-    if (!databaseContext()->databaseThread())
+    if (!databaseContext()->databaseThreadAvailable())
         return;
 
     OwnPtr<DatabaseTransactionTask> task =
DatabaseTransactionTask::create(transaction);
@@ -783,10 +783,9 @@ ExecutionContext* DatabaseBackend::executionContext() const
 void DatabaseBackend::closeImmediately()
 {
     ASSERT(executionContext()->isContextThread());
-    DatabaseThread* databaseThread = databaseContext()->databaseThread();
-    if (databaseThread && !databaseThread->terminationRequested() && opened())
{
+    if (databaseContext()->databaseThreadAvailable() && opened()) {
         logErrorMessage("forcibly closing database");
-        databaseThread->scheduleTask(DatabaseCloseTask::create(this, 0));
+       
databaseContext()->databaseThread()->scheduleTask(DatabaseCloseTask::create(this,
0));
     }
 }
 
@@ -913,7 +912,7 @@ Vector<String> DatabaseBackend::tableNames()
     // this may not be true anymore.
     Vector<String> result;
     TaskSynchronizer synchronizer;
-    if (!databaseContext()->databaseThread() ||
databaseContext()->databaseThread()->terminationRequested(&synchronizer))
+    if (!databaseContext()->databaseThreadAvailable())
         return result;
 
     OwnPtr<DatabaseTableNamesTask> task = DatabaseTableNamesTask::create(this,
&synchronizer, result);

[haraken, 2014-09-24 03:47:06]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-24 03:48:08]

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

[commit-bot: I haz the power, 2014-09-24 03:48:15]

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

[commit-bot: I haz the power, 2014-09-24 03:48:16]

Failed to apply patch for Source/modules/webdatabase/DatabaseBackend.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/modules/webdatabase/DatabaseBackend.cpp
  |diff --git a/Source/modules/webdatabase/DatabaseBackend.cpp
b/Source/modules/webdatabase/DatabaseBackend.cpp
  |index
ff6ace848514478212a88894ccaabb453333aafd..73bfd2d630c6054deda1a0326230b10c7ac59ea7
100644
  |--- a/Source/modules/webdatabase/DatabaseBackend.cpp
  |+++ b/Source/modules/webdatabase/DatabaseBackend.cpp
  --------------------------
  No file to patch.  Skipping patch.
  5 out of 5 hunks ignored

Patch:       Source/modules/webdatabase/DatabaseBackend.cpp
Index: Source/modules/webdatabase/DatabaseBackend.cpp
diff --git a/Source/modules/webdatabase/DatabaseBackend.cpp
b/Source/modules/webdatabase/DatabaseBackend.cpp
index
ff6ace848514478212a88894ccaabb453333aafd..73bfd2d630c6054deda1a0326230b10c7ac59ea7
100644
--- a/Source/modules/webdatabase/DatabaseBackend.cpp
+++ b/Source/modules/webdatabase/DatabaseBackend.cpp
@@ -259,7 +259,7 @@ void DatabaseBackend::trace(Visitor* visitor)
 bool DatabaseBackend::openAndVerifyVersion(bool setVersionInNewDatabase,
DatabaseError& error, String& errorMessage)
 {
     TaskSynchronizer synchronizer;
-    if (!databaseContext()->databaseThread() ||
databaseContext()->databaseThread()->terminationRequested(&synchronizer))
+    if (!databaseContext()->databaseThreadAvailable())
         return false;
 
     DatabaseTracker::tracker().prepareToOpenDatabase(this);
@@ -330,7 +330,7 @@ void DatabaseBackend::scheduleTransaction()
     if (m_isTransactionQueueEnabled && !m_transactionQueue.isEmpty())
         transaction = m_transactionQueue.takeFirst();
 
-    if (transaction && databaseContext()->databaseThread()) {
+    if (transaction && databaseContext()->databaseThreadAvailable()) {
         OwnPtr<DatabaseTransactionTask> task =
DatabaseTransactionTask::create(transaction);
         WTF_LOG(StorageAPI, "Scheduling DatabaseTransactionTask %p for
transaction %p\n", task.get(), task->transaction());
         m_transactionInProgress = true;
@@ -341,7 +341,7 @@ void DatabaseBackend::scheduleTransaction()
 
 void DatabaseBackend::scheduleTransactionStep(SQLTransactionBackend*
transaction)
 {
-    if (!databaseContext()->databaseThread())
+    if (!databaseContext()->databaseThreadAvailable())
         return;
 
     OwnPtr<DatabaseTransactionTask> task =
DatabaseTransactionTask::create(transaction);
@@ -782,10 +782,9 @@ ExecutionContext* DatabaseBackend::executionContext() const
 void DatabaseBackend::closeImmediately()
 {
     ASSERT(executionContext()->isContextThread());
-    DatabaseThread* databaseThread = databaseContext()->databaseThread();
-    if (databaseThread && !databaseThread->terminationRequested() && opened())
{
+    if (databaseContext()->databaseThreadAvailable() && opened()) {
         logErrorMessage("forcibly closing database");
-        databaseThread->scheduleTask(DatabaseCloseTask::create(this, 0));
+       
databaseContext()->databaseThread()->scheduleTask(DatabaseCloseTask::create(this,
0));
     }
 }
 
@@ -912,7 +911,7 @@ Vector<String> DatabaseBackend::tableNames()
     // this may not be true anymore.
     Vector<String> result;
     TaskSynchronizer synchronizer;
-    if (!databaseContext()->databaseThread() ||
databaseContext()->databaseThread()->terminationRequested(&synchronizer))
+    if (!databaseContext()->databaseThreadAvailable())
         return result;
 
     OwnPtr<DatabaseTableNamesTask> task = DatabaseTableNamesTask::create(this,
&synchronizer, result);

[haraken, 2014-09-24 03:55:20]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-24 03:56:11]

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

[commit-bot: I haz the power, 2014-09-24 05:03:40]

Committed patchset #4 (id:60001) as 182552

[Mads Ager (chromium), 2014-09-24 06:42:15]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/593423002/ by ager@chromium.org.

The reason for reverting is: This causes a lot of database layout test failures
on the buildbots. Reverting..

[haraken, 2014-09-24 10:42:40]

On 2014/09/24 06:42:15, Mads Ager (chromium) wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/593423002/ by mailto:ager@chromium.org.
> 
> The reason for reverting is: This causes a lot of database layout test
failures
> on the buildbots. Reverting..

Hmm, I cannot reproduce the failures locally. The try bots look fine as well
(See the patch set 5).

As far as I see the log of the failures on the build bots, it looks like
window.openDatabase is returning null for some reason. I have no idea why
window.openDatabase returns null...


http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=Ch...

[haraken, 2014-09-24 10:51:57]

Mads: Let me reland this (with a small fix). Feel free to revert this if this
causes the same failures on the build bots.

https://codereview.chromium.org/596083005/diff/80001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseContext.cpp (right):

https://codereview.chromium.org/596083005/diff/80001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseContext.cpp:170: return m_databaseThread &&
!m_hasRequestedTermination;

The only suspicious change is here. In order to preserve the current behavior,
we need to use databaseThread() instead of m_databaseThread.

Let me change this to databaseThread(), reland the CL and see how it goes.

[haraken, 2014-09-24 10:52:01]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-24 10:52:59]

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

[Mads Ager (chromium), 2014-09-24 10:53:33]

On 2014/09/24 10:51:57, haraken wrote:
> Mads: Let me reland this (with a small fix). Feel free to revert this if this
> causes the same failures on the build bots.
> 
>
https://codereview.chromium.org/596083005/diff/80001/Source/modules/webdataba...
> File Source/modules/webdatabase/DatabaseContext.cpp (right):
> 
>
https://codereview.chromium.org/596083005/diff/80001/Source/modules/webdataba...
> Source/modules/webdatabase/DatabaseContext.cpp:170: return m_databaseThread &&
> !m_hasRequestedTermination;
> 
> The only suspicious change is here. In order to preserve the current behavior,
> we need to use databaseThread() instead of m_databaseThread.
> 
> Let me change this to databaseThread(), reland the CL and see how it goes.

Please wait, haraken. I'm just getting the tree in a state where I can make a
roll. :-\

[Mads Ager (chromium), 2014-09-24 10:53:42]

The CQ bit was unchecked by ager@chromium.org

[Mads Ager (chromium), 2014-09-24 11:45:04]

On 2014/09/24 10:53:33, Mads Ager (chromium) wrote:
> On 2014/09/24 10:51:57, haraken wrote:
> > Mads: Let me reland this (with a small fix). Feel free to revert this if
this
> > causes the same failures on the build bots.
> > 
> >
>
https://codereview.chromium.org/596083005/diff/80001/Source/modules/webdataba...
> > File Source/modules/webdatabase/DatabaseContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/596083005/diff/80001/Source/modules/webdataba...
> > Source/modules/webdatabase/DatabaseContext.cpp:170: return m_databaseThread
&&
> > !m_hasRequestedTermination;
> > 
> > The only suspicious change is here. In order to preserve the current
behavior,
> > we need to use databaseThread() instead of m_databaseThread.
> > 
> > Let me change this to databaseThread(), reland the CL and see how it goes.
> 
> Please wait, haraken. I'm just getting the tree in a state where I can make a
> roll. :-\

Let's give this a try. Checking the commit bit again...

[Mads Ager (chromium), 2014-09-24 11:45:09]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-09-24 11:45:42]

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

[commit-bot: I haz the power, 2014-09-24 12:00:58]

Committed patchset #6 (id:100001) as 182593