Refactor DatabaseTracker.cpp for thread safety

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

It currently uses 4 different locks, and yet isn't completely thread-safe.  In particular if you've got more than one context thread [say a page thread and a worker thread] it'll have major problems.  Also, if you store quota information from a worker thread that then exits, you'll hit asserts due to the check on the refcount of the thread-local empty string.

Umbrella bug 34990 depends on this.

[Eric U.]

Created attachment 49333 [details]
Patch

[Eric U.]

That patch is actually a good deal smaller than its size would indicate--there's a fair amount of code that's merely indented differently or moved slightly.

[Eric U.]

Created attachment 49337 [details]
Patch

[Dmitry Titov]

Comment on attachment 49337 [details]
Patch

> Index: WebCore/ChangeLog

I usually look at the ChangeLog to educate myself on details of the change, both 'what' and 'why'. The description here is a bit broad, and frankly looking at each file's changes, it's hard to figure out if each of them is correct. It would be really easier to review if the bug or ChangeLog tried to explain the changes better.

> +    // Set this flag to allow access from multiple threads.  Not all multi-threaded accesses are safe!
> +    // See http://www.sqlite.org/cvstrac/wiki?p=MultiThreading for more info, and you must be running with compile flags and SQLite version appropriate
> +    // to your cross-thread uses.
Are we ensuring compile flags/version somehow? Across major ports? Is it possible to assert it?

If that assert in sqlite3Handle() was meant to ensure that sqlite can be built w/o multithreaded flags and now we require so, why not simply remove the ASSERT as well?

> +    bool sharable() { return m_sharable; }
This method is not used. It seems the m_shareable is only used for an ASSERT. Maybe make it DEBUG-only?

> Index: WebCore/storage/Database.cpp
> +    // DatabaseCloseTask tells Database::close not to do this, so that we can get it over with here.

I had to read this comment a few times to parse... It is better to avoid 'this', 'it' and 'here' since it is not always clear if 'it' relates to the next line, or more, etc.
Could it be a bit more descriptive?

> -void Database::close()
> +void Database::close(bool removeDatabaseFromContext)

It feels that instead of the bool parameter the thread detection could be used... If on database thread, then equivalent to close(true)?

At any case, WebKit always tries to use enum parameters, even if there is only bool semantics - to document code better at call site.

> Index: WebCore/storage/DatabaseTracker.cpp

>  void DatabaseTracker::setDatabaseDirectoryPath(const String& path)
>  {
> -    ASSERT(currentThread() == m_thread);
>      ASSERT(!m_database.isOpen());
>      m_databaseDirectoryPath = path;
>  }

Why this ASSERT can be removed? It can be probably any thread, but should it be at most one thread? Strings are not threadsafe.

>  String DatabaseTracker::trackerDatabasePath() const
>  {
> -    ASSERT(currentThread() == m_thread);
>      return SQLiteFileSystem::appendDatabaseFileNameToPath(m_databaseDirectoryPath, "Databases.db");
>  }

Ditto. If this method can be called from 2 threads, it may or may not cause String's refcount to be racey. At least it's not obvious at this call site and there are multiple port implementations of the call tree below.

