DomStorage: Add OnDiskExperimental suffix to Recovery UMAs [chromium/src : main]
Steve Becker (Gerrit)
Steve Becker voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Rahul Singh
- 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 added 1 comment![]( "Open in Gerrit")
Adding leimy@ for an OWNERS review of updates to storage/histograms.xml.
@ste...@microsoft.com could you PTAL as the rebase required some fixups that removed your earlier vote.
Thanks both!
Related details
- Mingyu Lei
- Steve Becker
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingyu Lei (Gerrit)
Mingyu Lei added 1 comment![]( "Open in Gerrit")
data was silently lost without retry. The {DomStorageSqliteRolloutStage}This will be replaced by the actual description of the `DomStorageSqliteRolloutStage` right? so for the empty case, this sentence becomes `The suffix distinguishes the data...`, which might be strange.
Related details
- Rahul Singh
- 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. |
Rahul Singh (Gerrit)
Rahul Singh voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +1 |
1 comment
data was silently lost without retry. The {DomStorageSqliteRolloutStage}This will be replaced by the actual description of the `DomStorageSqliteRolloutStage` right? so for the empty case, this sentence becomes `The suffix distinguishes the data...`, which might be strange.
Good catch. Fixed by breaking out the `.OnDiskExperimental` specifically to talk about its significance.
Related details
- Mingyu Lei
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingyu Lei (Gerrit)
Mingyu Lei added 1 comment![]( "Open in Gerrit")
const DatabaseMetricsType metrics_type = database_->metrics_type();What if the database_ is null here? The comments below says "ignore additional errors from the old database while waiting for the new database to open." so it's possible that the database is not re-opened yet at this moment right?
Related details
- Rahul Singh
- Steve Becker
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
| Commit-Queue | +1 |
1 comment
const DatabaseMetricsType metrics_type = database_->metrics_type();What if the database_ is null here? The comments below says "ignore additional errors from the old database while waiting for the new database to open." so it's possible that the database is not re-opened yet at this moment right?
Good catch! `database_` can't actually be null here. OnCommitResult only runs via StorageAreaImpl::OnCommitComplete. That method is invoked using a WeakPtr to the StorageAreaImpl.
Recovery clears all areas `areas_.clear()` before resetting `database_`. So, any in-flight completion from the old database is dropped and never reaches this method.
Additionally, new areas can't be created mid-recovery either. GetOrCreateStorageArea does `CHECK_EQ(connection_state_, CONNECTION_FINISHED);`. So this method only runs when `connection_state_ == CONNECTION_FINISHED`. In that scenario, we also have a valid `database_`.
I've added a `CHECK(database_);` at the top to make this explicit.
Finally, the "connection_state_ !=
CONNECTION_FINISHED" early-exit below (and its comment) is now effectively
dead code. To keep this CL focused, in a follow up CL, I'll clean that up and add a
`CHECK_EQ(connection_state_, CONNECTION_FINISHED)` instead.
Related details
- Mingyu Lei
- Steve Becker
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +1 |
1 comment
const DatabaseMetricsType metrics_type = database_->metrics_type();Rahul SinghWhat if the database_ is null here? The comments below says "ignore additional errors from the old database while waiting for the new database to open." so it's possible that the database is not re-opened yet at this moment right?
Good catch! `database_` can't actually be null here. OnCommitResult only runs via StorageAreaImpl::OnCommitComplete. That method is invoked using a WeakPtr to the StorageAreaImpl.
Recovery clears all areas `areas_.clear()` before resetting `database_`. So, any in-flight completion from the old database is dropped and never reaches this method.
Additionally, new areas can't be created mid-recovery either. GetOrCreateStorageArea does `CHECK_EQ(connection_state_, CONNECTION_FINISHED);`. So this method only runs when `connection_state_ == CONNECTION_FINISHED`. In that scenario, we also have a valid `database_`.
I've added a `CHECK(database_);` at the top to make this explicit.
Finally, the "connection_state_ !=
CONNECTION_FINISHED" early-exit below (and its comment) is now effectively
dead code. To keep this CL focused, in a follow up CL, I'll clean that up and add a
`CHECK_EQ(connection_state_, CONNECTION_FINISHED)` instead.
I missed analyzing SessionStorage::OnCommitResult before posting my previous comment. That one is different. Some operations in that class (ShallowClone, DeleteSessions, DeleteStorageKeys) pass OnCommitResult as a completion callback bound to the SessionStorageImpl itself.
So they can run after recovery resets `database_`. So in that case, I kept a runtime guard `if (connection_state_ != FINISHED || !database_)` instead of CHECKing.
And now that we are pulling up the `if (connection_state_ != FINISHED)` in SessionStorage, I went ahead and also did the clean up on the LocalStorage side in this change itself. Thanks again for catching this!
Related details
- Mingyu Lei
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steve Becker
| Code-Review | +1 |
1 comment
// Some operations in this class pass this method as a completion callback,
// which can run after `database_` has been reset by recovery. Ignore those
// stale results.nit: what's wrong with the old comment?
```
// Previous commit errors deleted and recreated the database below. Ignore
// additional errors from the old database while waiting for the new
// database to open.
```
The new condition `!database_` would be applicable after recovery completely failed and `SessionStorageImpl` decided to run without a database in the `database_` nullptr state. `DeleteAndRecreateDatabase()` resets `database_` but then immediately recreates it. Maybe we can append something to the old comment that explains the new condition?
Related details
- Mingyu Lei
- 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 1 comment![]( "Open in Gerrit")
Votes added by Rahul Singh
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
// Some operations in this class pass this method as a completion callback,
// which can run after `database_` has been reset by recovery. Ignore those
// stale results.nit: what's wrong with the old comment?
```
// Previous commit errors deleted and recreated the database below. Ignore
// additional errors from the old database while waiting for the new
// database to open.
```The new condition `!database_` would be applicable after recovery completely failed and `SessionStorageImpl` decided to run without a database in the `database_` nullptr state. `DeleteAndRecreateDatabase()` resets `database_` but then immediately recreates it. Maybe we can append something to the old comment that explains the new condition?
Good call. Updated!
Related details
- Mingyu Lei
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |