[webkit-reviews] review requested: [Bug 22725] Make SQL database storage work in Workers : [Attachment 46959] Next revision, rolling in more comments. One layout test changed.

[bugzilla-daemon at webkit.org]

Eric U. <ericu at chromium.org> has asked	for review:
Bug 22725: Make SQL database storage work in Workers
https://bugs.webkit.org/show_bug.cgi?id=22725

Attachment 46959: Next revision, rolling in more comments.  One layout test
changed.
https://bugs.webkit.org/attachment.cgi?id=46959&action=review

------- Additional Comments from Eric U. <ericu at chromium.org>
> (From update of attachment 46403 [details])
> Looks good, I think this might be the last iteration! Marking r- because of
> number of nits, but they are all minor.
> 
> I'm still not sure ScriptExecutionContext::isContextThread() is technically
> necessary but I can see that its use is beneficial in Database classes where
> many methods can be routinely called on 2 different threads (context's and
> database), while some other methods in the same classes must only be used on
> one certain thread. This 'pattern' is common to most of Database code. It
makes
> it hard to assume certain sequence of construction/destruction of the classes

> since apparently due to timing of things like GC, the code can take different

> code paths resulting in calling big chunks of code on one thread or another.
> Also, since those methods call quite a few of other functions, it's hard to
see
> right away if the whole call tree is cross-thread-safe, especially
considering
> that any Database method without ASSERT for a particular thread is a suspect.

> 
> Normally, most of WebKit objects is assumed to be created, used and destroyed

> on a single thread, with exceptions coded up as very small methods, often
> modifying some shared data, guarded by a mutex, so their cross-thread nature
is
> obvious to the reader of the code. Database code is somewhat different in
this
> regard (example: Database ctor can only be called on context's thread, while
> dtor can actually be called on either).  There are some big known exceptions
> that can be used cross-thread (String) but they are far in between and
perhaps
> Database classes are in the same category.
> 
> That being said, cross-thread usage of Database classes is not introduced by
> this patch of course, so if isContextThread allows adding more ASSERTs into
> this code, it is better to have it. So let it be and let add more ASSERTs
while
> adopting Database to Workers!

Amen--I'm no happier than you about the complexity, but I'm a big fan of
ASSERTs.

> Small nits below:
> 
> > Index: WebCore/dom/Document.cpp
> 
> > +bool Document::isContextThread() const
> > +{
> > +	 return WTF::isMainThread();
> > +}
> 
> 'WTF::' prefix is normally not used with functions that exposed from WTF, see

> other usage of isMainThread() in the same file.

Fixed.

> > Index: WebCore/dom/ScriptExecutionContext.cpp
> 
> > +void ScriptExecutionContext::addOpenDatabase(Database* database)
> > +void ScriptExecutionContext::removeOpenDatabase(Database* database)
> 
> Lets add ASSERTs on isContextThread() in these, since they modify
> m_openDatabaseSet.

Done.

> > +void ScriptExecutionContext::stopDatabases(DatabaseTaskSynchronizer
*cleanupSync)
> 
> In WebKit style, there is no space between type name and '*', it should be:
> DatabaseTaskSynchronizer* cleanupSync.

Fixed.

> Also, it could use the ASSERT(isContextThread()) too.

Done.

> > Index: WebCore/dom/ScriptExecutionContext.h
> 
> > +	     DatabaseThread *databaseThread();
> 
> space before '*'. 

Fixed.

> > +	     typedef HashSet<Database *> DatabaseSet;
> 
> space before '*'. 

Fixed.

> > Index: WebCore/storage/Database.cpp
> > -PassRefPtr<Database> Database::openDatabase(Document* document, const
String& name, const String& expectedVersion, const String& displayName,
unsigned long estimatedSize, ExceptionCode& e)
> > +PassRefPtr<Database> Database::openDatabase(ScriptExecutionContext
*context, const String& name, const String& expectedVersion, const String&
displayName, unsigned long estimatedSize, ExceptionCode& e)
> 
> space before '*'. 
> 
> > +Database::Database(ScriptExecutionContext *context, const String& name,
const String& expectedVersion, const String& displayName, unsigned long
estimatedSize)
> 
> space before '*'. 