>  bool DatabaseTracker::canEstablishDatabase(ScriptExecutionContext* context, const String& name, const String& displayName, unsigned long estimatedSize)
>  {
> -    ASSERT(currentThread() == m_thread);

> +    // Drop all locks before calling out; we don't know what they'll do.
>      context->databaseExceededQuota(name);

It seems in case of multiple threads, once locks are dropped, the m_proposedDatabase will be overwritten.
Is it even right to call this method on any thread different from main? It seems most likely implementation is to pop up some modal UI. That can not be done on any thread.

>  bool DatabaseTracker::hasEntryForOrigin(SecurityOrigin* origin)
>  {
> -    ASSERT(currentThread() == m_thread);
> +    MutexLocker lockDatabase(m_databaseGuard);
>      populateOrigins();
> -    MutexLocker lockQuotaMap(m_quotaMapGuard);
> -    return m_quotaMap->contains(origin);
> +    return hasEntryForOriginNoLock(origin);
>  }

The more I look at his the more I realize how dangerous this code is.
Strings in WebKit are not meant to be used cross thread. It is possible in some very limited cases that local and easily verifiable. DatabaseTracker essentially contains maps that use SecurityOrigins that contain a bunch of Strings. The maps get initialized on one thread, used on another.
Strings are passed in, concatenated, returned - while under local locks, the same Strings are getting accessed on different threads, and it's hard to verify how those strings are used outside of DatabaseTracker. Historically, Strings caused a lot of hard-to-deal-with issues, especially in Workers (since most of the rest of Webkit is basically single-thread). What usually causes it is an implicit copy, or an operator+ (which is sometimes a copy). The refcount goes wrong and causes crashes, sometimes. Normally, it is a (very!) good idea to keep Strings (and SecurityOrigins, etc) to one single thread, and only mix things on a single thread. Both String and SecurityOrigin have methods for that.

I am curious, can we design the tracker, which is essentially a shared database and a few maps, to live always on a single thread, and talk to all other threads via some specific tasks that could be examined separately for proper cross-thread semantics? That would make it possible to look at the code and be reasonably sure that thread-unsafe refcount of Strings won't be a problem. It looks like all operations on DatabaseTracker are serializable. It also could get rid of multiple locks and requirement to always lock stuff in order and have FooNoLock methods.

I think some of the concerns above are big enough to address, perhaps with another patch, so r- for now.

[Eric U.]

Created attachment 50557 [details]
Rolled in feedback, fixed bugs, improved Changelog

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/sql/SQLiteDatabase.cpp:258:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/sql/SQLiteDatabase.cpp:260:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/sql/SQLiteDatabase.cpp:262:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/sql/SQLiteDatabase.cpp:263:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/storage/DatabaseTracker.cpp:351:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/storage/DatabaseTracker.cpp:802:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 6 in 11 files


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

[Eric U.]

Arg--I forgot to check style before uploading.  I'll fix that when I'm back at my desk in a couple of hours.  In the mean time, here's a play-by-play of my changes.

(In reply to comment #4)
> (From update of attachment 49337 [details])
> > Index: WebCore/ChangeLog
> 
> I usually look at the ChangeLog to educate myself on details of the change,
> both 'what' and 'why'. The description here is a bit broad, and frankly looking
> at each file's changes, it's hard to figure out if each of them is correct. It
> would be really easier to review if the bug or ChangeLog tried to explain the
> changes better.

I've fleshed out the Changelog comments and broken out each section by its changes.  How does that look?

> > +    // Set this flag to allow access from multiple threads.  Not all multi-threaded accesses are safe!
> > +    // See http://www.sqlite.org/cvstrac/wiki?p=MultiThreading for more info, and you must be running with compile flags and SQLite version appropriate
> > +    // to your cross-thread uses.
> Are we ensuring compile flags/version somehow? Across major ports? Is it
> possible to assert it?

Added an assert for version; we can't guarantee more than that, really.  You can always foul this up if you try hard enough.  I've added a comment so that you'll know what's going wrong if you're doing odd things.

> If that assert in sqlite3Handle() was meant to ensure that sqlite can be built
> w/o multithreaded flags and now we require so, why not simply remove the ASSERT
> as well?

This class only requires that we compile with threading support if tell it you want to use it that way [by calling setSharable].  If you don't use the Database, but want to use this class, you don't need the threading support.

> > +    bool sharable() { return m_sharable; }
> This method is not used. It seems the m_shareable is only used for an ASSERT.
> Maybe make it DEBUG-only?

Done.

> > Index: WebCore/storage/Database.cpp
> > +    // DatabaseCloseTask tells Database::close not to do this, so that we can get it over with here.
> 
> I had to read this comment a few times to parse... It is better to avoid
> 'this', 'it' and 'here' since it is not always clear if 'it' relates to the
> next line, or more, etc.
> Could it be a bit more descriptive?

How about now?

> > -void Database::close()
> > +void Database::close(bool removeDatabaseFromContext)
> 
> It feels that instead of the bool parameter the thread detection could be
> used... If on database thread, then equivalent to close(true)?

By the time we get here, we're always on the same thread.  The flag really tells us which thread we came from before that.

> At any case, WebKit always tries to use enum parameters, even if there is only
> bool semantics - to document code better at call site.

Changed to an enum.

> > Index: WebCore/storage/DatabaseTracker.cpp
> 
> >  void DatabaseTracker::setDatabaseDirectoryPath(const String& path)
> >  {
> > -    ASSERT(currentThread() == m_thread);
> >      ASSERT(!m_database.isOpen());
> >      m_databaseDirectoryPath = path;
> >  }
> 
> Why this ASSERT can be removed? It can be probably any thread, but should it be
> at most one thread? Strings are not threadsafe.

It could be any execution context thread, now.  More on strings below.

> >  String DatabaseTracker::trackerDatabasePath() const
> >  {
> > -    ASSERT(currentThread() == m_thread);
> >      return SQLiteFileSystem::appendDatabaseFileNameToPath(m_databaseDirectoryPath, "Databases.db");
> >  }
> 
> Ditto. If this method can be called from 2 threads, it may or may not cause
> String's refcount to be racey. At least it's not obvious at this call site and
> there are multiple port implementations of the call tree below.

Here there and everywhere I've now sanitized all strings on input if we're going to store them for later.  I've likewise tried to sanitize all strings on output if they could come from something we've stored.  Please do check me on that to see if I've missed any.

I also had to do the same for the SecurityOrigins in OriginQuotaManager.

> >  bool DatabaseTracker::canEstablishDatabase(ScriptExecutionContext* context, const String& name, const String& displayName, unsigned long estimatedSize)
> >  {
> > -    ASSERT(currentThread() == m_thread);
> 
> > +    // Drop all locks before calling out; we don't know what they'll do.
> >      context->databaseExceededQuota(name);
> 
> It seems in case of multiple threads, once locks are dropped, the
> m_proposedDatabase will be overwritten.

That's why it is now a set of databases instead of a single one; each reentrant caller adds its own proposed database and later cleans it up.

> Is it even right to call this method on any thread different from main? It
> seems most likely implementation is to pop up some modal UI. That can not be
> done on any thread.

It'll get called from the worker's context thread as well.  Each port will naturally have to deal with it in its own way; in Chrome I hear talk of mustering all such messages through the Browser process.

> >  bool DatabaseTracker::hasEntryForOrigin(SecurityOrigin* origin)
> >  {
> > -    ASSERT(currentThread() == m_thread);
> > +    MutexLocker lockDatabase(m_databaseGuard);
> >      populateOrigins();
> > -    MutexLocker lockQuotaMap(m_quotaMapGuard);
> > -    return m_quotaMap->contains(origin);
> > +    return hasEntryForOriginNoLock(origin);
> >  }
> 
> The more I look at his the more I realize how dangerous this code is.
> Strings in WebKit are not meant to be used cross thread. It is possible in some
> very limited cases that local and easily verifiable. DatabaseTracker
> essentially contains maps that use SecurityOrigins that contain a bunch of
> Strings. The maps get initialized on one thread, used on another.
> Strings are passed in, concatenated, returned - while under local locks, the
> same Strings are getting accessed on different threads, and it's hard to verify
> how those strings are used outside of DatabaseTracker. Historically, Strings
> caused a lot of hard-to-deal-with issues, especially in Workers (since most of
> the rest of Webkit is basically single-thread). What usually causes it is an
> implicit copy, or an operator+ (which is sometimes a copy). The refcount goes
> wrong and causes crashes, sometimes. Normally, it is a (very!) good idea to
> keep Strings (and SecurityOrigins, etc) to one single thread, and only mix
> things on a single thread. Both String and SecurityOrigin have methods for
> that.

As said above, Iv'e used those methods to transfer safe copies to the maps.  Those copies are guaranteed not to share internal references/storage with Strings or SecurityOrigins elsewhere, and not to use the ThreadLocal empty String, so we should now be safe from such issues.

> I am curious, can we design the tracker, which is essentially a shared database
> and a few maps, to live always on a single thread, and talk to all other
> threads via some specific tasks that could be examined separately for proper
> cross-thread semantics? That would make it possible to look at the code and be
> reasonably sure that thread-unsafe refcount of Strings won't be a problem. It
> looks like all operations on DatabaseTracker are serializable. It also could
> get rid of multiple locks and requirement to always lock stuff in order and
> have FooNoLock methods.

At a quick glance, while that sounds good, it would be fairly complicated to arrange.  There are too many synchronous calls into the DatabaseTracker from both the context and database threads, and synchronizing cross-thread calls is very deadlock-prone.  There may be a way that we could do a major rewrite that would end up cleaner, but it's not a small task.  I'm afraid we'd just end up with different complexity that we've tested less.

> I think some of the concerns above are big enough to address, perhaps with
> another patch, so r- for now.

As you pointed out, there were a number of real bugs in my first patch.  I think I've removed them; does the general approach here look OK with the new documentation?

[Eric U.]

Created attachment 50571 [details]
Patch

[Dmitry Titov]

This looks very close!
Here is the first half, I'll do the rest soon. I'm not sure all my notes are valid, since I tried to err on a side of caution here.


> Index: WebCore/ChangeLog

Changelog is very good now, thanks!

> +    // Set this flag to allow access from multiple threads.  Not all multi-threaded accesses are safe!
> +    // See http://www.sqlite.org/cvstrac/wiki?p=MultiThreading for more info.
> +#ifndef NDEBUG
> +    bool sharable() { return m_sharable; }
> +#endif
> +    void setSharable(bool sharable);

This variable is used to drop the check that the db is accessed on the same thread where it was created. It does not really allow access, just removes assert. In release build, it doesn't do anything. That's why my first thought last time was that the variable itself together with setSharable should be DEBUG-only. Also, the name could be more specific, perhaps just disableThreadingChecks(), w/o parameters - since there is no real case where it would be set back to 'false'.

Another variant for the name would be assumeSerializedThreadingMode(), following terminology from http://www.sqlite.org/threadsafe.html


> Index: WebCore/storage/DatabaseTracker.cpp

>  void DatabaseTracker::setDatabaseDirectoryPath(const String& path)
>  {
> -    ASSERT(currentThread() == m_thread);
>      ASSERT(!m_database.isOpen());
> -    m_databaseDirectoryPath = path;
> +    m_databaseDirectoryPath = path.threadsafeCopy();

So if we call this from 2 threads, they will create 2 separate copies but then call a copy ctor on the same receiving m_databaseDirectoryPath. The copy constructor is not threadsafe.
Maybe it's ok to use m_databaseGuard for a mutex lock around here?

BTW threadsafeCopy() here maybe replaced with crossThreadString(), but in other places in this file it's not true, so perhaps it's better to use a bigger hammer indeed..

> -const String& DatabaseTracker::databaseDirectoryPath() const
> +String DatabaseTracker::databaseDirectoryPath() const
>  {
> -    ASSERT(currentThread() == m_thread);
> -    return m_databaseDirectoryPath;
> +    return m_databaseDirectoryPath.threadsafeCopy();

Here is the place which actually needs threadsafeCopy() rather then crossThreadString(), so I think using threadsafeCopy() across in this file is a good idea overall. Some comment to this effect may be nice.


>  String DatabaseTracker::trackerDatabasePath() const
>  {
> -    ASSERT(currentThread() == m_thread);
>      return SQLiteFileSystem::appendDatabaseFileNameToPath(m_databaseDirectoryPath, "Databases.db");

If setDatabaseDirectoryPath can be called from another thread in parallel to this (even with the same string value), then this needs a mutex around bacause m_databaseDirectoryPath can change in the middle. 

> +DatabaseDetails DatabaseTracker::detailsForNameAndOrigin(const String& name, SecurityOrigin* origin)

> +        for (HashSet<ProposedDatabase*>::iterator iter = m_proposedDatabases.begin(); iter != m_proposedDatabases.end(); ++iter)
> +            if ((*iter)->first == origin && (*iter)->second.name() == name)
> +                return (*iter)->second;

Does DatabaseDetail need a threadsafeCopy() method? If this method is always called from the "right" thread, we shoudl assert for it, otherwise DatabaseDetails may need same treatment as SecurityOrigins.

>  void DatabaseTracker::setDatabaseDetails(SecurityOrigin* origin, const String& name, const String& displayName, unsigned long estimatedSize)

will look at the rest soon...

[Dmitry Titov]

Comment on attachment 50571 [details]
Patch

Ok, the rest:

>  void DatabaseTracker::setDatabaseDetails(SecurityOrigin* origin, const String& name, const String& displayName, unsigned long estimatedSize)
>  {
> -    ASSERT(currentThread() == m_thread);
> -
>      String originIdentifier = origin->databaseIdentifier();

databaseIdentifier() does not seem to be safe, it inits a global static of type String...

> @@ -428,7 +473,7 @@ void DatabaseTracker::addOpenDatabase(Da
> -        nameMap->set(name, databaseSet);
> +        nameMap->set(name.threadsafeCopy(), databaseSet);

It seems the origin.threadsafeCopy() should be used earlier in this method when an origin gets inserted into a new openDatabaseMap.

> @@ -441,6 +486,7 @@ void DatabaseTracker::removeOpenDatabase

> +    // Throw away m_quotaMap whenever we've cleared out an origin--otherwise we never get rid of stale entries.  The next call that needs it will populateOrigins().
> +    m_quotaMap.clear();

m_quotaMap is protected by m_databaseGuard in other places, but I don't see it locked here.
Also, wouldn't a call to removeOrigin() right before this line remove the origin from the m_quotaMap? How would stale entries happen? Perhaps need a comment on this.

> @@ -614,19 +673,24 @@ void DatabaseTracker::deleteAllDatabases

This method asks for a copy of all origins, using origins() helper, then deletes the local copy on return. Meanwhile, some other thread may modify m_quotaMap (as removeOpenDatabase does). This will make origin refcount racey. Perhaps origins() helper should use threadsafeCopy() instead of straight copy. Also, it seems the origins() is also a public method that gives out the SecurityOrigins.

> +// It is the caller's responsibility to make sure that nobody is trying to create, delete, open, or close databases in this origin while the deletion is
> +// taking place.

Is this possible to request? deleteAllDatabases() is a part of WebKit API, perhaps a browser can call it to 'clean up', responding to a user request in browser UI. At the same time, there can be any number of pages/workers trying to add/open/close databases. What will actually break if that will happen?

This method can probably even reenter since it drops the locks in the middle and does file synchronous IO which may be implemented in a way allowing for reentrancy via WebKit API.

Is it possible to do some refactoring as you did with "NoLock" methods to keep at least m_databaseGuard lock across this method and perhaps a bool inside to prevent reentrance?

>  void DatabaseTracker::deleteOrigin(SecurityOrigin* origin)

>  void DatabaseTracker::deleteDatabase(SecurityOrigin* origin, const String& name)

> +    // We drop the lock here because holding locks during a call to deleteDatabaseFile will deadlock.

Same here.

> +// deleteDatabaseFile has to release locks between looking up the list of databases to close and closing them.  While this is in progress, the caller
> +// is responsible for making sure no new databases are opened in the file to be deleted.
>  bool DatabaseTracker::deleteDatabaseFile(SecurityOrigin* origin, const String& name)

Same here. Need either a mechanism to prevent this or an ASSERT to ensure 'the caller' does the right thing.

I'm going to do r- since some changes to the patch probably will be needed. I tried to mention all things I didn't find easy-to-verify looking at the code. If some of notes are invalid because the code ensures correctness somehow else, please feel free to add ASSERT or comment to that effect.

[Eric U.]

Created attachment 50968 [details]
Roll in more feedback, add protections against simultaneous create+delete of databases.

[Eric U.]

(In reply to comment #9)
> This looks very close!
> Here is the first half, I'll do the rest soon. I'm not sure all my notes are
> valid, since I tried to err on a side of caution here.

I'm happy with that approach.

> This variable is used to drop the check that the db is accessed on the same
> thread where it was created. It does not really allow access, just removes
> assert. In release build, it doesn't do anything. That's why my first thought
> last time was that the variable itself together with setSharable should be
> DEBUG-only. Also, the name could be more specific, perhaps just
> disableThreadingChecks(), w/o parameters - since there is no real case where it
> would be set back to 'false'.

Ah, I see what you mean now.  Changed.  I've left in an empty version if you're compiling in release mode; the compiler can optimize it out, but users won't have to guard their call with ifdefs.

> > Index: WebCore/storage/DatabaseTracker.cpp
> 
> >  void DatabaseTracker::setDatabaseDirectoryPath(const String& path)
> >  {
> > -    ASSERT(currentThread() == m_thread);
> >      ASSERT(!m_database.isOpen());
> > -    m_databaseDirectoryPath = path;
> > +    m_databaseDirectoryPath = path.threadsafeCopy();
> 
> So if we call this from 2 threads, they will create 2 separate copies but then
> call a copy ctor on the same receiving m_databaseDirectoryPath. The copy
> constructor is not threadsafe.
> Maybe it's ok to use m_databaseGuard for a mutex lock around here?

Good catch; I've put in the mutex.

> BTW threadsafeCopy() here maybe replaced with crossThreadString(), but in other
> places in this file it's not true, so perhaps it's better to use a bigger
> hammer indeed..

I'm still a little iffy on when crossThreadString is safe, but it's not safe
everywhere, so I think we're OK erring on the side of safety.  I can't find the code right now; where does crossThreadString make sure that thread-local empty strings don't get carried across?

> >  String DatabaseTracker::trackerDatabasePath() const
> >  {
> > -    ASSERT(currentThread() == m_thread);
> >      return SQLiteFileSystem::appendDatabaseFileNameToPath(m_databaseDirectoryPath, "Databases.db");
> 
> If setDatabaseDirectoryPath can be called from another thread in parallel to
> this (even with the same string value), then this needs a mutex around bacause
> m_databaseDirectoryPath can change in the middle. 

This is only ever called with m_databaseGuard locked, so the new mutex call in
setDatabaseDirectoryPath takes care of it.  I can't add an assert(!tryLock())
because it's const, but it's only got a few calls and they're all safe.

> > +DatabaseDetails DatabaseTracker::detailsForNameAndOrigin(const String& name, SecurityOrigin* origin)
> 
> > +        for (HashSet<ProposedDatabase*>::iterator iter = m_proposedDatabases.begin(); iter != m_proposedDatabases.end(); ++iter)
> > +            if ((*iter)->first == origin && (*iter)->second.name() == name)
> > +                return (*iter)->second;
> 
> Does DatabaseDetail need a threadsafeCopy() method? If this method is always
> called from the "right" thread, we shoudl assert for it, otherwise
> DatabaseDetails may need same treatment as SecurityOrigins.

In the old single-context-thread world it was guaranteed to be the same thread
that created the DatabaseDetails.  In the post-worker-db world I'm not sure if
it'll be needed cross-thread [to let the UI thread look at Worker-created
details] or not; it'll likely be a port-specific thing.  I've added a
per-DatabaseDetails thread identifier and an assert for now.  If we find we need to use DatabaseDetails objects across threads, we can add the threadsafeCopy.

> >  void DatabaseTracker::setDatabaseDetails(SecurityOrigin* origin, const String& name, const String& displayName, unsigned long estimatedSize)
> >  {
> > -    ASSERT(currentThread() == m_thread);
> > -
> >      String originIdentifier = origin->databaseIdentifier();
> 
> databaseIdentifier() does not seem to be safe, it inits a global static of type
> String...

It uses a static global, but emits a new thread-specific String each call.
You must be using the SecurityOrigin on the right thread, so the returned String should be safe.  We're not returning that string--we're just doing a database operation with it--so I think we're OK with that part.  However, regarding that static global--is it safe to concatenate a static global string with various strings on arbitrary threads, or do we need a new copy of the global per thread?

If it's not safe, I think we can just remove the static variable.  It looks like it's just there as an optimization, and I can't think that it's actually saving us all that much time.  [I haven't done this yet--waiting for your response].

> > @@ -428,7 +473,7 @@ void DatabaseTracker::addOpenDatabase(Da
> > -        nameMap->set(name, databaseSet);
> > +        nameMap->set(name.threadsafeCopy(), databaseSet);
> 
> It seems the origin.threadsafeCopy() should be used earlier in this method when
> an origin gets inserted into a new openDatabaseMap.

Fixed.

> > @@ -441,6 +486,7 @@ void DatabaseTracker::removeOpenDatabase
> 
> > +    // Throw away m_quotaMap whenever we've cleared out an origin--otherwise we never get rid of stale entries.  The next call that needs it will populateOrigins().
> > +    m_quotaMap.clear();
> 
> m_quotaMap is protected by m_databaseGuard in other places, but I don't see it
> locked here.

Fixed.

> Also, wouldn't a call to removeOrigin() right before this line remove the
> origin from the m_quotaMap? How would stale entries happen? Perhaps need a
> comment on this.

I'm not sure what you mean about a call to removeOrigin(); the
originQuotaManager uses a different data structure.  However, you're right that the clear call is unnecessary.  I'd needed it during an earlier incarnation of this CL that never made it as far as posting, and I hadn't realized that it was now obsolete.  Thanks!

> > @@ -614,19 +673,24 @@ void DatabaseTracker::deleteAllDatabases
> 
> This method asks for a copy of all origins, using origins() helper, then
> deletes the local copy on return. Meanwhile, some other thread may modify
> m_quotaMap (as removeOpenDatabase does). This will make origin refcount racey.
> Perhaps origins() helper should use threadsafeCopy() instead of straight copy.
> Also, it seems the origins() is also a public method that gives out the
> SecurityOrigins.

SecurityOrigins are ThreadSafeShared; what's racy about the refcounting?

Yes, origins is both public and called by another DatabaseTracker method.  It
and the deletion functions are the only ones with those characteristics.  It
doesn't seem worthwhile to pull out wrappers, given how little they'd do.  The
restrictions on locking during deletions would mean that they'd just be extra
code.

> > +// It is the caller's responsibility to make sure that nobody is trying to create, delete, open, or close databases in this origin while the deletion is
> > +// taking place.
> 
> Is this possible to request? deleteAllDatabases() is a part of WebKit API,
> perhaps a browser can call it to 'clean up', responding to a user request in
> browser UI. At the same time, there can be any number of pages/workers trying
> to add/open/close databases. What will actually break if that will happen?

I'm not sure what the worst thing that could happen is.  You could delete the
file from underneath a working Database, but I'm not sure that would do anything worse than corrupt data.  It might depend on the platform.  However, you could easily end up with a database stored on disk, but deleted from the tracker, such that it would be inaccessible but take up disk space.

I've put in code to guard against this; it took a fair bit of it, unfortunately.  If you try to delete a database while it's being created, or create one while it or its origin is being deleted, you'll fail.  In cases where we can't return an error, you'll also assert in debug builds.

> This method can probably even reenter since it drops the locks in the middle
> and does file synchronous IO which may be implemented in a way allowing for
> reentrancy via WebKit API.
> 
> Is it possible to do some refactoring as you did with "NoLock" methods to keep
> at least m_databaseGuard lock across this method and perhaps a bool inside to
> prevent reentrance?

The problem with holding locks across the deletion calls is that there's a
cross-thread call [from the context to the database thread] to shut each
database before deleting its containing file.  If the database thread has to
wait on the context thread for anything at all, or call back into the tracker
for anything, you'll get a deadlock.  The new state variables that I put in for the aforementioned checks now serve as deadlock-free locks.

[Dmitry Titov]

I think it looks really good now, the 'recordDeleting/doneDeleting and friends look a very good addition, they are easy to read and I think provide good checks.

A few things:

> > >  void DatabaseTracker::setDatabaseDetails(SecurityOrigin* origin, const String& name, const String& displayName, unsigned long estimatedSize)
> > >  {
> > > -    ASSERT(currentThread() == m_thread);
> > > -
> > >      String originIdentifier = origin->databaseIdentifier();
> > 
> > databaseIdentifier() does not seem to be safe, it inits a global static of type
> > String...
> 
> It uses a static global, but emits a new thread-specific String each call.
> You must be using the SecurityOrigin on the right thread, so the returned
> String should be safe.  We're not returning that string--we're just doing a
> database operation with it--so I think we're OK with that part.  However,
> regarding that static global--is it safe to concatenate a static global string
> with various strings on arbitrary threads, or do we need a new copy of the
> global per thread?
> 
> If it's not safe, I think we can just remove the static variable.  It looks
> like it's just there as an optimization, and I can't think that it's actually
> saving us all that much time.  [I haven't done this yet--waiting for your
> response].

I think we should just remove this optimization. It doesn't look like it actually gives a benefit. The way it's implemented, I think it can be dangerous because it multiple threads could access the shared static String, and potentially copy it, as the operator+ can do. Right now it's probably ok but if someone changes operator+ or DEFINE_STATIC_LOCAL it can be broken. I think we better get rid of it.

> > > @@ -614,19 +673,24 @@ void DatabaseTracker::deleteAllDatabases
> > 
> > This method asks for a copy of all origins, using origins() helper, then
> > deletes the local copy on return. Meanwhile, some other thread may modify
> > m_quotaMap (as removeOpenDatabase does). This will make origin refcount racey.
> > Perhaps origins() helper should use threadsafeCopy() instead of straight copy.
> > Also, it seems the origins() is also a public method that gives out the
> > SecurityOrigins.
> 
> SecurityOrigins are ThreadSafeShared; what's racy about the refcounting?
> 
> Yes, origins is both public and called by another DatabaseTracker method.  It
> and the deletion functions are the only ones with those characteristics.  It
> doesn't seem worthwhile to pull out wrappers, given how little they'd do.  The
> restrictions on locking during deletions would mean that they'd just be extra
> code.

Agree. Also, the copy operator for SecurityOrigin is doing threadsafeCopy on inner strings anyways, so it seems we are ok in any case.

Since you'll need cq+, I am doign r- on this patch, if you change the SecurityOrigin::databaseIdentifier() and re-upload, we shoudl be able to use commit bot.

[Dmitry Titov]

Comment on attachment 50968 [details]
Roll in more feedback, add protections against simultaneous create+delete of databases.

r- as mentioned above.

[Eric U.]

Created attachment 51177 [details]
Removed static variable optimization in SecurityOrigin::databaseIdentifier

Note that I haven't added any threadsafeCopy calls for this string, as it doesn't appear necessary.

[Dmitry Titov]

Could you update ChangeLog to include SecurityOrigin.cpp and re-upload?

If you were a committer, I'd simply said do it on landing, but I can't say that to a commit bot... (Hint: more smaller patches move you faster to a committer bit :-)

[Eric U.]

Created attachment 51180 [details]
Added Changelog entry for static variable change.

[Dmitry Titov]

Comment on attachment 51180 [details]
Added Changelog entry for static variable change.

r=me

[WebKit Commit Bot]

Comment on attachment 51180 [details]
Added Changelog entry for static variable change.

Clearing flags on attachment: 51180

Committed r56293: <http://trac.webkit.org/changeset/56293>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.