Stale database version persists through browser refresh (changeVersion doesn't work)

• Component: New Bugs
• Status: RESOLVED FIXED

[Matthew Bolohan]

Created attachment 33778 [details]
Script that demonstrates incorrect caching of database version

r46507

The version property of the database object is stale after invoking changeVersion() and remains stale after a refresh. Killing webkit results in a correct value for db.version after calling window.openDatabase().

I have attached a test script that demonstrates this behavior. The script performs the following steps:

1) open a new database without a version restriction
2) check if the database version matches a predefined string constant (report success if the versions match)
3) if the versions don't match, change the database version to the predefined version constant
4) reload the page

Expected behavior:
- version doesn't match
- change version (alert changeVersion success and reload)
- version matches (alert success with version)

Observed behavior:
- version doesn't match
- change version (alert changeVersion success and reload)
- version doesn't match
- change version (alert Error: current version of the database and `oldVersion` argument do not match)

Note: Killing webkit and loading the script results in a successful version match.

[Alexey Proskuryakov]

<rdar://problem/5737246>

[Alexey Proskuryakov]

<rdar://problem/5556376> even.

[Alexey Proskuryakov]

*** Bug 28418 has been marked as a duplicate of this bug. ***

[Ben Murdoch]

I think I know what is happening here and will send a patch shortly.

[Ben Murdoch]

Created attachment 39150 [details]
Proposed patch with layout test.

This patch fixes this bug and 24818 which although similar is actually not a duplicate.

The problem described in this bug is caused by the database only being removed from the guid->version map in the Database dtor. The dtor is only called when the Database thread is destroyed which may come after the page has been unloaded and the next page loaded. Thus in the test script attached to this bug, the map still contains the old database version number when the page is reloaded because the dtor has at that point not been run. This is fixed by moving the code from the dtor into the Database::close() method.

The bug described in 24818 comes from the fact that in the Database::setExcpectedVersion (called from the ChangeVersionWrapper postflight code) we do not update the guid->version map. This is fixed by adding code to update the map at that point.

I've also implemented the changeVersion function in the V8 bindings and written a layout test that is a combination of the test attached to this bug and that attached to 24818.

[Alexey Proskuryakov]

Created attachment 39154 [details]
another test

FWIW, here is a test that I had sitting in my tree disabled for quite a while.

[Ben Murdoch]