Fixed all instances in this file.

> > +class DerefContextTask : public ScriptExecutionContext::Task {
> > +public:
> > +	 static PassOwnPtr<DerefContextTask> create(ScriptExecutionContext
*context)
> > +	 {
> > +	     return new DerefContextTask(context);
> > +	 }
> > +
> > +	 virtual void performTask(ScriptExecutionContext*)
> > +	 {
> > +	     m_context->deref();
> > +	 }
> 
> I probably was not clear in the comment for previous patch (sorry about
that),
> what I meant is that it should be:
> 
> virtual void performTask(ScriptExecutionContext* context)
> {
>     context->deref();
> }
> 
> since the Task already has a pointer to the context. And then this whole
> 'private' section is not needed:

Arg--no, you were perfectly clear.  I fixed this in about 3 places,
incrementally, but somehow missed this one.  Fixed.

> > +private:
> > +	 explicit DerefContextTask(ScriptExecutionContext *context)
> > +	     : m_context(context)
> > +	 {
> > +	 }
> > +
> > +	 ScriptExecutionContext* m_context;
> > +};
> 
> >  Database::~Database()
> > +	 if (!m_scriptExecutionContext->isContextThread())
> > +	    
m_scriptExecutionContext->postTask(DerefContextTask::create(m_scriptExecutionCo
ntext.release().releaseRef()));
> 
> It seems this should crash since release() will nullify the pointer inside
> m_scriptExecutionContext. If it does not crash for you, perhaps ~Database()
is
> always called on context thread? It would be nice to remove this code then.

I believe it's a matter of timing and circumstance whether it gets called from
the context thread or not, so I'll leave the code in [after removing the bug].
As I read it I walked right off the C++ spec into explicitly undefined
behavior.
That's pretty bad; on the other threads, we can't even count on a crash!  At
any
rate I've fixed the local bug.

