[webkit-reviews] review denied: [Bug 27836] Stale database version persists through browser refresh (changeVersion doesn't work) : [Attachment 39426] Patch incorporating Alexey's test

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Ben Murdoch
<benm at google.com>'s request for review:
Bug 27836: Stale database version persists through browser refresh
(changeVersion doesn't work)
https://bugs.webkit.org/show_bug.cgi?id=27836

Attachment 39426: Patch incorporating Alexey's test
https://bugs.webkit.org/attachment.cgi?id=39426&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> Index: WebCore/storage/Database.cpp
> ===================================================================
> --- WebCore/storage/Database.cpp	(revision 48292)
> +++ WebCore/storage/Database.cpp	(working copy)
> @@ -328,6 +314,20 @@ void Database::close()
>	   m_sqliteDatabase.close();
>	   m_document->databaseThread()->recordDatabaseClosed(this);
>	   m_opened = false;
> +
> +	   {
> +	       MutexLocker locker(guidMutex());
> +
> +	       HashSet<Database*>* hashSet = guidToDatabaseMap().get(m_guid);
> +	       ASSERT(hashSet);
> +	       ASSERT(hashSet->contains(this));
> +	       hashSet->remove(this);
> +	       if (hashSet->isEmpty()) {
> +		   guidToDatabaseMap().remove(m_guid);
> +		   delete hashSet;
> +		   guidToVersionMap().remove(m_guid);
> +	       }
> +	   }
>      }
>  }
>  

I think moving this code from the Database destructor to the close() method is
fine.  (Running all database tests with a debug build of WebKit after this
change would be a really good idea.  Have you done that?)

> @@ -631,6 +631,9 @@ Vector<String> Database::tableNames()
>  void Database::setExpectedVersion(const String& version)
>  {
>      m_expectedVersion = version.copy();
> +    // Update the in memory database version map.
> +    MutexLocker locker(guidMutex());
> +    guidToVersionMap().set(m_guid, version);
>  }

There is similar GUID-setting code in Database::performOpenAndVerify() that
does two things this code does not:

1. It maps an empty string to a null string before updating the
guidToVersionMap().
2. It makes a defensive copy of the version string (if it is not an empty
string).

I think it would make sense to extract this code into its own static inline
method (perhaps after the guidToVersionMap() method itself) and update the two
call sites to use it.

r- to fix Database::setExpectedVersion().