Extension activity log database refactoring (step 1)

chrome/browser/extensions/activity_log/activity_log.cc

Index: chrome/browser/extensions/activity_log/activity_log.cc
diff --git a/chrome/browser/extensions/activity_log/activity_log.cc b/chrome/browser/extensions/activity_log/activity_log.cc
index f2b38ba97372581f83387c85c23dc65cb37cadb7..e9219999d2de6f6935b087d6f22b31db34228602 100644
--- a/chrome/browser/extensions/activity_log/activity_log.cc
+++ b/chrome/browser/extensions/activity_log/activity_log.cc
@@ -132,7 +132,8 @@ ActivityLogFactory::~ActivityLogFactory() {
 void ActivityLog::SetDefaultPolicy(ActivityLogPolicy::PolicyType policy_type) {
   // Can't use IsLogEnabled() here because this is called from inside Init.
   if (policy_type != policy_type_ && enabled_) {
-    delete policy_;
+    if (policy_)
+      policy_->Close();

[felt, 2013/07/09 22:14:51]

What if Close() hasn't finished before policy is reassigned to a new policy?
Since ActivityLogPolicy->Close() asynchronously calls ActivityDatabase->Close(),
which then needs to flush things to disk before deleting the policy. 

[mvrable, 2013/07/09 23:03:22]

That occurred to me earlier; I think the current code has no more races than
before but is still suboptimal.

If the two policies use different databases there's no problem, as the old
database contents can finish being flushed even after the new database is
opened.  If they share the same file, we'll run into problems.

We'd either need some way to block until the database contents are all flushed
(there's no notification mechanism currently for this), or we could just
disallow switching policies at runtime--make it an error to set a new default
policy after one has been initialized.  But would enforcing a single policy (per
ActivityLog instance) cause trouble for unit tests?

[felt, 2013/07/10 00:14:36]

At present, I don't expect this code path will be triggered anytime except
during unit tests. Maybe we should make that explicit, by adding a flag or
something that is set by the unit tests and DCHECKs otherwise if policy_ is
already set when SetDefaultPolicy is called. At the very least can you add a
comment specifying that it's dangerous to call SetDefaultPolicy more than once?

However, don't the unit tests currently use the same file but switch policies?
The one mitigation here is that I think we have batching turned off in the unit
tests, so there would be very little delay.

[mvrable, 2013/07/10 17:49:17]

I've changed this to a CHECK to catch cases where we switch policies when we
shouldn't, and it doesn't even seem that the unit tests are doing so.

If we need to support it in the future, perhaps we can consider using different
database files for different policies in the unit tests.

[felt, 2013/07/10 18:35:17]

LogWithoutArguments should be triggering this code. You can see it sets the
policy twice: once at the beginning and again at the end. If it's not triggering
this code then there must be a bug. :/

[mvrable, 2013/07/10 18:53:56]

I somehow missed this since I wasn't running that particular test locally. 
Investigating a bit more what changes to make.

[mvrable, 2013/07/10 20:48:41]

I convinced myself (and Adrienne, in an offline discussion) that this code is
actually safe.  I've added a comment explaining.

     switch (policy_type) {
       case ActivityLogPolicy::POLICY_FULLSTREAM:
         policy_ = new FullStreamUIPolicy(profile_);
@@ -207,8 +208,7 @@ void ActivityLog::Shutdown() {
 }
 
 ActivityLog::~ActivityLog() {
-  // TODO(felt): Turn policy_ into a scoped_ptr.
-  delete policy_;
+  policy_->Close();
 }
 
 bool ActivityLog::IsLogEnabled() {