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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(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...