Fix multibuffer crashing bug

media/blink/multibuffer.cc

Index: media/blink/multibuffer.cc
diff --git a/media/blink/multibuffer.cc b/media/blink/multibuffer.cc
index a8d981eba88b812ec3694af4fe8698082c49dcf9..48e7fbc63a868825931d3a879f9a2679eb4ec7f0 100644
--- a/media/blink/multibuffer.cc
+++ b/media/blink/multibuffer.cc
@@ -115,9 +115,13 @@ MultiBuffer::MultiBuffer(int32_t block_size_shift,
     : max_size_(0), block_size_shift_(block_size_shift), lru_(global_lru) {}
 
 MultiBuffer::~MultiBuffer() {
+  DCHECK(pinned_.empty());
+  DCHECK_EQ(max_size_, 0);
   // Remove all blocks from the LRU.
   for (const auto& i : data_) {
-    lru_->Remove(this, i.first);
+    if (!pinned_[i.first]) {

[liberato (no reviews please), 2016/03/02 21:45:46]

since |pinned_.empty()| from the dcheck, how could this ever fail?

[hubbe, 2016/03/02 22:10:15]

This is the "suspenders and belt" part.
*if* there is a pinning leak somewhere, we still don't crash in release mode.

[liberato (no reviews please), 2016/03/02 22:13:33]

i see.  worth a UMA?

[hubbe, 2016/03/02 22:22:54]

Hopefully the DCHECK will be sufficient.

[DaleCurtis, 2016/03/03 00:12:22]

We don't DCHECK things that we have checks for.  See the style guide:

https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

Instead you might change the DCHECK(empty()) to CHECK(empty()) so it's not a
security bug if it does crash.

[hubbe, 2016/03/03 01:10:10]

Done.

+      lru_->Remove(this, i.first);
+    }
   }
   lru_->IncrementDataSize(-static_cast<int64_t>(data_.size()));
   lru_->IncrementMaxSize(-max_size_);
@@ -379,6 +383,7 @@ void MultiBuffer::OnDataProviderEvent(DataProvider* provider_tmp) {
 
 void MultiBuffer::MergeFrom(MultiBuffer* other) {
   // Import data and update LRU.
+  size_t data_size = data_.size();
   for (const auto& data : other->data_) {
     if (data_.insert(std::make_pair(data.first, data.second)).second) {
       if (!pinned_[data.first]) {
@@ -386,6 +391,7 @@ void MultiBuffer::MergeFrom(MultiBuffer* other) {
       }
     }
   }
+  lru_->IncrementDataSize(static_cast<int64_t>(data_.size() - data_size));

[DaleCurtis, 2016/03/03 00:12:22]

Should lru_->Insert() automatically increase the lru data size?

[hubbe, 2016/03/03 01:10:10]

No, the data size includes pinned data.

   // Update present_
   for (const auto& r : other->present_) {
     if (r.second) {
@@ -424,6 +430,7 @@ void MultiBuffer::PinRange(const BlockId& from,
 
   auto range = pinned_.find(to - 1);
   while (1) {
+    DCHECK_GE(range.value(), 0);
     if (range.value() == 0 || range.value() == how_much) {
       bool pin = range.value() == how_much;
       Interval<BlockId> transition_range =