Misc. cleanup, primarily removing unused locals.

net/disk_cache/simple/simple_synchronous_entry.cc

 // Copyright (c) 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 "net/disk_cache/simple/simple_synchronous_entry.h"
 
 #include <algorithm>
 #include <cstring>
 #include <functional>
 #include <limits>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_util.h"
 #include "base/hash.h"
 #include "base/location.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/sha1.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/simple/simple_backend_version.h"
 #include "net/disk_cache/simple/simple_histogram_macros.h"
 #include "net/disk_cache/simple/simple_util.h"
 #include "third_party/zlib/zlib.h"
 
 using base::File;
@@ skipping 39 matching lines @@
 
 // Used in histograms, please only add entries at the end.
 enum CloseResult {
   CLOSE_RESULT_SUCCESS,
   CLOSE_RESULT_WRITE_FAILURE,
 };
 
 void RecordSyncOpenResult(net::CacheType cache_type,
                           OpenEntryResult result,
                           bool had_index) {
-  DCHECK_GT(OPEN_ENTRY_MAX, result);
+  DCHECK_LT(result, OPEN_ENTRY_MAX);
   SIMPLE_CACHE_UMA(ENUMERATION,
                    "SyncOpenResult", cache_type, result, OPEN_ENTRY_MAX);
   if (had_index) {
     SIMPLE_CACHE_UMA(ENUMERATION,
                      "SyncOpenResult_WithIndex", cache_type,
                      result, OPEN_ENTRY_MAX);
   } else {
     SIMPLE_CACHE_UMA(ENUMERATION,
                      "SyncOpenResult_WithoutIndex", cache_type,
                      result, OPEN_ENTRY_MAX);
@@ skipping 10 matching lines @@
                    "SyncCheckEOFResult", cache_type,
                    result, CHECK_EOF_RESULT_MAX);
 }
 
 void RecordCloseResult(net::CacheType cache_type, CloseResult result) {
   SIMPLE_CACHE_UMA(ENUMERATION,
                    "SyncCloseResult", cache_type, result, WRITE_RESULT_MAX);
 }
 
 bool CanOmitEmptyFile(int file_index) {
-  DCHECK_LE(0, file_index);
-  DCHECK_GT(disk_cache::kSimpleEntryFileCount, file_index);
+  DCHECK_GE(file_index, 0);

[rvargas (doing something else), 2014/10/08 20:01:14]

Thanks for fixing all of these.

+  DCHECK_LT(file_index, disk_cache::kSimpleEntryFileCount);
   return file_index == disk_cache::simple_util::GetFileIndexFromStreamIndex(2);
 }
 
 }  // namespace
 
 namespace disk_cache {
 
 using simple_util::GetEntryHashKey;
 using simple_util::GetFilenameFromEntryHashAndFileIndex;
 using simple_util::GetSparseFilenameFromEntryHash;
@@ skipping 158 matching lines @@
                                       uint32* out_crc32,
                                       SimpleEntryStat* entry_stat,
                                       int* out_result) const {
   DCHECK(initialized_);
   DCHECK_NE(0, in_entry_op.index);
   const int64 file_offset =
       entry_stat->GetOffsetInFile(key_, in_entry_op.offset, in_entry_op.index);
   int file_index = GetFileIndexFromStreamIndex(in_entry_op.index);
   // Zero-length reads and reads to the empty streams of omitted files should
   // be handled in the SimpleEntryImpl.
-  DCHECK_LT(0, in_entry_op.buf_len);
+  DCHECK_GT(in_entry_op.buf_len, 0);
   DCHECK(!empty_file_omitted_[file_index]);
   File* file = const_cast<File*>(&files_[file_index]);
   int bytes_read =
       file->Read(file_offset, out_buf->data(), in_entry_op.buf_len);
   if (bytes_read > 0) {
     entry_stat->set_last_used(Time::Now());
     *out_crc32 = crc32(crc32(0L, Z_NULL, 0),
                        reinterpret_cast<const Bytef*>(out_buf->data()),
                        bytes_read);
   }
@@ skipping 103 matching lines @@
 
   // Find the first sparse range at or after the requested offset.
   SparseRangeIterator it = sparse_ranges_.lower_bound(offset);
 
   if (it != sparse_ranges_.begin()) {
     // Hop back one range and read the one overlapping with the start.
     --it;
     SparseRange* found_range = &it->second;
     DCHECK_EQ(it->first, found_range->offset);
     if (found_range->offset + found_range->length > offset) {
-      DCHECK_LE(0, found_range->length);
-      DCHECK_GE(kint32max, found_range->length);
-      DCHECK_LE(0, offset - found_range->offset);
-      DCHECK_GE(kint32max, offset - found_range->offset);
-      int range_len_after_offset = found_range->length -
-                                   (offset - found_range->offset);
-      DCHECK_LE(0, range_len_after_offset);
+      DCHECK_GE(found_range->length, 0);
+      DCHECK_LE(found_range->length, kint32max);
+      DCHECK_GE(offset - found_range->offset, 0);
+      DCHECK_LE(offset - found_range->offset, kint32max);
+      int net_offset = static_cast<int>(offset - found_range->offset);
+      int range_len_after_offset =
+          static_cast<int>(found_range->length - net_offset);
+      DCHECK_GE(range_len_after_offset, 0);
 
       int len_to_read = std::min(buf_len, range_len_after_offset);
-      if (!ReadSparseRange(found_range,
-                           offset - found_range->offset,
-                           len_to_read,
-                           buf)) {
+      if (!ReadSparseRange(found_range, net_offset, len_to_read, buf)) {
         *out_result = net::ERR_CACHE_READ_FAILURE;
         return;
       }
       read_so_far += len_to_read;
     }
     ++it;
   }
 
   // Keep reading until the buffer is full or there is not another contiguous
   // range.
   while (read_so_far < buf_len &&
          it != sparse_ranges_.end() &&
          it->second.offset == offset + read_so_far) {
     SparseRange* found_range = &it->second;
     DCHECK_EQ(it->first, found_range->offset);
-    int range_len = (found_range->length > kint32max) ?
-                    kint32max : found_range->length;
+    int range_len = base::saturated_cast<int>(found_range->length);

[pasko, 2015/12/09 18:41:06]

Is this actually right?

I am not very familiar with the sparse cache code, but it seems wrong to CHECK
the browser process on reading inconsistent data from disk without ability to
recover. Some bug in former versions of Chrome or a bit flip on disk can make us
a query of death.

hopefully range lengths are corrected earlier, +ttuttle, +gavinp on this

[Peter Kasting, 2015/12/09 22:07:23]

saturated_cast is not checked_cast.  This change does not change the function of
the code.

     int len_to_read = std::min(buf_len - read_so_far, range_len);
     if (!ReadSparseRange(found_range, 0, len_to_read, buf + read_so_far)) {
       *out_result = net::ERR_CACHE_READ_FAILURE;
       return;
     }
     read_so_far += len_to_read;
     ++it;
   }
 
   *out_result = read_so_far;
@@ skipping 26 matching lines @@
              << buf_len << " > " << max_sparse_data_size << ")";
     TruncateSparseFile();
   }
 
   SparseRangeIterator it = sparse_ranges_.lower_bound(offset);
 
   if (it != sparse_ranges_.begin()) {
     --it;
     SparseRange* found_range = &it->second;
     if (found_range->offset + found_range->length > offset) {
-      DCHECK_LE(0, found_range->length);
-      DCHECK_GE(kint32max, found_range->length);
-      DCHECK_LE(0, offset - found_range->offset);
-      DCHECK_GE(kint32max, offset - found_range->offset);
-      int range_len_after_offset = found_range->length -
-                                   (offset - found_range->offset);
-      DCHECK_LE(0, range_len_after_offset);
+      DCHECK_GE(found_range->length, 0);
+      DCHECK_LE(found_range->length, kint32max);
+      DCHECK_GE(offset - found_range->offset, 0);
+      DCHECK_LE(offset - found_range->offset, kint32max);
+      int net_offset = static_cast<int>(offset - found_range->offset);
+      int range_len_after_offset =
+          static_cast<int>(found_range->length - net_offset);
+      DCHECK_GE(range_len_after_offset, 0);
 
       int len_to_write = std::min(buf_len, range_len_after_offset);
-      if (!WriteSparseRange(found_range,
-                            offset - found_range->offset,
-                            len_to_write,
-                            buf)) {
+      if (!WriteSparseRange(found_range, net_offset, len_to_write, buf)) {
         *out_result = net::ERR_CACHE_WRITE_FAILURE;
         return;
       }
       written_so_far += len_to_write;
     }
     ++it;
   }
 
   while (written_so_far < buf_len &&
          it != sparse_ranges_.end() &&
          it->second.offset < offset + buf_len) {
     SparseRange* found_range = &it->second;
     if (offset + written_so_far < found_range->offset) {
-      int len_to_append = found_range->offset - (offset + written_so_far);
+      int len_to_append =
+          static_cast<int>(found_range->offset - (offset + written_so_far));
       if (!AppendSparseRange(offset + written_so_far,
                              len_to_append,
                              buf + written_so_far)) {
         *out_result = net::ERR_CACHE_WRITE_FAILURE;
         return;
       }
       written_so_far += len_to_append;
       appended_so_far += len_to_append;
     }
-    int range_len = (found_range->length > kint32max) ?
-                    kint32max : found_range->length;
+    int range_len = base::saturated_cast<int>(found_range->length);
     int len_to_write = std::min(buf_len - written_so_far, range_len);
     if (!WriteSparseRange(found_range,
                           0,
                           len_to_write,
                           buf + written_so_far)) {
       *out_result = net::ERR_CACHE_WRITE_FAILURE;
       return;
     }
     written_so_far += len_to_write;
     ++it;
@@ skipping 25 matching lines @@
     const EntryOperationData& in_entry_op,
     int64* out_start,
     int* out_result) {
   DCHECK(initialized_);
   int64 offset = in_entry_op.sparse_offset;
   int len = in_entry_op.buf_len;
 
   SparseRangeIterator it = sparse_ranges_.lower_bound(offset);
 
   int64 start = offset;
-  int avail_so_far = 0;
+  int64 avail_so_far = 0;

[Peter Kasting, 2014/10/08 19:21:56]

The type changes and typecasts inserted in this file are to avoid various
currently-disabled MSVC compiler warnings.

 
   if (it != sparse_ranges_.end() && it->second.offset < offset + len)
     start = it->second.offset;
 
   if ((it == sparse_ranges_.end() || it->second.offset > offset) &&
       it != sparse_ranges_.begin()) {
     --it;
     if (it->second.offset + it->second.length > offset) {
       start = offset;
       avail_so_far = (it->second.offset + it->second.length) - offset;
     }
     ++it;
   }
 
   while (start + avail_so_far < offset + len &&
          it != sparse_ranges_.end() &&
          it->second.offset == start + avail_so_far) {
     avail_so_far += it->second.length;
     ++it;
   }
 
-  int len_from_start = len - (start - offset);
+  int64 len_from_start = len - (start - offset);
   *out_start = start;
-  *out_result = std::min(avail_so_far, len_from_start);
+  *out_result = static_cast<int>(std::min(avail_so_far, len_from_start));
 }
 
 void SimpleSynchronousEntry::CheckEOFRecord(int index,
                                             const SimpleEntryStat& entry_stat,
                                             uint32 expected_crc32,
                                             int* out_result) const {
   DCHECK(initialized_);
   uint32 crc32;
   bool has_crc32;
   int stream_size;
@@ skipping 68 matching lines @@
       continue;
 
     files_[i].Close();
     const int64 file_size = entry_stat.GetFileSize(key_, i);
     SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                      "LastClusterSize", cache_type_,
                      file_size % 4096, 0, 4097, 50);
     const int64 cluster_loss = file_size % 4096 ? 4096 - file_size % 4096 : 0;
     SIMPLE_CACHE_UMA(PERCENTAGE,
                      "LastClusterLossPercent", cache_type_,
-                     cluster_loss * 100 / (cluster_loss + file_size));
+                     static_cast<base::HistogramBase::Sample>(
+                         cluster_loss * 100 / (cluster_loss + file_size)));
   }
 
   if (sparse_file_open())
     sparse_file_.Close();
 
   if (files_created_) {
     const int stream2_file_index = GetFileIndexFromStreamIndex(2);
     SIMPLE_CACHE_UMA(BOOLEAN, "EntryCreatedAndStream2Omitted", cache_type_,
                      empty_file_omitted_[stream2_file_index]);
   }
