Fixes a bug where QuotaPolicyChannelIDStore would ignore some deletes.

chrome/browser/net/quota_policy_channel_id_store_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 142 matching lines @@
       NULL);
 
   // Reload and check if the cert has been removed.
   Load(&channel_ids);
   ASSERT_EQ(0U, channel_ids.size());
 }
 
 // Test if data is stored as expected in the QuotaPolicy database.
 TEST_F(QuotaPolicyChannelIDStoreTest, TestPolicy) {
   store_->AddChannelID(
-      net::DefaultChannelIDStore::ChannelID("foo.com",
+      net::DefaultChannelIDStore::ChannelID("nonpersistent.com",
                                             base::Time::FromInternalValue(3),
                                             base::Time::FromInternalValue(4),
                                             "c",
                                             "d"));
 
   ScopedVector<net::DefaultChannelIDStore::ChannelID> channel_ids;
   // Replace the store effectively destroying the current one and forcing it
   // to write its data to disk. Then we can see if after loading it again it
   // is still there.
   store_ = NULL;
   // Make sure we wait until the destructor has run.
   base::RunLoop().RunUntilIdle();
-  // Specify storage policy that makes "foo.com" session only.
+  // Specify storage policy that makes "nonpersistent.com" session only.
   scoped_refptr<content::MockSpecialStoragePolicy> storage_policy =
       new content::MockSpecialStoragePolicy();
   storage_policy->AddSessionOnly(
-      net::cookie_util::CookieOriginToURL("foo.com", true));
+      net::cookie_util::CookieOriginToURL("nonpersistent.com", true));
   // Reload store, it should still have both channel ids.
   store_ = new QuotaPolicyChannelIDStore(
       temp_dir_.path().Append(kTestChannelIDFilename),
       base::MessageLoopProxy::current(),
       storage_policy);
   Load(&channel_ids);
   ASSERT_EQ(2U, channel_ids.size());
 
-  // Now close the store, and "foo.com" should be deleted according to policy.
+  // Add another two channel ids before closing the store.  Because additions
+  // are delayed and committed to disk in batches, these will not be committed
+  // until the store is destroyed, which is after the policy is applied.  The
+  // pending operation pruning logic should prevent the "nonpersistent.com" ID
+  // from being committed to disk.
+  store_->AddChannelID(net::DefaultChannelIDStore::ChannelID(
+      "nonpersistent.com", base::Time::FromInternalValue(5),
+      base::Time::FromInternalValue(6), "e", "f"));

[Ryan Sleevi, 2015/04/06 18:46:55]

I'm not actually sure that this is valid / consistent with the API contract.

[rohitrao (ping after 24h), 2015/04/06 19:48:24]

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?

+  store_->AddChannelID(net::DefaultChannelIDStore::ChannelID(
+      "persistent.com", base::Time::FromInternalValue(7),
+      base::Time::FromInternalValue(8), "g", "h"));
+
+  // Now close the store, and the session-only IDs should be deleted according
+  // to policy.
   store_ = NULL;
   // Make sure we wait until the destructor has run.
   base::RunLoop().RunUntilIdle();
   channel_ids.clear();
   store_ = new QuotaPolicyChannelIDStore(
       temp_dir_.path().Append(kTestChannelIDFilename),
       base::MessageLoopProxy::current(),
       NULL);
 
-  // Reload and check that the "foo.com" cert has been removed.
+  // Reload and check that the proper IDs have been removed.

[mef, 2015/04/06 20:17:44]

maybe say 'proper (nonpersistent.com)'?

[rohitrao (ping after 24h), 2015/04/07 14:21:37]

Done.

   Load(&channel_ids);
-  ASSERT_EQ(1U, channel_ids.size());
-  ASSERT_EQ("google.com", channel_ids[0]->server_identifier());
+  ASSERT_EQ(2U, channel_ids.size());
+  for (const auto& id : channel_ids) {
+    ASSERT_NE("nonpersistent.com", id->server_identifier());
+  }
 }