Reland "Moves SQLitePersistentCookieStore to net/extras/sqlite."

Reland "Moves SQLitePersistentCookieStore to net/extras/sqlite."

The application of special storage policy is split out into a new class,
QuotaPolicyCookieStore, in content/browser/net.

This reverts commit ac4f49641d2b1246fdcfdb35993b21200d43460f.

TBR=avi@chromium.org
BUG=467596
TEST=No visible impact.

Committed: https://crrev.com/1860223cfc30f7fedb0e98730518a4cc31ccd04f
Cr-Commit-Position: refs/heads/master@{#330257}

[rohitrao (ping after 24h), 2015-05-15 16:02:16]

rohitrao@chromium.org changed reviewers:
+ rsleevi@chromium.org

[rohitrao (ping after 24h), 2015-05-15 16:02:17]

Threading is hard.  Here's what I think went wrong with my first try:

When QuotaPolicyCookieStore is destroyed, it calls DeleteAllInList(), then it
releases its reference to the underlying SqlitePersistentCookieStore.

DeleteAllInList() calls Backend::DeleteAllInList(), which schedules a background
task to run BackgroundDeleteAllInList(), which is what actually deletes the
cookies.

The SqlitePersistentCookieStore destructor calls Backend::Close().  If Close()
is called on a non-background thread, it posts a task to run
InternalBackgroundClose() on the background thread.  However, if it's already
running on the background thread, it Closes() immediately.

So, if for some reason the store happens to get destroyed on the background
thread, we will:
1) Schedule BackgroundDeleteAllInList() to delete cookies.
2) Run the destructor, which *immediately* calls InternalBackgroundClose() and
closes the db_.
3) When BackgroundDeleteAllInList() gets to run, db_ is already reset, so it
no-ops.

My fix is to copy the logic from Close() into DeleteAllInList() -- if we're
already on the background thread, run the delete immediately.  (The
ChannelIDStore sidesteps this by always posting a task to the background thread
in Close().  I don't know why the cookiedb is different.)

I still have no idea why (or if) the store is racily getting destroyed on the
background thread, but this looks like a real bug and would definitely account
for the failures I was seeing.

[Ryan Sleevi, 2015-05-15 22:18:04]

lgtm

https://codereview.chromium.org/1138643004/diff/20001/content/browser/net/quo...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1138643004/diff/20001/content/browser/net/quo...
content/browser/net/quota_policy_cookie_store_unittest.cc:67:
destroy_event_.Wait();
DANGER BUG: This is an inherently dangerous pattern, but I suppose you're
duplicating the Load/OnLoaded pattern.

If the SequencedWorkerPool were 1 thread instead of 3, for example, this would
deadlock.

The 'safe' way to do this would be to do things like PostTaskAndReply to post
the 'background runner' task (and wait for its completion on the background
thread) and then post a task back to the 'calling' task runner, which would then
run RunLoop::Quit(). Outside (e.g. in the Test Body) is where you'd setup the
RunLoop, do 'the thing', then RunLoop.Run(), which would be equivalent to the
blocking behaviour.

https://codereview.chromium.org/1138643004/diff/20001/content/browser/net/quo...
content/browser/net/quota_policy_cookie_store_unittest.cc:277: // is still
there.
grumble grumble pronouns in comments

// Replace the store, which forces the current store to
// flush data to disk. Then, after reloading the store,
// confirm that the data was flushed by making sure it
// loads successfully.
// This ensures that all pending commits are made to the
// store before allowing it to be closed.

[Ryan Sleevi, 2015-05-15 22:21:06]

FYI filed https://code.google.com/p/chromium/issues/detail?id=488710 for the
anti-pattern

[rohitrao (ping after 24h), 2015-05-15 22:53:26]

rohitrao@chromium.org changed reviewers:
+ avi@chromium.org

[rohitrao (ping after 24h), 2015-05-15 22:53:27]

+TBR=avi because this is substantially the same as the CL he approved yesterday.

Let's see if it sticks this time =)

https://codereview.chromium.org/1138643004/diff/20001/content/browser/net/quo...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1138643004/diff/20001/content/browser/net/quo...
content/browser/net/quota_policy_cookie_store_unittest.cc:67:
destroy_event_.Wait();
On 2015/05/15 22:18:04, Ryan Sleevi wrote:
> DANGER BUG: This is an inherently dangerous pattern, but I suppose you're
> duplicating the Load/OnLoaded pattern.
> 

Acknowledged, will fix in a follow.

For some reason I had the impression that spinning a nested runloop was a bigger
anti-pattern.  I guess calling base::MessageLoop::current()->Run() is bad, but
going through a base::RunLoop is not?

https://codereview.chromium.org/1138643004/diff/20001/content/browser/net/quo...
content/browser/net/quota_policy_cookie_store_unittest.cc:277: // is still
there.
On 2015/05/15 22:18:03, Ryan Sleevi wrote:
> grumble grumble pronouns in comments

Fixed everywhere.

[rohitrao (ping after 24h), 2015-05-15 22:54:14]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2015-05-15 22:54:15]

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/1138643004/#ps40001
(title: "Review.")

[commit-bot: I haz the power, 2015-05-15 22:55:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138643004/40001

[commit-bot: I haz the power, 2015-05-16 01:07:21]

Committed patchset #3 (id:40001)

[Avi (use Gerrit), 2015-05-16 03:34:07]

On 2015/05/15 22:53:27, rohitrao wrote:
> +TBR=avi because this is substantially the same as the CL he approved
yesterday.

Okeydoke. lgtm

[commit-bot: I haz the power, 2015-05-18 11:30:25]

Patchset 3 (id:??) landed as
https://crrev.com/1860223cfc30f7fedb0e98730518a4cc31ccd04f
Cr-Commit-Position: refs/heads/master@{#330257}