[Webkit-unassigned] [Bug 22725] Make SQL database storage work in Workers
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 19 16:32:55 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=22725
Eric U. <ericu at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #46403|0 |1
is obsolete| |
Attachment #46959| |review?
Flag| |
--- Comment #21 from Eric U. <ericu at chromium.org> 2010-01-19 16:32:52 PST ---
Created an attachment (id=46959)
--> (https://bugs.webkit.org/attachment.cgi?id=46959)
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.
--
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