Better ActivityLog error handling

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/string_util.h"
 #include "base/threading/thread_checker.h"
 #include "chrome/browser/extensions/activity_log/activity_log.h"
 #include "chrome/browser/extensions/activity_log/api_actions.h"
 #include "chrome/browser/extensions/activity_log/blocked_actions.h"
 #include "chrome/browser/extensions/api/activity_log_private/activity_log_private_api.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/web_contents.h"
 #include "googleurl/src/gurl.h"
-#include "sql/error_delegate_util.h"
 #include "third_party/re2/re2/re2.h"
 
 namespace {
 
 // Concatenate arguments.
 std::string MakeArgList(const ListValue* args) {
   std::string call_signature;
   ListValue::const_iterator it = args->begin();
   for (; it != args->end(); ++it) {
     std::string arg;
@@ skipping 89 matching lines @@
 
   // If the database cannot be initialized for some reason, we keep
   // chugging along but nothing will get recorded. If the UI is
   // available, things will still get sent to the UI even if nothing
   // is being written to the database.
   db_ = new ActivityDatabase();
   if (!IsLogEnabled()) return;
   base::FilePath base_dir = profile->GetPath();
   base::FilePath database_name = base_dir.Append(
       chrome::kExtensionActivityLogFilename);
-  db_->SetErrorCallback(base::Bind(&ActivityLog::DatabaseErrorCallback,
-                                   base::Unretained(this)));
   ScheduleAndForget(&ActivityDatabase::Init, database_name);
 }
 
 ActivityLog::~ActivityLog() {
-  ScheduleAndForget(&ActivityDatabase::Close);
+  if (dispatch_thread_ == BrowserThread::UI)  // Cleanup fast in a unittest.

[Matt Perry, 2013/06/07 20:58:01]

I think it would be better for the unit tests to flush the message loop (using
RunLoop().RunUntilIdle()) than to put special cases in the production code.

[Matt Perry, 2013/06/07 20:58:01]

OK, I see you're already doing RunUntilIdle. Are you adding this code
speculatively? I'd be surprised if RunUntilIdle is not enough.

[felt, 2013/06/08 00:01:48]

the problem is in other people's unit tests. our constructor and destructor get
called in every test where someone has a Profile. sometimes other tests lack the
right threads, or even worse, threads get prematurely/unexpectedly made and
destroyed. it's really frustrating to have to accommodate these cases.

On 2013/06/07 20:58:01, Matt Perry wrote:

[Matt Perry, 2013/06/08 00:34:17]

Ah, that is unfortunate. I don't have a general solution for that.

On 2013/06/08 00:01:48, felt wrote:

+    db_->Close();
+  else
+    ScheduleAndForget(&ActivityDatabase::Close);
 }
 
 void ActivityLog::SetArgumentLoggingForTesting(bool log_arguments) {
   testing_mode_ = log_arguments;
 }
 
 // static
 ActivityLog* ActivityLog::GetInstance(Profile* profile) {
   return ActivityLogFactory::GetForProfile(profile);
 }
@@ skipping 201 matching lines @@
                    on_url,
                    web_contents->GetTitle(),
                    std::string(),  // no api call here
                    script_names.get(),
                    DomActionType::INSERTED,
                    std::string());  // no extras either
     }
   }
 }
 
-void ActivityLog::DatabaseErrorCallback(int error, sql::Statement* stmt) {
-  if (sql::IsErrorCatastrophic(error))
-    ScheduleAndForget(&ActivityDatabase::KillDatabase);
-}
-
 }  // namespace extensions