New issue
Advanced search Search tips

Issue 808285 link

Starred by 2 users

Issue metadata

Status: Untriaged
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Enhance SQLitePersistentCookieStore to more gracefully handle DB errors

Project Member Reported by rdsmith@chromium.org, Feb 2 2018

Issue description

Currently SQLitePersistentCookieStore is somewhat haphazard in how it handles database errors.  If in a debug build, it may assert, but in an optimized build it basically ignores them.  The different errors that occur should be evaluated and appropriate error handling built in.  Off the cuff suggestions:

* An "out of disk space" error should probably be ignored, or possibly logged to a system log.
* A DB constraint violation should at minimum result in a UMA report, and possibly on Canary (/Dev?) should CHECK().
* Evidence of database corruption should trigger a rewrite out to the disk from internal cookie store.

 
I'm not sure we should be checking if the DB is corrupt - that could permanently host people's Canary installs, and few enough people use Canary already.  Also, I don't think we should ever assume that on-disk files are well formed.  Disk corruption exists, even if there's practically no chance that disk corruption will affect a file in a way that results in DB constraint violation, I'd rather not have have Chrome depend on that assumption.
Sorry, that should be "I'm not sure we should be CHECKing if the DB is corrupt."  checking for that case is certainly reasonable.
Agreed that DB corruption shouldn't result in checking for exactly the reasons you call out.  I think of a DB constraint violation as not being about DB corruption, but about layers higher than the SQLitePersistentCookieStore not respecting cookie semantics.  If I had put my bullets in a different order (with checking for DB corruption first :-}), would that resolve your concern?
I'm not sure how you have a DB constraint violation without evidence of a DB constraint violation?  I don't see how you'd even run into the CHECK case, if you prefer rewrite the DB over a violation instead of CHECKing.
My mental example case would be the cookie monster handing a cookie to the persistent store to be added to it which duplicates a cookie already in the store.  The store addition would fail because of a uniqueness constraint, but that's a reflection of the consumer doing something stupid, not of the DB being corrupt.  The cookie store *should* have said "That cookie already exists" and deleted it before doing the addition, or just updated the access time.  (I should write this into the comments in SQLitePersistentCookieStore, but I do think of it as part of the interface contract.)

Does that help?  I could totally get that there are cases where it's uncertain whether it's a cookie DB corruption or an interface contract violation, and in such cases we should (at a minimum) nuke the DB before CHECKing, but I think CHECKing on interface contract violations is a possible technique that we may want to do.  And I can think of at least one case where it's clearly (fairly clear--DB corruptions can be tricky) the consumer rather than the on disk DB.



Ah, you mean a corrupt/broken/demonically possessed CookieMonster.  That makes sense.

Comment 7 by mmenke@chromium.org, Feb 14 2018

Labels: Network-Triaged
Bulk edit**

This bug has the label Postmortem-Followup but has not been updated in 3+ weeks. We are working on a new workflow to improve postmortem followthrough. Postmortems and postmortem bugs are very important in making sure we don't repeat prior mistakes and for making Chrome better for all.

We will be taking a closer look at these bugs in the coming weeks. Please take some time to work on this, reassign, or close if the issue has been fixed. Thank you.

Sign in to add a comment