Bug 88467 – IndexedDB: Snapshot metadata in front end to avoid IPC round-trips

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

IndexedDB: Snapshot metadata in front end to avoid IPC round-trips

[Joshua Bell]

Created attachment 146139 [details]
Patch

[Joshua Bell]

Alec, David - please take an initial look. This needs MANY additional tests, but passes with existing layout tests (modulo a few tweaks). This implements:

* Metadata is snapshotted on connection and after version-change transactions finish. All the little calls to name(), keyPath(), autoIncrement() and friends are now done w/o IPC. (Eventually this should move into the IDBDatabase creation code itself.)
* Metadata for object stores is reverted after an aborted version change transaction.
* Calls to store and index methods fail if the objects have been deleted.

There's a corresponding Chromium change for the IPC plumbing to route the WebIDBDatabase::metaData() call around.

Obviously, this adds a lot of code, in the hope that more code can then be removed/simplified. That said, it's still not filling my heart with glee, so needs refinement.

[Joshua Bell]

Created attachment 146368 [details]
Patch

[Joshua Bell]

(In reply to comment #3)
> Created an attachment (id=146368) [details]
> Patch

Latest patch adds two layout tests, and adds a few missing "if deleted throw exception" cases caught by the tests.

[David Grogan]

Comment on attachment 146368 [details]
Patch

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

Do you know what you didn't like about this patch?

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:154
> +        m_versionChangeTransaction->objectStoreDeleted(name);

Not checking for ec was an error in our existing code and a gap in test coverage, right?

> Source/WebCore/Modules/indexeddb/IDBDatabase.h:108
> +    IDBDatabaseMetaData m_metaData;

How does aborting a transaction work?  Why doesn't IDBDatabase need a m_previousMetaData member like IDBObjectStore?

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:64
> +    if (m_deleted) {

Before this change what happens if we do

var index = os.index("myindex")
os.deleteIndex("myindex")
var request = index.openCursor();

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:353
> +        objectStoreMetaData.indexes.remove(name);

Commenting these 4 lines doesn't cause any tests to fail. We still need a test that ensures a deleted index doesn't show up in transaction.objectStore("mystore").indexNames?

> Source/WebKit/chromium/public/WebIDBMetaData.h:35
> +template<typename T, size_t inlineCapacity> class Vector;

Could you include wtf/Forward.h in place of these?

> Source/WebKit/chromium/public/WebIDBMetaData.h:93
> +    WebPrivateOwnPtr<WebCore::IDBObjectStoreMetaData> m_private;

I'm assuming you copied the WEBKIT_IMPLEMENTATION / WebPrivateOwnPtr idiom from surrounding files?  I don't know much about it.

> LayoutTests/storage/indexeddb/resources/transaction-basics.js:-51
> -    evalAndLog("store.deleteIndex('indexFail')");

This used to just succeed?  Script could still access deleted objectstores?

[Alec Flett]

Comment on attachment 146368 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Joshua Bell  <[e-mail]>

first pass through is mostly "more constness" - comments below. Also, we had talked some about using the database-id for the store/index/etc though poking through the code I can see that this can be hard to get to at times..

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:68
> +    m_metaData = m_backend->metaData();

the extra camelcase on "Data" rubs me the wrong way here but that's just a personal preference :) "metadata" is fine IMO

> Source/WebCore/Modules/indexeddb/IDBDatabase.h:91
> +    IDBDatabaseMetaData& metaData() { return m_metaData; }

Can this be returned as const, and can the method be const too?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:137
> +IDBDatabaseMetaData IDBDatabaseBackendImpl::metaData() const

Seems like a lot of work to construct the metadata each time, plus that means this can't return 'const IDBDatabaseMetadata&' like IDBDatabase.h does...

> Source/WebCore/Modules/indexeddb/IDBIndex.h:87
> +    IDBIndex(IDBIndexMetaData, PassRefPtr<IDBIndexBackendInterface>, IDBObjectStore*, IDBTransaction*);

in the constructors, pass these as const IDBIndexMetaData& etc

> Source/WebCore/Modules/indexeddb/IDBMetaData.h:49
> +    String name;

more const lovin' - seems like most non-array member vars can be const as well? Looks like that will make it have trouble getting through the proxies though... hrm..

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:350
> +        IDBDatabaseMetaData& databaseMetaData = m_transaction->db()->metaData();

I'd put this logic in IDBDatabase - here you're doing direct manipulation of a datstructure that's stored inside IDBDatabase (which again if there were more const-ness this would be harder to do)

[Joshua Bell]

Comment on attachment 146368 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:68
>> +    m_metaData = m_backend->metaData();
> 
> the extra camelcase on "Data" rubs me the wrong way here but that's just a personal preference :) "metadata" is fine IMO

It looks like there's precedent for "metadata" as a single word in WebKit. I'll follow up and remove unnecessary CamelCasing.

>> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:154
>> +        m_versionChangeTransaction->objectStoreDeleted(name);
> 
> Not checking for ec was an error in our existing code and a gap in test coverage, right?

Correct. (But harmless, I believe.)

>> Source/WebCore/Modules/indexeddb/IDBDatabase.h:91
>> +    IDBDatabaseMetaData& metaData() { return m_metaData; }
> 
> Can this be returned as const, and can the method be const too?

Yes. I had it non-const when IDBObjectStore had to reach in and mutate it, but that's no longer necessary (see discussion below)

>> Source/WebCore/Modules/indexeddb/IDBDatabase.h:108
>> +    IDBDatabaseMetaData m_metaData;
> 
> How does aborting a transaction work?  Why doesn't IDBDatabase need a m_previousMetaData member like IDBObjectStore?

In IDBDatabase's clearVersionChangeTransaction it fetches a fresh copy of the metadata from the back end, which is rolled back by that point.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:137
>> +IDBDatabaseMetaData IDBDatabaseBackendImpl::metaData() const
> 
> Seems like a lot of work to construct the metadata each time, plus that means this can't return 'const IDBDatabaseMetadata&' like IDBDatabase.h does...

True, but it is called lazily - only when an updated copy is needed.

>> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:64
>> +    if (m_deleted) {
> 
> Before this change what happens if we do
> 
> var index = os.index("myindex")
> os.deleteIndex("myindex")
> var request = index.openCursor();

No exception. A success event is fired at the request, with a null result (i.e. no cursor).

>> Source/WebCore/Modules/indexeddb/IDBIndex.h:87
>> +    IDBIndex(IDBIndexMetaData, PassRefPtr<IDBIndexBackendInterface>, IDBObjectStore*, IDBTransaction*);
> 
> in the constructors, pass these as const IDBIndexMetaData& etc

Will do.

>> Source/WebCore/Modules/indexeddb/IDBMetaData.h:49
>> +    String name;
> 
> more const lovin' - seems like most non-array member vars can be const as well? Looks like that will make it have trouble getting through the proxies though... hrm..

I'll poke at it some more. If the default constructor can be eliminated then this is possible.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:61
>  String IDBObjectStore::name() const

FYI, I'm going to make these inline like the IDBDatabase and IDBIndex counterparts.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:67
>  PassRefPtr<IDBAny> IDBObjectStore::keyPath() const

ditto

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:89
>  bool IDBObjectStore::autoIncrement() const

ditto

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:292
> +    // Need to update canonical metadata in IDBDatabase so future copies of this store have the new index metadata.

These lines can go away (see discussion below)

>> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:350
>> +        IDBDatabaseMetaData& databaseMetaData = m_transaction->db()->metaData();
> 
> I'd put this logic in IDBDatabase - here you're doing direct manipulation of a datstructure that's stored inside IDBDatabase (which again if there were more const-ness this would be harder to do)

Per discussion below, this can go away.

>> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:353
>> +        objectStoreMetaData.indexes.remove(name);
> 
> Commenting these 4 lines doesn't cause any tests to fail. We still need a test that ensures a deleted index doesn't show up in transaction.objectStore("mystore").indexNames?

Good point. Thinking through it, those four lines are unnecessary (which is good, as they're ugly!):
* Once you've called transaction.objectStore("mystore") the first time the IDBObjectStore is saved (in m_objectStoreMap) and that object is re-used for the lifetime of the IDBTransaction. 
* Once the version change transaction is finished the IDBDatabase gets updated from the backing store.
Therefore, there is no way during the lifetime of the transaction to access the IDBDatabase's copy of the object store metadata again and see it be out-of-sync.

>> Source/WebKit/chromium/public/WebIDBMetaData.h:35
>> +template<typename T, size_t inlineCapacity> class Vector;
> 
> Could you include wtf/Forward.h in place of these?

That should be unnecessary and can be removed, thanks.

>> Source/WebKit/chromium/public/WebIDBMetaData.h:93
>> +    WebPrivateOwnPtr<WebCore::IDBObjectStoreMetaData> m_private;
> 
> I'm assuming you copied the WEBKIT_IMPLEMENTATION / WebPrivateOwnPtr idiom from surrounding files?  I don't know much about it.

Yeah, it's a common pattern. It was used in WebIDBKeyPath as well.

>> LayoutTests/storage/indexeddb/resources/transaction-basics.js:-51
>> -    evalAndLog("store.deleteIndex('indexFail')");
> 
> This used to just succeed?  Script could still access deleted objectstores?

Yes.

[Joshua Bell]

Created attachment 146420 [details]
Patch

[Joshua Bell]

New patch addresses feedback except for de-camel-casing "MetaData". I'll upload that tomorrow.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Joshua Bell]

Created attachment 146616 [details]
Patch

[Joshua Bell]

New patch removes camel casing.

This will need to be split up for landing to handle the chromium-side changes which are at: http://codereview.chromium.org/10533057/

[Joshua Bell]

Chromium WebKit API impl is http://webkit.org/b/88678 which needs to land first.

[Joshua Bell]

Created attachment 147127 [details]
Patch

[Joshua Bell]

Patch rebased on top of https://bugs.webkit.org/show_bug.cgi?id=88678

[Joshua Bell]

Created attachment 147359 [details]
Patch

[Joshua Bell]

I factored out the "deleted objects should throw" part of this patch into http://webkit.org/b/89243 - that will need to land first and reduce the size of this patch.

[Joshua Bell]

Created attachment 147921 [details]
Patch

[Joshua Bell]

Rebased on top of http://trac.webkit.org/changeset/120504 - still waiting on https://bugs.webkit.org/show_bug.cgi?id=88678 to land

[Joshua Bell]

Created attachment 148463 [details]
Patch

[Joshua Bell]

Rebased on top of http://trac.webkit.org/changeset/120753

Needs Chromium side to land, and still needs the xcode project addition.

[Joshua Bell]

Created attachment 148681 [details]
Patch

[Joshua Bell]

Okay, ready for actual feedback. Some things I'm not sure about:

Should we have IDBDatabaseMetadata, IDBObjectStoreMetadata and IDBIndexMetadata, or instead have IDBMetadata, IDBMetadata::ObjectStore and IDBMetadata::Index?

Naming of the m_previousMetadata member of IDBObjectStore, which is a hideous abomination required by the spec only in the case of version change transaction abort. Anything more evocative?

I noticed two things while polishing this patch which I'll check before landing.

* I believe the call to m_metadata = m_backend->metadata(); in transactionCreated() is superfluous and can be removed.

* I believe there is a missing case: a pre-existing object store that has an index added/removed will not be reset per spec. I'll add a test to verify. If needed, it just needs IDBTransaction::objectStore() to add the store to the potential change set if it's a version change transaction.

[Joshua Bell]

(In reply to comment #23)
> * I believe the call to m_metadata = m_backend->metadata(); in transactionCreated() is superfluous and can be removed.

Wrong, and covered by tests. :) This is to capture the change to the version number made by the setVersion() call. When the upgradeneeded changes land this can probably be removed.
 
> * I believe there is a missing case: a pre-existing object store that has an index added/removed will not be reset per spec. 

Wrong, and covered by tests. :) However, I can optimize the implementation slightly. New patch incoming, just scopes population of the m_objectStoreCleanupSet to version change transactions and clears the set when it's no longer needed.

[Joshua Bell]

Created attachment 148690 [details]
Patch

[David Grogan]

Comment on attachment 148690 [details]
Patch

LGTM

I prefer the current IDBObjectStoreMetadata-style vs IDBMetadata::ObjectStore and m_previousMetadata is fine.

Did you consciously reuse the test from your public-webapps message?

[Joshua Bell]

(In reply to comment #26)
> 
> Did you consciously reuse the test from your public-webapps message?

Yep! I added index counts, but otherwise it seemed like a good idea. It also subsumes the repro in in https://bugs.webkit.org/show_bug.cgi?id=86127 which I think was a direct copy/paste of that message.

[Joshua Bell]

fishd@ - review of the Chromium public WebKit API changes? (trivial additions)

tony@ - review of the rest?

[Alec Flett]

Comment on attachment 148690 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabase.h:61
> +    const String& name() const { return m_metadata.name; }

I dug around a little bit with this.. for background, I suggested last week to use a lot more use of 'const' and refs as a cheap way to avoid copying. 

But after seeing this, I voiced concern to jsbell yesterday that this could be problematic in case m_metadata goes away or gets overwritten. (or reset...) I think whats here is probably generally safe, but only because of the way it's currently used, (for example, the bindings call into this looks safe) but that could change. I think this is a valid enough reason to warrent dropping the '&' but of course keeping the const, especially given that names and versions tend to be short.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.h:100
> +    void resetMetadata() { m_metadata = m_previousMetadata; }

I think the only thing I find odd about m_previousMetadata is that it's the first time that the IDBObjectStore maintains any kind of explicit before/after transaction state. It feels like something that belongs in IDBTransaction itself. 

I wonder, since you're already maintaining an IDBObjectStoreSet, and since IDBTransaction is already initiating the 'resetMetadata', if this should just be a mapping from IDBObjectStore->[previous]IDBObjectStoreMetadata inside the transaction, and then the transaction is the one that hands off the old metadata.

Otherwise, looks good. I too prefer the IDBObjectStoreMetadata name over IDBObjectStore::Metadata, if only to be consistent with things like IDBObjectStore[BackendImpl] and IDBObjectStore[BackingStore]

[Joshua Bell]

Comment on attachment 148690 [details]
Patch

Thanks, alec. I agree; I'll take care of those and re-up the patch.

[Joshua Bell]

Created attachment 148838 [details]
Patch

[Joshua Bell]

alec's feedback incorporated. 

fishd@, tony@ - r?

[Darin Fisher (:fishd, Google)]

Comment on attachment 148838 [details]
Patch

LGTM for WebKit API change.

[Tony Chang]

Comment on attachment 148838 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBMetadata.h:87
> +#endif // IDBTracing_h

Nit: IDBMetadata_h

[Joshua Bell]

Created attachment 149096 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 149096 [details]
Patch for landing

Clearing flags on attachment: 149096

Committed r121059: <http://trac.webkit.org/changeset/121059>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.