Bug 68903 – IndexedDB: Use LevelDB also for in-memory databases

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: Use LevelDB also for in-memory databases

[Hans Wennborg]

Created attachment 108850 [details]
Patch

[David Grogan]

Comment on attachment 108850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108850&action=review

LGTM

I'm a little concerned that making our tests run in memory might mask problems that occur as a result of something in the default env implementation.  Though I suppose our browser tests would pick up some of those.

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113
> +    const String name = path.isNull() ? "/in-mem-db" : path;

"in-mem-db" isn't magic, right?  Nothing in leveldb looks for that string, it's just a dummy?

> Source/WebCore/platform/leveldb/LevelDBDatabase.h:66
> +    OwnPtr<leveldb::Comparator> m_comparatorAdapter;

Did you move this to match the order that the destructor deletes these?

[Hans Wennborg]

(In reply to comment #2)
> (From update of attachment 108850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108850&action=review
> 
> LGTM
> 
> I'm a little concerned that making our tests run in memory might mask problems that occur as a result of something in the default env implementation.  Though I suppose our browser tests would pick up some of those.

I'm not too concerned about that. Like you said, the browser tests should cover it.

We should look into running leveldb's own test suite as part of chromium's tests, but that's out-of-scope for this bug.

> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113
> > +    const String name = path.isNull() ? "/in-mem-db" : path;
> 
> "in-mem-db" isn't magic, right?  Nothing in leveldb looks for that string, it's just a dummy?

No magic, just a dummy name.

> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.h:66
> > +    OwnPtr<leveldb::Comparator> m_comparatorAdapter;
> 
> Did you move this to match the order that the destructor deletes these?

Yes, even though it doesn't matter because we clear them in the right order in the destructor, it bothered me to see them in the wrong order here.

[Steve Block]

Comment on attachment 108850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108850&action=review

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:110
> +    if (env)

Do you need this check? What's the default value of Options::env ?

>>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113
>>> +    const String name = path.isNull() ? "/in-mem-db" : path;
>> 
>> "in-mem-db" isn't magic, right?  Nothing in leveldb looks for that string, it's just a dummy?
> 
> No magic, just a dummy name.

This seems a little odd. It looks like leveldb uses Options::env to determine whether the DB should be in-memory. But here we make a decision based on path. This means we're making an implicit assumption about the values of env and path passed to this method. Can't we test env for in-memory-ness, then ASSERT(!path.isNull()) if it's not? Also, add a comment that this method ignores path if the DB is in-memory.

On a related note, why do you assign a value to path for an in memory DB? If leveldb::DB::Open() ignores it, why can't you pass NULL? At least add a comment.

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:150
> +    OwnPtr<LevelDBDatabase> result = adoptPtr(new LevelDBDatabase);

Is there any point in using an OwnPtr here? There's only one code path and we always release it. Same applies to existing code in open() I think.

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:1015
> +    String path = ":memory:";

Is this a magic token for SQLite? If so, I'm surprised there isn't an SQLite constant for it. Probably warrants a comment.

[Hans Wennborg]

(In reply to comment #4)
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:110
> > +    if (env)
> 
> Do you need this check? What's the default value of Options::env ?

The default of Options::env is Env::Default(). I guess I could just pass that in explicitly.

> >>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113
> >>> +    const String name = path.isNull() ? "/in-mem-db" : path;
> >> 
> >> "in-mem-db" isn't magic, right?  Nothing in leveldb looks for that string, it's just a dummy?
> > 
> > No magic, just a dummy name.
> 
> This seems a little odd. It looks like leveldb uses Options::env to determine whether the DB should be in-memory. But here we make a decision based on path. This means we're making an implicit assumption about the values of env and path passed to this method. Can't we test env for in-memory-ness, then ASSERT(!path.isNull()) if it's not? Also, add a comment that this method ignores path if the DB is in-memory.

You're right, that is weird. I think we can just scrap this line actually.

> 
> On a related note, why do you assign a value to path for an in memory DB? If leveldb::DB::Open() ignores it, why can't you pass NULL? At least add a comment.

Technically, it's not really ignored (it will be used as a path in the in-memory filesystem that the Env implements), but passing in an empty path works, so let's do that.

> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:150
> > +    OwnPtr<LevelDBDatabase> result = adoptPtr(new LevelDBDatabase);
> 
> Is there any point in using an OwnPtr here? There's only one code path and we always release it. Same applies to existing code in open() I think.

After https://bugs.webkit.org/show_bug.cgi?id=59656, I got the impression that "naked new" was bad, and sticking things in OwnPtr was the way to do things?

> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:1015
> > +    String path = ":memory:";
> 
> Is this a magic token for SQLite? If so, I'm surprised there isn't an SQLite constant for it. Probably warrants a comment.

Yes, that's a magic token for SQLite. I'll add a comment.

[Hans Wennborg]

Created attachment 109018 [details]
Patch

[Hans Wennborg]

Steve, would you like to take another look?

[Steve Block]

Comment on attachment 109018 [details]
Patch

> After https://bugs.webkit.org/show_bug.cgi?id=59656, I got the impression that "naked new" was bad, and
> sticking things in OwnPtr was the way to do things?
Yes, you're right.

[WebKit Review Bot]

Comment on attachment 109018 [details]
Patch

Clearing flags on attachment: 109018

Committed r96322: <http://trac.webkit.org/changeset/96322>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.