[Webkit-unassigned] [Bug 22725] Make SQL database storage work in Workers
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 11 19:20:34 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=22725
Eric U. <ericu at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #46193|0 |1
is obsolete| |
Attachment #46331| |review?
Flag| |
--- Comment #13 from Eric U. <ericu at chromium.org> 2010-01-11 19:20:31 PST ---
Created an attachment (id=46331)
--> (https://bugs.webkit.org/attachment.cgi?id=46331)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list