Do not CHECK on the result of Sql::Statement::Run.

content/browser/media/webrtc_identity_store_backend.cc

 // Copyright 2013 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 "content/browser/media/webrtc_identity_store_backend.h"
 
 #include "base/file_util.h"
 #include "base/files/file_path.h"
+#include "base/strings/string_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/net_errors.h"
 #include "sql/error_delegate_util.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 #include "url/gurl.h"
 #include "webkit/browser/quota/special_storage_policy.h"
 
 namespace content {
 
 static const char* kWebRTCIdentityStoreDBName = "webrtc_identity_store";
 
 static const base::FilePath::CharType kWebRTCIdentityStoreDirectory[] =
     FILE_PATH_LITERAL("WebRTCIdentityStore");
 
 // Initializes the identity table, returning true on success.
 static bool InitDB(sql::Connection* db) {
   if (db->DoesTableExist(kWebRTCIdentityStoreDBName)) {
     if (db->DoesColumnExist(kWebRTCIdentityStoreDBName, "origin") &&
         db->DoesColumnExist(kWebRTCIdentityStoreDBName, "identity_name") &&
         db->DoesColumnExist(kWebRTCIdentityStoreDBName, "common_name") &&
         db->DoesColumnExist(kWebRTCIdentityStoreDBName, "certificate") &&
         db->DoesColumnExist(kWebRTCIdentityStoreDBName, "private_key") &&
         db->DoesColumnExist(kWebRTCIdentityStoreDBName, "creation_time"))
       return true;
+
     if (!db->Execute("DROP TABLE webrtc_identity_store"))
       return false;
   }
 
   return db->Execute(
       "CREATE TABLE webrtc_identity_store"
       " ("
       "origin TEXT NOT NULL,"
       "identity_name TEXT NOT NULL,"
       "common_name TEXT NOT NULL,"
@@ skipping 292 matching lines @@
 }
 
 WebRTCIdentityStoreBackend::~WebRTCIdentityStoreBackend() {}
 
 void WebRTCIdentityStoreBackend::OnLoaded(scoped_ptr<IdentityMap> out_map) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   if (state_ != LOADING)
     return;
 
-  DVLOG(2) << "WebRTC identity store has loaded.";
+  DVLOG(3) << "WebRTC identity store has loaded.";
 
   state_ = LOADED;
   identities_.swap(*out_map);
 
   for (size_t i = 0; i < pending_find_requests_.size(); ++i) {
     FindIdentity(pending_find_requests_[i]->origin,
                  pending_find_requests_[i]->identity_name,
                  pending_find_requests_[i]->common_name,
                  pending_find_requests_[i]->callback);
     delete pending_find_requests_[i];
   }
   pending_find_requests_.clear();
 }
 
 //
 // Implementation of SqlLiteStorage.
 //
 
 void WebRTCIdentityStoreBackend::SqlLiteStorage::Load(IdentityMap* out_map) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   DCHECK(!db_.get());
 
   // Ensure the parent directory for storing certs is created before reading
   // from it.
   const base::FilePath dir = path_.DirName();
   if (!base::PathExists(dir) && !base::CreateDirectory(dir)) {
-    DLOG(ERROR) << "Unable to open DB file path.";
+    DVLOG(2) << "Unable to open DB file path.";
     return;
   }
 
   db_.reset(new sql::Connection());
 
   db_->set_error_callback(base::Bind(&SqlLiteStorage::OnDatabaseError, this));
 
   if (!db_->Open(path_)) {
-    DLOG(ERROR) << "Unable to open DB.";
+    DVLOG(2) << "Unable to open DB.";
     db_.reset();
     return;
   }
 
   if (!InitDB(db_.get())) {
-    DLOG(ERROR) << "Unable to init DB.";
+    DVLOG(2) << "Unable to init DB.";
     db_.reset();
     return;
   }
 
   db_->Preload();
 
   // Delete expired identities.
   DeleteBetween(base::Time(), base::Time::Now() - validity_period_);
 
   // Slurp all the identities into the out_map.
@@ skipping 64 matching lines @@
       SQL_FROM_HERE,
       "DELETE FROM webrtc_identity_store"
       " WHERE creation_time >= ? AND creation_time <= ?"));
   CHECK(del_stmt.is_valid());
 
   del_stmt.BindInt64(0, delete_begin.ToInternalValue());
   del_stmt.BindInt64(1, delete_end.ToInternalValue());
 
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin()) {
-    DLOG(ERROR) << "Failed to begin the transaction.";
+    DVLOG(2) << "Failed to begin the transaction.";
     return;
   }
 
