Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1015233002:
Fixes a bug where QuotaPolicyChannelIDStore would ignore some deletes. (Closed)
|
Created:
5 years, 9 months ago by rohitrao (ping after 24h) Modified:
5 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixes a bug where QuotaPolicyChannelIDStore would ignore some deletes.
In cases where a SpecialStoragePolicy is set, the channel id store could still
incorrectly persist some newly-added IDs to disk if their writes did not commit
before the store was destroyed. This CL fixes the bug by forcing a commit of
all pending writes before issuing any policy-mandated deletes.
BUG=
TEST=
Committed: https://crrev.com/6fb4e15bef960a41144cb5abf366c1618afed07f
Cr-Commit-Position: refs/heads/master@{#326780}
Patch Set 1 #Patch Set 2 : With fix. #
Total comments: 1
Patch Set 3 : #
Total comments: 5
Patch Set 4 : Review. #Patch Set 5 : DCHECK. #
Total comments: 7
Patch Set 6 : Review. #
Total comments: 8
Patch Set 7 : Review. #
Total comments: 1
Patch Set 8 : Rebased. #
Messages
Total messages: 19 (4 generated)
rohitrao@chromium.org changed reviewers: + mef@chromium.org, rsleevi@chromium.org
Wanted to see what you thought of this bug/fix. I am working on refactoring SQLitePersistentCookieStore to follow the same pattern as the channel id store, so that we can split the non-storagepolicy parts out into net/extras and use them from iOS. A few browser_tests were failing after my cookies change, and I tracked it down to the fact that this sequence of operations is broken: 1) Create a store and set a policy with some session-only domains. 2) Load objects from the store. 3) Add a new object to the store that is covered by the blacklist from #1. 4) Immediately destroy the store. Internally, the store is batching operations and committing them once every ~30 seconds. If #4 happens immediately after #3, DeleteAllInList() actually runs *before* the add from #3 is committed. One way to fix this is to force a Commit() on the background thread at the beginning of BackgroundDeleteAllInList(). I don't know if this will noticeably slow destruction of the store, or if there are better alternatives. I've modified a test to expose the bug, so tryjobs should fail on this first patchset.
https://codereview.chromium.org/1015233002/diff/20001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/1015233002/diff/20001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:571: Commit(); It seems wasteful to Commit-and-Delete. Would you be open to fixing it in a way that's performance friendly? That is, prune all PendingOp Add/Deletes for the origin in question, without actually committing them to the sqlite store?
Mostly LG, just a test nit https://codereview.chromium.org/1015233002/diff/40001/chrome/browser/net/quot... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/40001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:182: net::cookie_util::CookieOriginToURL("nonpersistent.com", true)); How is this distinct from foo.com, which is nonpersistent? https://codereview.chromium.org/1015233002/diff/40001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:196: base::Time::FromInternalValue(8), "g", "h")); Not clear what you're trying to do here. Could you add a comment explaining why?
https://codereview.chromium.org/1015233002/diff/40001/chrome/browser/net/quot... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/40001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:182: net::cookie_util::CookieOriginToURL("nonpersistent.com", true)); On 2015/04/06 18:09:57, Ryan Sleevi wrote: > How is this distinct from http://foo.com, which is nonpersistent? I wasn't sure if I could add two IDs with the same origin, but that seems to work ok. Renamed foo.com to nonpersistent.com. https://codereview.chromium.org/1015233002/diff/40001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:196: base::Time::FromInternalValue(8), "g", "h")); On 2015/04/06 18:09:57, Ryan Sleevi wrote: > Not clear what you're trying to do here. Could you add a comment explaining why? Added a comment to this block. The goal was to do a couple more Add()s that don't get flushed to disk, as a way to test the pruning logic. https://codereview.chromium.org/1015233002/diff/80001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/1015233002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:487: base::AutoLock locked(lock_); I did not see a way to avoid locking around this whole method. The swap trick from Commit() doesn't work, because another thread could add new entries while I'm pruning. (Although if anyone does actually call Add() at this point, all bets are off, because that call wouldn't get pruned in any case. Not sure how to deal with that. In this particular case, since we're calling DeleteAllInList() from the destructor, it's unlikely that this will happen in real life.)
https://codereview.chromium.org/1015233002/diff/40001/chrome/browser/net/quot... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/40001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:182: net::cookie_util::CookieOriginToURL("nonpersistent.com", true)); On 2015/04/06 18:42:22, rohitrao wrote: > I wasn't sure if I could add two IDs with the same origin, but that seems to > work ok. Renamed http://foo.com to http://nonpersistent.com. I guess that wasn't quite what I meant, and still leaves me confused ;) https://codereview.chromium.org/1015233002/diff/80001/chrome/browser/net/quot... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/80001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:196: base::Time::FromInternalValue(6), "e", "f")); I'm not actually sure that this is valid / consistent with the API contract.
https://codereview.chromium.org/1015233002/diff/80001/chrome/browser/net/quot... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/80001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:196: base::Time::FromInternalValue(6), "e", "f")); On 2015/04/06 18:46:55, Ryan Sleevi wrote: > I'm not actually sure that this is valid / consistent with the API contract. Ah, ok. I don't actually know what channel ids are, so I just based this on the (seemingly) arbitrary values chosen in other tests, including the existing tests in this file as well as the tests in net/ssl/default_channel_id_store_unittest.cc. None of the existing tests appear to use real key, cert, or timestamp data. For the purposes of this test, I only care about the origin, and I'm passing arbitrary unique values for all the other data. Would it help if I made constants like kArbitraryPrivateKey?
https://codereview.chromium.org/1015233002/diff/80001/chrome/browser/net/quot... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/80001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:212: // Reload and check that the proper IDs have been removed. maybe say 'proper (nonpersistent.com)'? https://codereview.chromium.org/1015233002/diff/80001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/1015233002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:596: // Force a commit of any pending writes before issuing deletes. I think this comment is stale.
I've left a couple questions in comments in Patchset #5. https://codereview.chromium.org/1015233002/diff/80001/chrome/browser/net/quot... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/80001/chrome/browser/net/quot... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:212: // Reload and check that the proper IDs have been removed. On 2015/04/06 20:17:44, mef wrote: > maybe say 'proper (nonpersistent.com)'? Done. https://codereview.chromium.org/1015233002/diff/80001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/1015233002/diff/80001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:596: // Force a commit of any pending writes before issuing deletes. On 2015/04/06 20:17:44, mef wrote: > I think this comment is stale. Done.
rsleevi@chromium.org changed reviewers: + nharper@chromium.org
Adding Nick, since this may potentially impact https://codereview.chromium.org/1076063002/ I think this change LGTM, but Nick's probably going to have to update your tests (I think) when he rebases his CL. https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:189: // Add another two channel ids before closing the store. Because additions s/channel ids/channel IDs/ https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:189: // Add another two channel ids before closing the store. Because additions Single spaces following periods (beyond being the preferred style of the appropriate English style arbiters in journalism, it's also the dominant form of the file) https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:201: // Now close the store, and the nonpersistent.com certs should be deleted s/certs/channel IDs/ https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:212: // Reload and check that the nonpersistent.com certs have been removed. s/certs/channel IDs/
https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... File chrome/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:189: // Add another two channel ids before closing the store. Because additions On 2015/04/21 15:16:17, Ryan Sleevi wrote: > s/channel ids/channel IDs/ Fixed elsewhere in the file too. https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:189: // Add another two channel ids before closing the store. Because additions On 2015/04/21 15:16:17, Ryan Sleevi wrote: > Single spaces following periods (beyond being the preferred style of the > appropriate English style arbiters in journalism, it's also the dominant form of > the file) Sigh =) https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:201: // Now close the store, and the nonpersistent.com certs should be deleted On 2015/04/21 15:16:17, Ryan Sleevi wrote: > s/certs/channel IDs/ Changed everywhere in the file. https://codereview.chromium.org/1015233002/diff/100001/chrome/browser/net/quo... chrome/browser/net/quota_policy_channel_id_store_unittest.cc:212: // Reload and check that the nonpersistent.com certs have been removed. On 2015/04/21 15:16:17, Ryan Sleevi wrote: > s/certs/channel IDs/ Done.
@rsleevi Your LGTM seemed a bit hedged. Are you OK with my submitting this CL?
Oh, no, it was fine by me, just a heads up to Nick. To be unambiguous, LGTM :) https://codereview.chromium.org/1015233002/diff/120001/net/extras/sqlite/sqli... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/1015233002/diff/120001/net/extras/sqlite/sqli... net/extras/sqlite/sqlite_channel_id_store.cc:489: for (PendingOperationsList::iterator it = pending_.begin(); If you want, you could use auto here too; auto will bind to the non-const form first.
The CQ bit was checked by rohitrao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1015233002/#ps140001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015233002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6fb4e15bef960a41144cb5abf366c1618afed07f Cr-Commit-Position: refs/heads/master@{#326780} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
