Bug 61000 – Control Indexeddb backends from LayoutTestController

• Component: New Bugs
• Status: RESOLVED FIXED

[Greg Simon]

Control Indexeddb backends from LayoutTestController

[Greg Simon]

Created attachment 93837 [details]
Patch

[Greg Simon]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Greg Simon]

Created attachment 93872 [details]
Patch

[Hans Wennborg]

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

[Greg Simon]

Created attachment 94368 [details]
Patch

[Greg Simon]

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

[WebKit Review Bot]

Comment on attachment 94368 [details]
Patch

Attachment 94368 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8723727

[Hans Wennborg]

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?

[Greg Simon]

Created attachment 94446 [details]
Patch

[Greg Simon]

(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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Greg Simon]

Created attachment 94514 [details]
Patch

[Hans Wennborg]

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.

[Greg Simon]

Created attachment 94659 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 94659 [details]
Patch

rs=me.

[WebKit Commit Bot]

Comment on attachment 94659 [details]
Patch

Clearing flags on attachment: 94659

Committed r87257: <http://trac.webkit.org/changeset/87257>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

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

[Greg Simon]

Created attachment 96144 [details]
Patch

[Adam Barth]

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.

[Hans Wennborg]

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

[Greg Simon]

Created attachment 96256 [details]
Patch

[Greg Simon]

(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)!

[Hans Wennborg]

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).

[Greg Simon]

(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.

[Greg Simon]

Created attachment 96431 [details]
Patch

[Hans Wennborg]

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.

[Dimitri Glazkov (Google)]

I don't think cq will take patches from a closed bug.

[WebKit Review Bot]

Comment on attachment 96431 [details]
Patch

Clearing flags on attachment: 96431

Committed r88358: <http://trac.webkit.org/changeset/88358>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.