Bug 62382 – IndexedDB: backingStoreMap is per backing store, not per database

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: backingStoreMap is per backing store, not per database

[Hans Wennborg]

Created attachment 96597 [details]
Patch

[Hans Wennborg]

This fixes a bug introduced in r88358 which could cause the same LevelDB database to opened multiple times, and makes sure that it's possible to use a SQLite backing store even though a LevelDB one has been previously created.

[David Grogan]

Comment on attachment 96597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99
> +            if (hasSQLBackingStore) {

It looks like the old code checked to see if the migration already occurred.  We don't need that anymore?

[Hans Wennborg]

(In reply to comment #3)
> (From update of attachment 96597 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99
> > +            if (hasSQLBackingStore) {
> 
> It looks like the old code checked to see if the migration already occurred.  We don't need that anymore?

I think it makes more sense to always try to migrate if there is a SQLite backing store.

When we flip the switch and make LevelDB the default, the migration test will have problems otherwise: 

- first some tests run with leveldb,
- then the migration test writes some data to sqlite,
- then it opens a leveldb backing store.. and since there already exists a leveldb backing store, it doesn't do migration.

[David Grogan]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 96597 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review
> > 
> > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99
> > > +            if (hasSQLBackingStore) {
> > 
> > It looks like the old code checked to see if the migration already occurred.  We don't need that anymore?
> 
> I think it makes more sense to always try to migrate if there is a SQLite backing store.

Maybe I just don't know how the migration works. Is the sqlite database removed after migration runs?  The case I'm thinking of is where a user:

1) visits an indexeddb-enabled site that stores stuff in sqlite
2) upgrades chrome to a version that uses leveldb
3) revisits that site: migration code runs and the site stores some more stuff in indexeddb/leveldb
4) user later revisits that site: original sqlite database is migrated, blowing away the new stuff stored in (3).

> When we flip the switch and make LevelDB the default, the migration test will have problems otherwise: 
> 
> - first some tests run with leveldb,
> - then the migration test writes some data to sqlite,

> - then it opens a leveldb backing store.. and since there already exists a leveldb backing store, it doesn't do migration.

Because DumpRenderTree would give all the tests the same origin?

[Hans Wennborg]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 96597 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review
> > > 
> > > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99
> > > > +            if (hasSQLBackingStore) {
> > > 
> > > It looks like the old code checked to see if the migration already occurred.  We don't need that anymore?
> > 
> > I think it makes more sense to always try to migrate if there is a SQLite backing store.
> 
> Maybe I just don't know how the migration works. Is the sqlite database removed after migration runs?  The case I'm thinking of is where a user:

Since there's no migration code landed yet, I think we're free to define the best way for it to work :)

I'm thinking that removing the sqlite database after migration would make sense. Or have the migration code be smart enough to only migrate the databases/objectstores/data from sqlite which are not already present in leveldb.

> 
> > When we flip the switch and make LevelDB the default, the migration test will have problems otherwise: 
> > 
> > - first some tests run with leveldb,
> > - then the migration test writes some data to sqlite,
> 
> > - then it opens a leveldb backing store.. and since there already exists a leveldb backing store, it doesn't do migration.
> 
> Because DumpRenderTree would give all the tests the same origin?

Exactly.

[Greg Simon]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (From update of attachment 96597 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review
> > > > 
> > > > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99
> > > > > +            if (hasSQLBackingStore) {
> > > > 
> > > > It looks like the old code checked to see if the migration already occurred.  We don't need that anymore?
> > > 
> > > I think it makes more sense to always try to migrate if there is a SQLite backing store.
> > 
> > Maybe I just don't know how the migration works. Is the sqlite database removed after migration runs?  The case I'm thinking of is where a user:
> 
> Since there's no migration code landed yet, I think we're free to define the best way for it to work :)
> 
> I'm thinking that removing the sqlite database after migration would make sense. Or have the migration code be smart enough to only migrate the databases/objectstores/data from sqlite which are not already present in leveldb.


It would not be difficult to migrate only object stores that do not exist. 
 
> > 
> > > When we flip the switch and make LevelDB the default, the migration test will have problems otherwise: 
> > > 
> > > - first some tests run with leveldb,
> > > - then the migration test writes some data to sqlite,
> > 
> > > - then it opens a leveldb backing store.. and since there already exists a leveldb backing store, it doesn't do migration.
> > 
> > Because DumpRenderTree would give all the tests the same origin?
> 
> Exactly.

[Hans Wennborg]

(In reply to comment #7)
> > I'm thinking that removing the sqlite database after migration would make sense. Or have the migration code be smart enough to only migrate the databases/objectstores/data from sqlite which are not already present in leveldb.
> 
> 
> It would not be difficult to migrate only object stores that do not exist. 

That would be great

[David Grogan]

LGTM

(In reply to comment #8)
> (In reply to comment #7)
> > > I'm thinking that removing the sqlite database after migration would make sense. Or have the migration code be smart enough to only migrate the databases/objectstores/data from sqlite which are not already present in leveldb.
> > 
> > 
> > It would not be difficult to migrate only object stores that do not exist. 
> 
> That would be great

Either of smart migration or delete-after-migrate sounds good to me.

[Tony Gentilcore]

Comment on attachment 96597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:125
> +    return false; // FIXME: To be implemented.

If you want, could replace this with a notImplemented() call which will just log.

[Hans Wennborg]

Committed r89130: <http://trac.webkit.org/changeset/89130>