Align IndexedDB metadata rollback on transaction abort to spec.

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

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 22 matching lines @@
 #include "core/events/EventListener.h"
 #include "modules/EventModules.h"
 #include "modules/EventTargetModules.h"
 #include "modules/ModulesExport.h"
 #include "modules/indexeddb/IDBMetadata.h"
 #include "modules/indexeddb/IndexedDB.h"
 #include "platform/heap/Handle.h"
 #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;
 
 class MODULES_EXPORT IDBTransaction final : public EventTargetWithInlineData,
                                             public ActiveScriptWrappable,
                                             public ActiveDOMObject {
   USING_GARBAGE_COLLECTED_MIXIN(IDBTransaction);
   DEFINE_WRAPPERTYPEINFO();
 
  public:
@@ skipping 28 matching lines @@
   // Implement the IDBTransaction IDL
   const String& mode() const;
   DOMStringList* objectStoreNames() const;
   IDBDatabase* db() const { return m_database.get(); }
   DOMException* error() const { return m_error; }
   IDBObjectStore* objectStore(const String& name, ExceptionState&);
   void abort(ExceptionState&);
 
   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*);
 
   DEFINE_ATTRIBUTE_EVENT_LISTENER(abort);
   DEFINE_ATTRIBUTE_EVENT_LISTENER(complete);
   DEFINE_ATTRIBUTE_EVENT_LISTENER(error);
 
   void onAbort(DOMException*);
   void onComplete();
 
   // EventTarget
   const AtomicString& interfaceName() const override;
   ExecutionContext* getExecutionContext() const override;
 
   // ScriptWrappable
   bool hasPendingActivity() const final;
 
   // 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>&,
                  WebIDBTransactionMode,
                  IDBDatabase*,
                  IDBOpenDBRequest*,
                  const IDBDatabaseMetadata&);
 
   void enqueueEvent(Event*);
 
@@ skipping 41 matching lines @@
 #endif  // DCHECK_IS_ON()
 
   // 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.
   //
   // 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;
 };
 
 }  // namespace blink
 
 #endif  // IDBTransaction_h