[webkit-reviews] review denied: [Bug 34991] Refactor DatabaseTracker.cpp for thread safety : [Attachment 50571] 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 50571: Patch
https://bugs.webkit.org/attachment.cgi?id=50571&action=review

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