ServiceWorker: Add UMA for ServiceWorkerDatabase

content/browser/service_worker/service_worker_database.cc

Index: content/browser/service_worker/service_worker_database.cc
diff --git a/content/browser/service_worker/service_worker_database.cc b/content/browser/service_worker/service_worker_database.cc
index e6bd79f6a2ebd17b8d25d31703425161a73a9f80..7ff6108238bcf1a87abdaa39adea598121547129 100644
--- a/content/browser/service_worker/service_worker_database.cc
+++ b/content/browser/service_worker/service_worker_database.cc
@@ -9,6 +9,7 @@
 #include "base/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
@@ -76,6 +77,14 @@ const char kPurgeableResIdKeyPrefix[] = "PRES:";
 
 const int64 kCurrentSchemaVersion = 1;
 
+// For histogram.
+const char kOpenResultHistogramLabel[] =
+    "ServiceWorker.Database.OpenResult";
+const char kReadResultHistogramLabel[] =
+    "ServiceWorker.Database.ReadResult";
+const char kWriteResultHistogramLabel[] =
+    "ServiceWorker.Database.WriteResult";

[michaeln, 2014/06/03 02:09:09]

I think this set of stats is good.

+
 bool RemovePrefix(const std::string& str,
                   const std::string& prefix,
                   std::string* out) {
@@ -168,19 +177,46 @@ void PutPurgeableResourceIdToBatch(int64 resource_id,
   batch->Put(CreateResourceIdKey(kPurgeableResIdKeyPrefix, resource_id), "");
 }
 
-bool ParseRegistrationData(const std::string& serialized,
-                           ServiceWorkerDatabase::RegistrationData* out) {
+ServiceWorkerDatabase::Status ParseId(
+    const std::string& serialized,
+    int64* out) {
+  DCHECK(out);
+  int64 id;
+  if (!base::StringToInt64(serialized, &id) || id < 0)
+    return ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED;
+  *out = id;
+  return ServiceWorkerDatabase::STATUS_OK;
+}
+
+ServiceWorkerDatabase::Status ParseDatabaseVersion(
+    const std::string& serialized,
+    int64* out) {
+  DCHECK(out);
+  const int kFirstValidVersion = 1;
+  int64 version;
+  if (!base::StringToInt64(serialized, &version) ||
+      version < kFirstValidVersion ||
+      kCurrentSchemaVersion < version) {

[michaeln, 2014/06/03 02:09:09]

The last case may not necessarily mean corruption, say if somebody reverted to
an older version of chrome. I don't think actually happens very often, so i
think this is ok. That and i think it would be work to actually differentiate
between those two cases.

[nhiroki, 2014/06/03 06:59:32]

Ah, good point! Added special logging for that case.

+    return ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED;
+  }
+  *out = version;
+  return ServiceWorkerDatabase::STATUS_OK;
+}
+
+ServiceWorkerDatabase::Status ParseRegistrationData(
+    const std::string& serialized,
+    ServiceWorkerDatabase::RegistrationData* out) {
   DCHECK(out);
   ServiceWorkerRegistrationData data;
   if (!data.ParseFromString(serialized))
-    return false;
+    return ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED;
 
   GURL scope_url(data.scope_url());
   GURL script_url(data.script_url());
   if (!scope_url.is_valid() ||
       !script_url.is_valid() ||
       scope_url.GetOrigin() != script_url.GetOrigin()) {
-    return false;
+    return ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED;
   }
 
   // Convert ServiceWorkerRegistrationData to RegistrationData.
@@ -192,24 +228,25 @@ bool ParseRegistrationData(const std::string& serialized,
   out->has_fetch_handler = data.has_fetch_handler();
   out->last_update_check =
       base::Time::FromInternalValue(data.last_update_check_time());
-  return true;
+  return ServiceWorkerDatabase::STATUS_OK;
 }
 
-bool ParseResourceRecord(const std::string& serialized,
-                         ServiceWorkerDatabase::ResourceRecord* out) {
+ServiceWorkerDatabase::Status ParseResourceRecord(
+    const std::string& serialized,
+    ServiceWorkerDatabase::ResourceRecord* out) {
   DCHECK(out);
   ServiceWorkerResourceRecord record;
   if (!record.ParseFromString(serialized))
-    return false;
+    return ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED;
 
   GURL url(record.url());
   if (!url.is_valid())
-    return false;
+    return ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED;
 
   // Convert ServiceWorkerResourceRecord to ResourceRecord.
   out->resource_id = record.resource_id();
   out->url = url;
-  return true;
+  return ServiceWorkerDatabase::STATUS_OK;
 }
 
 ServiceWorkerDatabase::Status LevelDBStatusToStatus(
@@ -218,12 +255,70 @@ ServiceWorkerDatabase::Status LevelDBStatusToStatus(
     return ServiceWorkerDatabase::STATUS_OK;
   else if (status.IsNotFound())
     return ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND;
+  else if (status.IsIOError())
+    return ServiceWorkerDatabase::STATUS_ERROR_IO_ERROR;
   else if (status.IsCorruption())
     return ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED;
   else
     return ServiceWorkerDatabase::STATUS_ERROR_FAILED;
 }
 
+const char* StatusToString(ServiceWorkerDatabase::Status status) {
+  switch (status) {
+    case ServiceWorkerDatabase::STATUS_OK:
+      return "Database OK";
+    case ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND:
+      return "Database not found";
+    case ServiceWorkerDatabase::STATUS_ERROR_IO_ERROR:
+      return "Database IO error";
+    case ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED:
+      return "Database corrupted";
+    case ServiceWorkerDatabase::STATUS_ERROR_FAILED:
+      return "Database operation failed";
+    case ServiceWorkerDatabase::STATUS_ERROR_MAX:
+      NOTREACHED();
+      return "Database unknown error";
+  }
+  NOTREACHED();
+  return "Database unknown error";
+}
+
+void ReportOpenResult(
+    const tracked_objects::Location& from_here,
+    ServiceWorkerDatabase::Status status) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    DLOG(ERROR) << "Failed at: " << from_here.ToString()
+                << " with error: " << StatusToString(status);
+  }
+  UMA_HISTOGRAM_ENUMERATION(kOpenResultHistogramLabel,
+                            status,
+                            ServiceWorkerDatabase::STATUS_ERROR_MAX);
+}
+
+void ReportReadResult(
+    const tracked_objects::Location& from_here,
+    ServiceWorkerDatabase::Status status) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    DLOG(ERROR) << "Failed at: " << from_here.ToString()
+                << " with error: " << StatusToString(status);
+  }
+  UMA_HISTOGRAM_ENUMERATION(kReadResultHistogramLabel,
+                            status,
+                            ServiceWorkerDatabase::STATUS_ERROR_MAX);
+}
+
+void ReportWriteResult(
+    const tracked_objects::Location& from_here,
+    ServiceWorkerDatabase::Status status) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    DLOG(ERROR) << "Failed at: " << from_here.ToString()
+                << " with error: " << StatusToString(status);
+  }
+  UMA_HISTOGRAM_ENUMERATION(kWriteResultHistogramLabel,
+                            status,
+                            ServiceWorkerDatabase::STATUS_ERROR_MAX);
+}
+
 }  // namespace
 
 ServiceWorkerDatabase::RegistrationData::RegistrationData()
