IndexedDB: correct quota calculations after relaxed txns (Windows)

See comment in `IndexedDBContextImpl::bucket_size_map_` for full
details. Briefly,

* When a LevelDB write is not "synced" (in LDB parlance), LevelDB
  will keep the same file handles open after writing.
* Some Windows APIs will not report the correct file size until after
  the file handle is closed.
* One workaround is to simply open and close a file handle to the
  relevant file(s).

This change also removes some usage of FileSystemProxy since we've
decided not to move IDB to a different process.

Bug: 1489517
Change-Id: Ie74fc76afeef74e5de258816f451511b8e9f3616
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4977286
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Jesse McKenna <jessemckenna@google.com>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1217074}

components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_database.cc

@@ -46,21 +46,10 @@ namespace content {
 
 namespace {
 
-// Forcing flushes to disk at the end of a transaction guarantees that the
-// data hit disk, but drastically impacts throughput when the filesystem is
-// busy with background compactions. Not syncing trades off reliability for
-// performance. Note that background compactions which move data from the
-// log to SSTs are always done with reliable writes.
-//
-// Sync writes are necessary on Windows for quota calculations; POSIX
-// calculates file sizes correctly even when not synced to disk.
-#if BUILDFLAG(IS_WIN)
+// As `TransactionLevelDBDatabase` is only used for internal transactions such
+// as DB initialization and via `LevelDBDirectTransaction`, this constant
+// doesn't apply to web API IndexedDB "readwrite" transactions.
 const bool kSyncWrites = true;
-#else
-// TODO(dgrogan): Either remove the #if block or change this back to false.
-// See http://crbug.com/338385.
-const bool kSyncWrites = true;
-#endif
 
 }  // namespace
 

content/browser/indexed_db/indexed_db_backing_store.h

@@ -174,6 +174,9 @@ class CONTENT_EXPORT IndexedDBBackingStore {
 
     base::WeakPtr<Transaction> AsWeakPtr();
 
+    blink::mojom::IDBTransactionDurability durability() const {
+      return durability_;
+    }
     blink::mojom::IDBTransactionMode mode() const { return mode_; }
 
     IndexedDBBackingStore* backing_store() {

content/browser/indexed_db/indexed_db_browsertest.cc

@@ -758,15 +758,7 @@ IN_PROC_BROWSER_TEST_F(IndexedDBBrowserTest, LevelDBLogFileTest) {
   }
 }
 
-// TODO(crbug.com/1489517): this test fails sometimes on Windows when durability
-// is relaxed, in particular on ASAN builds, but disable across all Windows
-// builds for safety.
-#if BUILDFLAG(IS_WIN)
-#define MAYBE_CanDeleteWhenOverQuotaTest DISABLED_CanDeleteWhenOverQuotaTest
-#else
-#define MAYBE_CanDeleteWhenOverQuotaTest CanDeleteWhenOverQuotaTest
-#endif
-IN_PROC_BROWSER_TEST_F(IndexedDBBrowserTest, MAYBE_CanDeleteWhenOverQuotaTest) {
+IN_PROC_BROWSER_TEST_F(IndexedDBBrowserTest, CanDeleteWhenOverQuotaTest) {
   SetQuota(5);
   const GURL kTestUrl = GetTestUrl("indexeddb", "fill_quota.html");
   SimpleTest(kTestUrl);

content/browser/indexed_db/indexed_db_bucket_context.h

@@ -144,8 +144,10 @@ class CONTENT_EXPORT IndexedDBBucketContext {
         on_content_changed;
 
     // Called to inform the quota system that a transaction which may have
-    // updated the amount of disk space used has completed.
-    base::RepeatingClosure on_writing_transaction_complete;
+    // updated the amount of disk space used has completed. The parameter is
+    // true for transactions that caused the backing store to flush.
+    base::RepeatingCallback<void(bool /*did_sync*/)>
+        on_writing_transaction_complete;
 
     // Called to run a given callback on every bucket context (including the one
     // in the current sequence and those in other sequences/associated with

content/browser/indexed_db/indexed_db_context_impl.cc

@@ -28,7 +28,6 @@
 #include "base/time/time.h"
 #include "base/trace_event/base_tracing.h"
 #include "base/values.h"
-#include "components/services/storage/filesystem_proxy_factory.h"
 #include "components/services/storage/indexed_db/leveldb/leveldb_factory.h"
 #include "components/services/storage/indexed_db/scopes/varint_coding.h"
 #include "components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_database.h"
@@ -130,8 +129,7 @@ IndexedDBContextImpl::IndexedDBContextImpl(
       quota_client_wrapper_(
           std::make_unique<storage::QuotaClientCallbackWrapper>(
               quota_client_.get())),
-      quota_client_receiver_(quota_client_wrapper_.get()),
-      filesystem_proxy_(storage::CreateFilesystemProxy()) {
+      quota_client_receiver_(quota_client_wrapper_.get()) {
   TRACE_EVENT0("IndexedDB", "init");
 
   // QuotaManagerProxy::RegisterClient() must be called during construction
@@ -313,8 +311,7 @@ void IndexedDBContextImpl::DoDeleteBucketData(
           idb_file_path);
   bool success = s.ok();
   if (success) {
-    success = filesystem_proxy_->DeletePathRecursively(
-        GetBlobStorePath(bucket_locator));
+    success = base::DeletePathRecursively(GetBlobStorePath(bucket_locator));
   }
 
   NotifyOfBucketModification(bucket_locator);
@@ -851,11 +848,19 @@ int64_t IndexedDBContextImpl::GetBucketDiskUsage(
   if (!LookUpBucket(bucket_locator.id))
     return 0;
 
-  if (bucket_size_map_.find(bucket_locator) == bucket_size_map_.end()) {
-    bucket_size_map_[bucket_locator] = ReadUsageFromDisk(bucket_locator);
+  bool write_in_progress = false;
+  const auto iter = bucket_size_map_.find(bucket_locator);
+  if (iter != bucket_size_map_.end()) {
+    if (iter->second >= 0) {
+      return iter->second;
+    }
+    write_in_progress = true;
   }
 
-  return bucket_size_map_[bucket_locator];
+  const int64_t value = ReadUsageFromDisk(bucket_locator, write_in_progress);
+  CHECK_GE(value, 0);
+  bucket_size_map_[bucket_locator] = value;
+  return value;
 }
 
 base::Time IndexedDBContextImpl::GetBucketLastModified(
@@ -871,11 +876,11 @@ base::Time IndexedDBContextImpl::GetBucketLastModified(
   }
 
   base::FilePath idb_directory = GetLevelDBPath(bucket_locator);
-  absl::optional<base::File::Info> info =
-      filesystem_proxy_->GetFileInfo(idb_directory);
-  if (!info.has_value())
-    return base::Time();
-  return info->last_modified;
+  base::File::Info info;
+  if (base::GetFileInfo(idb_directory, &info)) {
+    return info.last_modified;
+  }
+  return base::Time();
 }
 
 std::vector<base::FilePath> IndexedDBContextImpl::GetStoragePaths(
@@ -925,10 +930,16 @@ void IndexedDBContextImpl::FactoryOpened(
 }
 
 void IndexedDBContextImpl::WritingTransactionComplete(
-    const storage::BucketLocator& bucket_locator) {
+    const storage::BucketLocator& bucket_locator,
+    bool flushed) {
   DCHECK(!indexeddb_factory_.get() ||
          indexeddb_factory_->GetConnectionCount(bucket_locator.id) > 0);
   NotifyOfBucketModification(bucket_locator);
+  if (!flushed) {
+    // A negative value indicates "not cached, and LevelDB file write is
+    // potentially in progress". See `bucket_size_map_` docs.
+    bucket_size_map_[bucket_locator] = -1;
+  }
 }
 
 void IndexedDBContextImpl::DatabaseDeleted(
@@ -993,9 +1004,8 @@ void IndexedDBContextImpl::ShutdownOnIDBSequence() {
 
     if (delete_bucket) {
       GetIDBFactory()->ForceClose(bucket_locator.id, false);
-      filesystem_proxy_->DeletePathRecursively(GetLevelDBPath(bucket_locator));
-      filesystem_proxy_->DeletePathRecursively(
-          GetBlobStorePath(bucket_locator));
+      base::DeletePathRecursively(GetLevelDBPath(bucket_locator));
+      base::DeletePathRecursively(GetBlobStorePath(bucket_locator));
     }
   }
 }
@@ -1035,16 +1045,32 @@ base::FilePath IndexedDBContextImpl::GetLevelDBPathForTesting(
 }
 
 int64_t IndexedDBContextImpl::ReadUsageFromDisk(
-    const storage::BucketLocator& bucket_locator) const {
+    const storage::BucketLocator& bucket_locator,
+    bool write_in_progress) const {
   if (is_incognito()) {
     if (!indexeddb_factory_)
       return 0;
     return indexeddb_factory_->GetInMemoryDBSize(bucket_locator);
   }
 
+#if BUILDFLAG(IS_WIN)
+  // Touch all files in the LevelDB directory to update directory entry
+  // metadata. See note for `bucket_size_map_` about why this is necessary.
+  if (write_in_progress) {
+    const base::FilePath leveldb_dir = GetLevelDBPath(bucket_locator);
+    base::FileEnumerator file_iter(leveldb_dir, /*recursive=*/true,
+                                   base::FileEnumerator::FILES);
+    for (base::FilePath file_path = file_iter.Next(); !file_path.empty();
+         file_path = file_iter.Next()) {
+      base::File file(
+          file_path, base::File::FLAG_OPEN | base::File::FLAG_WIN_SHARE_DELETE);
+    }
+  }
+#endif
+
   int64_t total_size = 0;
   for (const base::FilePath& path : GetStoragePaths(bucket_locator))
-    total_size += filesystem_proxy_->ComputeDirectorySize(path);
+    total_size += base::ComputeDirectorySize(path);
   return total_size;
 }
 

content/browser/indexed_db/indexed_db_context_impl.h

@@ -166,7 +166,11 @@ class CONTENT_EXPORT IndexedDBContextImpl
   // Methods called by IndexedDBFactory or IndexedDBDispatcherHost for
   // quota support.
   void FactoryOpened(const storage::BucketLocator& bucket_locator);
-  void WritingTransactionComplete(const storage::BucketLocator& bucket_locator);
+  // Called when a transaction has completed for the given bucket. `flushed` is
+  // set to true if the transaction had strict durability (i.e. changes are
+  // flushed/synced to disk).
+  void WritingTransactionComplete(const storage::BucketLocator& bucket_locator,
+                                  bool flushed);
   void DatabaseDeleted(const storage::BucketLocator& bucket_locator);
 
   // Called when blob files have been cleaned (an aggregated delayed task).
@@ -261,7 +265,8 @@ class CONTENT_EXPORT IndexedDBContextImpl
   base::FilePath GetLevelDBPath(
       const storage::BucketLocator& bucket_locator) const;
 
-  int64_t ReadUsageFromDisk(const storage::BucketLocator& bucket_locator) const;
+  int64_t ReadUsageFromDisk(const storage::BucketLocator& bucket_locator,
+                            bool write_in_progress) const;
   void NotifyOfBucketModification(const storage::BucketLocator& bucket_locator);
   base::Time GetBucketLastModified(
       const storage::BucketLocator& bucket_locator);
@@ -309,7 +314,40 @@ class CONTENT_EXPORT IndexedDBContextImpl
   bool force_keep_session_state_;
   const scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy_;
   std::set<storage::BucketLocator> bucket_set_;
+
+  // This map is a cache of the size used by a given bucket. It's calculated by
+  // summing the system-reported sizes of all blob and LevelDB files. This cache
+  // is cleared after transactions that can change the size of the database
+  // (i.e. those that are not readonly), and re-populated lazily. There are
+  // three possible states for each bucket in this map:
+  //
+  // 1) Not present. This indicates that the `ReadUsageFromDisk()` should be
+  //    called to calculate usage (and be stored in the map).
+  // 2) Present, with a non-negative value. This indicates that the cache is, as
+  //    far as we know, valid and up to date for the given bucket. This state
+  //    persists until the next writing transaction occurs.
+  // 3) Present, with a negative value. This indicates that the usage is not
+  //    cached AND the last readwrite transaction did NOT flush changes to disk
+  //    (i.e. durability was 'relaxed').
+  //
+  // On POSIX, the first and third states are treated equivalently. However, on
+  // Windows, `base::FileEnumerator` (and therefore
+  // `base::ComputeDirectorySize()`) will not report up-to-date sizes when a
+  // file is currently being written. When transactions are not set to
+  // "flush"/"sync" (terminology varies based on context), LevelDB will keep
+  // open its file handles. Therefore, on Windows, `ReadUsageFromDisk()` may
+  // not take into account recent writes, leading to situations where
+  // `navigator.storage.estimate()` will not report updates when interleaved
+  // with relaxed durability IDB transactions. The workaround for this is to
+  // open and close new file handles for all the files in the LevelDB data
+  // directory before calculating usage, as this updates the file system
+  // directory entry's metadata. See crbug.com/1489517 and
+  // https://devblogs.microsoft.com/oldnewthing/20111226-00/?p=8813
+  //
+  // TODO(crbug.com/1493696): use an abstract model for quota instead of real
+  // world bytes.
   std::map<storage::BucketLocator, int64_t> bucket_size_map_;
+
   // The set of sites whose storage should be cleared on shutdown. These are
   // matched against the origin and top level site in each bucket's StorageKey.
   std::set<url::Origin> origins_to_purge_on_shutdown_;
@@ -325,7 +363,6 @@ class CONTENT_EXPORT IndexedDBContextImpl
       mock_failure_injector_;
   mojo::RemoteSet<storage::mojom::IndexedDBObserver> observers_;
   mojo::Receiver<storage::mojom::QuotaClient> quota_client_receiver_;
-  const std::unique_ptr<storage::FilesystemProxy> filesystem_proxy_;
 
   // weak_factory_->GetWeakPtr() may be used on any thread, but the resulting
   // pointer must only be checked/used on idb_task_runner_.

content/browser/indexed_db/indexed_db_factory.cc

@@ -799,9 +799,10 @@ IndexedDBFactory::GetOrCreateBucketContext(const storage::BucketInfo& bucket,
       bucket_context_destruction_weak_factory_.GetWeakPtr(), bucket_locator);
   bucket_delegate.on_writing_transaction_complete = base::BindRepeating(
       [](base::WeakPtr<IndexedDBFactory> factory,
-         storage::BucketLocator bucket_locator) {
+         storage::BucketLocator bucket_locator, bool did_sync) {
         if (factory) {
-          factory->context_->WritingTransactionComplete(bucket_locator);
+          factory->context_->WritingTransactionComplete(bucket_locator,
+                                                        did_sync);
         }
       },
       bucket_context_destruction_weak_factory_.GetWeakPtr(), bucket_locator);

content/browser/indexed_db/indexed_db_transaction.cc

@@ -628,7 +628,11 @@ leveldb::Status IndexedDBTransaction::CommitPhaseTwo() {
     }
 
     if (mode() != blink::mojom::IDBTransactionMode::ReadOnly) {
-      bucket_context_->delegate().on_writing_transaction_complete.Run();
+      const bool did_sync =
+          mode() == blink::mojom::IDBTransactionMode::VersionChange ||
+          backing_store_transaction_->durability() ==
+              blink::mojom::IDBTransactionDurability::Strict;
+      bucket_context_->delegate().on_writing_transaction_complete.Run(did_sync);
     }
 
     if (database_) {