(In reply to comment #6)
> Created an attachment (id=39154) [details]
> another test
> 
> FWIW, here is a test that I had sitting in my tree disabled for quite a while.

Thanks Alexey, confirming that that test seems to work fine with the fix applied (displays the foobar error and no asserts on reload). Would you like me to add it to the patch?

Cheers, Ben

[Alexey Proskuryakov]

Not sure, I haven't compared it with your test carefully enough. If you think there's something additional that it tests, then adding it would be great.

[Ben Murdoch]

(In reply to comment #8)
> Not sure, I haven't compared it with your test carefully enough. If you think
> there's something additional that it tests, then adding it would be great.

I think your test is valuable in addition to mine as it verifies that the DB handle stays alive (my test opens new handles). After I get some review feedback I will add your test into the patch.

Cheers, Ben

[Ben Murdoch]

cc'ing dglazkov as part of this patch is an edit to V8 bindings.

[Ben Murdoch]

ping...

[Adam Barth]

Comment on attachment 39150 [details]
Proposed patch with layout test.

The V8 parts of this patch look fine to me.  I'm not that confident in the non-V8 parts, but I'm presuming AP has looked them over.

[Alexey Proskuryakov]

> I'm presuming AP has looked them over.

I did not. I'm not very familiar with this code.

[Adam Barth]

Comment on attachment 39150 [details]
Proposed patch with layout test.

Hum...  Ok.  Then I'm putting this back to r?.  The V8 parts are fine.  We need to find someone familiar with database to look at the rest of the patch.

[Ben Murdoch]

(In reply to comment #14)
> (From update of attachment 39150 [details])
> Hum...  Ok.  Then I'm putting this back to r?.  The V8 parts are fine.  We need
> to find someone familiar with database to look at the rest of the patch.

Thanks for looking over the V8 portion Adam. There have been a few patches to the WebCore storage code since I sent this patch so I need to regenerate it. dglazkov seems to have reviewed those refactorings, so perhaps he could review this one too? I will also cc Brady when I send the new patch because AFAIK he wrote the code originally.

Cheers, Ben

[Ben Murdoch]

Seems that the recent DB refactorings didn't touch the same areas as this patch as it still applied after a sync. I've added ap's test into the patch now and will upload a new version.

[Ben Murdoch]

Created attachment 39426 [details]
Patch incorporating Alexey's test

New patch generated more recently and including Alexey's test.

[Ben Murdoch]

Brady, would you mind taking a look at this patch?

Many thanks, Ben

[Ben Murdoch]

Ping... anyone able to take a look at this?

Thanks, Ben

[Eric Seidel (no email)]

Comment on attachment 39426 [details]
Patch incorporating Alexey's test

Why not just put:
178      {
179          MutexLocker locker(guidMutex());
180  
181          HashSet<Database*>* hashSet = guidToDatabaseMap().get(m_guid);
182          ASSERT(hashSet);
183          ASSERT(hashSet->contains(this));
184          hashSet->remove(this);
185          if (hashSet->isEmpty()) {
186              guidToDatabaseMap().remove(m_guid);
187              delete hashSet;
188              guidToVersionMap().remove(m_guid);
189          }
190      }
191  
into a static inline function?

Also, I think Alexey is really your man here.

[Ben Murdoch]

(In reply to comment #20)

Hi Eric,

Thanks for looking at my patch!

> (From update of attachment 39426 [details])
> Why not just put:
> 178      {
> 179          MutexLocker locker(guidMutex());
> 180  
> 181          HashSet<Database*>* hashSet = guidToDatabaseMap().get(m_guid);
> 182          ASSERT(hashSet);
> 183          ASSERT(hashSet->contains(this));
> 184          hashSet->remove(this);
> 185          if (hashSet->isEmpty()) {
> 186              guidToDatabaseMap().remove(m_guid);
> 187              delete hashSet;
> 188              guidToVersionMap().remove(m_guid);
> 189          }
> 190      }
> 191  
> into a static inline function?

As this code is only called once and my intention was to just move it from the destructor to the close() method, it's not clear to me what moving it into a static inline would buy us. Could you elaborate?

> Also, I think Alexey is really your man here.

Alexey already looked at it and said he wasn't familiar enough with the code to review it (comment 13 above). So could you please help me with getting this reviewed? Adam has OK'd the V8 portion (comment 12).

Cheers, Ben

[David Kilzer (:ddkilzer)]

Comment on attachment 39426 [details]
Patch incorporating Alexey's test

> 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().

[Dumitru Daniliuc]

the DB changes look good to me. thanks for catching this problem and fixing it!

minor: please add a short comment to Database::setVersionInDatabase() explaining that INSERT will replace the old version because that's how the table is defined.

> Index: LayoutTests/storage/change-version-expected.txt
> ===================================================================
> --- LayoutTests/storage/change-version-expected.txt	(revision 0)
> +++ LayoutTests/storage/change-version-expected.txt	(revision 0)
> @@ -0,0 +1,4 @@
> +This test verifies that the database.changeVersion function works as expected.

minor: Database::changeVersion().

> Property changes on: LayoutTests/storage/change-version-expected.txt
> ___________________________________________________________________
> Added: svn:eol-style
>    + native

i don't think these are necessary.

> Index: LayoutTests/storage/change-version-handle-reuse.html
> +function finishTest()
> +{
> +	log("TEST COMPLETE.");

replace tab with spaces.

> +function runTest()
> +{
> +    if (window.layoutTestController) {
> +	    layoutTestController.waitUntilDone();

tab --> spaces.

> Index: LayoutTests/storage/change-version.html
> +function changeVersionCallback(tx)
> +{
> +    tx.executeSql('drop table if exists info;', [], emptyFunction, emptyFunction);
> +    tx.executeSql('create table if not exists info (version INTEGER);',[], emptyFunction, emptyFunction);
> +    tx.executeSql('insert into info values(?);', [EXPECTED_VERSION_AFTER_RELOAD], emptyFunction, emptyFunction);
> +}

minor: i believe webkit code and tests always capitalize keywords such as DROP, CREATE TABLE, INSERT, etc. and uses double-quotes (") for strings.

> +function runTest()
> +{
> +    if (window.location.search == "?2") {
> +        db = window.openDatabase('changeversion-test', '', 'Test for the database.changeVersion() function', 1024);
> +        log('Finished tests with version ' + db.version);

minor: maybe change the log message to something like:

log("Finished tests with version " + db.version + "; expected version: " + EXPECTED_VERSION_AFTER_RELOAD);

> +        finishTest();
> +    } else {
> +        testPart1();
> +    }

1-line else: remove {}.

minor: maybe put the if-block into a testPart2() function?

> +function testPart1() {
> +    if (window.layoutTestController) {
> +        layoutTestController.clearAllDatabases();
> +        layoutTestController.dumpAsText();
> +        layoutTestController.waitUntilDone();
> +    }
> +    
> +    db = window.openDatabase('changeversion-test', '', 'Test for the database.changeVersion() function', 1024);
> +
> +    if (db.version != EXPECTED_VERSION_AFTER_RELOAD) {
> +        // First run Hixie's test to ensure basic changeVersion functionality works (see bug 28418).
> +        db.changeVersion('', EXPECTED_VERSION_AFTER_HIXIE_TEST, emptyFunction, function (e) {
> +            log('FAIL in changeVersion:' + e);
> +            finishTest();
> +        }, function () {
> +            try {
> +                var db2 = openDatabase('change-version-test', EXPECTED_VERSION_AFTER_HIXIE_TEST, '', 0);
> +            } catch (e) {
> +                log('FAIL in openDatabase: ' + e);
> +                finishTest();
> +            }
> +            // Following versions should match.
> +            log('Database version: ' + db.version);
> +            log('Second database version: ' + db2.version);

instead of expecting the user to know that the two versions are supposed to match, maybe replace these lines with something like this?

if (db.version == db2.version)
    log("db.version(" + db.version + ") matches db2.version(" + db2.version + ") as expected.");
else
    log("db.version(" + db.version + ") does not db2.version(" + db2.version + "): failing.");

[Ben Murdoch]

Thank you for the feedback David and Dumi. I'll incorporate your comments into a new patch and upload.

Cheers!

[Ben Murdoch]

(In reply to comment #22)
> (Running all database tests with a debug build of WebKit after this
> change would be a really good idea.  Have you done that?)

Yes, I've run the storage layout tests on Mac and Windows and they all pass.

> 
> 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.
> 

Fixed.


(In reply to comment #23)
> minor: please add a short comment to Database::setVersionInDatabase()
> explaining that INSERT will replace the old version because that's how the
> table is defined.

Done.

> > Index: LayoutTests/storage/change-version-expected.txt
> > ===================================================================
> > --- LayoutTests/storage/change-version-expected.txt	(revision 0)
> > +++ LayoutTests/storage/change-version-expected.txt	(revision 0)
> > @@ -0,0 +1,4 @@
> > +This test verifies that the database.changeVersion function works as expected.
>
>minor: Database::changeVersion().

IMHO as it's the JS function we're testing, I think database.changeVersion is fine.

>
> > Property changes on: LayoutTests/storage/change-version-expected.txt
> > ___________________________________________________________________
> > Added: svn:eol-style
> >    + native
> 
> i don't think these are necessary.

Hmm, I was advised on a past review that they should be set. Keeping for now.

> 
> > Index: LayoutTests/storage/change-version-handle-reuse.html
> > +function finishTest()
> > +{
> > +	log("TEST COMPLETE.");
> 
> replace tab with spaces.

Done.

> 
> > +function runTest()
> > +{
> > +    if (window.layoutTestController) {
> > +	    layoutTestController.waitUntilDone();
> 
> tab --> spaces.

Done.

> 
> > Index: LayoutTests/storage/change-version.html
> > +function changeVersionCallback(tx)
> > +{
> > +    tx.executeSql('drop table if exists info;', [], emptyFunction, emptyFunction);
> > +    tx.executeSql('create table if not exists info (version INTEGER);',[], emptyFunction, emptyFunction);
> > +    tx.executeSql('insert into info values(?);', [EXPECTED_VERSION_AFTER_RELOAD], emptyFunction, emptyFunction);
> > +}
> 
> minor: i believe webkit code and tests always capitalize keywords such as DROP,
> CREATE TABLE, INSERT, etc. and uses double-quotes (") for strings.

Done.

> 
> > +function runTest()
> > +{
> > +    if (window.location.search == "?2") {
> > +        db = window.openDatabase('changeversion-test', '', 'Test for the database.changeVersion() function', 1024);
> > +        log('Finished tests with version ' + db.version);
> 
> minor: maybe change the log message to something like:
> 
> log("Finished tests with version " + db.version + "; expected version: " +
> EXPECTED_VERSION_AFTER_RELOAD);

Done.

> 
> > +        finishTest();
> > +    } else {
> > +        testPart1();
> > +    }
> 
> 1-line else: remove {}.

Fixed.

> 
> minor: maybe put the if-block into a testPart2() function?
> 
> > +function testPart1() {
> > +    if (window.layoutTestController) {
> > +        layoutTestController.clearAllDatabases();
> > +        layoutTestController.dumpAsText();
> > +        layoutTestController.waitUntilDone();
> > +    }
> > +    
> > +    db = window.openDatabase('changeversion-test', '', 'Test for the database.changeVersion() function', 1024);
> > +
> > +    if (db.version != EXPECTED_VERSION_AFTER_RELOAD) {
> > +        // First run Hixie's test to ensure basic changeVersion functionality works (see bug 28418).
> > +        db.changeVersion('', EXPECTED_VERSION_AFTER_HIXIE_TEST, emptyFunction, function (e) {
> > +            log('FAIL in changeVersion:' + e);
> > +            finishTest();
> > +        }, function () {
> > +            try {
> > +                var db2 = openDatabase('change-version-test', EXPECTED_VERSION_AFTER_HIXIE_TEST, '', 0);
> > +            } catch (e) {
> > +                log('FAIL in openDatabase: ' + e);
> > +                finishTest();
> > +            }
> > +            // Following versions should match.
> > +            log('Database version: ' + db.version);
> > +            log('Second database version: ' + db2.version);
> 
> instead of expecting the user to know that the two versions are supposed to
> match, maybe replace these lines with something like this?
> 
> if (db.version == db2.version)
>     log("db.version(" + db.version + ") matches db2.version(" + db2.version +
> ") as expected.");
> else
>     log("db.version(" + db.version + ") does not db2.version(" + db2.version +
> "): failing.");

Done.

New patch is on it's way!

[Ben Murdoch]

Created attachment 40445 [details]
Revised patch.

[Dumitru Daniliuc]

Comment on attachment 40445 [details]
Revised patch.

LGTM, with 2 minor comments below. I'm not a reviewer though, so I can't r+ the patch.

> Index: WebCore/storage/Database.cpp
> ===================================================================
> --- WebCore/storage/Database.cpp	(revision 48292)
> +++ WebCore/storage/Database.cpp	(working copy)
> @@ -89,6 +89,18 @@ static GuidVersionMap& guidToVersionMap(
>      return map;
>  }
>  
> +static inline void updateGuidVersionMap(int guid, String newVersion)
> +{
> +    // Note: It is not safe to put an empty string into the guidToVersionMap() map.
> +    // That's because the map is cross-thread, but empty strings are per-thread.
> +    // The copy() function makes a version of the string you can use on the current
> +    // thread, but we need a string we can keep in a cross-thread data structure.
> +    // FIXME: This is a quite-awkward restriction to have to program with.
> +
> +    // NOTE: Caller must lock guidMutex().

optional: might be a good idea to make this a function-level comment (ie. put it right before 'static inline void ...').

> Index: LayoutTests/storage/change-version.html
> ===================================================================
> +            // The two database versions should match.
> +            if (db.version == db2.version)
> +                log("PASS: db.version(" + db.version + ") matches db2.version(" + db2.version +") as expected.");
> +            else
> +                log("FAIL: db.version(" + db.version + ") does not db2.version(" + db2.version +")");

typo (that i had in my comment too :)): does not MATCH db2.version

[Ben Murdoch]

(In reply to comment #27)

Thanks Dumi -- if David is happy with the patch and can r+ I will address your comments when I commit.

Cheers, Ben


> (From update of attachment 40445 [details])
> LGTM, with 2 minor comments below. I'm not a reviewer though, so I can't r+ the
> patch.
> 
> > Index: WebCore/storage/Database.cpp
> > ===================================================================
> > --- WebCore/storage/Database.cpp	(revision 48292)
> > +++ WebCore/storage/Database.cpp	(working copy)
> > @@ -89,6 +89,18 @@ static GuidVersionMap& guidToVersionMap(
> >      return map;
> >  }
> >  
> > +static inline void updateGuidVersionMap(int guid, String newVersion)
> > +{
> > +    // Note: It is not safe to put an empty string into the guidToVersionMap() map.
> > +    // That's because the map is cross-thread, but empty strings are per-thread.
> > +    // The copy() function makes a version of the string you can use on the current
> > +    // thread, but we need a string we can keep in a cross-thread data structure.
> > +    // FIXME: This is a quite-awkward restriction to have to program with.
> > +
> > +    // NOTE: Caller must lock guidMutex().
> 
> optional: might be a good idea to make this a function-level comment (ie. put
> it right before 'static inline void ...').
> 
> > Index: LayoutTests/storage/change-version.html
> > ===================================================================
> > +            // The two database versions should match.
> > +            if (db.version == db2.version)
> > +                log("PASS: db.version(" + db.version + ") matches db2.version(" + db2.version +") as expected.");
> > +            else
> > +                log("FAIL: db.version(" + db.version + ") does not db2.version(" + db2.version +")");
> 
> typo (that i had in my comment too :)): does not MATCH db2.version

[David Kilzer (:ddkilzer)]

(In reply to comment #27)
> (From update of attachment 40445 [details])
> > Index: WebCore/storage/Database.cpp
> > ===================================================================
> > --- WebCore/storage/Database.cpp	(revision 48292)
> > +++ WebCore/storage/Database.cpp	(working copy)
> > @@ -89,6 +89,18 @@ static GuidVersionMap& guidToVersionMap(
> >      return map;
> >  }
> >  
> > +static inline void updateGuidVersionMap(int guid, String newVersion)
> > +{
> > +    // Note: It is not safe to put an empty string into the guidToVersionMap() map.
> > +    // That's because the map is cross-thread, but empty strings are per-thread.
> > +    // The copy() function makes a version of the string you can use on the current
> > +    // thread, but we need a string we can keep in a cross-thread data structure.
> > +    // FIXME: This is a quite-awkward restriction to have to program with.
> > +
> > +    // NOTE: Caller must lock guidMutex().
> 
> optional: might be a good idea to make this a function-level comment (ie. put
> it right before 'static inline void ...').

I prefer the comment inside the method as it is above.  Putting it outside the method makes it harder to visually parse the file when looking for the method name (IMO).

Reviewing the new patch now.

[David Kilzer (:ddkilzer)]

Comment on attachment 40445 [details]
Revised patch.

> Index: WebCore/ChangeLog
> [...]
> +        * bindings/v8/custom/V8DatabaseCustom.cpp:
> +        (WebCore::CALLBACK_FUNC_DECL): implement the V8 binding for database.changeVersion
> +        (WebCore::createTransaction): Fix a bug that was checking the wrong argument index to save the success callback

Would be nice if these comments both started with capital letters and ended with periods (like the ones below).

> +        * storage/Database.cpp:
> +        (WebCore::updateGuidVersionMap): Safely update the Guid/version hash map.
> +        (WebCore::Database::~Database): Remove code that removes the database from the guid->database and guid->version maps.
> +        (WebCore::Database::setVersionInDatabase): Add a comment to explain some behaviour.
> +        (WebCore::Database::close): Move the code that updates the maps from the destructor to here.
> +        (WebCore::Database::performOpenAndVerify): Call updateGuidVersionMap instead of setting the hash map directly.
> +        (WebCore::Database::setExpectedVersion): Update the in memory guid->version map when we want to update the database version.


> Index: WebCore/storage/Database.cpp
> [...]
> +static inline void updateGuidVersionMap(int guid, String newVersion)
> +{
> +    // Note: It is not safe to put an empty string into the guidToVersionMap() map.
> +    // That's because the map is cross-thread, but empty strings are per-thread.
> +    // The copy() function makes a version of the string you can use on the current
> +    // thread, but we need a string we can keep in a cross-thread data structure.
> +    // FIXME: This is a quite-awkward restriction to have to program with.
> +
> +    // NOTE: Caller must lock guidMutex().

Oops, I see what Dumi meant.  This one-line comment could go above the 'static inline' method declaration.

It would be nice to add this comment above the code (in case someone skips over the large block above):

    // Map null string to empty string (see comment above).

> +    guidToVersionMap().set(guid, newVersion.isEmpty() ? String() : newVersion.copy());
> +}

>  bool Database::setVersionInDatabase(const String& version)
>  {
> +    // The INSERT will replace an existing entry for the database with the new version number, due to the UNIQUE ON CONFLICT REPLACE
> +    // clause in the CREATE statement (see Database::performOpenAndVerify)

This comment needs a period.  Would be nice to add "()" after "performOpenAndVerify" so you know you're looking for a method.

> @@ -447,12 +461,6 @@ bool Database::performOpenAndVerify(Exce
>      {
>          MutexLocker locker(guidMutex());
>  
> -        // Note: It is not safe to put an empty string into the guidToVersionMap() map.
> -        // That's because the map is cross-thread, but empty strings are per-thread.
> -        // The copy() function makes a version of the string you can use on the current
> -        // thread, but we need a string we can keep in a cross-thread data structure.
> -        // FIXME: This is a quite-awkward restriction to have to program with.
> -
>          GuidVersionMap::iterator entry = guidToVersionMap().find(m_guid);
>          if (entry != guidToVersionMap().end()) {
>              // Map null string to empty string (see comment above).

This comment needs to change since you moved the big comment:

    // Map null string to empty string (see updateGuidVersionMap()).

r=me with the above changes.

[Ben Murdoch]

Landed as r49018.