Align IndexedDB metadata rollback on transaction abort to spec.

third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h

Index: third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h
diff --git a/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h b/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h
index b071d0f3348b3a42022c149a1d091f2c33a2bdb1..83a6118b448a6658d1974c0d25ed1dc9268f417c 100644
--- a/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h
+++ b/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h
@@ -40,12 +40,14 @@
 #include "public/platform/modules/indexeddb/WebIDBDatabase.h"
 #include "public/platform/modules/indexeddb/WebIDBTypes.h"
 #include "wtf/HashSet.h"
+#include "wtf/Vector.h"
 
 namespace blink {
 
 class DOMException;
 class ExceptionState;
 class IDBDatabase;
+class IDBIndex;
 class IDBObjectStore;
 class IDBOpenDBRequest;
 
@@ -83,9 +85,15 @@ public:
 
     void registerRequest(IDBRequest*);
     void unregisterRequest(IDBRequest*);
-    void objectStoreCreated(const String&, IDBObjectStore*);
-    void objectStoreDeleted(const String&);
+
+    // The methods below are called right before the changes are applied to the
+    // database's metadata. We use this unusual sequencing because some
+    // implementations need to access the metadata values before the change.

[jsbell, 2016/09/26 22:23:40]

"some implementations" sounds odd; does this mean tests?

[pwnall, 2016/09/27 00:12:19]

I that the implementations of some of the methods below assume this sequencing.
I wanted a consistent sequencing for all these callbacks, so I don't have to
remember a lot of details, and it seems to me that invoking the callbacks before
changing the metadata requires less mental state than changing the metadata
first and passing old information to the callbacks.

WDYT?

+    void objectStoreCreated(const String& name, IDBObjectStore*);
+    void objectStoreDeleted(const int64_t objectStoreId, const String& name);
     void objectStoreRenamed(const String& oldName, const String& newName);
+    void indexDeleted(IDBIndex*); // only called when the index's IDBIndex had been created
+
     void setActive(bool);
     void setError(DOMException*);
 
@@ -106,6 +114,17 @@ public:
     // ActiveDOMObject
     void stop() override;
 
+
+    // Object store IDs are allocated sequentially, so we can tell if an object
+    // store was created in this transaction by comparing its ID against the
+    // database's maximum object store ID at the time when the transaction was
+    // started.
+    int64_t oldMaxObjectStoreId() const

[jsbell, 2016/09/26 22:23:40]

This is odd data to expose just for this use case, and means the logic is
fragile (e.g. if we switched IDs to be UUIDs instead or something).

Can this be redone as e.g. a "isNewlyCreated()" method that does the checks
internally, or even just holds a flag? I'm not positive that's a huge
improvement, though.

Perhaps (per below), each store/index holds its own m_oldMetadata member; if
that's null then the store/index is newly created in this transaction.

[pwnall, 2016/09/27 00:12:19]

TBH, I was trying to be clever and avoid a few (cache-trashing) HashMap lookups.
I think we can do HashMap lookups everywhere I introduced this trick.

I avoided adding versionchange-specific members outside IDBTransaction, because
I figured that versionchange transactions should be much less frequent (at most
one per site push) than readonly / readwrite transactions (at least one per
pageview).

I'm open to any of the suggestions here:
(1) isNewlyCreated() on IDBObjectStore and IDBIndex
(2) take the hit for HashMap lookups
(3) m_oldMetadata members

How would you like to proceed?

[jsbell, 2016/10/06 20:01:31]

#3 if possible (assuming I'm correct that m_oldMetadata is the right signal), #1
if not.

[pwnall, 2016/10/06 22:15:22]

Done.
We decided on #1 in an offline discussion.

The code is a lot more readable now. Thank you very much!

+    {
+        DCHECK(isVersionChange());
+        return m_oldDatabaseMetadata.maxObjectStoreId;
+    }
+
 protected:
     // EventTarget
     DispatchEventResult dispatchEventInternal(Event*) override;
@@ -169,18 +188,36 @@ private:
     using IDBObjectStoreMap = HeapHashMap<String, Member<IDBObjectStore>>;
     IDBObjectStoreMap m_objectStoreMap;
 
-    // Used to mark stores created in an aborted upgrade transaction as
-    // deleted.
-    HeapHashSet<Member<IDBObjectStore>> m_createdObjectStores;
+    // The metadata of object stores when they are opened by this transaction.
+    //
+    // Only valid for versionchange transactions.
+    HeapHashMap<Member<IDBObjectStore>, RefPtr<IDBObjectStoreMetadata>> m_oldStoreMetadata;

[jsbell, 2016/09/26 22:23:40]

What do you think about storing the data on the stores themselves, e.g. in an
m_oldMetadata member?

[pwnall, 2016/09/27 00:12:19]

After having explained my rationale above, I will follow your decision :)

 
-    // Used to notify object stores (which are no longer in m_objectStoreMap)
-    // when the transaction is finished.
-    HeapHashSet<Member<IDBObjectStore>> m_deletedObjectStores;
+    // The metadata of deleted object stores without IDBObjectStore instances.
+    //
+    // Only valid for versionchange transactions.
+    Vector<RefPtr<IDBObjectStoreMetadata>> m_deletedObjectStores;
 
-    // Holds stores created, deleted, or used during upgrade transactions to
-    // reset metadata in case of abort.
-    HeapHashMap<Member<IDBObjectStore>, IDBObjectStoreMetadata> m_objectStoreCleanupMap;
+    // Tracks the indexes deleted by this transaction.
+    //
+    // This set only includes indexes that were created before this transaction,
+    // and were deleted during this transaction. Once marked for deletion, these
+    // indexes are removed from their object stores' index maps, so we need to
+    // stash them somewhere else in case the transaction gets aborted.
+    //
+    // This set does not include indexes created and deleted during this
+    // transaction, because we don't need to change their metadata when the
+    // transaction aborts, as they will still be marked for deletion.
+    //
+    // Only valid for versionchange transactions.
+    HeapVector<Member<IDBIndex>> m_deletedIndexes;
 
+    // Shallow snapshot of the database metadata when the transaction start.

[jsbell, 2016/09/26 22:23:40]

nit: wording; should that be "starts" ?

[pwnall, 2016/09/27 00:12:19]

Done.

+    //
+    // This does not include a snapshot of the database's object store / index
+    // metadata.
+    //
+    // Only valid for versionchange transactions.
     IDBDatabaseMetadata m_oldDatabaseMetadata;
 };