Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 252217 Cookie deletion on behalf of user request shouldn't return until deletion has been sync'd to on-disk cookie store.
Starred by 5 users Reported by philip.j...@gmail.com, Jun 20 2013 Back to list
Status: Fixed
Owner:
Email to this user bounced
Closed: Oct 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 302914



Sign in to add a comment
Steps to reproduce the problem:
While investigating an Opera (for Android) cookie bug I found that CookieMonster::InternalDeleteCookie() doesn't seem to do what it claims to do. There's a bool sync_to_store which will cause store_->DeleteCookie(*cc) to be called, but the implementation (SQLitePersistentCookieStore::Backend::DeleteCookie) isn't synchronous, it just batches operations for later. However, CookieMonster::DeleteAllCreatedBetweenTask, which uses InternalDeleteCookie, doesn't wait for the sync to disk, it just calls the "done" callback.

Given that BrowsingDataRemover uses these code paths, the result is that it can report being done even though the cookies are still on disk.

What is the expected behavior?
If BrowsingDataRemover reports being done, the data should really be gone, even if the browser isn't shut down properly.

What went wrong?
If the browser crashes or is forcefully killed at that point, the cookies will remain on restart.

Did this work before? N/A 

Chrome version: master  Channel: n/a
OS Version: 

I'd be happy to write a patch, if it is confirmed that the current behavior is not intentional.
 
Labels: Cr-Internals-Network-Cookies
Comment 2 by jochen@chromium.org, Jun 20 2013
Cc: jochen@chromium.org
Labels: -OS-Android OS-All Cr-Privacy
I would argue that even when deleting a single cookie via the UI, we want it to be deleted.

