Fix multibuffer crashing bug

Fix multibuffer crashing bug

The multibuffer reader had a problem where it would unpin a block if it was initialized
with a position that was not divisible by the block size. This generally only happens
on a redirect. When this happens, everything was fine up until the multibuffer is freed, where
it would crash when it tried to remove a pinned block from the global LRU.

Seval actions taken:

 o Add a test that covers the redirect path properly
   * this uncovered another bug where the data size was incorrectly calculated (fixed)
 o Fix it so that if blocks are pinned in ~MultiBuffer, we don't crash unless we're in debug mode.
 o Add DCHECKS to make sure that pin counts don't go negative.
 o Fix multibuffer reader so that it always unpins exactly what it pinned, no more, no less.

BUG=591359
Committed: https://crrev.com/5b6dbf76bfa39c24ad97c29ac8a8a0c44dcf3099
Cr-Commit-Position: refs/heads/master@{#378925}

[hubbe, 2016-03-02 20:43:54]

hubbe@chromium.org changed reviewers:
+ dalecurtis@chromium.org, liberato@chromium.org

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

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:122: if (!pinned_[i.first]) {
since |pinned_.empty()| from the dcheck, how could this ever fail?

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer_rea...
File media/blink/multibuffer_reader.cc (left):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer_rea...
media/blink/multibuffer_reader.cc:52: tmp.IncrementInterval(block(pos_ -
max_buffer_backward_),
x -= y;
x += y;
// success! :)

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

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:122: if (!pinned_[i.first]) {
On 2016/03/02 21:45:46, liberato wrote:
> since |pinned_.empty()| from the dcheck, how could this ever fail?

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

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer_rea...
File media/blink/multibuffer_reader.cc (left):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer_rea...
media/blink/multibuffer_reader.cc:52: tmp.IncrementInterval(block(pos_ -
max_buffer_backward_),
On 2016/03/02 21:45:46, liberato wrote:
> x -= y;
> x += y;
> // success! :)

que?

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

lgtm.

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:122: if (!pinned_[i.first]) {
On 2016/03/02 22:10:15, hubbe wrote:
> On 2016/03/02 21:45:46, liberato wrote:
> > since |pinned_.empty()| from the dcheck, how could this ever fail?
> 
> This is the "suspenders and belt" part.
> *if* there is a pinning leak somewhere, we still don't crash in release mode.

i see.  worth a UMA?

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer_rea...
File media/blink/multibuffer_reader.cc (left):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer_rea...
media/blink/multibuffer_reader.cc:52: tmp.IncrementInterval(block(pos_ -
max_buffer_backward_),
On 2016/03/02 22:10:15, hubbe wrote:
> On 2016/03/02 21:45:46, liberato wrote:
> > x -= y;
> > x += y;
> > // success! :)
> 
> que?

|pos_| vs |pos| .  maybe time for me to increase the font size.  :)

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

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:122: if (!pinned_[i.first]) {
On 2016/03/02 22:13:33, liberato wrote:
> On 2016/03/02 22:10:15, hubbe wrote:
> > On 2016/03/02 21:45:46, liberato wrote:
> > > since |pinned_.empty()| from the dcheck, how could this ever fail?
> > 
> > This is the "suspenders and belt" part.
> > *if* there is a pinning leak somewhere, we still don't crash in release
mode.
> 
> i see.  worth a UMA?

Hopefully the DCHECK will be sufficient.

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer_rea...
File media/blink/multibuffer_reader.cc (left):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer_rea...
media/blink/multibuffer_reader.cc:52: tmp.IncrementInterval(block(pos_ -
max_buffer_backward_),
On 2016/03/02 22:13:33, liberato wrote:
> On 2016/03/02 22:10:15, hubbe wrote:
> > On 2016/03/02 21:45:46, liberato wrote:
> > > x -= y;
> > > x += y;
> > > // success! :)
> > 
> > que?
> 
> |pos_| vs |pos| .  maybe time for me to increase the font size.  :)

Acknowledged.

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

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:122: if (!pinned_[i.first]) {
On 2016/03/02 at 22:22:54, hubbe wrote:
> On 2016/03/02 22:13:33, liberato wrote:
> > On 2016/03/02 22:10:15, hubbe wrote:
> > > On 2016/03/02 21:45:46, liberato wrote:
> > > > since |pinned_.empty()| from the dcheck, how could this ever fail?
> > > 
> > > This is the "suspenders and belt" part.
> > > *if* there is a pinning leak somewhere, we still don't crash in release
mode.
> > 
> > i see.  worth a UMA?
> 
> Hopefully the DCHECK will be sufficient.

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.

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:394:
lru_->IncrementDataSize(static_cast<int64_t>(data_.size() - data_size));
Should lru_->Insert() automatically increase the lru data size?

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

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:122: if (!pinned_[i.first]) {
On 2016/03/03 00:12:22, DaleCurtis wrote:
> On 2016/03/02 at 22:22:54, hubbe wrote:
> > On 2016/03/02 22:13:33, liberato wrote:
> > > On 2016/03/02 22:10:15, hubbe wrote:
> > > > On 2016/03/02 21:45:46, liberato wrote:
> > > > > since |pinned_.empty()| from the dcheck, how could this ever fail?
> > > > 
> > > > This is the "suspenders and belt" part.
> > > > *if* there is a pinning leak somewhere, we still don't crash in release
> mode.
> > > 
> > > i see.  worth a UMA?
> > 
> > Hopefully the DCHECK will be sufficient.
> 
> 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.

Done.

https://codereview.chromium.org/1754893006/diff/1/media/blink/multibuffer.cc#...
media/blink/multibuffer.cc:394:
lru_->IncrementDataSize(static_cast<int64_t>(data_.size() - data_size));
On 2016/03/03 00:12:22, DaleCurtis wrote:
> Should lru_->Insert() automatically increase the lru data size?

No, the data size includes pinned data.

[DaleCurtis, 2016-03-03 02:04:54]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-03-03 02:04:54]

lgtm

[DaleCurtis, 2016-03-03 02:04:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1754893006/#ps20001
(title: "DCHECK -> CHECK")

[commit-bot: I haz the power, 2016-03-03 02:05:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1754893006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1754893006/20001

[commit-bot: I haz the power, 2016-03-03 02:43:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-03 02:45:04]

Description was changed from

==========
Fix multibuffer crashing bug

The multibuffer reader had a problem where it would unpin a block if it was
initialized
with a position that was not divisible by the block size. This generally only
happens
on a redirect. When this happens, everything was fine up until the multibuffer
is freed, where
it would crash when it tried to remove a pinned block from the global LRU.

Seval actions taken:

 o Add a test that covers the redirect path properly
   * this uncovered another bug where the data size was incorrectly calculated
(fixed)
 o Fix it so that if blocks are pinned in ~MultiBuffer, we don't crash unless
we're in debug mode.
 o Add DCHECKS to make sure that pin counts don't go negative.
 o Fix multibuffer reader so that it always unpins exactly what it pinned, no
more, no less.

BUG=591359
==========

to

==========
Fix multibuffer crashing bug

The multibuffer reader had a problem where it would unpin a block if it was
initialized
with a position that was not divisible by the block size. This generally only
happens
on a redirect. When this happens, everything was fine up until the multibuffer
is freed, where
it would crash when it tried to remove a pinned block from the global LRU.

Seval actions taken:

 o Add a test that covers the redirect path properly
   * this uncovered another bug where the data size was incorrectly calculated
(fixed)
 o Fix it so that if blocks are pinned in ~MultiBuffer, we don't crash unless
we're in debug mode.
 o Add DCHECKS to make sure that pin counts don't go negative.
 o Fix multibuffer reader so that it always unpins exactly what it pinned, no
more, no less.

BUG=591359

Committed: https://crrev.com/5b6dbf76bfa39c24ad97c29ac8a8a0c44dcf3099
Cr-Commit-Position: refs/heads/master@{#378925}
==========

[commit-bot: I haz the power, 2016-03-03 02:45:05]

Patchset 2 (id:??) landed as
https://crrev.com/5b6dbf76bfa39c24ad97c29ac8a8a0c44dcf3099
Cr-Commit-Position: refs/heads/master@{#378925}