Bug 42843 - Interrupt all DB operations when a worker is shutting down
Summary: Interrupt all DB operations when a worker is shutting down
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on: 43233
Blocks: 34990
  Show dependency treegraph
 
Reported: 2010-07-22 13:14 PDT by Dumitru Daniliuc
Modified: 2010-07-30 16:20 PDT (History)
6 users (show)

See Also:


Attachments
patch (31.70 KB, patch)
2010-07-22 14:18 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (18.75 KB, patch)
2010-07-24 03:52 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (18.80 KB, patch)
2010-07-24 04:14 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (18.99 KB, patch)
2010-07-26 12:53 PDT, Dumitru Daniliuc
levin: review-
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (33.22 KB, patch)
2010-07-28 02:15 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (34.33 KB, patch)
2010-07-28 16:51 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (34.73 KB, patch)
2010-07-28 19:26 PDT, Dumitru Daniliuc
levin: review+
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (28.93 KB, patch)
2010-07-30 15:04 PDT, Dumitru Daniliuc
levin: review+
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2010-07-22 13:14:40 PDT
We should interrupt all DB operations as soon as worker.terminate() is called.
Comment 1 Dumitru Daniliuc 2010-07-22 14:18:50 PDT
Created attachment 62337 [details]
patch
Comment 2 Adam Barth 2010-07-22 21:24:37 PDT
Comment on attachment 62337 [details]
patch

This threading stuff is all very complicated.  I'm not sure I fully grasp the locking discipline, but this looks reasonable to me.

WebCore/storage/chromium/DatabaseTrackerChromium.cpp:181
 +  void DatabaseTracker::interruptAllDatabasesForContext(ScriptExecutionContext* context)
