WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
62382
IndexedDB: backingStoreMap is per backing store, not per database
https://bugs.webkit.org/show_bug.cgi?id=62382
Summary
IndexedDB: backingStoreMap is per backing store, not per database
Hans Wennborg
Reported
2011-06-09 09:31:48 PDT
IndexedDB: backingStoreMap is per backing store, not per database
Attachments
Patch
(7.94 KB, patch)
2011-06-09 09:41 PDT
,
Hans Wennborg
tonyg
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Hans Wennborg
Comment 1
2011-06-09 09:41:10 PDT
Created
attachment 96597
[details]
Patch
Hans Wennborg
Comment 2
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.
David Grogan
Comment 3
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?
Hans Wennborg
Comment 4
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.
David Grogan
Comment 5
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?
Hans Wennborg
Comment 6
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.
Greg Simon
Comment 7
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.
Hans Wennborg
Comment 8
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
David Grogan
Comment 9
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.
Tony Gentilcore
Comment 10
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.
Hans Wennborg
Comment 11
2011-06-17 02:44:36 PDT
Committed
r89130
: <
http://trac.webkit.org/changeset/89130
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug