IndexedDB: Ensure overlapping commits correctly update blob journals

content/browser/indexed_db/indexed_db_backing_store.h

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_INDEXED_DB_INDEXED_DB_BACKING_STORE_H_
 #define CONTENT_BROWSER_INDEXED_DB_INDEXED_DB_BACKING_STORE_H_
 
 #include <map>
 #include <set>
 #include <string>
@@ skipping 105 matching lines @@
     DISALLOW_COPY_AND_ASSIGN(BlobChangeRecord);
   };
   typedef std::map<std::string, BlobChangeRecord*> BlobChangeMap;
 
   class CONTENT_EXPORT Transaction {
    public:
     explicit Transaction(IndexedDBBackingStore* backing_store);
     virtual ~Transaction();
 
     virtual void Begin();
+
+    // CommitPhaseOne determines what blobs (if any) need to be written to disk
+    // and updates the primary blob journal, and kicks off the async writing
+    // of the blob files. In case of crash/rollback, the journal indicates what
+    // files should be cleaned up.
     // The callback will be called eventually on success or failure, or
     // immediately if phase one is complete due to lack of any blobs to write.
     virtual leveldb::Status CommitPhaseOne(scoped_refptr<BlobWriteCallback>);
+
+    // CommitPhaseTwo is called once the blob files (if any) have been written
+    // to disk, and commits the actual transaction to the backing store,
+    // including blob journal updates, then deletes any blob files deleted
+    // by the transaction and not referenced by running scripts.
     virtual leveldb::Status CommitPhaseTwo();
+
     virtual void Rollback();
     void Reset() {
       backing_store_ = NULL;
       transaction_ = NULL;
     }
     leveldb::Status PutBlobInfoIfNeeded(
         int64 database_id,
         int64 object_store_id,
         const std::string& object_store_data_key,
         std::vector<IndexedDBBlobInfo>*,
@@ skipping 66 matching lines @@
       virtual ~ChainedBlobWriter() {}
     };
 
     class ChainedBlobWriterImpl;
 
     typedef std::vector<WriteDescriptor> WriteDescriptorVec;
 
    private:
     class BlobWriteCallbackWrapper;
 
+    // Called by CommitPhaseOne: Identifies the blob entries to write and adds
+    // them to the primary blob journal directly (i.e. not as part of the
+    // transaction). Populates blobs_to_write_.
     leveldb::Status HandleBlobPreTransaction(
         BlobEntryKeyValuePairVec* new_blob_entries,
         WriteDescriptorVec* new_files_to_write);
-    // Returns true on success, false on failure.
+
+    // Called by CommitPhaseOne: Populates blob_files_to_remove_ by
+    // determining which blobs are deleted as part of the transaction, and
+    // adds blob entry cleanup operations to the transaction. Returns true on
+    // success, false on failure.
     bool CollectBlobFilesToRemove();
-    // The callback will be called eventually on success or failure.
+
+    // Called by CommitPhaseOne: Kicks off the asynchronous writes of blobs
+    // identified in HandleBlobPreTransaction. The callback will be called
+    // eventually on success or failure.
     void WriteNewBlobs(BlobEntryKeyValuePairVec* new_blob_entries,
                        WriteDescriptorVec* new_files_to_write,
                        scoped_refptr<BlobWriteCallback> callback);
-    leveldb::Status SortBlobsToRemove();
+
+    // Called by CommitPhaseTwo: Partition blob references in blobs_to_remove_
+    // into live (active references) and dead (no references).
+    void PartitionBlobsToRemove(BlobJournalType* dead_blobs,
+                                BlobJournalType* live_blobs) const;
 
     IndexedDBBackingStore* backing_store_;
     scoped_refptr<LevelDBTransaction> transaction_;
     BlobChangeMap blob_change_map_;
     BlobChangeMap incognito_blob_map_;
     int64 database_id_;
+
+    // List of blob files being newly written as part of this transaction.
+    // These will be added to the primary blob journal prior to commit, then
+    // removed after a sucessful commit.
+    BlobJournalType blobs_to_write_;
+
+    // List of blob files being deleted as part of this transaction. These will
+    // be added to either the primary or live blob journal as appropriate
+    // following a successful commit.
     BlobJournalType blobs_to_remove_;
     scoped_refptr<ChainedBlobWriter> chained_blob_writer_;
+
+    // Set to true between CommitPhaseOne and CommitPhaseTwo/Rollback, to
+    // indicate that the committing_transaction_count_ on the backing store
+    // has been bumped, and journal cleaning should be deferred.
+    bool committing_;
   };
 
   class Cursor {
    public:
     enum IteratorState { READY = 0, SEEK };
 
     virtual ~Cursor();
 
     struct CursorOptions {
       CursorOptions();
@@ skipping 219 matching lines @@
       int64 database_id,
       int64 object_store_id,
       int64 index_id,
       const IndexedDBKey& key,
       scoped_ptr<IndexedDBKey>* found_primary_key,
       bool* exists) WARN_UNUSED_RESULT;
 
   // Public for IndexedDBActiveBlobRegistry::ReleaseBlobRef.
   virtual void ReportBlobUnused(int64 database_id, int64 blob_key);
 
-  base::FilePath GetBlobFileName(int64 database_id, int64 key);
+  base::FilePath GetBlobFileName(int64 database_id, int64 key) const;
 
   virtual scoped_ptr<Cursor> OpenObjectStoreKeyCursor(
       IndexedDBBackingStore::Transaction* transaction,
       int64 database_id,
       int64 object_store_id,
       const IndexedDBKeyRange& key_range,
       blink::WebIDBCursorDirection,
       leveldb::Status*);
   virtual scoped_ptr<Cursor> OpenObjectStoreCursor(
       IndexedDBBackingStore::Transaction* transaction,
@@ skipping 32 matching lines @@
   virtual ~IndexedDBBackingStore();
 
   bool is_incognito() const { return !indexed_db_factory_; }
 
   leveldb::Status SetUpMetadata();
 
   virtual bool WriteBlobFile(
       int64 database_id,
       const Transaction::WriteDescriptor& descriptor,
       Transaction::ChainedBlobWriter* chained_blob_writer);
-  virtual bool RemoveBlobFile(int64 database_id, int64 key);
+
+  // Remove the referenced file on disk.
+  virtual bool RemoveBlobFile(int64 database_id, int64 key) const;
+
+  // Schedule a call to CleanPrimaryJournalIgnoreReturn() via
+  // an owned timer. If this object is destroyed, the timer
+  // will automatically be cancelled.
   virtual void StartJournalCleaningTimer();
+
+  // Attempt to clean the primary journal. This will remove
+  // any referenced files and delete the journal entry. If any
+  // transaction is currently committing this will be deferred
+  // via StartJournalCleaningTimer().
   void CleanPrimaryJournalIgnoreReturn();
 
  private:
   static scoped_refptr<IndexedDBBackingStore> Create(
       IndexedDBFactory* indexed_db_factory,
       const GURL& origin_url,
       const base::FilePath& blob_path,
       net::URLRequestContext* request_context,
       scoped_ptr<LevelDBDatabase> db,
       scoped_ptr<LevelDBComparator> comparator,
       base::SequencedTaskRunner* task_runner,
       leveldb::Status* status);
 
   static bool ReadCorruptionInfo(const base::FilePath& path_base,
                                  const GURL& origin_url,
                                  std::string* message);
 
   leveldb::Status FindKeyInIndex(
       IndexedDBBackingStore::Transaction* transaction,
       int64 database_id,
       int64 object_store_id,
       int64 index_id,
       const IndexedDBKey& key,
       std::string* found_encoded_primary_key,
       bool* found);
   leveldb::Status GetIndexes(int64 database_id,
                              int64 object_store_id,
                              IndexedDBObjectStoreMetadata::IndexMap* map)
       WARN_UNUSED_RESULT;
-  bool RemoveBlobDirectory(int64 database_id);
-  leveldb::Status CleanUpBlobJournal(const std::string& level_db_key);
+
+  // Remove the blob directory for the specified database.

[cmumford, 2015/01/22 00:56:57]

Nit: Comment pretty obvious from function name/args.

But, as you know, the blob stuff is really quite complex so maybe erring on the
side of too many comments isn't bad. Does this function delete all blobs
contained within - that's good info for this comment.

[jsbell, 2015/01/22 19:40:45]

Noted that it removes any contained files too.

+  bool RemoveBlobDirectory(int64 database_id) const;
+
+  // Synchronously read the key-specified blob journal entry from the backing
+  // store, delete all referenced blob files, and erase the journal entry.
+  // This must not be used while temporary entries are present e.g. during
+  // a two-stage transaction commit with blobs.
+  leveldb::Status CleanUpBlobJournal(const std::string& level_db_key) const;
+
+  // Synchronously delete the files and/or directories on disk referenced by
+  // the blob journal.
+  leveldb::Status CleanUpBlobJournalEntries(
+      const BlobJournalType& journal) const;
 
   IndexedDBFactory* indexed_db_factory_;
   const GURL origin_url_;
   base::FilePath blob_path_;
 
   // The origin identifier is a key prefix unique to the origin used in the
   // leveldb backing store to partition data by origin. It is a normalized
   // version of the origin URL with a versioning suffix appended, e.g.
   // "http_localhost_81@1" Since only one origin is stored per backing store
   // this is redundant but necessary for backwards compatibility; the suffix
   // provides for future flexibility.
   const std::string origin_identifier_;
 
   net::URLRequestContext* request_context_;
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
   std::set<int> child_process_ids_granted_;
   BlobChangeMap incognito_blob_map_;
   base::OneShotTimer<IndexedDBBackingStore> journal_cleaning_timer_;
 
   scoped_ptr<LevelDBDatabase> db_;
   scoped_ptr<LevelDBComparator> comparator_;
   // Whenever blobs are registered in active_blob_registry_, indexed_db_factory_
   // will hold a reference to this backing store.
   IndexedDBActiveBlobRegistry active_blob_registry_;
   base::OneShotTimer<IndexedDBBackingStore> close_timer_;
 
+  // Incremented whenever a transaction starts committing, decremented when
+  // complete. While > 0, temporary journal entries may exist so out-of-band
+  // journal cleaning must be deferred.
+  size_t committing_transaction_count_;
+
   DISALLOW_COPY_AND_ASSIGN(IndexedDBBackingStore);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_INDEXED_DB_INDEXED_DB_BACKING_STORE_H_