Sad that we have to copy/paste this code.
Comment 3 David Levin 2010-07-22 23:03:54 PDT
(In reply to comment #2)
> (From update of attachment 62337 [details])
> This threading stuff is all very complicated.  I'm not sure I fully grasp the locking discipline, but this looks reasonable to me.

Very honest but not too reassuring. Perhaps the r+ should wait until someone can take the time to understand it/Dumi can explain it to someone.

I say this because I've seen a lot of instances of messed up threading stuff once the time was taken to understand it and this isn't the type of thing that is easy caught when it is wrong. (Bad threading issues result in random corruptions which are hard to track down.)
Comment 4 Adam Barth 2010-07-22 23:07:24 PDT
Comment on attachment 62337 [details]
patch

Ok.  I'm more than happy to have someone who understands the threading issues review this patch too.
Comment 5 Michael Nordman 2010-07-23 12:51:24 PDT
Dumi asked me to take a look at this and I will, just haven't done it yet. It's real easy for me to probe him for details since he sits nearby.

To really see what's going on, I'm going to have to patch this in locally and look at the diffs with a better diff viewer.
Comment 6 Michael Nordman 2010-07-23 18:05:23 PDT
Comment on attachment 62337 [details]
patch

http://wkrietveld.appspot.com/42843/diff/1/6
File WebCore/storage/AbstractDatabase.cpp (right):

http://wkrietveld.appspot.com/42843/diff/1/6#newcode480
WebCore/storage/AbstractDatabase.cpp:480: while (!m_interruptedGuard.tryLock())
i wonder if we can find a way to do this w/o looping like this

http://wkrietveld.appspot.com/42843/diff/1/8
File WebCore/storage/Database.cpp (right):

http://wkrietveld.appspot.com/42843/diff/1/8#newcode92
WebCore/storage/Database.cpp:92: MutexLocker interruptedMutexLock(DatabaseTracker::tracker().interruptedMutex());
This lock can be held for a very long time. There are some actions below that go ranging far and wide.

See DatabaseTracker::canEstablishDatabase and in particular this comment...

    // Drop all locks before calling out; we don't know what they'll do.
    context->databaseExceededQuota(name);

And openAndVerifyVersion() requires a task to be performed on the db thread while the current thread waits for it to complete (with this lock held). Is this mutex ever held on the db thread?

http://wkrietveld.appspot.com/42843/diff/1/9
File WebCore/storage/DatabaseSync.cpp (right):

http://wkrietveld.appspot.com/42843/diff/1/9#newcode53
WebCore/storage/DatabaseSync.cpp:53: MutexLocker interruptedMutexLock(DatabaseTracker::tracker().interruptedMutex());
Same comment here about holding this lock for a mighty long time and calling out into things that are very open ended... webframeClient callbacks and javascript callbacks.

http://wkrietveld.appspot.com/42843/diff/1/9#newcode73
WebCore/storage/DatabaseSync.cpp:73: creationCallback->handleEvent(context, database.get());
For example, what if the creation callback being invoked here calls openDatabaseSync().

http://wkrietveld.appspot.com/42843/diff/1/10
File WebCore/storage/DatabaseTracker.cpp (right):

http://wkrietveld.appspot.com/42843/diff/1/10#newcode262
WebCore/storage/DatabaseTracker.cpp:262: 
It looks like the loop is interrupt all dbs for a particular origin rather than for a particular context.

http://wkrietveld.appspot.com/42843/diff/1/11
File WebCore/storage/DatabaseTracker.h (right):

http://wkrietveld.appspot.com/42843/diff/1/11#newcode69
WebCore/storage/DatabaseTracker.h:69: Mutex& interruptedMutex() { return m_openDatabaseMapGuard; }
oh... this is one of the existing locks, not a new one.

http://wkrietveld.appspot.com/42843/diff/1/17
File WebCore/workers/WorkerThread.cpp (right):

http://wkrietveld.appspot.com/42843/diff/1/17#newcode228
WebCore/workers/WorkerThread.cpp:228: DatabaseTracker::tracker().interruptAllDatabasesForContext(workerContext());
workerContext may be NULL here
Comment 7 Dumitru Daniliuc 2010-07-24 03:52:30 PDT
Created attachment 62497 [details]
patch

(In reply to comment #6)
> (From update of attachment 62337 [details])
> http://wkrietveld.appspot.com/42843/diff/1/6
> File WebCore/storage/AbstractDatabase.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/6#newcode480
> WebCore/storage/AbstractDatabase.cpp:480: while (!m_interruptedGuard.tryLock())
> i wonder if we can find a way to do this w/o looping like this

i don't think there's a way to do this without looping. i moved this code to SQLiteDatabase though.

> http://wkrietveld.appspot.com/42843/diff/1/8
> File WebCore/storage/Database.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/8#newcode92
> WebCore/storage/Database.cpp:92: MutexLocker interruptedMutexLock(DatabaseTracker::tracker().interruptedMutex());
> This lock can be held for a very long time. There are some actions below that go ranging far and wide.
> 
> See DatabaseTracker::canEstablishDatabase and in particular this comment...
> 
>     // Drop all locks before calling out; we don't know what they'll do.
>     context->databaseExceededQuota(name);
> 
> And openAndVerifyVersion() requires a task to be performed on the db thread while the current thread waits for it to complete (with this lock held). Is this mutex ever held on the db thread?
> 

reverted the changes to Database and DatabaseSync. DatabaseTracker::interruptAllDatabasesForContext() is the only new place where this lock is being held, but only for a short period of time.

> http://wkrietveld.appspot.com/42843/diff/1/9#newcode73
> WebCore/storage/DatabaseSync.cpp:73: creationCallback->handleEvent(context, database.get());
> For example, what if the creation callback being invoked here calls openDatabaseSync().

no more locks held here.

> http://wkrietveld.appspot.com/42843/diff/1/10
> File WebCore/storage/DatabaseTracker.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/10#newcode262
> WebCore/storage/DatabaseTracker.cpp:262: 
> It looks like the loop is interrupt all dbs for a particular origin rather than for a particular context.

i believe every ScriptExecutionContext has only one origin. see ScriptExecutionContext::m_securityOrigin().

> http://wkrietveld.appspot.com/42843/diff/1/11
> File WebCore/storage/DatabaseTracker.h (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/11#newcode69
> WebCore/storage/DatabaseTracker.h:69: Mutex& interruptedMutex() { return m_openDatabaseMapGuard; }
> oh... this is one of the existing locks, not a new one.

reverted this change; this method is no longer needed.

> http://wkrietveld.appspot.com/42843/diff/1/17
> File WebCore/workers/WorkerThread.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/1/17#newcode228
> WebCore/workers/WorkerThread.cpp:228: DatabaseTracker::tracker().interruptAllDatabasesForContext(workerContext());
> workerContext may be NULL here

moved inside the if-block, after the script execution is forbidden.
Comment 8 WebKit Review Bot 2010-07-24 03:55:29 PDT
Attachment 62497 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/sql/SQLiteDatabase.h:63:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Dumitru Daniliuc 2010-07-24 04:14:12 PDT
Created attachment 62499 [details]
patch

Same patch, fixed the style problem.
Comment 10 Michael Nordman 2010-07-26 12:09:29 PDT
Haven't looked at the new code yet, just responding to this comment.

>> http://wkrietveld.appspot.com/42843/diff/1/10
>> File WebCore/storage/DatabaseTracker.cpp (right):
>> 
>> http://wkrietveld.appspot.com/42843/diff/1/10#newcode262
>> WebCore/storage/DatabaseTracker.cpp:262: 
>> It looks like the loop is interrupt all dbs for a particular origin rather 
>> than for a particular context.
>
> i believe every ScriptExecutionContext has only one origin. see
> ScriptExecutionContext::m_securityOrigin().

The issue is that several SECs can refer to the same origin, and it looks like this loop will interrupt dbs in other SECs that aren't going away. Is that not the case?
Comment 11 Dumitru Daniliuc 2010-07-26 12:17:26 PDT
(In reply to comment #10)
> Haven't looked at the new code yet, just responding to this comment.
> 
> >> http://wkrietveld.appspot.com/42843/diff/1/10
> >> File WebCore/storage/DatabaseTracker.cpp (right):
> >> 
> >> http://wkrietveld.appspot.com/42843/diff/1/10#newcode262
> >> WebCore/storage/DatabaseTracker.cpp:262: 
> >> It looks like the loop is interrupt all dbs for a particular origin rather 
> >> than for a particular context.
> >
> > i believe every ScriptExecutionContext has only one origin. see
> > ScriptExecutionContext::m_securityOrigin().
> 
> The issue is that several SECs can refer to the same origin, and it looks like this loop will interrupt dbs in other SECs that aren't going away. Is that not the case?

you're right. uploading a new patch shortly.
Comment 12 Dumitru Daniliuc 2010-07-26 12:53:31 PDT
Created attachment 62596 [details]
patch
Comment 13 Michael Nordman 2010-07-26 13:35:51 PDT
Could you do the webkit-patch post-attachment-to-rietveld voodoo again please :)
Comment 14 David Levin 2010-07-26 14:51:15 PDT
Comment on attachment 62596 [details]
patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 64062)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,35 @@
> +2010-07-26  Dumitru Daniliuc  <dumi@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.
Comment 15 Dumitru Daniliuc 2010-07-28 02:15:18 PDT
Created attachment 62804 [details]
patch

(In reply to comment #14)
> (From update of attachment 62596 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> 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).

done.

> > 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.

fixed. i thought that database would be opened until the context is destroyed, but then i realized that it could be GCed by the JS engine too, which would force it to close.

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

can't make it const because of the assertion.

> > 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?

all other methods either bind a parameter, or return a value, which is fast. so we don't need to interrupt them.

> > 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);

fixed.

> > 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.

changed, in this file and in DatabaseTracker.cpp too.

> > +            DatabaseNameMap* nameMap = m_openDatabaseMap->get(context->securityOrigin());
> > +            if (nameMap) {
> 
> You could just return here.

done, in both .cpp files.

> > +                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.

fixed.

> > +    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.

DatabaseTracker.cpp is WebKit's implementation. DatabaseTrackerChromium.cpp is Chromium's implementation. we had to split them, because Chromium uses only a few methods declared in DatabaseTracker.h, while other ports use more methods, keep track of more things and include more classes. in theory, DatabaseTracker.cpp should look like DatabaseTrackerChromium.cpp, and the rest of it should go into separate files that are only included by the ports that use that functionality. but it hasn't been easy to make that happen in practice.

> > 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.

moved, wasn't sure what the rule was, so i was trying to keep them ordered alphabetically.

> > 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.

i used your first suggestion. i knew that setTimeout() is evil, but for some reason haven't spent too much time thinking about ways to not use it...
Comment 16 David Levin 2010-07-28 10:42:50 PDT
(In reply to comment #15)
> > 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.
> 
> DatabaseTracker.cpp is WebKit's implementation. DatabaseTrackerChromium.cpp is Chromium's implementation. we had to split them, because Chromium uses only a few methods declared in DatabaseTracker.h, while other ports use more methods, keep track of more things and include more classes. in theory, DatabaseTracker.cpp should look like DatabaseTrackerChromium.cpp, and the rest of it should go into separate files that are only included by the ports that use that functionality. but it hasn't been easy to make that happen in practice.

This doesn't seem to answer the question:
Why can't there be a shared function in this case (since the code is exactly the same)? Not a class member function but just a plain old function some place where both chromium's implementation and WebKit's implementation can call it?

Duplicate code is bad.
Comment 17 David Levin 2010-07-28 11:29:21 PDT
I've been thinking about m_interrupted and m_lockingMutex. 

It seems like the code could do "m_interrupted = true;" in SQLiteDatabase::interrupt() before acquiring the mutex.

I do understand why you want to get the mutex before checking its value because if SQLiteDatabase::interrupt() has exited, you don't want any more operations happening.
Comment 18 Dumitru Daniliuc 2010-07-28 15:38:09 PDT
(In reply to comment #16)
> This doesn't seem to answer the question:
> Why can't there be a shared function in this case (since the code is exactly the same)? Not a class member function but just a plain old function some place where both chromium's implementation and WebKit's implementation can call it?
> 
> Duplicate code is bad.

If we had such a function, it would have to live either in DatabaseTracker.h, or in a new file that would have only 1 function, and could easily become the dumping place for other DB-related "utility" functions. I'm not sure either option is better than having this code duplicated. Don't know if it makes it any better, but there's a bug opened to clean up DatabaseTracker (https://bugs.webkit.org/show_bug.cgi?id=31482).

(In reply to comment #17)
> I've been thinking about m_interrupted and m_lockingMutex. 
> 
> It seems like the code could do "m_interrupted = true;" in SQLiteDatabase::interrupt() before acquiring the mutex.

done.
Comment 19 Dumitru Daniliuc 2010-07-28 16:51:37 PDT
Created attachment 62892 [details]
patch
Comment 20 Michael Nordman 2010-07-28 18:22:31 PDT
Comment on attachment 62804 [details]
patch

http://wkrietveld.appspot.com/42843/diff/11001/12006
File JavaScriptCore/wtf/ThreadingWin.cpp (right):

http://wkrietveld.appspot.com/42843/diff/11001/12006#newcode271
JavaScriptCore/wtf/ThreadingWin.cpp:271: ::SwitchToThread();
Is SwitchToThread() or Sleep(0) better for our particular use case?

http://wkrietveld.appspot.com/42843/diff/11001/12018
File WebCore/storage/SQLStatement.cpp (right):

http://wkrietveld.appspot.com/42843/diff/11001/12018#newcode81
WebCore/storage/SQLStatement.cpp:81: m_error = SQLError::create(db->isInterrupted() ? SQLError::DATABASE_ERR : SQLError::SYNTAX_ERR, database->lastErrorMsg());
Would it make sense to test result for equality with SQLITE_INTERRUPT here instead of sampling isInterrupted?
Comment 21 Dumitru Daniliuc 2010-07-28 19:26:09 PDT
Created attachment 62912 [details]
patch

> http://wkrietveld.appspot.com/42843/diff/11001/12006#newcode271
> JavaScriptCore/wtf/ThreadingWin.cpp:271: ::SwitchToThread();
> Is SwitchToThread() or Sleep(0) better for our particular use case?

looks like Sleep(1) is better than both SwitchToThread() and Sleep(0).

> http://wkrietveld.appspot.com/42843/diff/11001/12018
> File WebCore/storage/SQLStatement.cpp (right):
> 
> http://wkrietveld.appspot.com/42843/diff/11001/12018#newcode81
> WebCore/storage/SQLStatement.cpp:81: m_error = SQLError::create(db->isInterrupted() ? SQLError::DATABASE_ERR : SQLError::SYNTAX_ERR, database->lastErrorMsg());
> Would it make sense to test result for equality with SQLITE_INTERRUPT here instead of sampling isInterrupted?

done.
Comment 22 Dumitru Daniliuc 2010-07-29 14:52:41 PDT
At Ossy's recommendation, I changed qt's yield() implementation to QThread::yieldCurrentThread().
Comment 23 Dumitru Daniliuc 2010-07-29 15:31:12 PDT
landed: r64313.
Comment 24 Csaba Osztrogonác 2010-07-30 02:28:49 PDT
I cc-ed Andras, could you check JavaScriptCore/wtf/qt/ThreadingQt.cpp, please ?

And reopen, because it was rolled out by http://trac.webkit.org/changeset/64334
Comment 25 Dumitru Daniliuc 2010-07-30 15:04:05 PDT
Created attachment 63108 [details]
patch

same patch, with one minor change. the ASSERT in SQLiteDatabase::close() was commented out before this patch. i thought it would be ok to uncomment it, but turns out i was wrong -- it's ok to uncomment it for WebSQLDatabases, but there's other code that doesn't close the database on the same thread that opened it. so i left that ASSERT commented out as it was before.
Comment 26 Dumitru Daniliuc 2010-07-30 16:20:02 PDT
re-landed: r64384.