[webkit-reviews] review denied: [Bug 42843] Interrupt all DB operations when a worker is shutting down : [Attachment 62596] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 42843: Interrupt all DB operations when a worker is shutting down
https://bugs.webkit.org/show_bug.cgi?id=42843

Attachment 62596: patch
https://bugs.webkit.org/attachment.cgi?id=62596&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 64062)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,35 @@
> +2010-07-26  Dumitru Daniliuc  <dumi at chromium.org>
> +
> +	   Interrupt all DB operations when a worker is terminated.
> +	   https://bugs.webkit.org/show_bug.cgi?id=42843
> +
> +	   Tests: fast/workers/storage/interrupt-database-sync.html
> +		  fast/workers/storage/interrupt-database.html
> +
> +	   * platform/sql/SQLiteDatabase.cpp:
> +	   (WebCore::SQLiteDatabase::SQLiteDatabase):
> +	   (WebCore::SQLiteDatabase::interrupt):
> +	   * platform/sql/SQLiteDatabase.h:
> +	   (WebCore::SQLiteDatabase::isInterrupted):
> +	   (WebCore::SQLiteDatabase::databaseMutex):
> +	   * platform/sql/SQLiteStatement.cpp:
> +	   (WebCore::SQLiteStatement::prepare):
> +	   (WebCore::SQLiteStatement::step):
> +	   * storage/AbstractDatabase.cpp:
> +	   (WebCore::AbstractDatabase::interrupt):
> +	   * storage/AbstractDatabase.h:
> +	   * storage/DatabaseTracker.cpp:
> +	   (WebCore::DatabaseTracker::interruptAllDatabasesForContext):
> +	   * storage/DatabaseTracker.h:
> +	   * storage/SQLStatement.cpp:
> +	   (WebCore::SQLStatement::execute):
> +	   * storage/SQLStatementSync.cpp:
> +	   (WebCore::SQLStatementSync::execute):
> +	   * storage/chromium/DatabaseTrackerChromium.cpp:
> +	   (WebCore::DatabaseTracker::interruptAllDatabasesForContext):
> +	   * workers/WorkerThread.cpp:
> +	   (WebCore::WorkerThread::stop):

In general, it is nice to have short comments here explaining what was done in
each place (see any of Darin Adler's commits for good examples).

> Index: WebCore/platform/sql/SQLiteDatabase.cpp
> +void SQLiteDatabase::interrupt()
> +{
> +    if (!m_db)
> +	   return;
> +
> +    while (!m_lockingMutex.tryLock())
> +	   sqlite3_interrupt(m_db);

How does this avoid doing the interrupt while with a database connection that
is closed on might be closed before this call returns. According to
http://www.sqlite.org/c3ref/interrupt.html, that is a dangerous thing to do.

> Index: WebCore/platform/sql/SQLiteDatabase.h
> +    bool isInterrupted();

const?

> Index: WebCore/platform/sql/SQLiteStatement.cpp
> @@ -61,6 +61,11 @@ SQLiteStatement::~SQLiteStatement()
>  int SQLiteStatement::prepare()
>  {
>      ASSERT(!m_isPrepared);
> +
> +    MutexLocker databaseLock(m_database.databaseMutex());
> +    if (m_database.isInterrupted())
> +	   return SQLITE_INTERRUPT;

For the naive folks (me), why do you need to take the lock in prepare() and
step() but no other methods?

> Index: WebCore/storage/DatabaseTracker.cpp
> +		       for (DatabaseSet::const_iterator dbSetIt =
databaseSet->begin(); dbSetIt != dbSetEndIt; ++dbSetIt)

Style nit: need braces on this "for"

> +			   if ((*dbSetIt)->scriptExecutionContext() == context)

> +			       openDatabases.append(*dbSetIt);


> Index: WebCore/storage/chromium/DatabaseTrackerChromium.cpp
> ===================================================================
> --- WebCore/storage/chromium/DatabaseTrackerChromium.cpp	(revision
64062)
> +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp	(working copy)
> @@ -172,4 +172,30 @@ unsigned long long DatabaseTracker::getM
>      return databaseSize + spaceAvailable;
>  }
>  
> +void DatabaseTracker::interruptAllDatabasesForContext(const
ScriptExecutionContext* context)
> +{
> +    Vector<RefPtr<AbstractDatabase> > openDatabases;
> +    {
> +	   MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard);
> +
> +	   if (m_openDatabaseMap) {

You could just return here.

> +	       DatabaseNameMap* nameMap =
m_openDatabaseMap->get(context->securityOrigin());
> +	       if (nameMap) {

You could just return here.

> +		   DatabaseNameMap::const_iterator dbNameMapEndIt =
nameMap->end();
> +		   for (DatabaseNameMap::const_iterator dbNameMapIt =
nameMap->begin(); dbNameMapIt != dbNameMapEndIt; ++dbNameMapIt) {
> +		       DatabaseSet* databaseSet = dbNameMapIt->second;
> +		       DatabaseSet::const_iterator dbSetEndIt =
databaseSet->end();
> +		       for (DatabaseSet::const_iterator dbSetIt =
databaseSet->begin(); dbSetIt != dbSetEndIt; ++dbSetIt)

Needs braces as before.

> +			   if ((*dbSetIt)->scriptExecutionContext() == context)

> +			       openDatabases.append(*dbSetIt);
> +		   }
> +	       }
> +	   }
> +    }
> +
> +    Vector<RefPtr<AbstractDatabase> >::const_iterator openDatabasesEndIt =
openDatabases.end();
> +    for (Vector<RefPtr<AbstractDatabase> >::const_iterator openDatabasesIt =
openDatabases.begin(); openDatabasesIt != openDatabasesEndIt;
++openDatabasesIt)
> +	   (*openDatabasesIt)->interrupt();

Why are there two copies of this code?

Given the small set of member variables used, it seems like it could easily be
a shared function if nothing else.

> Index: WebCore/workers/WorkerThread.cpp

> +#if ENABLE(DATABASE)
>  #include "DatabaseTask.h"
> +#include "DatabaseTracker.h"
> +#endif

Ideally these #if includes should be placed after all other non-if'ed includes.



> Index: LayoutTests/fast/workers/storage/interrupt-database-sync.html
> +function runTest()
> +{
> +    if (window.layoutTestController) {
> +	   layoutTestController.dumpAsText();
> +	   layoutTestController.waitUntilDone();
> +    }
> +
> +    worker = new Worker('resources/interrupt-database-sync.js');
> +    worker.onmessage = function(event) {
> +	   if (event.data == "done")
> +	       finishTest();
> +	   else
> +	       log(event.data);
> +    };
> +
> +    setTimeout('terminateWorker()', 500);

Is it possible to avoid doing this as a timeout? The timeout has two downsides:

1. It may make the test take longer than necessary.
2. It may make the test flaky if somehow things take longer than expected on a
machine. (For example if running the test under valgrind.)

Here's some possible alternative. 
1. Post a message from the worker after the db transactions have happened at
least once. Then do the terminate when you get this message back.
2. Put a specific value in the the db in the worker. Wait for that value to
appear before doing the terminate.

Ditto for the other test that uses a timeout for the same type of thing.