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 "content/public/browser/browser_thread.h"
 #include "net/base/net_errors.h"
@@ skipping 325 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))

[Ami GONE FROM CHROMIUM, 2014/05/23 23:05:37]

CL description implies that all the things that previously were CHECK failures
would now be handled by OnDatabaseError but this has the potential to falsify
that.  Do you have good reason to believe this is safe?

Should you track an "end_the_world_" bool that any detected failure should cause
to make this function treat as catastrophic?

[jiayl, 2014/05/23 23:25:40]

Changed to always RazeAndClose for "DELETE" failures. Transient "INSERT"
failures are safe to ignore.

     return;
+
+  DVLOG(2) << "Database error " << error << " for statement "
+           << stmt->GetSQLStatement();
   db_->RazeAndClose();
+  db_.reset();
 }
 
 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.";

[Ami GONE FROM CHROMIUM, 2014/05/23 23:05:37]

return?

[jiayl, 2014/05/23 23:25:40]

Done.

         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.";

[Ami GONE FROM CHROMIUM, 2014/05/23 23:05:37]

ditto

[jiayl, 2014/05/23 23:25:40]

Done.

         break;
 
       default:
         NOTREACHED();
         break;
     }
   }
-  transaction.Commit();
+
+  if (!transaction.Commit())
+    DVLOG(2) << "Failed to commit the transaction.";
+
   pending_operations_.clear();
 }
 
 }  // namespace content