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 86b4c8124b7719881a3455db0cd0c99a9b877188..0efca7004196b84ad5cb5d96498d938c6ab1c25e 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;
 
@@ -94,9 +96,16 @@ class MODULES_EXPORT IDBTransaction final : public EventTargetWithInlineData,
 
   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.
+  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

[cmumford, 2016/10/05 21:15:27]

Might read a little cleaner if comment is above declaration.

[pwnall, 2016/10/05 23:15:45]

Done.
Wow, the new formatting really messed this one up :D

+
   void setActive(bool);
   void setError(DOMException*);
 
@@ -117,11 +126,22 @@ class MODULES_EXPORT IDBTransaction final : public EventTargetWithInlineData,
   // 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 {
+    DCHECK(isVersionChange());
+    return m_oldDatabaseMetadata.maxObjectStoreId;
+  }
+
  protected:
   // EventTarget
   DispatchEventResult dispatchEventInternal(Event*) override;
 
  private:
+  using IDBObjectStoreMap = HeapHashMap<String, Member<IDBObjectStore>>;
+
   IDBTransaction(ScriptState*,
                  int64_t,
                  const HashSet<String>&,
@@ -183,22 +203,39 @@ class MODULES_EXPORT IDBTransaction final : public EventTargetWithInlineData,
   //
   // objectStore() throws for completed/aborted transactions, so this is not
   // used after a transaction is finished, and can be cleared.
-  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;
 
-  // 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;

[cmumford, 2016/10/05 21:15:27]

I wonder if we can/should make an IDBVersionChangeTransaction in some future CL?

[pwnall, 2016/10/05 23:15:45]

I was thinking about the same thing!!

I tried it out while preparing this CL, and I stopped when the diff got too big.
I was also scared of perf regressions because of BTB trashing introduced by
virtual methods.

I filed http://crbug.com/653295 to track this.

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

Agreed, separate CL.

(Perf is almost certainly not an issue; the serialize/deserialize/IPC overhead
from most IDB operations is orders of magnitude more expensive than the impact
of virtual methods here is likely to be. It does matter for things like DOM node
manipulation which is synchronous and has cases where 100k operations are not
uncommon in big documents.)

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