Bug 22725 – Refactor database and worker code to prep the way for DB in Workers

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

It would be nice to have HTML5 SQL Databases available in Workers.

The Database API should likely be extended to support synchronous operation to make it more easily usable, as there is no danger of blocking UI thread with Workers.

[Alexey Proskuryakov]

<rdar://problem/6425815>

[Dimitri Glazkov (Google)]

One proposed sync API is here: http://code.google.com/p/gears/wiki/Database2API

[William J. Edney]

All -

I'd add that the latest draft of what is now called the 'Web Database' specification was updated today and is here:

http://dev.w3.org/html5/webdatabase/

I noticed that a 'synchronous' API has been added since Webkit/Safari implemented the HTML5 DB functionality a while back. Having that synchronous API (with or without Worker support) would be a definite plus for those of us out here who need it :-).

Cheers,

- Bill

[Eric U.]

Created attachment 46164 [details]
Infrastructure and refactoring to prepare the way for this feature.  No new APIs exposed yet.

This is just the first changelist for this bug.  It prepares the way for the JSC bindings, which I'll work on next.  I've tested this briefly with V8 bindings, but they'll be in a separate patch.  Since no new APIs are exposed, there are no new layout tests yet.

This touches a large number of files, but most of the changes are quite small.  The big stuff is concentrated in just a few areas [worker thread shutdown, Document/WorkerContext/ScriptExecutionContext, and of course the Database classes].

[WebKit Review Bot]

Attachment 46164 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/workers/WorkerContext.h:50:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/dom/Document.h:440:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 2

[Dmitry Titov]

Since I've reviewed early version of this change, I'll take a look.

[Dmitry Titov]

Comment on attachment 46164 [details]
Infrastructure and refactoring to prepare the way for this feature.  No new APIs exposed yet.

Database in Workers is great, and the patch look good. Here is the first look's result (not complete yet):


> Index: WebCore/ChangeLog

This is quite a significant change, and ChangeLog could use more comments on what and why was actually refactored.
Right now, it has no comments at all except the generic description on top. Depending on how people search trac or ChangeLogs, more info is useful.


> Index: WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp

> +#if ENABLE(DATABASE)
> +#include "Database.h"
> +#endif

Database.h already contains this #ifdef inside. No need to duplicate.

> Index: WebCore/dom/Document.cpp

> +bool Document::isDatabaseReadOnly() const
> +{
> +    Page* page = document()->page();
> +    if (!page || page->settings()->privateBrowsingEnabled())
> +        return true;
> +    return false;
> +}
> +
> +void Document::databaseExceededQuota(const String& name)
> +{
> +    Page* currentPage = page();
> +    ASSERT(currentPage);
> +    currentPage->chrome()->client()->exceededDatabaseQuota(document()->frame(), name);
> +}

One of those expects page() to be possibly 0, while another asserts on it. If there is indeed such a difference in expectations, it'd be nice to have a comment explaining it.

> +bool Document::isJavaScriptThread() const
> +{
> +    return WTF::isMainThread();
> +}

I think this new virtual function is redundant. For the cases it is used in, it always means "not the database thread", and it seems the database thread ID is available in all points of use.


> Index: WebCore/dom/Document.h

> -    Page* page() const; // can be NULL
> +    virtual Page* page() const; // can be NULL

page() is a pretty 'hot' function. Making it virtual may not improve perf, a lot of work has been done in WebKit to remove 'hot' virtuals...
In fact, I think SEC does not need this virtual - looking at how it is used in the patch, it is in fact detecting that the context is not a document (it always returns 0 if context is not a Document).
We already have ScriptExecutionContext::isDocument() that can be used instead of checking the returning page for 0.

> +    virtual bool isJavaScriptThread() const;

Probably unnecessary method, see above.


