DomStorage: Make DomStorageDatabaseFactory async and rollout-aware [chromium/src : main]
Steve Becker (Gerrit)
Steve Becker added 6 comments![]( "Open in Gerrit")
base::ThreadPool::PostTaskAndReplyWithResult(Can we use the LevelDB's task runner for this?
`GetTaskRunnerForDb(path)`
That will avoid a race where we're checking this while other operations are in flight. While this is unlikely, it could happen if a user closes the browser profile and then re-opens it.
base::BindOnce(&DomStorageDatabaseFactory::OnDiskStateChecked,This delays Open(), while some delay is unavoidable since we're doing extra work, maybe we can minimize the delay by doing more work in the task that checks the leveldbstate. That task could:
1. Check the level db state.
2. Create the right database based on the result.
3. Call the async Open() operation.
4. The factory create callback should run after open completes.
Maybe we can rename DomStorageDatabaseFactory::Create to Open. It's confusing that we have two create callbacks (CreateCallback and CreateResultCallback) and three Create functions, Create, CreateImpl and CreateDatabase(). Maybe we can end up with just Open/OpenImp and CreateDatabase() instead?
For this plan, we'll have to take care that everything ends up on running on the right sequence. You can use BindPostTask on the callback to make sure it runs in the expected place.
base::ThreadPool::PostTaskAndReplyWithResult(Same as Create above, can we use the LevelDB's task runner for this?
`GetTaskRunnerForDb(path)`
std::string basename = database_path.BaseName().MaybeAsASCII();
bool is_sqlite = (basename == "LocalStorage" || basename == "SessionStorage");Should the caller pass this parameter instead? This is fragile if the path constant changes. It also duplicates logic.
base::BindOnce(&SessionStorageImpl::OnDiskStateCleared,We should try to avoid waiting for deletion if possible. It puts us into a weird state where database_ is nullptr while waiting for the delete to finish. Maybe we can add a flag to the proposed DomStorageDatabaseFactory::Open (see other comments) that will kick off the deletion before creating and opening the database.
// TODO(crbug.com/377242771): Move this histogram toLet's make a new bug for this so it does not get lost.
Related details
- Rahul Singh
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
// If a LevelDB database exists, destroy it. Otherwise destroy the SQLite
// path (which is a no-op if no SQLite database exists either).
base::FilePath path = (leveldb_state != LevelDbOnDiskState::kNone)
? DomStorageDatabase::GetLevelDbPath(
storage_type, storage_partition_dir)
: DomStorageDatabase::GetSqlitePath(
storage_type, storage_partition_dir);Question: in a normal rollout I think only one active backend should exist, but should we defensively clear both possible backend paths in case a partial clean up leaves both LevelDB and SQLite on disk? Is that possible?
Related details
- Rahul Singh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted and added 4 comments![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +1 |
4 comments
Can we use the LevelDB's task runner for this?
`GetTaskRunnerForDb(path)`
That will avoid a race where we're checking this while other operations are in flight. While this is unlikely, it could happen if a user closes the browser profile and then re-opens it.
Done
Same as Create above, can we use the LevelDB's task runner for this?
`GetTaskRunnerForDb(path)`
Done
std::string basename = database_path.BaseName().MaybeAsASCII();
bool is_sqlite = (basename == "LocalStorage" || basename == "SessionStorage");Should the caller pass this parameter instead? This is fragile if the path constant changes. It also duplicates logic.
Done
Let's make a new bug for this so it does not get lost.
This TODO has been handled in CL5 in this relation chain. See here:
https://chromium-review.googlesource.com/c/chromium/src/+/7919708
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh added 2 comments![]( "Open in Gerrit")
base::BindOnce(&DomStorageDatabaseFactory::OnDiskStateChecked,This delays Open(), while some delay is unavoidable since we're doing extra work, maybe we can minimize the delay by doing more work in the task that checks the leveldbstate. That task could:
1. Check the level db state.
2. Create the right database based on the result.
3. Call the async Open() operation.
4. The factory create callback should run after open completes.Maybe we can rename DomStorageDatabaseFactory::Create to Open. It's confusing that we have two create callbacks (CreateCallback and CreateResultCallback) and three Create functions, Create, CreateImpl and CreateDatabase(). Maybe we can end up with just Open/OpenImp and CreateDatabase() instead?
For this plan, we'll have to take care that everything ends up on running on the right sequence. You can use BindPostTask on the callback to make sure it runs in the expected place.
Great suggestion @ste...@microsoft.com! I took a stab at this in PS 12. PTAL, if it looks good, I'll follow up with an update that address your concern around waiting for deletion. Thank you!
// If a LevelDB database exists, destroy it. Otherwise destroy the SQLite
// path (which is a no-op if no SQLite database exists either).
base::FilePath path = (leveldb_state != LevelDbOnDiskState::kNone)
? DomStorageDatabase::GetLevelDbPath(
storage_type, storage_partition_dir)
: DomStorageDatabase::GetSqlitePath(
storage_type, storage_partition_dir);Question: in a normal rollout I think only one active backend should exist, but should we defensively clear both possible backend paths in case a partial clean up leaves both LevelDB and SQLite on disk? Is that possible?
Good question! Steve and I previously discussed this. IIRC we settled on this is worth a TODO but not an OnDisk Experiment blocker. @ste...@microsoft.com does that sound accurate?
Related details
- Steve Becker
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker added 11 comments![]( "Open in Gerrit")
Thanks for iterating. Please let me know if you have any questions.
// `database_` and `is_sqlite_` must not be used until `is_database_opened_`nit: Should the `is_sqlite()` accessor above assert that the database is opened?
is_database_opened_ = result.open_status.ok();nit: if `is_database_opened_` is true, should we assert that `database` is valid?
OpenResultCallback reply_to_caller);nit: callback?
// Production implementation of Open(). Routes through GetOpenCallback().nit: I'm not sure if this comment is needed since OpenImpl does not use GetOpenCallback().
struct OpenDatabaseParams {nit: Should we call this `InitializeDatabaseParams` since it's used by InitializeDatabase()?
struct OpenDatabaseParams {nit: Please declare near use.
void DomStorageDatabaseFactory::InitializeDatabase(
OpenDatabaseParams params,
OpenResultCallback reply_to_caller) {nit: instead of having both InitializeOnDatabaseSequence() and InitializeDatabase(), we could combine these by having InitializeDatabase() get database's the task runner and then post a task to re-run the function if it's not on the database's task runner.
I believe this pattern is used throughout Chromium. That way, the caller of InitializeDatabase() does not need to worry about the calling sequence.
We could also maybe eliminate OpenDatabaseParams since it would only be used once.
result.database = base::SequenceBound<std::unique_ptr<DomStorageDatabase>>(Unfortunately, to avoid DCHECKS I think we're going to need to leak the database pointer here. See
and
// If a LevelDB database exists, destroy it. Otherwise destroy the SQLite
// path (which is a no-op if no SQLite database exists either).
base::FilePath path = (leveldb_state != LevelDbOnDiskState::kNone)
? DomStorageDatabase::GetLevelDbPath(
storage_type, storage_partition_dir)
: DomStorageDatabase::GetSqlitePath(
storage_type, storage_partition_dir);Rahul SinghQuestion: in a normal rollout I think only one active backend should exist, but should we defensively clear both possible backend paths in case a partial clean up leaves both LevelDB and SQLite on disk? Is that possible?
Good question! Steve and I previously discussed this. IIRC we settled on this is worth a TODO but not an OnDisk Experiment blocker. @ste...@microsoft.com does that sound accurate?
For this experiment, I think we should only delete one type of database. This is a good question for our future database migration. To keep things simple, I don't think we should consider the migration successful unless we've deleted the levelDB after migrating to sqlite.
GetParam() ? DomStorageSqliteRolloutStage::kUseSqliteForNewDatabases
: DomStorageSqliteRolloutStage::kUseLevelDbAsControl;Do we have test coverage for kUseLevelDbOnly and kUseSqliteOnly?
Related details
- Rahul Singh
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted and added 11 comments![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +1 |
11 comments
// `database_` and `is_sqlite_` must not be used until `is_database_opened_`nit: Should the `is_sqlite()` accessor above assert that the database is opened?
Done
nit: if `is_database_opened_` is true, should we assert that `database` is valid?
Done
OpenResultCallback reply_to_caller);Rahul Singhnit: callback?
Done
// Production implementation of Open(). Routes through GetOpenCallback().nit: I'm not sure if this comment is needed since OpenImpl does not use GetOpenCallback().
Done
nit: Should we call this `InitializeDatabaseParams` since it's used by InitializeDatabase()?
I removed the POD struct
nit: Please declare near use.
Just removed it instead
void DomStorageDatabaseFactory::InitializeDatabase(
OpenDatabaseParams params,
OpenResultCallback reply_to_caller) {nit: instead of having both InitializeOnDatabaseSequence() and InitializeDatabase(), we could combine these by having InitializeDatabase() get database's the task runner and then post a task to re-run the function if it's not on the database's task runner.
I believe this pattern is used throughout Chromium. That way, the caller of InitializeDatabase() does not need to worry about the calling sequence.
We could also maybe eliminate OpenDatabaseParams since it would only be used once.
Done
result.database = base::SequenceBound<std::unique_ptr<DomStorageDatabase>>(Unfortunately, to avoid DCHECKS I think we're going to need to leak the database pointer here. See
and
Done
// If a LevelDB database exists, destroy it. Otherwise destroy the SQLite
// path (which is a no-op if no SQLite database exists either).
base::FilePath path = (leveldb_state != LevelDbOnDiskState::kNone)
? DomStorageDatabase::GetLevelDbPath(
storage_type, storage_partition_dir)
: DomStorageDatabase::GetSqlitePath(
storage_type, storage_partition_dir);Rahul SinghQuestion: in a normal rollout I think only one active backend should exist, but should we defensively clear both possible backend paths in case a partial clean up leaves both LevelDB and SQLite on disk? Is that possible?
Steve BeckerGood question! Steve and I previously discussed this. IIRC we settled on this is worth a TODO but not an OnDisk Experiment blocker. @ste...@microsoft.com does that sound accurate?
For this experiment, I think we should only delete one type of database. This is a good question for our future database migration. To keep things simple, I don't think we should consider the migration successful unless we've deleted the levelDB after migrating to sqlite.
Thanks Steve! I've added a short TODO in code for now.
GetParam() ? DomStorageSqliteRolloutStage::kUseSqliteForNewDatabases
: DomStorageSqliteRolloutStage::kUseLevelDbAsControl;Do we have test coverage for kUseLevelDbOnly and kUseSqliteOnly?
Good catch. I extended the existing tests to cover the other 2 param states too. Thanks!
base::BindOnce(&SessionStorageImpl::OnDiskStateCleared,We should try to avoid waiting for deletion if possible. It puts us into a weird state where database_ is nullptr while waiting for the delete to finish. Maybe we can add a flag to the proposed DomStorageDatabaseFactory::Open (see other comments) that will kick off the deletion before creating and opening the database.
Good idea! I’ll tackle this in a follow up CL as this one is already fairly large. I can chain it on top of this relation chain so we’ll have it before we start the Finch.
Related details
- Steve Becker
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted and added 2 comments![]( "Open in Gerrit")
Votes added by Steve Becker
| Code-Review | +1 |
2 comments
if (is_database_opened_) {
CHECK(database_);
}nit: simplify:
CHECK_EQ(is_database_opened_, database_)
return std::move(*base::WrapUnique(database_ptr));nit: don't need std::move for the return value. It's implicit.
Related details
- Rahul Singh
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rahul Singh
| Code-Review | +1 |
| Commit-Queue | +2 |
2 comments
if (is_database_opened_) {
CHECK(database_);
}Rahul Singhnit: simplify:
CHECK_EQ(is_database_opened_, database_)
SequenceBound only has an explicit operator bool() (its null check), and CHECK_EQ expands to ==, which requires an implicit bool conversion. So, CHECK_EQ(is_database_opened_, database_) won't compile.
We also can't do CHECK_EQ(!database_.is_null(), is_database_opened) because we still return a database_ when Open fails. So we will trigger the CHECK in cases where Open fails.
As such, I think we should leave this as is.
nit: don't need std::move for the return value. It's implicit.
Dereferencing the unique_ptr will return an lvalue reference. The compiler tries to copy it on return. But SequenceBound has a deleted copy constructor. So this doesn't compile without the std::move.
Related details
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rahul Singh
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
base::BindOnce(&SessionStorageImpl::OnDiskStateCleared,Rahul SinghWe should try to avoid waiting for deletion if possible. It puts us into a weird state where database_ is nullptr while waiting for the delete to finish. Maybe we can add a flag to the proposed DomStorageDatabaseFactory::Open (see other comments) that will kick off the deletion before creating and opening the database.
Good idea! I’ll tackle this in a follow up CL as this one is already fairly large. I can chain it on top of this relation chain so we’ll have it before we start the Finch.
Here's the follow up CL that will tackle this. I'll open it up for review within the next day.
7966139: DomStorage: Fold DomStorageDatabase Destroy into Open | https://chromium-review.googlesource.com/c/chromium/src/+/7966139
Related details
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
17 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: components/services/storage/dom_storage/dom_storage_database.cc
Insertions: 17, Deletions: 18.
@@ -4,7 +4,6 @@
#include "components/services/storage/dom_storage/dom_storage_database.h"
-#include "base/debug/leak_annotations.h"
#include "base/feature_list.h"
#include "base/functional/bind.h"
#include "base/memory/ptr_util.h"
@@ -194,29 +193,29 @@
scoped_refptr<base::SequencedTaskRunner> task_runner,
std::unique_ptr<DomStorageDatabase> database) {
CHECK(!database_);
- // Binds `database` to `task_runner`, then heap-allocates and deliberately
- // leaks the `SequenceBound` so ownership can cross to the caller's sequence
- // as a raw pointer, to be re-owned by `TakeDatabase()`. Leaking matters
- // because the reply may be dropped after shutdown; destroying an owned
- // `SequenceBound` then would post a shutdown-blocking task to delete the
- // database and DCHECK, whereas a dropped raw pointer simply leaks. See
+ // Hold the database with a deleter that destroys it on `task_runner` (its
+ // backend sequence) if this result is dropped before `TakeDatabase()` (e.g.
+ // the open reply is dropped because its owner was torn down). Without this we
+ // leak the opened database, which keeps its file handle open and prevents
+ // Windows from deleting TempDirs created during test runs.
+ //
+ // Unlike a `SequenceBound`, a `std::unique_ptr` with an `OnTaskRunnerDeleter`
+ // relies on `DeleteSoon`, which closes the database's file handle on drop
+ // while fizzling the destruction if posted after thread pool shutdown. See
// https://crbug.com/40746642.
- auto holder = std::make_unique<
- base::SequenceBound<std::unique_ptr<DomStorageDatabase>>>(
- std::move(task_runner), std::move(database));
- auto* database_ptr = holder.release();
- ANNOTATE_LEAKING_OBJECT_PTR(database_ptr);
- database_ = database_ptr;
+ database_ = std::unique_ptr<DomStorageDatabase, base::OnTaskRunnerDeleter>(
+ database.release(), base::OnTaskRunnerDeleter(std::move(task_runner)));
}
base::SequenceBound<std::unique_ptr<DomStorageDatabase>>
DomStorageDatabaseFactory::OpenResult::TakeDatabase() {
CHECK(database_);
- // Re-own the leaked `SequenceBound` and clear the raw pointer so it doesn't
- // dangle.
- auto* database_ptr = database_.get();
- database_ = nullptr;
- return std::move(*base::WrapUnique(database_ptr));
+ // Re-bind the opened database to its backend sequence for the caller.
+ // Adopting the pointer does not touch the database off-sequence.
+ scoped_refptr<base::SequencedTaskRunner> task_runner =
+ database_.get_deleter().task_runner_;
+ return base::SequenceBound<std::unique_ptr<DomStorageDatabase>>(
+ std::move(task_runner), base::WrapUnique(database_.release()));
}
// static
```
```
The name of the file: components/services/storage/dom_storage/dom_storage_database.h
Insertions: 12, Deletions: 10.
@@ -17,7 +17,6 @@
#include "base/byte_size.h"
#include "base/containers/span.h"
#include "base/functional/callback.h"
-#include "base/memory/raw_ptr.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_refptr.h"
#include "base/task/sequenced_task_runner.h"
@@ -374,14 +373,15 @@
OpenResult(OpenResult&&);
OpenResult& operator=(OpenResult&&);
- // Binds `database` to `task_runner` and stores it for transport to the
- // caller's sequence. Producers of an `OpenResult` must set the database via
- // this. See the definition for why the stored pointer is leaked.
+ // Stores `database` for transport to the caller's sequence, with a deleter
+ // that destroys it on `task_runner` (its backend sequence) if the result is
+ // dropped before `TakeDatabase()`. Producers of an `OpenResult` must set
+ // the database via this.
void SetDatabase(scoped_refptr<base::SequencedTaskRunner> task_runner,
std::unique_ptr<DomStorageDatabase> database);
- // Re-owns and returns the database previously stored by `SetDatabase()`.
- // Must be called once, on the caller's sequence.
+ // Returns the database stored by `SetDatabase()`, bound to its backend
+ // sequence. Must be called once, on the caller's sequence.
base::SequenceBound<std::unique_ptr<DomStorageDatabase>> TakeDatabase();
base::FilePath database_path;
@@ -390,10 +390,12 @@
DbStatus open_status = DbStatus::IOError("uninitialized");
private:
- // Leaked, non-owning pointer to the opened database while the result is in
- // flight to the caller's sequence. See `SetDatabase()`.
- raw_ptr<base::SequenceBound<std::unique_ptr<DomStorageDatabase>>>
- database_ = nullptr;
+ // The opened database while the result is in flight to the caller's
+ // sequence. If the result is dropped before `TakeDatabase()`, the
+ // `OnTaskRunnerDeleter` destroys it on its backend sequence, closing its
+ // file handle.
+ std::unique_ptr<DomStorageDatabase, base::OnTaskRunnerDeleter> database_{
+ nullptr, base::OnTaskRunnerDeleter(nullptr)};
};
using OpenResultCallback = base::OnceCallback<void(OpenResult)>;
```
Change information
DomStorage: Make DomStorageDatabaseFactory async and rollout-aware
Converts `DomStorageDatabaseFactory::Open` and `Destroy` to asynchronous
APIs that take a `storage_partition_dir` plus a callback, and moves the
rollout decision into the factory. For on-disk databases in an
experimental stage, both methods post a blocking task to inspect the
on-disk state (existing LevelDB directory and exp-v1 tag) and use it to
pick the backend, database path, `write_exp_tag` value, and histogram
metrics_type.
`Open` opens the database and returns its related state via a new
`OpenResult` struct, which `AsyncDomStorageDatabase::OnDatabaseOpened`
consumes to take the opened database and record metrics with
`GetHistogramSuffix(metrics_type_)`. Callers (`LocalStorageImpl`,
`SessionStorageImpl`, their tests, and `FakeDomStorageDatabaseFactory`)
are updated to the new signatures, and the now-redundant
`GetDatabasePath()` helpers are removed.
- M components/services/storage/dom_storage/async_dom_storage_database.cc
- M components/services/storage/dom_storage/async_dom_storage_database.h
- M components/services/storage/dom_storage/dom_storage_database.cc
- M components/services/storage/dom_storage/dom_storage_database.h
- M components/services/storage/dom_storage/dom_storage_database_unittest.cc
- M components/services/storage/dom_storage/leveldb/local_storage_leveldb.cc
- M components/services/storage/dom_storage/leveldb/local_storage_leveldb.h
- M components/services/storage/dom_storage/leveldb/local_storage_leveldb_unittest.cc
- M components/services/storage/dom_storage/leveldb/session_storage_leveldb.cc
- M components/services/storage/dom_storage/leveldb/session_storage_leveldb.h
- M components/services/storage/dom_storage/leveldb/session_storage_leveldb_unittest.cc
- M components/services/storage/dom_storage/local_storage_impl.cc
- M components/services/storage/dom_storage/local_storage_impl.h
- M components/services/storage/dom_storage/local_storage_impl_unittest.cc
- M components/services/storage/dom_storage/session_storage_impl.cc
- M components/services/storage/dom_storage/session_storage_impl.h
- M components/services/storage/dom_storage/session_storage_impl_unittest.cc
- M components/services/storage/dom_storage/sqlite/local_storage_sqlite.h
- M components/services/storage/dom_storage/sqlite/session_storage_sqlite.h
- M components/services/storage/dom_storage/test_support/fake_dom_storage_database.h
- M components/services/storage/dom_storage/test_support/fake_dom_storage_database_factory.cc
- M components/services/storage/dom_storage/test_support/fake_dom_storage_database_factory.h
- M components/services/storage/dom_storage/test_support/scoped_dom_storage_database_factory_for_testing.cc
- M components/services/storage/dom_storage/test_support/scoped_dom_storage_database_factory_for_testing.h
- M content/browser/dom_storage/dom_storage_context_wrapper.cc
- M content/browser/storage_partition_impl_unittest.cc
Code-Review: +1 by Rahul Singh, +1 by Steve Becker
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |