Fix multibuffer crashing bug

media/blink/multibuffer.cc

 // Copyright 2015 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 <utility>
 
 #include "media/blink/multibuffer.h"
 
 #include "base/bind.h"
 
@@ skipping 97 matching lines @@
 }
 
 //
 // MultiBuffer
 //
 MultiBuffer::MultiBuffer(int32_t block_size_shift,
                          const scoped_refptr<GlobalLRU>& global_lru)
     : 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_);
 }
 
 void MultiBuffer::AddReader(const BlockId& pos, Reader* reader) {
   std::set<Reader*>* set_of_readers = &readers_[pos];
   bool already_waited_for = !set_of_readers->empty();
   set_of_readers->insert(reader);
 
@@ skipping 241 matching lines @@
         break;
       case ProviderStateDead:
         RemoveProvider(provider_tmp);
         break;
     }
   }
 }
 
 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]) {
         lru_->Insert(this, data.first);
       }
     }
   }
+  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) {
       present_.SetInterval(r.first.begin, r.first.end, 1);
     }
   }
   // Notify existing readers.
   auto last = present_.begin();
   for (const auto& r : other->present_) {
     if (r.second) {
@@ skipping 18 matching lines @@
   // transitioned in or out of the unlocked state. If so, we iterate over
   // all buffers in that range and add/remove them from the LRU as approperiate.
   // We iterate *backwards* through the ranges, with the idea that data in a
   // continous range should be freed from the end first.
 
   if (data_.empty())
     return;
 
   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 =
           modified_range.Intersect(range.interval());
       if (transition_range.Empty())
         break;
 
       // For each range that has transitioned to/from a pinned state,
       // we iterate over the corresponding ranges in |present_| to find
       // the blocks that are actually in the multibuffer.
@@ skipping 34 matching lines @@
 }
 
 void MultiBuffer::IncrementMaxSize(int32_t size) {
   max_size_ += size;
   lru_->IncrementMaxSize(size);
   DCHECK_GE(max_size_, 0);
   // Pruning only happens when blocks are added.
 }
 
 }  // namespace media