Extension activity log database refactoring (step 1)

chrome/browser/extensions/activity_log/activity_log.cc

 // Copyright (c) 2011 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 <set>
 #include <vector>
 #include "base/command_line.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
@@ skipping 114 matching lines @@
 }
 
 ActivityLogFactory::~ActivityLogFactory() {
 }
 
 // ActivityLog
 
 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_);
         break;
       case ActivityLogPolicy::POLICY_NOARGS:
         policy_ = new StreamWithoutArgsUIPolicy(profile_);
         break;
       default:
         NOTREACHED();
     }
@@ skipping 54 matching lines @@
     SetDefaultPolicy(ActivityLogPolicy::POLICY_FULLSTREAM);
   else
     SetDefaultPolicy(ActivityLogPolicy::POLICY_NOARGS);
 }
 
 void ActivityLog::Shutdown() {
   if (tracker_) tracker_->RemoveObserver(this);
 }
 
 ActivityLog::~ActivityLog() {
-  // TODO(felt): Turn policy_ into a scoped_ptr.
-  delete policy_;
+  policy_->Close();
 }
 
 bool ActivityLog::IsLogEnabled() {
   if (!has_threads_ || !initialized_) return false;
   return enabled_;
 }
 
 void ActivityLog::OnExtensionInstalled(const Extension* extension) {
   if (extension->id() != kActivityLogExtensionId) return;
   enabled_ = true;
@@ skipping 282 matching lines @@
                    web_contents->GetTitle(),
                    std::string(),  // no api call here
                    script_names.get(),
                    DomActionType::INSERTED,
                    extra);
     }
   }
 }
 
 }  // namespace extensions