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

[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*);
 
     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

[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;
 
 private:
     IDBTransaction(ScriptState*, int64_t, const HashSet<String>&, WebIDBTransactionMode, IDBDatabase*, IDBOpenDBRequest*, const IDBDatabaseMetadata&);
 
     void enqueueEvent(Event*);
 
     // Called when a transaction is aborted.
@@ skipping 43 matching lines @@
     //
     // 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;

[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;
 };
 
 } // namespace blink
 
 #endif // IDBTransaction_h