@@ -295,21 +390,26 @@ ServiceWorkerDatabase::GetOriginsWithRegistrations(std::set<GURL>* origins) {
     return STATUS_OK;
   if (status != STATUS_OK)
     return status;
+  DCHECK_EQ(STATUS_OK, status);
 
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   for (itr->Seek(kUniqueOriginKey); itr->Valid(); itr->Next()) {
-    if (!itr->status().ok()) {
-      HandleError(FROM_HERE, itr->status());
-      origins->clear();
-      return LevelDBStatusToStatus(itr->status());
-    }
-
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK)
+      break;
     std::string origin;
     if (!RemovePrefix(itr->key().ToString(), kUniqueOriginKey, &origin))
       break;
     origins->insert(GURL(origin));
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK) {
+    Disable();
+    origins->clear();
+  }
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin(
@@ -323,6 +423,7 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin(
     return STATUS_OK;
   if (status != STATUS_OK)
     return status;
+  DCHECK_EQ(STATUS_OK, status);

[michaeln, 2014/06/03 02:09:09]

given line 424, not sure this dcheck is needed

[nhiroki, 2014/06/03 06:59:32]

Removed them.

 
   // Create a key prefix for registrations.
   std::string prefix = base::StringPrintf(
@@ -330,24 +431,25 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin(
 
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   for (itr->Seek(prefix); itr->Valid(); itr->Next()) {
-    if (!itr->status().ok()) {
-      HandleError(FROM_HERE, itr->status());
-      registrations->clear();
-      return LevelDBStatusToStatus(itr->status());
-    }
-
-    if (!RemovePrefix(itr->key().ToString(), prefix, NULL))
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK ||
+        !RemovePrefix(itr->key().ToString(), prefix, NULL)) {
       break;
-
-    RegistrationData registration;
-    if (!ParseRegistrationData(itr->value().ToString(), &registration)) {
-      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-      registrations->clear();
-      return STATUS_ERROR_CORRUPTED;
     }
+    RegistrationData registration;
+    status = ParseRegistrationData(itr->value().ToString(), &registration);
+    if (status != STATUS_OK)
+      break;
     registrations->push_back(registration);
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK) {
+    Disable();
+    registrations->clear();
+  }
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetAllRegistrations(
@@ -360,27 +462,29 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetAllRegistrations(
     return STATUS_OK;
   if (status != STATUS_OK)
     return status;
+  DCHECK_EQ(STATUS_OK, status);
 
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   for (itr->Seek(kRegKeyPrefix); itr->Valid(); itr->Next()) {
-    if (!itr->status().ok()) {
-      HandleError(FROM_HERE, itr->status());
-      registrations->clear();
-      return LevelDBStatusToStatus(itr->status());
-    }
-
-    if (!RemovePrefix(itr->key().ToString(), kRegKeyPrefix, NULL))
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK ||
+        !RemovePrefix(itr->key().ToString(), kRegKeyPrefix, NULL)) {
       break;
-
-    RegistrationData registration;
-    if (!ParseRegistrationData(itr->value().ToString(), &registration)) {
-      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-      registrations->clear();
-      return STATUS_ERROR_CORRUPTED;
     }
+    RegistrationData registration;
+    status = ParseRegistrationData(itr->value().ToString(), &registration);
+    if (status != STATUS_OK)
+      break;
     registrations->push_back(registration);
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK) {
+    Disable();
+    registrations->clear();
+  }
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadRegistration(
@@ -674,18 +778,19 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::LazyOpen(
   }
 
   leveldb::DB* db = NULL;
-  leveldb::Status db_status =
-      leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db);
-  if (!db_status.ok()) {
+  Status status = LevelDBStatusToStatus(
+      leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db));
+  ReportOpenResult(FROM_HERE, status);
+  if (status != STATUS_OK) {
     DCHECK(!db);
     // TODO(nhiroki): Should we retry to open the database?
-    HandleError(FROM_HERE, db_status);
-    return LevelDBStatusToStatus(db_status);
+    Disable();
+    return status;
   }
   db_.reset(db);
 
   int64 db_version;
-  Status status = ReadDatabaseVersion(&db_version);
+  status = ReadDatabaseVersion(&db_version);
   if (status != STATUS_OK)
     return status;
   DCHECK_LE(0, db_version);
@@ -710,26 +815,25 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadNextAvailableId(
   DCHECK(next_avail_id);
 
   std::string value;
-  leveldb::Status status = db_->Get(leveldb::ReadOptions(), id_key, &value);
-  if (status.IsNotFound()) {
+  Status status = LevelDBStatusToStatus(
+      db_->Get(leveldb::ReadOptions(), id_key, &value));
+  if (status == STATUS_ERROR_NOT_FOUND) {
     // Nobody has gotten the next resource id for |id_key|.
     *next_avail_id = 0;
+    ReportReadResult(FROM_HERE, STATUS_OK);
     return STATUS_OK;
+  } else if (status != STATUS_OK) {
+    Disable();
+    ReportReadResult(FROM_HERE, status);
+    return status;
   }
 
-  if (!status.ok()) {
-    HandleError(FROM_HERE, status);
-    return LevelDBStatusToStatus(status);
-  }
-
-  int64 parsed;
-  if (!base::StringToInt64(value, &parsed)) {
-    HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-    return STATUS_ERROR_CORRUPTED;
-  }
+  status = ParseId(value, next_avail_id);
+  if (status != STATUS_OK)
+    Disable();
 
-  *next_avail_id = parsed;
-  return STATUS_OK;
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadRegistrationData(
@@ -738,52 +842,56 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadRegistrationData(
     RegistrationData* registration) {
   DCHECK(registration);
 
-  std::string key = CreateRegistrationKey(registration_id, origin);
-
+  const std::string key = CreateRegistrationKey(registration_id, origin);
   std::string value;
-  leveldb::Status status = db_->Get(leveldb::ReadOptions(), key, &value);
-  if (!status.ok()) {
-    if (!status.IsNotFound())
-      HandleError(FROM_HERE, status);
-    return LevelDBStatusToStatus(status);
+  Status status = LevelDBStatusToStatus(
+      db_->Get(leveldb::ReadOptions(), key, &value));
+  if (status == STATUS_ERROR_NOT_FOUND) {
+    ReportReadResult(FROM_HERE, STATUS_OK);
+    return status;
+  } else if (status != STATUS_OK) {
+    Disable();
+    ReportReadResult(FROM_HERE, status);
+    return status;
   }
 
-  RegistrationData parsed;
-  if (!ParseRegistrationData(value, &parsed)) {
-    HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-    return STATUS_ERROR_CORRUPTED;
-  }
+  status = ParseRegistrationData(value, registration);
+  if (status != STATUS_OK)
+    Disable();
 
-  *registration = parsed;
-  return STATUS_OK;
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadResourceRecords(
     int64 version_id,
     std::vector<ResourceRecord>* resources) {
-  DCHECK(resources);
+  DCHECK(resources->empty());
+
+  Status status = STATUS_OK;
+  const std::string prefix = CreateResourceRecordKeyPrefix(version_id);
 
-  std::string prefix = CreateResourceRecordKeyPrefix(version_id);
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   for (itr->Seek(prefix); itr->Valid(); itr->Next()) {
-    if (!itr->status().ok()) {
-      HandleError(FROM_HERE, itr->status());
-      resources->clear();
-      return LevelDBStatusToStatus(itr->status());
-    }
-
-    if (!RemovePrefix(itr->key().ToString(), prefix, NULL))
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK ||
+        !RemovePrefix(itr->key().ToString(), prefix, NULL)) {
       break;
-
-    ResourceRecord resource;
-    if (!ParseResourceRecord(itr->value().ToString(), &resource)) {
-      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-      resources->clear();
-      return STATUS_ERROR_CORRUPTED;
     }
+    ResourceRecord resource;
+    status = ParseResourceRecord(itr->value().ToString(), &resource);
+    if (status != STATUS_OK)
+      break;
     resources->push_back(resource);
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK) {
+    Disable();
+    resources->clear();
+  }
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::DeleteResourceRecords(
@@ -792,24 +900,24 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::DeleteResourceRecords(
     leveldb::WriteBatch* batch) {
   DCHECK(batch);
 
-  std::string prefix = CreateResourceRecordKeyPrefix(version_id);
+  Status status = STATUS_OK;
+  const std::string prefix = CreateResourceRecordKeyPrefix(version_id);
+
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   for (itr->Seek(prefix); itr->Valid(); itr->Next()) {
-    if (!itr->status().ok()) {
-      HandleError(FROM_HERE, itr->status());
-      return LevelDBStatusToStatus(itr->status());
-    }
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK)
+      break;
 
-    std::string key = itr->key().ToString();
+    const std::string key = itr->key().ToString();
     std::string unprefixed;
     if (!RemovePrefix(key, prefix, &unprefixed))
       break;
 
     int64 resource_id;
-    if (!base::StringToInt64(unprefixed, &resource_id)) {
-      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-      return STATUS_ERROR_CORRUPTED;
-    }
+    status = ParseId(unprefixed, &resource_id);
+    if (status != STATUS_OK)
+      break;
 
     // Remove a resource record.
     batch->Delete(key);
@@ -819,7 +927,12 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::DeleteResourceRecords(
     PutPurgeableResourceIdToBatch(resource_id, batch);
     newly_purgeable_resources->push_back(resource_id);
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK)
+    Disable();
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadResourceIds(
@@ -834,28 +947,32 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadResourceIds(
     return STATUS_OK;
   if (status != STATUS_OK)
     return status;
+  DCHECK_EQ(STATUS_OK, status);
 
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   for (itr->Seek(id_key_prefix); itr->Valid(); itr->Next()) {
-    if (!itr->status().ok()) {
-      HandleError(FROM_HERE, itr->status());
-      ids->clear();
-      return LevelDBStatusToStatus(itr->status());
-    }
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK)
+      break;
 
     std::string unprefixed;
     if (!RemovePrefix(itr->key().ToString(), id_key_prefix, &unprefixed))
       break;
 
     int64 resource_id;
-    if (!base::StringToInt64(unprefixed, &resource_id)) {
-      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-      ids->clear();
-      return STATUS_ERROR_CORRUPTED;
-    }
+    status = ParseId(unprefixed, &resource_id);
+    if (status != STATUS_OK)
+      break;
     ids->insert(resource_id);
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK) {
+    Disable();
+    ids->clear();
+  }
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::WriteResourceIds(
@@ -924,32 +1041,25 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::DeleteResourceIdsInBatch(
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadDatabaseVersion(
     int64* db_version) {
   std::string value;
-  leveldb::Status status =
-      db_->Get(leveldb::ReadOptions(), kDatabaseVersionKey, &value);
-  if (status.IsNotFound()) {
+  Status status = LevelDBStatusToStatus(
+      db_->Get(leveldb::ReadOptions(), kDatabaseVersionKey, &value));
+  if (status == STATUS_ERROR_NOT_FOUND) {
     // The database hasn't been initialized yet.
     *db_version = 0;
+    ReportReadResult(FROM_HERE, STATUS_OK);
     return STATUS_OK;
-  }
-  if (!status.ok()) {
-    HandleError(FROM_HERE, status);
-    return LevelDBStatusToStatus(status);
-  }
-
-  int64 parsed;
-  if (!base::StringToInt64(value, &parsed)) {
-    HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-    return STATUS_ERROR_CORRUPTED;
+  } else if (status != STATUS_OK) {
+    Disable();
+    ReportReadResult(FROM_HERE, status);
+    return status;
   }
 
-  const int kFirstValidVersion = 1;
-  if (parsed < kFirstValidVersion || kCurrentSchemaVersion < parsed) {
-    HandleError(FROM_HERE, leveldb::Status::Corruption("invalid DB version"));
-    return STATUS_ERROR_CORRUPTED;
-  }
+  status = ParseDatabaseVersion(value, db_version);
+  if (status != STATUS_OK)
+    Disable();
 
-  *db_version = parsed;
-  return STATUS_OK;
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::WriteBatch(
@@ -963,10 +1073,13 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::WriteBatch(
     state_ = INITIALIZED;
   }
 
-  leveldb::Status status = db_->Write(leveldb::WriteOptions(), batch);
-  if (!status.ok())
-    HandleError(FROM_HERE, status);
-  return LevelDBStatusToStatus(status);
+  Status status = LevelDBStatusToStatus(
+      db_->Write(leveldb::WriteOptions(), batch));
+  if (status != STATUS_OK)
+    Disable();
+
+  ReportWriteResult(FROM_HERE, status);
+  return status;
 }
 
 void ServiceWorkerDatabase::BumpNextRegistrationIdIfNeeded(
@@ -991,12 +1104,8 @@ bool ServiceWorkerDatabase::IsOpen() {
   return db_ != NULL;
 }
 
-void ServiceWorkerDatabase::HandleError(

[michaeln, 2014/06/03 02:09:09]

The new code looks correct. It maybe could have been accomplished with fewer
code changes approaching it from the point of view of refactoring this
HandleError instance method into HandleReadResult() HandleWriteResult() and
HandleOpenResult() and providing the OK results for logging to those methods as
well the errors. We generally prefer early return where possible instead and
instance methods like that which could do the disabling internally could have
helped with that. As coded more care has to be taken to call Disable()
throughout and that might be easy to miss.

[nhiroki, 2014/06/03 06:59:32]

SGTM! I'd prefer to work with Handle{...}Result() functions. Revamped.
Agree. I think the latest patchset would be more secure.

-    const tracked_objects::Location& from_here,
-    const leveldb::Status& status) {
-  // TODO(nhiroki): Add an UMA histogram.
-  DLOG(ERROR) << "Failed at: " << from_here.ToString()
-              << " with error: " << status.ToString();
+void ServiceWorkerDatabase::Disable() {
+  DLOG(ERROR) << "ServiceWorkerDatabase is disabled.";
   state_ = DISABLED;
   db_.reset();
 }