Bug 88467

Created attachment 146139 [details] Patch

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.

Created attachment 146368 [details] Patch

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

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?

Comment on attachment 146368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146368&action=review > Source/WebCore/ChangeLog:1 > +2012-06-07 Joshua Bell <jsbell@chromium.org> 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)

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.

Created attachment 146420 [details] Patch

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

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Created attachment 146616 [details] Patch

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/

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

Created attachment 147127 [details] Patch

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

Created attachment 147359 [details] Patch

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.

Created attachment 147921 [details] Patch

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

Created attachment 148463 [details] Patch

Rebased on top of http://trac.webkit.org/changeset/120753 Needs Chromium side to land, and still needs the xcode project addition.

Created attachment 148681 [details] Patch

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.

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

Created attachment 148690 [details] Patch

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?

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

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

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]

Comment on attachment 148690 [details] Patch Thanks, alec. I agree; I'll take care of those and re-up the patch.

Created attachment 148838 [details] Patch

alec's feedback incorporated. fishd@, tony@ - r?

Comment on attachment 148838 [details] Patch LGTM for WebKit API change.

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

Created attachment 149096 [details] Patch for landing

Comment on attachment 149096 [details] Patch for landing Clearing flags on attachment: 149096 Committed r121059: <http://trac.webkit.org/changeset/121059>

All reviewed patches have been landed. Closing bug.