[webkit-reviews] review denied: [Bug 22725] Make SQL database storage work in Workers : [Attachment 46403] Fixed chromium compile; removed WorkerContext.idl

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 13 17:49:23 PST 2010


Dmitry Titov <dimich at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 22725: Make SQL database storage work in Workers
https://bugs.webkit.org/show_bug.cgi?id=22725

Attachment 46403: Fixed chromium compile; removed WorkerContext.idl
https://bugs.webkit.org/attachment.cgi?id=46403&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
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!

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.

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

> +void ScriptExecutionContext::stopDatabases(DatabaseTaskSynchronizer
*cleanupSync)

In WebKit style, there is no space between type name and '*', it should be:
DatabaseTaskSynchronizer* cleanupSync.
Also, it could use the ASSERT(isContextThread()) too.

> Index: WebCore/dom/ScriptExecutionContext.h

> +	   DatabaseThread *databaseThread();

space before '*'. 

> +	   typedef HashSet<Database *> DatabaseSet;

space before '*'. 

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

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

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

> +class ContextRemoveOpenDatabaseTask : public ScriptExecutionContext::Task {
> +private:
> +    explicit ContextRemoveOpenDatabaseTask(Database *database) :
m_database(database)

the initializer should be on it own line.

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

> 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 '*'

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

space before '*'

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

space before '*'

> +    DatabaseTaskSynchronizer *m_cleanupSync;

space before '*'

> 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 '*'

> +    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 '*'

> Index: WebCore/storage/chromium/SQLTransactionClientChromium.cpp

> +	   transaction->database()->scriptExecutionContext()->postTask(
> +	       NotifyDatabaseChangedTask::create(transaction->database()));

No need to break the line.

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

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

> Index: WebCore/workers/WorkerThread.cpp

> +class WorkerThreadShutdownFinishTask : public ScriptExecutionContext::Task {

> +
> +private:
> +    explicit WorkerThreadShutdownFinishTask()
> +    {
> +    }

This constructor can be removed.

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

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


More information about the webkit-reviews mailing list