Bug 62382

Summary: IndexedDB: backingStoreMap is per backing store, not per database
Product: WebKit Reporter: Hans Wennborg <hans>
Component: New BugsAssignee: Hans Wennborg <hans>
Status: RESOLVED FIXED    
Severity: Normal CC: dgrogan, gregsimon, tonyg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 62780    
Attachments:
Description Flags
Patch tonyg: review+

Description Hans Wennborg 2011-06-09 09:31:48 PDT
IndexedDB: backingStoreMap is per backing store, not per database
Comment 1 Hans Wennborg 2011-06-09 09:41:10 PDT
Created attachment 96597 [details]
Patch
Comment 2 Hans Wennborg 2011-06-09 09:43:49 PDT
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.
Comment 3 David Grogan 2011-06-10 23:57:25 PDT
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?
Comment 4 Hans Wennborg 2011-06-13 01:41:33 PDT
(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.
Comment 5 David Grogan 2011-06-13 17:09:09 PDT
(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?
Comment 6 Hans Wennborg 2011-06-14 03:22:52 PDT
(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.
Comment 7 Greg Simon 2011-06-14 09:56:10 PDT
(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.
Comment 8 Hans Wennborg 2011-06-14 10:03:41 PDT
(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
Comment 9 David Grogan 2011-06-14 14:44:11 PDT
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.
Comment 10 Tony Gentilcore 2011-06-17 01:53:50 PDT
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.
Comment 11 Hans Wennborg 2011-06-17 02:44:36 PDT
Committed r89130: <http://trac.webkit.org/changeset/89130>