[webkit-reviews] review denied: [Bug 34991] Refactor DatabaseTracker.cpp for thread safety : [Attachment 49337] Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 34991: Refactor DatabaseTracker.cpp for thread safety
https://bugs.webkit.org/show_bug.cgi?id=34991

Attachment 49337: Patch
https://bugs.webkit.org/attachment.cgi?id=49337&action=review

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