Bug 42843 – Interrupt all DB operations when a worker is shutting down

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

We should interrupt all DB operations as soon as worker.terminate() is called.

[Dumitru Daniliuc]

Created attachment 62337 [details]
patch

[Adam Barth]

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.

[David Levin]

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

[Adam Barth]

Comment on attachment 62337 [details]
patch

Ok.  I'm more than happy to have someone who understands the threading issues review this patch too.

[Michael Nordman]

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.

[Michael Nordman]

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

[Dumitru Daniliuc]

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.

[WebKit Review Bot]

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.

[Dumitru Daniliuc]

Created attachment 62499 [details]
patch

Same patch, fixed the style problem.

[Michael Nordman]

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?

[Dumitru Daniliuc]

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

[Dumitru Daniliuc]

Created attachment 62596 [details]
patch

[Michael Nordman]

Could you do the webkit-patch post-attachment-to-rietveld voodoo again please :)

[David Levin]

Comment on attachment 62596 [details]
patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 64062)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,35 @@
> [phone]  Dumitru Daniliuc  <[e-mail]>
> +
> +        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.

[Dumitru Daniliuc]

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

[David Levin]

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

[David Levin]

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.

[Dumitru Daniliuc]

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

[Dumitru Daniliuc]

Created attachment 62892 [details]
patch

[Michael Nordman]

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?

[Dumitru Daniliuc]

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.

[Dumitru Daniliuc]

At Ossy's recommendation, I changed qt's yield() implementation to QThread::yieldCurrentThread().

[Dumitru Daniliuc]

landed: r64313.

[Csaba Osztrogonác]

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

[Dumitru Daniliuc]

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.

[Dumitru Daniliuc]

re-landed: r64384.