> Index: WebCore/dom/ScriptExecutionContext.cpp
> ===================================================================
> --- WebCore/dom/ScriptExecutionContext.cpp	(revision 52995)
> +++ WebCore/dom/ScriptExecutionContext.cpp	(working copy)
> @@ -28,7 +28,8 @@
>  #include "ScriptExecutionContext.h"
>  
>  #include "ActiveDOMObject.h"
> -#include "Document.h"
> +#include "DatabaseTask.h"
> +#include "DatabaseThread.h"
>  #include "MessagePort.h"
>  #include "SecurityOrigin.h"
>  #include "WorkerContext.h"
> @@ -56,6 +57,9 @@ public:
>  };
>  
>  ScriptExecutionContext::ScriptExecutionContext()
> +#if ENABLE(DATABASE)
> +    : m_hasOpenDatabases(false)
> +#endif
>  {
>  }
>  
> @@ -72,8 +76,64 @@ ScriptExecutionContext::~ScriptExecution
>          ASSERT((*iter)->scriptExecutionContext() == this);
>          (*iter)->contextDestroyed();
>      }
> +    if (m_databaseThread) {
> +        ASSERT(m_databaseThread->terminationRequested());
> +        m_databaseThread = 0;
> +    }
>  }
>  
> +#if ENABLE(DATABASE)
> +
> +DatabaseThread* ScriptExecutionContext::databaseThread()
> +{
> +    if (!m_databaseThread && !m_hasOpenDatabases) {
> +        // Create the database thread on first request - but not if at least one database was already opened,
> +        // because in that case we already had a database thread and terminated it and should not create another.
> +        m_databaseThread = DatabaseThread::create();
> +        if (!m_databaseThread->start())
> +            m_databaseThread = 0;
> +    }
> +
> +    return m_databaseThread.get();
> +}
> +
> +void ScriptExecutionContext::addOpenDatabase(Database* database)
> +{
> +    if (!m_openDatabaseSet)
> +        m_openDatabaseSet.set(new DatabaseSet());
> +
> +    ASSERT(!m_openDatabaseSet->contains(database));
> +    m_openDatabaseSet->add(database);
> +}
> +
> +void ScriptExecutionContext::removeOpenDatabase(Database* database)
> +{
> +    ASSERT(m_openDatabaseSet && m_openDatabaseSet->contains(database));
> +    if (!m_openDatabaseSet)
> +        return;
> +    m_openDatabaseSet->remove(database);
> +}
> +
> +void ScriptExecutionContext::stopDatabases(DatabaseTaskSynchronizer *cleanupSync)
> +{
> +    if (m_openDatabaseSet) {
> +        DatabaseSet::iterator i = m_openDatabaseSet->begin();
> +        DatabaseSet::iterator end = m_openDatabaseSet->end();
> +        for (; i != end; ++i) {
> +            (*i)->stop();
> +            if (m_databaseThread)
> +                m_databaseThread->unscheduleDatabaseTasks(*i);
> +        }
> +    }
> +    
> +    if (m_databaseThread)
> +        m_databaseThread->requestTermination(cleanupSync);
> +    else if (cleanupSync)
> +        cleanupSync->taskCompleted();
> +}
> +
> +#endif
> +
>  void ScriptExecutionContext::processMessagePortMessagesSoon()
>  {
>      postTask(ProcessMessagesSoonTask::create());

> Index: WebCore/dom/ScriptExecutionContext.h

> +    class Database;
> +    class DatabaseTaskSynchronizer;
> +    class DatabaseThread;

@if ENABLE(DATABASE) ?

> +    class Page;

If 'virtual Page page();' is not here, this can be removed as well.

> +        void stopDatabases(DatabaseTaskSynchronizer *cleanupSync);

> +            // Certain tasks get marked specially so that we don't forget to run
> +            // them while shutting down.

WebKit usually does not try to wrap lines at 80 characters. It's also not clear what "we don't forget" means..
Perhaps 'so they are not discarded when context is shutting down its message queue'?

> Index: WebCore/loader/FrameLoader.cpp

>  #if ENABLE(DATABASE)
> +        // TODO(ericu): Anything about worker databases here?
>          && !m_frame->document()->hasOpenDatabases()

If doc has workers, the page is not cacheable because they are ActiveDOMObjects that can not be suspended.
If they ever will be, the Worker::canSuspend() will have to verify this for all internal objects, including databases.
So this TODO can be removed (or re-phrased).

> +#if ENABLE(DATABASE) // TODO(ericu): Likewise, anything about worker databases?
>          if (m_frame->document()->hasOpenDatabases())

Ditto.

> Index: WebCore/storage/Database.cpp

> -#include "InspectorController.h"

> +#if ENABLE(INSPECTOR)
> +#include "InspectorController.h"
>  #endif

Not sure why this include moved and gained ENABLE(INSPECTOR) protector...

>  #if ENABLE(INSPECTOR)
> -    if (Page* page = document->frame()->page())
> -        page->inspectorController()->didOpenDatabase(database.get(), document->securityOrigin()->host(), name, expectedVersion);
> +    if (Page* page = database->executionContext()->page())
> +        page->inspectorController()->didOpenDatabase(database.get(), context->securityOrigin()->host(), name, expectedVersion);

That I think is the only place in this patch that uses virtual page() method. Can just check for executionContext()->isDocument() and cast for now.
We'll need to figure out how to hook up inspector to workers separately...

>  Database::~Database()
>  {
> -    // Deref m_document on the main thread.
> -    callOnMainThread(derefDocument, m_document.release().releaseRef());
> +    if (!m_executionContext->isJavaScriptThread()) {
> +        // The reference to the ScriptExecutionContext needs to be cleared on
> +        // the JavaScript thread, not the Database thread.  However, if we're
> +        // already on that thread, for example cleaning up all databases during
> +        // worker thread shutdown, we don't want to post a task because it might
> +        // hit the queue after the WorkerThreadShutdownFinishTask and never get
> +        // executed.
> +        m_executionContext->postTask(DerefContextTask::create(
> +            m_executionContext.release().releaseRef()));

The comment sounds a bit confusing. If it is bad that sometimes deref won't happen if we have to post the task to the context's thread, how it is better to never deref at all?


I will do more review soon, please feel free to update the patch (or wait until I go through all of it). I'm leaning to r- it at the end due to 'virtual Page page()', but in general it looks good.

[Eric U.]

Created attachment 46193 [details]
Revision responding to first code review comments

[Eric U.]

(In reply to comment #7)
> > Index: WebCore/ChangeLog
> 
> This is quite a significant change, and ChangeLog could use more comments on
> what and why was actually refactored.
> Right now, it has no comments at all except the generic description on top.
> Depending on how people search trac or ChangeLogs, more info is useful.

I've added a bunch; how's that?

> > Index: WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
> 
> > +#if ENABLE(DATABASE)
> > +#include "Database.h"
> > +#endif
> 
> Database.h already contains this #ifdef inside. No need to duplicate.

Fixed.

> > Index: WebCore/dom/Document.cpp
> 
> > +bool Document::isDatabaseReadOnly() const
> > +{
> > +    Page* page = document()->page();
> > +    if (!page || page->settings()->privateBrowsingEnabled())
> > +        return true;
> > +    return false;
> > +}
> > +
> > +void Document::databaseExceededQuota(const String& name)
> > +{
> > +    Page* currentPage = page();
> > +    ASSERT(currentPage);
> > +    currentPage->chrome()->client()->exceededDatabaseQuota(document()->frame(), name);
> > +}
> 
> One of those expects page() to be possibly 0, while another asserts on it. If
> there is indeed such a difference in expectations, it'd be nice to have a
> comment explaining it.

Hmm...I'll look around a bit.  That's actually code I relocated, that was already there.  The assumptions about page() have more to do with the call sites from which I pulled the code than they really should, given the new abstraction.  I'll keep looking into that, but as I've addressed everything else, I'm uploading this new patch for your review.

> > +bool Document::isJavaScriptThread() const
> > +{
> > +    return WTF::isMainThread();
> > +}
> 
> I think this new virtual function is redundant. For the cases it is used in, it
> always means "not the database thread", and it seems the database thread ID is
> available in all points of use.

The database thread is available for checking anywhere you can call isJavaScriptThread.  However, that's not a sufficient check; since the JS thread isn't the main thread in worker context, it's also important to verify that we're not on the main thread there.

> > Index: WebCore/dom/Document.h
> 
> > -    Page* page() const; // can be NULL
> > +    virtual Page* page() const; // can be NULL
> 
> page() is a pretty 'hot' function. Making it virtual may not improve perf, a
> lot of work has been done in WebKit to remove 'hot' virtuals...
> In fact, I think SEC does not need this virtual - looking at how it is used in
> the patch, it is in fact detecting that the context is not a document (it
> always returns 0 if context is not a Document).
> We already have ScriptExecutionContext::isDocument() that can be used instead
> of checking the returning page for 0.

Fixed, as below.

> > +    virtual bool isJavaScriptThread() const;
> 
> Probably unnecessary method, see above.

See above.

> > Index: WebCore/dom/ScriptExecutionContext.h
> 
> > +    class Database;
> > +    class DatabaseTaskSynchronizer;
> > +    class DatabaseThread;
> 
> @if ENABLE(DATABASE) ?

Fixed.

> > +    class Page;
> 
> If 'virtual Page page();' is not here, this can be removed as well.

Done.

> > +        void stopDatabases(DatabaseTaskSynchronizer *cleanupSync);
> 
> > +            // Certain tasks get marked specially so that we don't forget to run
> > +            // them while shutting down.
> 
> WebKit usually does not try to wrap lines at 80 characters. It's also not clear
> what "we don't forget" means..
> Perhaps 'so they are not discarded when context is shutting down its message
> queue'?

Fixed.  Incidentally, I couldn't find a reference to a preferred line width limit; what's the proper way to wrap very long comments or statements?  Do you just let them go forever?

> > Index: WebCore/loader/FrameLoader.cpp
> 
> >  #if ENABLE(DATABASE)
> > +        // TODO(ericu): Anything about worker databases here?
> >          && !m_frame->document()->hasOpenDatabases()
> 
> If doc has workers, the page is not cacheable because they are ActiveDOMObjects
> that can not be suspended.
> If they ever will be, the Worker::canSuspend() will have to verify this for all
> internal objects, including databases.
> So this TODO can be removed (or re-phrased).

Excellent; thanks.  Fixed.

> > +#if ENABLE(DATABASE) // TODO(ericu): Likewise, anything about worker databases?
> >          if (m_frame->document()->hasOpenDatabases())
> 
> Ditto.

Fixed.

> > Index: WebCore/storage/Database.cpp
> 
> > -#include "InspectorController.h"
> 
> > +#if ENABLE(INSPECTOR)
> > +#include "InspectorController.h"
> >  #endif
> 
> Not sure why this include moved and gained ENABLE(INSPECTOR) protector...

I saw that include guard in some other files, so I assumed it was omitted accidentally here.  I see it unguarded in yet others--not sure what the right thing is.  I'll put it back for now.

> >  #if ENABLE(INSPECTOR)
> > -    if (Page* page = document->frame()->page())
> > -        page->inspectorController()->didOpenDatabase(database.get(), document->securityOrigin()->host(), name, expectedVersion);
> > +    if (Page* page = database->executionContext()->page())
> > +        page->inspectorController()->didOpenDatabase(database.get(), context->securityOrigin()->host(), name, expectedVersion);
> 
> That I think is the only place in this patch that uses virtual page() method.
> Can just check for executionContext()->isDocument() and cast for now.
> We'll need to figure out how to hook up inspector to workers separately...

Done.

> >  Database::~Database()
> >  {
> > -    // Deref m_document on the main thread.
> > -    callOnMainThread(derefDocument, m_document.release().releaseRef());
> > +    if (!m_executionContext->isJavaScriptThread()) {
> > +        // The reference to the ScriptExecutionContext needs to be cleared on
> > +        // the JavaScript thread, not the Database thread.  However, if we're
> > +        // already on that thread, for example cleaning up all databases during
> > +        // worker thread shutdown, we don't want to post a task because it might
> > +        // hit the queue after the WorkerThreadShutdownFinishTask and never get
> > +        // executed.
> > +        m_executionContext->postTask(DerefContextTask::create(
> > +            m_executionContext.release().releaseRef()));
> 
> The comment sounds a bit confusing. If it is bad that sometimes deref won't
> happen if we have to post the task to the context's thread, how it is better to
> never deref at all?

We do nothing here; our destructor completes, calls the RefPtr destructor, and that takes care of it.  In the cases where we're posting the task to the JS thread, we're guaranteed to complete it.  I've made the comment clearer, mainly by deleting most of it ;'>.

[WebKit Review Bot]

Attachment 46193 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/workers/WorkerContext.h:50:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/storage/Database.cpp:234:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2

[Dmitry Titov]

(In reply to comment #9)

> > > +bool Document::isJavaScriptThread() const
> > > +{
> > > +    return WTF::isMainThread();
> > > +}
> > 
> > I think this new virtual function is redundant. For the cases it is used in, it
> > always means "not the database thread", and it seems the database thread ID is
> > available in all points of use.
> 
> The database thread is available for checking anywhere you can call
> isJavaScriptThread.  However, that's not a sufficient check; since the JS
> thread isn't the main thread in worker context, it's also important to verify
> that we're not on the main thread there.

Perhaps, there is a naming issue here that makes me think this doesn't belong - why "JavaScript" thread? Perhaps it should be "isContextThread()"? It would check that current thread is the one that 'postTask()' targets... There are patches attempting to allow other languages to be run as 'script'. 

On top of that, I hope Database would never be used on 3 threads. Currently, all DOM objects created and used under SEC are accessed on 'context thread' only. In case of Workers this is a Worker thread. This is by design since none of DOM objects are natively cross-thread-capable. For example, XMLHttpRequest does not need to assert or do different things depending on which thread it is running - and in the only place it needs, it uses explicit ThreadableLoader class that explicitly encapsulates the cross-thread logic. Timers simply use TLS and are oblivious to the thread they are used on. It would be really messy if we had to assume that DOM objects can be accessed from different threads.

It seems that existing Database code uses isMainThread() as synonym of !isDatabaseThread() and if it is so then the new method can be avoided. At least this seems to be true in this patch. Please correct me if I miss something.

[Dmitry Titov]

Comment on attachment 46193 [details]
Revision responding to first code review comments

I think you can ignore first style bot complaint about indenting stuff inside namespace in WorkerContext.h. This is a recently-changed rule but this file was already indented. The other one is good to fix though.


> Index: WebCore/dom/Document.cpp
> +bool Document::isDatabaseReadOnly() const
> +{
> +    Page* page = document()->page();

no need to use document() here. 'this' is a document.


> +void Document::databaseExceededQuota(const String& name)
> +{
> +    Page* currentPage = page();
> +    ASSERT(currentPage);
> +    currentPage->chrome()->client()->exceededDatabaseQuota(document()->frame(), name);

Ditto. document().

> Index: WebCore/dom/Document.h

> +    virtual bool isJavaScriptThread() const;

See previous reply. If it is actually needed, the better name seems to be isContextThread().


> Index: WebCore/dom/ScriptExecutionContext.h

> +        void stopDatabases(DatabaseTaskSynchronizer *cleanupSync);

The parameter name is usually omitted here.

> +            // Certain tasks get marked specially so that they aren't discarded, and are executed, when the context is shutting down its message queue.
> +            virtual bool isCleanupTask() const { return false; }

I don't think it is possible to keep this contract for all ScriptExecutionContexts, for example Document does not control the run loop of the main thread so the tasks can be dropped even if they are marked as 'cleanup tasks'. Not sure how to change it - maybe have a specific WorkerTask class and add it there?

> Index: WebCore/storage/Database.cpp

> -    ASSERT(m_document->databaseThread());
> -
> -    m_filename = DatabaseTracker::tracker().fullPathForDatabase(m_mainThreadSecurityOrigin.get(), m_name);
> +    m_filename = DatabaseTracker::tracker().fullPathForDatabase(securityOrigin(), m_name);

Is the removal of ASSERT intentional?

> +private:
> +    explicit DerefContextTask(ScriptExecutionContext *context) : m_context(context)
> +    {
> +    }

the initializers (": m_context(conetxt)") should be on a separate line, indented 4 spaces.

> Index: WebCore/storage/Database.h

> +    ScriptExecutionContext* executionContext() const { return m_executionContext.get(); }

In other classes, this is called 'scriptExecutionContext()' (see Node.h for example). Lets rename it, for consistency.

> Index: WebCore/storage/DatabaseTracker.h

>  class DatabaseTracker : public Noncopyable {
>  public:
>      static DatabaseTracker& tracker();
> +    // NOTE: If at some point anyone implements multiple worker threads
> +    // running in the same process, this singleton will have to be synchronized.

Very explosive NOTE you have here :-) This is exactly what happens in non-Chromium builds - workers run in their own threads in the same process. So DatabaseTracker will have to be changed now. It can be a separate patch though.

You said you tested a prototype with Chromium bindings, but in Chromium Workers run in separate processes so there is no multiple Worker threads in the picture at all. Also, database code is not used from main thread in a Worker process since there is no Documents loaded. JSC case provides better threading 'test environment' so it's good to get JSC prototype up and running in order to get these things right.

Also, in WebKit the comments usually don't use 'NOTE:', the comment by itself is a note. FIXME perhaps.

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

>  void DatabaseTracker::addOpenDatabase(Database* database)
>  {
> -    ASSERT(isMainThread());
> +    ASSERT(database->executionContext()->isJavaScriptThread());
>      DatabaseObserver::databaseOpened(database);
>  }

Not sure this is correct replacement... It documents an assumption that this method is ok to call on multiple threads. Is it?

> +class TrackerRemoveOpenDatabaseTask : public ScriptExecutionContext::Task {
> +    virtual void performTask(ScriptExecutionContext *context)
> +    {
> +        ASSERT(context->isJavaScriptThread());

No need for ASSERT - by design the task is executed on the context's thread.

>  void DatabaseTracker::removeOpenDatabase(Database* database)
>  {
> -    if (!isMainThread()) {
> +    if (!database->executionContext()->isJavaScriptThread()) {

Does it mean the main thread is actually ok here? It can be that context's thread is not the main thread... It would be simpler to just check for database thread here.

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

> +    virtual void performTask(ScriptExecutionContext* context)
> +    {
> +        WebCore::DatabaseObserver::databaseModified(m_database.get());
> +    }

'context' is an unused parameter. Can be omitted.

> Index: WebCore/workers/WorkerContext.cpp

> +#if ENABLE(DATABASE)
> +#include "Database.h"
> +#endif

ENABLE() guard is not needed here.

> Index: WebCore/workers/WorkerContext.h

> +#if ENABLE(DATABASE)
> +#include "Database.h"
> +#endif

Ditto.

> +        virtual bool isDatabaseReadOnly() const { return false; /* TODO(ericu) */ }
> +        virtual void databaseExceededQuota(const String&) { /* TODO(ericu) */}

There is no TODO(name) in WebKit. A comment above the methods saying they are not yet implemented would suffice. It is also a good idea to file bug[s] to actually implement those.

> Index: WebCore/workers/WorkerThread.cpp

> +class WorkerThreadShutdownFinishTask : public ScriptExecutionContext::Task {
> +    virtual void performTask(ScriptExecutionContext *context)
> +    {
> +        ASSERT(context->isJavaScriptThread());
> +        m_workerContext->thread()->runLoop().terminate();

ASSERT is not needed (the postTask ensures that). Also, m_workerContext is the same thing as 'context' parameter - better use the parameter.

> +class WorkerThreadShutdownStartTask : public ScriptExecutionContext::Task {
> +    virtual void performTask(ScriptExecutionContext *context)
> +    {
> +        ASSERT(context->isJavaScriptThread());
> +        // We currently ignore any DatabasePolicy used for the document's
> +        // databases; if it's actually used anywhere, this should be revisited.
> +        DatabaseTaskSynchronizer cleanupSync;
> +        m_workerContext->stopDatabases(&cleanupSync);

Same as above.

>  void WorkerThread::stop()

> -    m_runLoop.terminate();
> +    // NOTE: this can likely use the same mechanism as used for databases above.
> +
> +        m_runLoop.postTask(WorkerThreadShutdownStartTask::create(m_workerContext.get()));

How did you verify that the code that calls stop() does not depend on synchronous result of terminate()?

[Eric U.]

Created attachment 46331 [details]
Next revision, rolling in more comments.

> > Index: WebCore/dom/Document.cpp
> 
> > +bool Document::isDatabaseReadOnly() const
> > +{
> > +    Page* page = document()->page();
> > +    if (!page || page->settings()->privateBrowsingEnabled())
> > +        return true;
> > +    return false;
> > +}
> > +
> > +void Document::databaseExceededQuota(const String& name)
> > +{
> > +    Page* currentPage = page();
> > +    ASSERT(currentPage);
> > +    currentPage->chrome()->client()->exceededDatabaseQuota(document()->frame(), name);
> > +}
> 
> One of those expects page() to be possibly 0, while another asserts on it. If
> there is indeed such a difference in expectations, it'd be nice to have a
> comment explaining it.

I spoke to dglazkov; the ASSERT is a bug.  Fixed.

> I think you can ignore first style bot complaint about indenting stuff inside
> namespace in WorkerContext.h. This is a recently-changed rule but this file was
> already indented. The other one is good to fix though.

Yup; that was an oversight, rushing to get a new patch up for the weekend.  Fixed.

> > Index: WebCore/dom/Document.cpp
> > +bool Document::isDatabaseReadOnly() const
> > +{
> > +    Page* page = document()->page();
> 
> no need to use document() here. 'this' is a document.

Fixed.

> > +void Document::databaseExceededQuota(const String& name)
> > +{
> > +    Page* currentPage = page();
> > +    ASSERT(currentPage);
> > +
> currentPage->chrome()->client()->exceededDatabaseQuota(document()->frame(),
> name);
> 
> Ditto. document().

Fixed.

> > Index: WebCore/dom/Document.h
> 
> > +    virtual bool isJavaScriptThread() const;
> 
> See previous reply. If it is actually needed, the better name seems to be
> isContextThread().

Done.

> > Index: WebCore/dom/ScriptExecutionContext.h
> 
> > +        void stopDatabases(DatabaseTaskSynchronizer *cleanupSync);
> 
> The parameter name is usually omitted here.

Done.

> discarded, and are executed, when the context is shutting down its message
> queue.
> > +            virtual bool isCleanupTask() const { return false; }
> 
> I don't think it is possible to keep this contract for all
> ScriptExecutionContexts, for example Document does not control the run loop of
> the main thread so the tasks can be dropped even if they are marked as 'cleanup
> tasks'. Not sure how to change it - maybe have a specific WorkerTask class and
> add it there?

There's already WorkerRunLoop::Task; that seemed like the place to put it, if
that's the way you want to go.  However, I started putting together an
implementation down that road, and it didn't actually look much cleaner.

The problem is that the reason I put that in at all was that the WorkerContext
was failing to guarantee something that the Document already provides.  During
cleanup you can post tasks to the Document's context and they'll get executed,
since the main thread outlives the cleanup.  So you don't need to control the
Document's run loop; it already provides the needed functionality.

So in my WorkerRunLoop::Task variation, at some point you hit something like
ContextRemoveOpenDatabaseTask, which on the Document you can just give to
context->postTask, but if context->isWorkerContext you've got to cast to
WorkerContext and call e.g. postCleanupTask.  It's a bit ugly, and in the end it
still assumes that the Document's context will execute all the cleanup tasks.

Summing up, I think this is OK as it is, though I can do it the other way if you
think it's stylistically better.

> > Index: WebCore/storage/Database.cpp
> 
> > -    ASSERT(m_document->databaseThread());
> > -
> > -    m_filename =
> DatabaseTracker::tracker().fullPathForDatabase(m_mainThreadSecurityOrigin.get()
> , m_name);
> > +    m_filename =
> DatabaseTracker::tracker().fullPathForDatabase(securityOrigin(), m_name);
> 
> Is the removal of ASSERT intentional?

No; fixed.

> > +private:
> > +    explicit DerefContextTask(ScriptExecutionContext *context) :
> m_context(context)
> > +    {
> > +    }
> 
> the initializers (": m_context(conetxt)") should be on a separate line,
> indented 4 spaces.

Fixed.

> > Index: WebCore/storage/Database.h
> 
> > +    ScriptExecutionContext* executionContext() const { return
> m_executionContext.get(); }
> 
> In other classes, this is called 'scriptExecutionContext()' (see Node.h for
> example). Lets rename it, for consistency.

Done.

> > Index: WebCore/storage/DatabaseTracker.h
> 
> >  class DatabaseTracker : public Noncopyable {
> >  public:
> >      static DatabaseTracker& tracker();
> > +    // NOTE: If at some point anyone implements multiple worker threads
> > +    // running in the same process, this singleton will have to be
> synchronized.
> 
> Very explosive NOTE you have here :-) This is exactly what happens in
> non-Chromium builds - workers run in their own threads in the same process. So
> DatabaseTracker will have to be changed now. It can be a separate patch though.

Changed to a FIXME.  I'll have to get that in before or with the JSC bindings.

> You said you tested a prototype with Chromium bindings, but in Chromium Workers
> run in separate processes so there is no multiple Worker threads in the picture
> at all. Also, database code is not used from main thread in a Worker process
> since there is no Documents loaded. JSC case provides better threading 'test
> environment' so it's good to get JSC prototype up and running in order to get
> these things right.

Yes indeed.

> Also, in WebKit the comments usually don't use 'NOTE:', the comment by itself
> is a note. FIXME perhaps.
> 
> > Index: WebCore/storage/chromium/DatabaseTrackerChromium.cpp
> 
> >  void DatabaseTracker::addOpenDatabase(Database* database)
> >  {
> > -    ASSERT(isMainThread());
> > +    ASSERT(database->executionContext()->isJavaScriptThread());
> >      DatabaseObserver::databaseOpened(database);
> >  }
> 
> Not sure this is correct replacement... It documents an assumption that this
> method is ok to call on multiple threads. Is it?

How so?  It seems to me to document [and enforce] the fact that it can only be
called on a single thread.  Or is this addressed by the change to
isContextThread?

> > +class TrackerRemoveOpenDatabaseTask : public ScriptExecutionContext::Task {
> > +    virtual void performTask(ScriptExecutionContext *context)
> > +    {
> > +        ASSERT(context->isJavaScriptThread());
> 
> No need for ASSERT - by design the task is executed on the context's thread.

Removed.

> >  void DatabaseTracker::removeOpenDatabase(Database* database)
> >  {
> > -    if (!isMainThread()) {
> > +    if (!database->executionContext()->isJavaScriptThread()) {
> 
> Does it mean the main thread is actually ok here? It can be that context's
> thread is not the main thread... It would be simpler to just check for database
> thread here.

As discussed privately [reposting for anyone else following along], the main
thread is only OK if it's the context thread [e.g. in the Chromium, it's a
renderer, not a worker].  Only the context thread gets to call
DatabaseObserver::databaseClosed() here.

> > Index: WebCore/storage/chromium/SQLTransactionClientChromium.cpp
> 
> > +    virtual void performTask(ScriptExecutionContext* context)
> > +    {
> > +        WebCore::DatabaseObserver::databaseModified(m_database.get());
> > +    }
> 
> 'context' is an unused parameter. Can be omitted.

Done.

> > Index: WebCore/workers/WorkerContext.cpp
> 
> > +#if ENABLE(DATABASE)
> > +#include "Database.h"
> > +#endif
> 
> ENABLE() guard is not needed here.

Removed.

> > Index: WebCore/workers/WorkerContext.h
> 
> > +#if ENABLE(DATABASE)
> > +#include "Database.h"
> > +#endif
> 
> Ditto.

Removed.

> > +        virtual bool isDatabaseReadOnly() const { return false; /*
> TODO(ericu) */ }
> > +        virtual void databaseExceededQuota(const String&) { /* TODO(ericu)
> */}
> 
> There is no TODO(name) in WebKit. A comment above the methods saying they are
> not yet implemented would suffice. It is also a good idea to file bug[s] to
> actually implement those.

Changed the comments; will log bugs once this patch is in.

> > Index: WebCore/workers/WorkerThread.cpp
> 
> > +class WorkerThreadShutdownFinishTask : public ScriptExecutionContext::Task {
> 
> > +    virtual void performTask(ScriptExecutionContext *context)
> > +    {
> > +        ASSERT(context->isJavaScriptThread());
> > +        m_workerContext->thread()->runLoop().terminate();
> 
> ASSERT is not needed (the postTask ensures that). Also, m_workerContext is the
> same thing as 'context' parameter - better use the parameter.

Done.

> > +class WorkerThreadShutdownStartTask : public ScriptExecutionContext::Task {
> > +    virtual void performTask(ScriptExecutionContext *context)
> > +    {
> > +        ASSERT(context->isJavaScriptThread());
> > +        // We currently ignore any DatabasePolicy used for the document's
> > +        // databases; if it's actually used anywhere, this should be
> revisited.
> > +        DatabaseTaskSynchronizer cleanupSync;
> > +        m_workerContext->stopDatabases(&cleanupSync);
> 
> Same as above.

Done.

> >  void WorkerThread::stop()
> 
> > -    m_runLoop.terminate();
> > +    // NOTE: this can likely use the same mechanism as used for databases
> above.
> > +
> > +
> m_runLoop.postTask(WorkerThreadShutdownStartTask::create(m_workerContext.get())
> );
> 
> How did you verify that the code that calls stop() does not depend on
> synchronous result of terminate()?

There really isn't much synchronous result of terminate(); there's no limit
on how long that takes to accomplish something, since the worker could be
executing an infinite loop in JS.  The only real guarantee was that no further
tasks in the runLoop would get executed, which is still roughly the case.  The
only exceptions are the pieces of the shutdown code that have been pulled back
into the runLoop, which would previously have run after it exited.  And of
course the new database shutdown code, which wasn't there at all before.  So
given the minimal guarantees, I think I've satisfied them.

I don't know what the other clients do, but Chrome has a 3-second timer that
forces a hard shutdown of the worker process when this more polite mechanism
fails, as I'm sure happens quite a bit with badly-behaved worker code.

[WebKit Review Bot]

Attachment 46331 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/workers/WorkerContext.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[WebKit Review Bot]

Attachment 46331 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/182206

[Eric U.]

Arg--passed all the layout tests, but I didn't check the Chromium build, so I missed a function rename.  I'll upload a fix tomorrow; nothing else will change.

[Eric U.]

Created attachment 46403 [details]
Fixed chromium compile; removed WorkerContext.idl

I fixed that compile error and also removed WorkerContext.idl, which had crept in from a future changelist.

[WebKit Review Bot]

Attachment 46403 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/workers/WorkerContext.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[Dmitry Titov]

Comment on attachment 46403 [details]
Fixed chromium compile; removed WorkerContext.idl

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

[Andrew Wilson]

Comment on attachment 46403 [details]
Fixed chromium compile; removed WorkerContext.idl

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

> +
> +        m_runLoop.postTask(WorkerThreadShutdownStartTask::create());
> +    } else
> +        m_runLoop.terminate();
>  }
>  
>  } // namespace WebCore

[Eric U.]

Created attachment 46959 [details]
Next revision, rolling in more comments.  One layout test changed.

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

[Eric U.]

Created attachment 46968 [details]
Updated to latest webkit code; resolved merge conflicts.

Trivial changes from the last patch to resolve merge conflicts.

[WebKit Review Bot]

Attachment 46968 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/workers/WorkerContext.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Titov]

Comment on attachment 46968 [details]
Updated to latest webkit code; resolved merge conflicts.

Just a few more. Lets try to get to a clean r+ so we can flip cq+ on it.

> Index: WebCore/page/DOMWindow.cpp
>      Document* document = m_frame->document();
> -    if (!document->securityOrigin()->canAccessDatabase())
> +    if (!document->securityOrigin()->canAccessDatabase()) {
> +        ec = SECURITY_ERR;
>          return 0;
> +    }

This is a new addition to the previously reviewed patch. It will take less time if we keep the scope of the change rather then expand it. In this particular case, it's a change of already shipped behavior which may have compatibility issues. Basically, if some website will happen to be broken, this change may have to be reverted. Adding Database to Workers is one thing (no compatibility issues since the stuff is new), changing behavior of Database on the regular pages is a different matter, and should have a separate bug. Lets move this into separate bug.

> Index: WebCore/storage/Database.cpp
>  
>  Database::~Database()
>  {
> +    // The reference to the ScriptExecutionContext needs to be cleared on the JavaScript thread.  If we're on that thread already, we can just let the RefPtr's destruction do the dereffing.
> +    if (!m_scriptExecutionContext->isContextThread()) {
> +        m_scriptExecutionContext->postTask(DerefContextTask::create());
> +        m_scriptExecutionContext.release().releaseRef();

This is something we'll want to cleanup, to possibly avoid having a destructor that can run on either thread, since Database may have ownership of non-thread-safe objects... But it's outside of scope of this change since this one simply refactors the existing code.

>  void Database::close()
>  {
>      if (!m_opened)
>          return;
>  
> +    // Must ref() before calling databaseThread()->recordDatabaseClosed().
> +    RefPtr<Database> holder = this;

The pattern used often in WebKit is to create a RefPtr named 'protect' at the very beginning of the function and then not use it in the code (since 'this' is guaranteed to be valid), like this:
RefPtr<Database> protect = this;

> +    m_scriptExecutionContext->databaseThread()->unscheduleDatabaseTasks(this);
> +    m_scriptExecutionContext->postTask(ContextRemoveOpenDatabaseTask::create(holder));

Can use 'this' for both, for consistency. It's guaranteed protected.


> Index: LayoutTests/ChangeLog

The Layouttest change should go with the DOMWindow change, in a different bug.

> +        No bug.  This is part of the checkin for
> +        https://bugs.webkit.org/show_bug.cgi?id=22725

Normally, all the ChangeLog files that are part of the same checkin, refer to the same bug number. So the first line here would not be needed.

[Eric U.]

Created attachment 47061 [details]
Removed DOMWindow.cpp change, fixed the last(?) few comments.

> --- Comment #24 from Dmitry Titov <[e-mail]>  2010-01-20 12:40:39 PST ---
> (From update of attachment 46968 [details])
> Just a few more. Lets try to get to a clean r+ so we can flip cq+ on it.
>
>> Index: WebCore/page/DOMWindow.cpp
>>      Document* document = m_frame->document();
>> -    if (!document->securityOrigin()->canAccessDatabase())
>> +    if (!document->securityOrigin()->canAccessDatabase()) {
>> +        ec = SECURITY_ERR;
>>          return 0;
>> +    }
>
> This is a new addition to the previously reviewed patch. It will take less time
> if we keep the scope of the change rather then expand it. In this particular
> case, it's a change of already shipped behavior which may have compatibility
> issues. Basically, if some website will happen to be broken, this change may
> have to be reverted. Adding Database to Workers is one thing (no compatibility
> issues since the stuff is new), changing behavior of Database on the regular
> pages is a different matter, and should have a separate bug. Lets move this
> into separate bug.

Gotcha.  I've logged the bug and reverted this part of the patch.  I'll resubmit
that part once this is in.

>> Index: WebCore/storage/Database.cpp
>>
>>  Database::~Database()
>>  {
>> +    // The reference to the ScriptExecutionContext needs to be cleared on the JavaScript thread.  If we're on that thread already, we can just let the RefPtr's destruction do the dereffing.
>> +    if (!m_scriptExecutionContext->isContextThread()) {
>> +        m_scriptExecutionContext->postTask(DerefContextTask::create());
>> +        m_scriptExecutionContext.release().releaseRef();
>
> This is something we'll want to cleanup, to possibly avoid having a destructor
> that can run on either thread, since Database may have ownership of
> non-thread-safe objects... But it's outside of scope of this change since this
> one simply refactors the existing code.

Yeah, it's kind of scary in there right now, thread-wise.

>>  void Database::close()
>>  {
>>      if (!m_opened)
>>          return;
>>
>> +    // Must ref() before calling databaseThread()->recordDatabaseClosed().
>> +    RefPtr<Database> holder = this;
>
> The pattern used often in WebKit is to create a RefPtr named 'protect' at the
> very beginning of the function and then not use it in the code (since 'this' is
> guaranteed to be valid), like this:
> RefPtr<Database> protect = this;

Changed the name to protect and moved it up to the top.  I debated putting it
after the easy-out "if (!m_opened) return;", but that's a rare failure path, so I
assumed we wouldn't put optimization over consistency.

>> +    m_scriptExecutionContext->databaseThread()->unscheduleDatabaseTasks(this);
>> +    m_scriptExecutionContext->postTask(ContextRemoveOpenDatabaseTask::create(holder));
>
> Can use 'this' for both, for consistency. It's guaranteed protected.

Done.

>> Index: LayoutTests/ChangeLog
>
> The Layouttest change should go with the DOMWindow change, in a different bug.
>
>> +        No bug.  This is part of the checkin for
>> +        https://bugs.webkit.org/show_bug.cgi?id=22725
>
> Normally, all the ChangeLog files that are part of the same checkin, refer to
> the same bug number. So the first line here would not be needed.

Gotcha.  Just as well that the test change will be in a separate checkin, then.

[Dmitry Titov]

Comment on attachment 47061 [details]
Removed DOMWindow.cpp change, fixed the last(?) few comments.

r=me.

Not sure if commit bot will land this since there is a style violation which is in fact a false positive (filed bug 33925 for that).

[WebKit Commit Bot]

Comment on attachment 47061 [details]
Removed DOMWindow.cpp change, fixed the last(?) few comments.

Clearing flags on attachment: 47061

Committed r53595: <http://trac.webkit.org/changeset/53595>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mikko Honkala]

I think this bug has been closed prematurely. Most of the code is there, but for instance the glue code in WebCore/binding is not there. See e.g., 
WebCore/workers/WorkerContext.idl, where the openDatabase function is commented out and no implementations for the binding are there either.

Since the title of the bug is  "Make SQL database storage work in Workers", I would reopen this bug.

http://trac.webkit.org/browser/trunk/WebCore/workers/WorkerContext.idl

[Dmitry Titov]

(In reply to comment #29)
> I think this bug has been closed prematurely. 
> Since the title of the bug is  "Make SQL database storage work in Workers", I
> would reopen this bug.

The better way of approaching multi-patch changes may be to have an 'umbrella bug' that says "Implement Foo" and then a few 1-patch bugs implementing actual parts of it, linking them to the main one.

Since this bug contained landed patch of first part of the work, it can be renamed to reflect what it actually achieved.

[Eric U.]

(In reply to comment #30)
> (In reply to comment #29)
> > I think this bug has been closed prematurely. 
> > Since the title of the bug is  "Make SQL database storage work in Workers", I
> > would reopen this bug.
> 
> The better way of approaching multi-patch changes may be to have an 'umbrella
> bug' that says "Implement Foo" and then a few 1-patch bugs implementing actual
> parts of it, linking them to the main one.
> 
> Since this bug contained landed patch of first part of the work, it can be
> renamed to reflect what it actually achieved.

Renaming this bug; I'll go and log a few more to fill out the task.