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 154120dbc681958c92aad62d807b81d6f05aad53..ac008a43f04a2adf9929881322b48eeb690f662f 100644
--- a/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h
+++ b/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h
@@ -40,15 +40,16 @@
 #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;
-struct IDBObjectStoreMetadata;
 
 class MODULES_EXPORT IDBTransaction final
     : public EventTargetWithInlineData
@@ -57,8 +58,8 @@ class MODULES_EXPORT IDBTransaction final
     USING_GARBAGE_COLLECTED_MIXIN(IDBTransaction);
     DEFINE_WRAPPERTYPEINFO();
 public:
-    static IDBTransaction* create(ScriptState*, int64_t, const HashSet<String>& objectStoreNames, WebIDBTransactionMode, IDBDatabase*);
-    static IDBTransaction* create(ScriptState*, int64_t, IDBDatabase*, IDBOpenDBRequest*, const IDBDatabaseMetadata& previousMetadata);
+    static IDBTransaction* createNonVersionChange(ScriptState*, int64_t, const HashSet<String>& objectStoreNames, WebIDBTransactionMode, IDBDatabase*);

[jsbell, 2016/09/16 18:17:29]

Why these renames?

If it's just to avoid the overload, I'd keep this one just "create".

[pwnall, 2016/09/17 01:34:22]

The renames document when each create version gets called.

If you don't like the "createNonVersionChange" name, I can move that information
into a comment with an accompanying DCHECK. Does this seem reasoanble?

+    static IDBTransaction* createVersionChange(ScriptState*, int64_t, IDBDatabase*, IDBOpenDBRequest*, const IDBDatabaseOwnMetadata& oldMetadata);
     ~IDBTransaction() override;
     DECLARE_VIRTUAL_TRACE();
 
@@ -84,9 +85,14 @@ public:
 
     void registerRequest(IDBRequest*);
     void unregisterRequest(IDBRequest*);
-    void objectStoreCreated(const String&, IDBObjectStore*);
-    void objectStoreDeleted(const String&);
+
+    // The methods below are called before the changes are applied to the
+    // database's metadata.

[jsbell, 2016/09/16 18:17:29]

Can you expand this comment to answer "why?"

[pwnall, 2016/09/17 01:34:22]

Done.

Can you please see if my revision makes sense?

+    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*);
 
@@ -112,10 +118,17 @@ protected:
     DispatchEventResult dispatchEventInternal(Event*) override;
 
 private:
-    IDBTransaction(ScriptState*, int64_t, const HashSet<String>&, WebIDBTransactionMode, IDBDatabase*, IDBOpenDBRequest*, const IDBDatabaseMetadata&);
+    IDBTransaction(ScriptState*, int64_t, const HashSet<String>&, WebIDBTransactionMode, IDBDatabase*, IDBOpenDBRequest*, const IDBDatabaseOwnMetadata&);
 
     void enqueueEvent(Event*);
 
+    // Called when a transaction is aborted.
+    void abortOutstandingRequests();
+    void revertDatabaseMetadata();
+
+    // Called when a transaction is completed (committed or aborted).
+    void finish();
+
     enum State {
         Inactive, // Created or started, but not in an event callback
         Active, // Created or started, in creation scope or an event callback
@@ -125,7 +138,11 @@ private:
 
     const int64_t m_id;
     Member<IDBDatabase> m_database;
-    const HashSet<String> m_objectStoreNames;
+    // The name of the object stores that the transaction may operate on.
+    //
+    // The scope is empty for versionchange transactions, which can operate on
+    // the entire database.
+    const HashSet<String> m_scope;
     Member<IDBOpenDBRequest> m_openDBRequest;
     const WebIDBTransactionMode m_mode;
     State m_state = Active;
@@ -135,21 +152,42 @@ private:
 
     HeapListHashSet<Member<IDBRequest>> m_requestList;
 
-    typedef HeapHashMap<String, Member<IDBObjectStore>> IDBObjectStoreMap;
+    // Caches the IDBObjectStore instances returned by the objectStore() method.
+    //
+    // The spec requires that a transaction's objectStore() returns the same
+    // IDBObjectStore instance for a specific store, so this cache is necessary
+    // for correctness.
+    //
+    // The cache may be in an inconsistent state after a transaction is aborted.

[jsbell, 2016/09/16 18:17:29]

I think this can be simplified to e.g.:

// objectStore() throws for completed/aborted transactions, 
// so this is not used after that occurs and can be cleared.

[pwnall, 2016/09/17 01:34:22]

Done.

I also revised the comment on IDBIndex.

+    // objectStore() throws for inactive transactions, which makes the cache not
+    // observable, so reverting the cache would be a waste of resources.
+    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;
-
-    // Used to notify object stores (which are no longer in m_objectStoreMap)
-    // when the transaction is finished.
-    HeapHashSet<Member<IDBObjectStore>> m_deletedObjectStores;
-
-    // Holds stores created, deleted, or used during upgrade transactions to
-    // reset metadata in case of abort.
-    HeapHashMap<Member<IDBObjectStore>, IDBObjectStoreMetadata> m_objectStoreCleanupMap;
-    IDBDatabaseMetadata m_previousMetadata;
+    // The metadata of object stores when they are opened by this transaction.
+    //
+    // Only valid for versionchange transactions.
+    HeapHashMap<Member<IDBObjectStore>, RefPtr<IDBObjectStoreMetadata>> m_oldStoreMetadata;
+
+    // The metadata of deleted object stores without IDBObjectStore instaces.

[jsbell, 2016/09/16 18:17:29]

typo: instances

[pwnall, 2016/09/17 01:34:22]

Done.

+    //
+    // Only valid for versionchange transactions.
+    Vector<RefPtr<IDBObjectStoreMetadata>> m_deletedObjectStores;
+
+    // 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.
+    HeapVector<Member<IDBIndex>> m_deletedIndexes;
+
+    // Only valid for versionchange transactions.
+    IDBDatabaseOwnMetadata m_oldDatabaseMetadata;
 };
 
 } // namespace blink