> > +class ContextRemoveOpenDatabaseTask : public ScriptExecutionContext::Task
{
> > +private:
> > +	 explicit ContextRemoveOpenDatabaseTask(Database *database) :
m_database(database)
> 
> the initializer should be on it own line.

Fixed.

> >  void Database::scheduleTransactionCallback(SQLTransaction* transaction)
> >  {
> > -	 transaction->ref();
> > -	 callOnMainThread(deliverPendingCallback, transaction);
> > +	 m_scriptExecutionContext->postTask(
> > +	     DeliverPendingCallbackTask::create(transaction));
> 
> This should be a single line. There is no hard rule on line length in WebKit
> style guide, although it's normally assumed that 80 is too tight... 'Blend
in'
> is a good general rule.

Blended.

> > Index: WebCore/storage/Database.h
> > +	 static PassRefPtr<Database> openDatabase(ScriptExecutionContext
*context, const String& name, const String& expectedVersion, const String&
displayName, unsigned long estimatedSize, ExceptionCode&);
> 
> space before '*'

Fixed.

> > +	 Database(ScriptExecutionContext *context, const String& name,
> > +		  const String& expectedVersion, const String& displayName,
> > +		  unsigned long estimatedSize);
> 
> space before '*'

Fixed.

> > Index: WebCore/storage/DatabaseThread.h
> > +	 void requestTermination(DatabaseTaskSynchronizer *cleanupSync);
> 
> space before '*'

Fixed.

> > +	 DatabaseTaskSynchronizer *m_cleanupSync;
> 
> space before '*'

Fixed.

> > Index: WebCore/storage/chromium/DatabaseTrackerChromium.cpp
> 
> > -bool DatabaseTracker::canEstablishDatabase(Document*, const String&, const
String&, unsigned long)
> > +bool DatabaseTracker::canEstablishDatabase(ScriptExecutionContext *, const
String&, const String&, unsigned long)
> 
> space before '*'

Fixed all in this file.

> > +	 virtual void performTask(ScriptExecutionContext *context)
> > +	 {
> > +	     DatabaseTracker::tracker().removeOpenDatabase(m_database);
> > +	     // Reffed in caller.
> > +	     m_database->deref();
> > +	 }
> > +
> > +private:
> > +	 explicit TrackerRemoveOpenDatabaseTask(Database *database) :
m_database(database)
> > +	 {
> > +	 }
> > +
> > +	 Database* m_database;
> > +};
> 
> This needs to use RefPtr<Database> instead of ref/deref with comments. You
seem
> to use RefPtr in other places, like NotifyDatabaseChangedTask.
> Also, m_database initializer should be on its own line, and there are spaces
> before '*'

Fixed.	In NotifyDatabaseChangedTask, I saw that I was taking in a raw pointer,

passing it to a PassRefPtr, and storing that in a RefPtr.  For uniformity I
made
all the raw pointers PassRefPtrs--is that right, or should I have kept it raw
until I put it in the RefPtr?  I've also changed ContextRemoveOpenDatabaseTask
and its caller to use RefPtrs, which I think is cleaner; how's that look?

> > Index: WebCore/storage/chromium/SQLTransactionClientChromium.cpp
> 
> > +	     transaction->database()->scriptExecutionContext()->postTask(
> > +		 NotifyDatabaseChangedTask::create(transaction->database()));
> 
> No need to break the line.

Fixed.

> > Index: WebCore/workers/WorkerContext.cpp
> 
> > +PassRefPtr<Database> WorkerContext::openDatabase(const String& name, const
String& version, const String& displayName, unsigned long estimatedSize,
ExceptionCode& ec)
> > +{
> > +	 if (!securityOrigin()->canAccessDatabase())
> > +	     return 0;
> 
> The spec says this should throw SECURITY_ERR, so 'ec' should be set.

True; set here and in DOMWindow.cpp, which required a test change.

> > +	 if (!Database::isAvailable())
> > +	     return 0;
> 
> Run-time enablement, if works correctly, will remove openDatabase in	a way
> that script wont be able to even detect it. It seems this should this be
> replaced (or accompanied) with ASSERT(Database::isAvailable).

Asserted.

> > Index: WebCore/workers/WorkerThread.cpp
> 
> > +class WorkerThreadShutdownFinishTask : public ScriptExecutionContext::Task
{
> > +
> > +private:
> > +	 explicit WorkerThreadShutdownFinishTask()
> > +	 {
> > +	 }
> 
> This constructor can be removed.

Done.

> > +class WorkerThreadShutdownStartTask : public ScriptExecutionContext::Task
{
> > +private:
> > +	 explicit WorkerThreadShutdownStartTask()
> > +	 {
> > +	 }
> 
> Ditto.

Done.

> In all your Task-derived classes, the private constructors are 'explicit'.
> WebKit does not require automatic 'explicit' on one-parameter constructors as

> Chromium style guide does, so I think it's better to remove this for
> consistency. It's hard to imagine using those private constructors for
implicit
> type conversion anyways.

Well, explicit is for the times that you didn't actually mean to use them that
way, but it happened automatically.  But I've removed them.

> ------- Comment #20 From Andrew Wilson 2010-01-14 17:58:39 PST (-) [reply]
-------
> 
> (From update of attachment 46403 [details])
> The worker shutdown behavior is somewhat subtle and has been a source of
> hard-to-track race conditions in the past, but I think you're doing the right

> things. The thing I'd want to be careful about is making sure no more events
> sneak in after the thread has been stopped (since we're now doing a couple of

> queue flushes before shutting down the message loop and exiting the thread),
> but it looks like the code you added to WorkerRunLoop *should* do that. Run
the
> worker layout tests in a loop for an hour to shake out any new race
conditions
> :)

I'm currently doing this for any test in a directory called "workers".	I'll
leave that going for at least an hour.