@@ skipping 124 matching lines @@
     // each will only be known when reading the EOF record for stream 0.
     //
     // The size for file 0 and 1 is temporarily kept in
     // |data_size(1)| and |data_size(2)| respectively. Reading the key in
     // InitializeForOpen yields the data size for each file. In the case of
     // file hash_1, this is the total size of stream 2, and is assigned to
     // data_size(2). In the case of file 0, it is the combined size of stream
     // 0, stream 1 and one EOF record. The exact distribution of sizes between
     // stream 1 and stream 0 is only determined after reading the EOF record
     // for stream 0 in ReadAndValidateStream0.
-    out_entry_stat->set_data_size(i + 1, file_info.size);
+    out_entry_stat->set_data_size(i + 1, static_cast<int>(file_info.size));
   }
   SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                    "SyncOpenEntryAge", cache_type_,
                    entry_age.InHours(), 1, 1000, 50);
 
   files_created_ = false;
 
   return true;
 }
 
@@ skipping 313 matching lines @@
       result = false;
   }
   FilePath to_delete = path.AppendASCII(
       GetSparseFilenameFromEntryHash(entry_hash));
   base::DeleteFile(to_delete, false);
   return result;
 }
 
 void SimpleSynchronousEntry::RecordSyncCreateResult(CreateEntryResult result,
                                                     bool had_index) {
-  DCHECK_GT(CREATE_ENTRY_MAX, result);
+  DCHECK_LT(result, CREATE_ENTRY_MAX);
   SIMPLE_CACHE_UMA(ENUMERATION,
                    "SyncCreateResult", cache_type_, result, CREATE_ENTRY_MAX);
   if (had_index) {
     SIMPLE_CACHE_UMA(ENUMERATION,
                      "SyncCreateResult_WithIndex", cache_type_,
                      result, CREATE_ENTRY_MAX);
   } else {
     SIMPLE_CACHE_UMA(ENUMERATION,
                      "SyncCreateResult_WithoutIndex", cache_type_,
                      result, CREATE_ENTRY_MAX);
@@ skipping 76 matching lines @@
 
   sparse_ranges_.clear();
   sparse_tail_offset_ = sizeof(header) + key_.size();
 
   return true;
 }
 
 bool SimpleSynchronousEntry::ScanSparseFile(int32* out_sparse_data_size) {
   DCHECK(sparse_file_open());
 
-  int32 sparse_data_size = 0;
+  int64 sparse_data_size = 0;
 
   SimpleFileHeader header;
   int header_read_result =
       sparse_file_.Read(0, reinterpret_cast<char*>(&header), sizeof(header));
   if (header_read_result != sizeof(header)) {
     DLOG(WARNING) << "Could not read header from sparse file.";
     return false;
   }
 
   if (header.initial_magic_number != kSimpleInitialMagicNumber) {
@@ skipping 30 matching lines @@
 
     SparseRange range;
     range.offset = range_header.offset;
     range.length = range_header.length;
     range.data_crc32 = range_header.data_crc32;
     range.file_offset = range_header_offset + sizeof(range_header);
     sparse_ranges_.insert(std::make_pair(range.offset, range));
 
     range_header_offset += sizeof(range_header) + range.length;
 
-    DCHECK_LE(sparse_data_size, sparse_data_size + range.length);
+    DCHECK_GE(sparse_data_size + range.length, sparse_data_size);
     sparse_data_size += range.length;
   }
 
-  *out_sparse_data_size = sparse_data_size;
+  *out_sparse_data_size = static_cast<int32>(sparse_data_size);
   sparse_tail_offset_ = range_header_offset;
 
   return true;
 }
 
 bool SimpleSynchronousEntry::ReadSparseRange(const SparseRange* range,
                                              int offset, int len, char* buf) {
   DCHECK(range);
   DCHECK(buf);
-  DCHECK_GE(range->length, offset);
-  DCHECK_GE(range->length, offset + len);
+  DCHECK_LE(offset, range->length);
+  DCHECK_LE(offset + len, range->length);
 
   int bytes_read = sparse_file_.Read(range->file_offset + offset, buf, len);
   if (bytes_read < len) {
     DLOG(WARNING) << "Could not read sparse range.";
     return false;
   }
 
   // If we read the whole range and we have a crc32, check it.
   if (offset == 0 && len == range->length && range->data_crc32 != 0) {
     uint32 actual_crc32 = crc32(crc32(0L, Z_NULL, 0),
                                 reinterpret_cast<const Bytef*>(buf),
                                 len);
     if (actual_crc32 != range->data_crc32) {
       DLOG(WARNING) << "Sparse range crc32 mismatch.";
       return false;
     }
   }
   // TODO(ttuttle): Incremental crc32 calculation?
 
   return true;
 }
 
 bool SimpleSynchronousEntry::WriteSparseRange(SparseRange* range,
                                               int offset, int len,
                                               const char* buf) {
   DCHECK(range);
   DCHECK(buf);
-  DCHECK_GE(range->length, offset);
-  DCHECK_GE(range->length, offset + len);
+  DCHECK_LE(offset, range->length);
+  DCHECK_LE(offset + len, range->length);
 
   uint32 new_crc32 = 0;
   if (offset == 0 && len == range->length) {
     new_crc32 = crc32(crc32(0L, Z_NULL, 0),
                       reinterpret_cast<const Bytef*>(buf),
                       len);
   }
 
   if (new_crc32 != range->data_crc32) {
     range->data_crc32 = new_crc32;
@@ skipping 18 matching lines @@
     DLOG(WARNING) << "Could not write sparse range.";
     return false;
   }
 
   return true;
 }
 
 bool SimpleSynchronousEntry::AppendSparseRange(int64 offset,
                                                int len,
                                                const char* buf) {
-  DCHECK_LE(0, offset);
-  DCHECK_LT(0, len);
+  DCHECK_GE(offset, 0);
+  DCHECK_GT(len, 0);
   DCHECK(buf);
 
   uint32 data_crc32 = crc32(crc32(0L, Z_NULL, 0),
                             reinterpret_cast<const Bytef*>(buf),
                             len);
 
   SimpleFileSparseRangeHeader header;
   header.sparse_range_magic_number = kSimpleSparseRangeMagicNumber;
   header.offset = offset;
   header.length = len;
@@ skipping 20 matching lines @@
   range.offset = offset;
   range.length = len;
   range.data_crc32 = data_crc32;
   range.file_offset = data_file_offset;
   sparse_ranges_.insert(std::make_pair(offset, range));
 
   return true;
 }
 
 }  // namespace disk_cache