-  CHECK(del_stmt.Run());
-  transaction.Commit();
+  if (!del_stmt.Run()) {
+    DVLOG(2) << "Failed to run the delete statement.";
+    return;
+  }
+
+  if (!transaction.Commit())
+    DVLOG(2) << "Failed to commit the transaction.";
 }
 
 void WebRTCIdentityStoreBackend::SqlLiteStorage::OnDatabaseError(
     int error,
     sql::Statement* stmt) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
-  if (!sql::IsErrorCatastrophic(error))
-    return;
+
   db_->RazeAndClose();
+  // It's not safe to reset |db_| here.
 }
 
 void WebRTCIdentityStoreBackend::SqlLiteStorage::BatchOperation(
     OperationType type,
     const GURL& origin,
     const std::string& identity_name,
     const Identity& identity) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   // Commit every 30 seconds.
   static const base::TimeDelta kCommitInterval(
@@ skipping 37 matching lines @@
   CHECK(add_stmt.is_valid());
 
   sql::Statement del_stmt(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "DELETE FROM webrtc_identity_store WHERE origin=? AND identity_name=?"));
 
   CHECK(del_stmt.is_valid());
 
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin()) {
-    DLOG(ERROR) << "Failed to begin the transaction.";
+    DVLOG(2) << "Failed to begin the transaction.";
     return;
   }
 
   for (PendingOperationList::iterator it = pending_operations_.begin();
        it != pending_operations_.end();
        ++it) {
     scoped_ptr<PendingOperation> po(*it);
     switch (po->type) {
       case ADD_IDENTITY: {
         add_stmt.Reset(true);
         add_stmt.BindString(0, po->origin.spec());
         add_stmt.BindString(1, po->identity_name);
         add_stmt.BindString(2, po->identity.common_name);
         const std::string& cert = po->identity.certificate;
         add_stmt.BindBlob(3, cert.data(), cert.size());
         const std::string& private_key = po->identity.private_key;
         add_stmt.BindBlob(4, private_key.data(), private_key.size());
         add_stmt.BindInt64(5, po->identity.creation_time);
-        CHECK(add_stmt.Run());
+        if (!add_stmt.Run()) {
+          DVLOG(2) << "Failed to add the identity to DB.";
+          return;

[Ami GONE FROM CHROMIUM, 2014/05/27 19:25:06]

In case of error here or at l.583 or 595 below, the rest of pending_operations_
will not be tried, and p_o_ will not be cleared (it may contain a mix of ptrs
pointing to live & freed memory, too).  Is that ok?
(should the first two return's be continue's?  Should p_o_.clear() be called no
matter what?)

[jiayl, 2014/05/27 20:57:25]

Fixed now. Always clear p_o_.

+        }
         break;
       }
       case DELETE_IDENTITY:
         del_stmt.Reset(true);
         del_stmt.BindString(0, po->origin.spec());
         del_stmt.BindString(1, po->identity_name);
-        CHECK(del_stmt.Run());
+        if (!del_stmt.Run()) {
+          DVLOG(2) << "Failed to delete the identity from DB.";
+          return;
+        }
         break;
 
       default:
         NOTREACHED();
         break;
     }
   }
-  transaction.Commit();
+
+  if (!transaction.Commit()) {
+    DVLOG(2) << "Failed to commit the transaction.";
+    return;
+  }
+
   pending_operations_.clear();
 }
 
 }  // namespace content