Status: Untriaged
Summary: Cookie deletion on behalf of user request shouldn't return until deletion has been sync'd to on-disk cookie store. (was: CookieMonster::InternalDeleteCookie() doesn't sync to store)
So historically I believe that that boolean was an implementation detail, specifically around whether or not the persistent store was told about the delete.  It wasn't intended to force a synchronous sync out to the cookie store.  

But that's a separate issue from whether BrowsingDataRemover shouldn't return until we've actually sync'd with the cookie store.  If we do decide to do that, I'm inclined to think that the implementation should be for DeleteAllCreatedBetween to chase the currently batched persistent store tasks out to the DB thread, bounce off the DB thread, and then call the callback.  I wouldn't think that would be very hard to do (given that we now have an async interface to the CM.



Comment 4 by jochen@chromium.org, Jun 20 2013
Randy, but what about the cookie dialog where you can delete individual cookies? Shouldn't that also commit before coming back?

I would argue that all async Delete* calls should delete before they come back, and only internal deletion (because of expiry or similar) are allowed to batch up their ops
Comment 5 by phil...@opera.com, Jun 20 2013
The solution I've imagined is to add a callback to CookieMonster::Delete*, to call FlushStore after the call(s) to InternalDeleteCookie, and to just link the callback to FlushStore to the callback argument by whatever glue is required. rdsmith, does that amount to about the same thing you were saying?

jochen, can Delete* really be (safely) made synchronous, given that CookieMonster::FlushStore isn't?

Comment 6 by phil...@opera.com, Jun 20 2013
To clarify, from the perspective of BrowsingDataRemover, it doesn't seem to matter if Delete* deletes synchronously or not, since it waits for lots of other things anyway.
Comment 7 by jochen@chromium.org, Jun 20 2013
I didn't mean to say that Delete* should be synchronous, I meant it shouldn't invoke the callback before the deletions were flushed to the store.
Comment 8 by phil...@opera.com, Jun 20 2013
Oh, I overlooked the "async" in "async Delete* calls".
Sorry, Jochen, I wrote my response before your c#2, and didn't sufficiently edit it.  I think it's fine for the "delay until on disk" callback behavior to apply to all delete methods.  I was mostly just talking about implementation; I defer to you/Erik as to what the behavior should be.   But if I wasn't deferring, I'd agree with you :-}.

I don't mind changing this, but we have to be careful to ensure that only user-initiated deletes block on disk output, not deletions due to overwrites or evictions.

Personally, though I'm not convinced that it's required. In the Chrome case the SQLitePersistentCookieStore will write to disk after some not too long period of time.

After all, if the machine crashes, having "written it to disk" doesn't mean that it's really on disk anyway. So this covers a browser crash but is still not complete. Does the underlying store take pains to ask the OS to flush the disk cache?

And why is this different than what happens when I explicitly log out of my bank?

My interpretation of the callbacks is simply that we promise the change will be reflected in subsequent calls to the CookieStore. In fact, I was tempted to not have callbacks for anything except for the Read operations because we _already_ guarantee that operations are applied in the order they are received.
I guess on Android, it's just far more likely to happen, as the browser doesn't technically crash, but the OS decides to kill it
(also, if you can continue your session with the bank after logging out just by restoring the cookie, change your bank now.)
Good points, jochen.

I'm OK with the public Delete methods on CookieStore and CookieMonster guaranteeing a flush-to-persistence before the callback and the implementation in CookieMonster being updated accordingly.

philipj, can you submit a patch?


Sure, I'll work on a patch when I'm back from a mini-vacation in Wednesday next week, assuming no one else deems this critical and does it first. (Sorry about using two accounts for commenting.)
Comment 15 by phil...@opera.com, Jun 27 2013
I've posted a trivial change in https://codereview.chromium.org/18032002/ for discussion.
Project Member Comment 16 by bugdroid1@chromium.org, Sep 16 2013
------------------------------------------------------------------------
r223324 | philipj@opera.com | 2013-09-16T10:15:05.318590Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster_unittest.cc?r1=223324&r2=223323&pathrev=223324
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster.cc?r1=223324&r2=223323&pathrev=223324
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster.h?r1=223324&r2=223323&pathrev=223324

Wait for store flush in CookieMonster::Delete*Task

BUG= 252217 

Review URL: https://chromiumcodereview.appspot.com/18032002
------------------------------------------------------------------------
Comment 17 by phil...@opera.com, Sep 16 2013
Can I close this myself or is a third party supposed to come along and verify it?
Owner: phil...@opera.com
Status: Fixed
You can describe here how to verify the fix, and somebody from QA will flip the bug from fixed to verified.

You could have also added a TEST= line to the CL describing how to test it.

Anyway, the initial bug report contains some repro steps, that might be good enough in this case

Comment 19 by phil...@opera.com, Sep 16 2013
Right, in order to test this a UI for deleting private data is needed. If the UI reports being done, no matter how the browser process terminates, those cookies should be gone.
Project Member Comment 20 by bugdroid1@chromium.org, Sep 16 2013
------------------------------------------------------------------------
r223359 | nasko@chromium.org | 2013-09-16T18:00:07.403641Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster_unittest.cc?r1=223359&r2=223358&pathrev=223359
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster.cc?r1=223359&r2=223358&pathrev=223359
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster.h?r1=223359&r2=223358&pathrev=223359

Revert 223324 "Wait for store flush in CookieMonster::Delete*Task"

ExtensionApiTest.CookiesEvents is failing intermittently, so reverting this
change.

> Wait for store flush in CookieMonster::Delete*Task
> 
> BUG= 252217 
> 
> Review URL: https://chromiumcodereview.appspot.com/18032002

TBR=philipj@opera.com,erikwright@chromium.org

Review URL: https://codereview.chromium.org/23530062
------------------------------------------------------------------------
Comment 21 by phil...@opera.com, Oct 2 2013
Blockedon: chromium:302914
Status: Started
Project Member Comment 22 by bugdroid1@chromium.org, Oct 3 2013
------------------------------------------------------------------------
r226720 | philipj@opera.com | 2013-10-03T10:13:16.288381Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster_unittest.cc?r1=226720&r2=226719&pathrev=226720
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster.cc?r1=226720&r2=226719&pathrev=226720
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/cookie_monster.h?r1=226720&r2=226719&pathrev=226720

Wait for store flush in CookieMonster::Delete*Task

Second attempt to commit this, after fixing a flaky test:
http://code.google.com/p/chromium/issues/detail?id=302914

BUG= 252217 

Review URL: https://codereview.chromium.org/18032002
------------------------------------------------------------------------
Comment 23 by phil...@opera.com, Oct 3 2013
Status: Fixed
Sign in to add a comment