Control Indexeddb backends from LayoutTestController
Created attachment 93837 [details] Patch
This is the first in a two-part change to get migration in IndexedDB into the build. This specific change is adds an API to LayoutTestController on chromium so a unit test can be written (attached) that will exercise the migration of data from the SQLite backend to the LeveLDB backend.
Comment on attachment 93837 [details] Patch Attachment 93837 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8710269 New failing tests: storage/indexeddb/migrate-basics.html
Created attachment 93842 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 93872 [details] Patch
Comment on attachment 93872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93872&action=review > LayoutTests/ChangeLog:9 > + to be used for IndexedDB so a unit test can be written s/unit test/layout test/ throughout; we have unit tests too, e.g. in Source/WebKit/chromium/tests/ > LayoutTests/storage/indexeddb/migrate-basics.html:96 > + } catch( ex ) { i suppose you get an exception here because trans.objectStore("store") returns null, and then store.openCursor(keyRange) fails. I think it would be nicer to do an explicit check that "store" exists in the database. That would make it easier to see why it fails. > LayoutTests/storage/indexeddb/migrate-basics.html:105 > + if(!cursor) { ultra nit: space between if and ( > LayoutTests/storage/indexeddb/migrate-basics.html:110 > + if (cursor.value.text != "testing 1,2,3") use the shouldBe() function that we use in the other layout tests > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:90 > + if (backingStoreType == DefaultBackingStore no need for a newline here > Source/WebKit/chromium/public/WebIDBFactory.h:65 > + // Used for DumpRenderTree tests ultra nit: period at end of comment. > Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:36 > +#include <base/memory/scoped_temp_dir.h> can we really use that here? isn't this a header in Chromium? > Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:49 > +static ScopedTempDir tempLevelDBDir; ScopedTempDir is a non-POD type, so I don't think it's allowed as a static variable > Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:64 > + LOG_ERROR("Failed to clear LevelDB temporary folder."); doesn't ScopedTempDir delete the dir on destruction? > Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:83 > + && backingStoreType == LevelDBBackingStore) no need for newline > Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:84 > + { curly brace goes on same line as the if > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:36 > +#include "WebIDBFactory.h" alpha order > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:158 > + bindMethod("setOverrideIndexedDBBackingStore", &LayoutTestController::setOverrideIndexedDBBackingStore); these statements look like they should be sorted
Created attachment 94368 [details] Patch
Comment on attachment 94368 [details] Patch This patch is waiting for the DEPS roll of chromium https://bugs.webkit.org/show_bug.cgi?id=61236
Comment on attachment 94368 [details] Patch Attachment 94368 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8723727
Comment on attachment 94368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94368&action=review > LayoutTests/ChangeLog:7 > + can be implemented and tested. I think this descriptive text is usually put after the bugzilla url below. > Source/WebCore/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=61000 Should probably have a description here too > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:80 > + // Also check that backing store types match: this is important for migration period. > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:82 > + && (backingStoreType == it->second->backingStore()->backingStoreType())) { no need for newline > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:96 > + bool hasSqlBackend = IDBSQLiteBackingStore::backingStoreExists(securityOrigin.get(), dataDir); i think this should be named hasSQLBackend.. same for hasLevelDBBackend below > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:97 > + bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir); hmm, now we're doing LevelDB stuff outside of the #if ENABLE(LEVELDB) guard.. i'm not sure that's a good idea > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:102 > + // Migration: if the database exists and is SQLite we want to migrate it to LevelDB period. > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1276 > + if (!makeAllDirectories(pathBase)) { we probably don't need this? > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1281 > + // FIXME: We should eventually use the same LevelDB database for all origins period. > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1284 > + // FIXME: It would be more thorough to open the database here but also more expensive yes, i think this is good enough. period after comment. > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996 > + if (!makeAllDirectories(pathBase)) { we probably don't need this? > Source/WebKit/chromium/public/WebIDBFactory.h:65 > + // Used for DumpRenderTree tests period. > Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:81 > +{ hmm, shouldn't this get passed on somewhere? > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:163 > + bindMethod("setOverrideIndexedDBBackingStore", &LayoutTestController::setOverrideIndexedDBBackingStore); one line further down to keep it sorted i believe > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:233 > + delete m_tempFolder; can we use an OwnPtr instead so we don't have to call delete explicitly?
Created attachment 94446 [details] Patch
(In reply to comment #10) > (From update of attachment 94368 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94368&action=review > > > LayoutTests/ChangeLog:7 > > + can be implemented and tested. > > I think this descriptive text is usually put after the bugzilla url below. Fixed > > Source/WebCore/ChangeLog:6 > > + https://bugs.webkit.org/show_bug.cgi?id=61000 > > Should probably have a description here too Fixed > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:80 > > + // Also check that backing store types match: this is important for migration > > period. Fixed > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:82 > > + && (backingStoreType == it->second->backingStore()->backingStoreType())) { > > no need for newline Fixed > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:96 > > + bool hasSqlBackend = IDBSQLiteBackingStore::backingStoreExists(securityOrigin.get(), dataDir); > > i think this should be named hasSQLBackend.. same for hasLevelDBBackend below Fixed > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:97 > > + bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir); > > hmm, now we're doing LevelDB stuff outside of the #if ENABLE(LEVELDB) guard.. i'm not sure that's a good idea Added ENABLE(LEVELDB) guards > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:102 > > + // Migration: if the database exists and is SQLite we want to migrate it to LevelDB > > period. Fixed > > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1276 > > + if (!makeAllDirectories(pathBase)) { > > we probably don't need this? Removed > > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1281 > > + // FIXME: We should eventually use the same LevelDB database for all origins > > period. Fixed > > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1284 > > + // FIXME: It would be more thorough to open the database here but also more expensive > > yes, i think this is good enough. period after comment. Fixed > > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996 > > + if (!makeAllDirectories(pathBase)) { > > we probably don't need this? Removed > > Source/WebKit/chromium/public/WebIDBFactory.h:65 > > + // Used for DumpRenderTree tests > > period. > > > Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:81 > > +{ > > hmm, shouldn't this get passed on somewhere? This path is not used for LayoutTests but I plumbed it through for chrome side of the world. > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:163 > > + bindMethod("setOverrideIndexedDBBackingStore", &LayoutTestController::setOverrideIndexedDBBackingStore); > > one line further down to keep it sorted i believe The "addMockSpeecInputResult" is the item out of order so I moved it instead. > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:233 > > + delete m_tempFolder; > > can we use an OwnPtr instead so we don't have to call delete explicitly? Replaced with OwnPtr
Comment on attachment 94446 [details] Patch Attachment 94446 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8731139 New failing tests: storage/indexeddb/mozilla/versionchange.html storage/indexeddb/mozilla/global-data.html storage/indexeddb/set_version_queue.html storage/indexeddb/set_version_blocked.html
Created attachment 94451 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 94514 [details] Patch
Comment on attachment 94514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94514&action=review LGTM barring some final nits. Is there a reviewer on the CC list who would like to take a look? > LayoutTests/ChangeLog:10 > + from sqlite to leveldb can be tested. There's no plumbing in the LayoutTests.. this message should probably be in the Source/WebCore/ChangeLog. > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:136 > + return false; Add a FIXME that this should be implemented? > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1279 > +// } Just delete these lines? > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996 > + if (!makeAllDirectories(pathBase)) { I still don't see any need to create the directory? db.open() will fail if it's not there, right? > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1145 > + m_tempFolder.clear(); i don't think you need the to clear() the OwnPtr before assigning to it.. if it has a non-null value, it will just free that, and take ownership of the new pointer.
Created attachment 94659 [details] Patch
Comment on attachment 94659 [details] Patch rs=me.
Comment on attachment 94659 [details] Patch Clearing flags on attachment: 94659 Committed r87257: <http://trac.webkit.org/changeset/87257>
All reviewed patches have been landed. Closing bug.
This change seems to cause a whole bunch of indexeddb layout test crashes (assertion failures), see https://bugs.webkit.org/show_bug.cgi?id=61431 and http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20chromium.org&tests=storage%2Findexeddb%2Fcreate-and-remove-object-store.html%2Cstorage%2Findexeddb%2Fcursor-continue.html%2Cstorage%2Findexeddb%2Fcursor-index-delete.html%2Cstorage%2Findexeddb%2Fcursor-update.html%2Cstorage%2Findexeddb%2Fdatabase-basics.html%2Cstorage%2Findexeddb%2Fduplicates.html%2Cstorage%2Findexeddb%2Fexception-in-event-aborts.html%2Cstorage%2Findexeddb%2Findex-cursor.html%2Cstorage%2Findexeddb%2Fmigrate-basics.html%2Cstorage%2Findexeddb%2Fmozilla%2Fadd-twice-failure.html%2Cstorage%2Findexeddb%2Fmozilla%2Fbad-keypath.html%2Cstorage%2Findexeddb%2Fobjectstore-basics.html%2Cstorage%2Findexeddb%2Fobjectstore-cursor.html%2Cstorage%2Findexeddb%2Fopen-cursor.html%2Cstorage%2Findexeddb%2Frequest-event-propagation.html%2Cstorage%2Findexeddb%2Fset_version_queue.html%2Cstorage%2Findexeddb%2Ftransaction-after-close.html%2Cstorage%2Findexeddb%2Ftransaction-basics.html%2Cstorage%2Findexeddb%2Ftransaction-event-propagation.html%2Cstorage%2Findexeddb%2Ftransaction-rollback.html
Created attachment 96144 [details] Patch
Comment on attachment 96144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96144&action=review > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1317 > +// if (!makeAllDirectories(pathBase)) { > +// LOG_ERROR("Unable to create IndexedDB database path %s", pathBase.utf8().data()); > +// return false; > +// } Commented out code.
Comment on attachment 96144 [details] Patch Hi Greg! Again, thank you very much for working on this! View in context: https://bugs.webkit.org/attachment.cgi?id=96144&action=review > Source/WebCore/storage/IDBBackingStore.h:32 > +#include "IDBFactoryBackendImpl.h" Why is this being included? > Source/WebCore/storage/IDBBackingStore.h:111 > + virtual IDBFactoryBackendInterface::BackingStoreType backingStoreType() = 0; let's make this const > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:91 > + if (m_migrateEnabled) { i'm trying to think why we're making this conditional on m_migrateEnabled, rather than on backingStoreType... shouldn't the logic of this function be more like "if backingStoreType is LevelDB and the origin has a SQLiteBackingStore, and the origin doesn't have a LevelDBBackingStore, then do migration?" as it is now, if m_migrateEnabled is true (which is always is??), the backingStoreType gets ignored and we migrate to LevelDB anyway? > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:93 > + bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir); i'd prefer if these were named hasSQLBackingStore and hasLevelDBBackingStore > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996 > + if (!makeAllDirectories(pathBase)) { I think I've commented five times that we shouldn't be creating directories should to see if the database exists
Created attachment 96256 [details] Patch
(In reply to comment #24) > (From update of attachment 96144 [details]) > Hi Greg! Again, thank you very much for working on this! > > View in context: https://bugs.webkit.org/attachment.cgi?id=96144&action=review > > > Source/WebCore/storage/IDBBackingStore.h:32 > > +#include "IDBFactoryBackendImpl.h" > > Why is this being included? Refactored to remove this include. > > Source/WebCore/storage/IDBBackingStore.h:111 > > + virtual IDBFactoryBackendInterface::BackingStoreType backingStoreType() = 0; > > let's make this const Done. > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:91 > > + if (m_migrateEnabled) { > > i'm trying to think why we're making this conditional on m_migrateEnabled, rather than on backingStoreType... > > shouldn't the logic of this function be more like > > "if backingStoreType is LevelDB and the origin has a SQLiteBackingStore, and the origin doesn't have a LevelDBBackingStore, then do migration?" > > as it is now, if m_migrateEnabled is true (which is always is??), the backingStoreType gets ignored and we migrate to LevelDB anyway? Agreed -- removed the enable/disable. > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:93 > > + bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir); > > i'd prefer if these were named hasSQLBackingStore and hasLevelDBBackingStore > > > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996 > > + if (!makeAllDirectories(pathBase)) { > > I think I've commented five times that we shouldn't be creating directories should to see if the database exists yes... I had a local git merge failure from when it was fixed before :-) Fixed (again)!
Comment on attachment 96256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96256&action=review > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:90 > + // Migrationp this comment doesn't add much value > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:95 > + backingStoreType = LevelDBBackingStore; I don't like that we override the backingStoreType. I think we should only migrate if backingStoreType == LevelDBBackingStore. This in fact will break the layout tests. After the migration test, hasLeveLDBBackingStore will be true, so any subsequent layout tests run in the same process will use LevelDB, which is not intended (they don't all pass with LevelDB at the moment).
(In reply to comment #27) > (From update of attachment 96256 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96256&action=review > > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:90 > > + // Migrationp > > this comment doesn't add much value > > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:95 > > + backingStoreType = LevelDBBackingStore; > > I don't like that we override the backingStoreType. I think we should only migrate if backingStoreType == LevelDBBackingStore. > > This in fact will break the layout tests. After the migration test, hasLeveLDBBackingStore will be true, so any subsequent layout tests run in the same process will use LevelDB, which is not intended (they don't all pass with LevelDB at the moment). This will not break the layout tests because the backend can be controlled from LayoutTestController. This is the point of this patch actually -- so you can set things up from individual layout tests to force migrations. If you look at the changes to WebIDBFactoryImpl.cpp there is a comment explaining how this works. With this patch and the migration code patch I am able to run through the (unchanged) indexeddb tests.
Created attachment 96431 [details] Patch
Comment on attachment 96431 [details] Patch Yay, the tests now pass. LGTM View in context: https://bugs.webkit.org/attachment.cgi?id=96431&action=review > LayoutTests/storage/indexeddb/migrate-basics.html:189 > + layoutTestController.clearAllDatabases(); Just for future reference, this is the major difference from the previous patch.
I don't think cq will take patches from a closed bug.
Comment on attachment 96431 [details] Patch Clearing flags on attachment: 96431 Committed r88358: <http://trac.webkit.org/changeset/88358>