ServiceWorker: Add UMA for ServiceWorkerDatabase

content/browser/service_worker/service_worker_database.cc

 // Copyright 2014 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/service_worker/service_worker_database.h"
 
 #include <string>
 
 #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"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "content/browser/service_worker/service_worker_database.pb.h"
 #include "third_party/leveldatabase/src/helpers/memenv/memenv.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "third_party/leveldatabase/src/include/leveldb/env.h"
 #include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
@@ skipping 47 matching lines @@
 
 const char kRegKeyPrefix[] = "REG:";
 const char kResKeyPrefix[] = "RES:";
 const char kKeySeparator = '\x00';
 
 const char kUncommittedResIdKeyPrefix[] = "URES:";
 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) {
   if (!StartsWithASCII(str, prefix, true))
     return false;
   if (out)
     *out = str.substr(prefix.size());
   return true;
 }
 
@@ skipping 72 matching lines @@
   // Value should be empty.
   batch->Put(CreateUniqueOriginKey(origin), "");
 }
 
 void PutPurgeableResourceIdToBatch(int64 resource_id,
                                    leveldb::WriteBatch* batch) {
   // Value should be empty.
   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.
   out->registration_id = data.registration_id();
   out->scope = scope_url;
   out->script = script_url;
   out->version_id = data.version_id();
   out->is_active = data.is_active();
   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(
     const leveldb::Status& status) {
   if (status.ok())
     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()
     : registration_id(-1),
       version_id(-1),
       is_active(false),
       has_fetch_handler(false) {
 }
 
 ServiceWorkerDatabase::RegistrationData::~RegistrationData() {
@@ skipping 51 matching lines @@
 ServiceWorkerDatabase::Status
 ServiceWorkerDatabase::GetOriginsWithRegistrations(std::set<GURL>* origins) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(origins->empty());
 
   Status status = LazyOpen(false);
   if (IsNewOrNonexistentDatabase(status))
     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(
     const GURL& origin,
     std::vector<RegistrationData>* registrations) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(registrations->empty());
 
   Status status = LazyOpen(false);
   if (IsNewOrNonexistentDatabase(status))
     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(
       "%s%s%c", kRegKeyPrefix, origin.spec().c_str(), kKeySeparator);
 
   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());
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK ||
+        !RemovePrefix(itr->key().ToString(), prefix, NULL)) {
+      break;
     }
-
-    if (!RemovePrefix(itr->key().ToString(), prefix, NULL))
+    RegistrationData registration;
+    status = ParseRegistrationData(itr->value().ToString(), &registration);
+    if (status != STATUS_OK)
       break;
-
-    RegistrationData registration;
-    if (!ParseRegistrationData(itr->value().ToString(), &registration)) {
-      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-      registrations->clear();
-      return STATUS_ERROR_CORRUPTED;
-    }
     registrations->push_back(registration);
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK) {
+    Disable();
+    registrations->clear();
+  }
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetAllRegistrations(
     std::vector<RegistrationData>* registrations) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(registrations->empty());
 
   Status status = LazyOpen(false);
   if (IsNewOrNonexistentDatabase(status))
     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());
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK ||
+        !RemovePrefix(itr->key().ToString(), kRegKeyPrefix, NULL)) {
+      break;
     }
-
-    if (!RemovePrefix(itr->key().ToString(), kRegKeyPrefix, NULL))
+    RegistrationData registration;
+    status = ParseRegistrationData(itr->value().ToString(), &registration);
+    if (status != STATUS_OK)
       break;
-
-    RegistrationData registration;
-    if (!ParseRegistrationData(itr->value().ToString(), &registration)) {
-      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-      registrations->clear();
-      return STATUS_ERROR_CORRUPTED;
-    }
     registrations->push_back(registration);
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK) {
+    Disable();
+    registrations->clear();
+  }
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadRegistration(
     int64 registration_id,
     const GURL& origin,
     RegistrationData* registration,
     std::vector<ResourceRecord>* resources) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(registration);
   DCHECK(resources);
@@ skipping 273 matching lines @@
   }
 
   leveldb::Options options;
   options.create_if_missing = create_if_missing;
   if (use_in_memory_db) {
     env_.reset(leveldb::NewMemEnv(leveldb::Env::Default()));
     options.env = env_.get();
   }
 
   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);
   if (db_version > 0)
     state_ = INITIALIZED;
   return STATUS_OK;
 }
 
 bool ServiceWorkerDatabase::IsNewOrNonexistentDatabase(
     ServiceWorkerDatabase::Status status) {
   if (status == STATUS_ERROR_NOT_FOUND)
     return true;
   if (status == STATUS_OK && state_ == UNINITIALIZED)
     return true;
   return false;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadNextAvailableId(
     const char* id_key,
     int64* next_avail_id) {
   DCHECK(id_key);
   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);
-  }
+  status = ParseId(value, next_avail_id);
+  if (status != STATUS_OK)
+    Disable();
 
