Summary: | IndexedDB: backingStoreMap is per backing store, not per database | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hans Wennborg <hans> | ||||
Component: | New Bugs | Assignee: | 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
Hans Wennborg
2011-06-09 09:31:48 PDT
Created attachment 96597 [details]
Patch
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 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? (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. (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? (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. (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. (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 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 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. Committed r89130: <http://trac.webkit.org/changeset/89130> |