-  int64 parsed;
-  if (!base::StringToInt64(value, &parsed)) {
-    HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-    return STATUS_ERROR_CORRUPTED;
-  }
-
-  *next_avail_id = parsed;
-  return STATUS_OK;
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadRegistrationData(
     int64 registration_id,
     const GURL& origin,
     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());
 
-  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());
-      resources->clear();
-      return LevelDBStatusToStatus(itr->status());
+    status = LevelDBStatusToStatus(itr->status());
+    if (status != STATUS_OK ||
+        !RemovePrefix(itr->key().ToString(), prefix, NULL)) {
+      break;
     }
-
-    if (!RemovePrefix(itr->key().ToString(), prefix, NULL))
+    ResourceRecord resource;
+    status = ParseResourceRecord(itr->value().ToString(), &resource);
+    if (status != STATUS_OK)
       break;
-
-    ResourceRecord resource;
-    if (!ParseResourceRecord(itr->value().ToString(), &resource)) {
-      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-      resources->clear();
-      return STATUS_ERROR_CORRUPTED;
-    }
     resources->push_back(resource);
   }
-  return STATUS_OK;
+
+  if (status != STATUS_OK) {
+    Disable();
+    resources->clear();
+  }
+
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::DeleteResourceRecords(
     int64 version_id,
     std::vector<int64>* newly_purgeable_resources,
     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);
 
     // Currently resource sharing across versions and registrations is not
     // supported, so we can purge this without caring about it.
     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(
     const char* id_key_prefix,
     std::set<int64>* ids) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(id_key_prefix);
   DCHECK(ids->empty());
 
   Status status = LazyOpen(false);
   if (IsNewOrNonexistentDatabase(status))
     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(
     const char* id_key_prefix,
     const std::set<int64>& ids) {
   leveldb::WriteBatch batch;
   Status status = WriteResourceIdsInBatch(id_key_prefix, ids, &batch);
   if (status != STATUS_OK)
     return status;
   return WriteBatch(&batch);
@@ skipping 48 matching lines @@
   for (std::set<int64>::const_iterator itr = ids.begin();
        itr != ids.end(); ++itr) {
     batch->Delete(CreateResourceIdKey(id_key_prefix, *itr));
   }
   return STATUS_OK;
 }
 
 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);
+  } else if (status != STATUS_OK) {
+    Disable();
+    ReportReadResult(FROM_HERE, status);
+    return status;
   }
 
-  int64 parsed;
-  if (!base::StringToInt64(value, &parsed)) {
-    HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-    return STATUS_ERROR_CORRUPTED;
-  }
+  status = ParseDatabaseVersion(value, db_version);
+  if (status != STATUS_OK)
+    Disable();
 
-  const int kFirstValidVersion = 1;
-  if (parsed < kFirstValidVersion || kCurrentSchemaVersion < parsed) {
-    HandleError(FROM_HERE, leveldb::Status::Corruption("invalid DB version"));
-    return STATUS_ERROR_CORRUPTED;
-  }
-
-  *db_version = parsed;
-  return STATUS_OK;
+  ReportReadResult(FROM_HERE, status);
+  return status;
 }
 
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::WriteBatch(
     leveldb::WriteBatch* batch) {
   DCHECK(batch);
   DCHECK_NE(DISABLED, state_);
 
   if (state_ == UNINITIALIZED) {
     // Write the database schema version.
     batch->Put(kDatabaseVersionKey, base::Int64ToString(kCurrentSchemaVersion));
     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(
     int64 used_id, leveldb::WriteBatch* batch) {
   DCHECK(batch);
   if (next_avail_registration_id_ <= used_id) {
     next_avail_registration_id_ = used_id + 1;
     batch->Put(kNextRegIdKey, base::Int64ToString(next_avail_registration_id_));
   }
 }
 
 void ServiceWorkerDatabase::BumpNextVersionIdIfNeeded(
     int64 used_id, leveldb::WriteBatch* batch) {
   DCHECK(batch);
   if (next_avail_version_id_ <= used_id) {
     next_avail_version_id_ = used_id + 1;
     batch->Put(kNextVerIdKey, base::Int64ToString(next_avail_version_id_));
   }
 }
 
 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();
 }
 
 }  // namespace content