Fix a timing bug in multibuffer.

media/blink/multibuffer_data_source.cc

 // Copyright 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 "media/blink/multibuffer_data_source.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 499 matching lines @@
   UpdateLoadingState(true);
 }
 
 void MultibufferDataSource::ProgressCallback(int64_t begin, int64_t end) {
   DVLOG(1) << __func__ << "(" << begin << ", " << end << ")";
   DCHECK(render_task_runner_->BelongsToCurrentThread());
 
   if (assume_fully_buffered())
     return;
 
+  base::AutoLock auto_lock(lock_);
+
   if (end > begin) {
     // TODO(scherkus): we shouldn't have to lock to signal host(), see
     // http://crbug.com/113712 for details.
-    base::AutoLock auto_lock(lock_);
     if (stop_signal_received_)
       return;
 
     host_->AddBufferedByteRange(begin, end);
   }
 
   UpdateLoadingState(false);
 }
 
 void MultibufferDataSource::UpdateLoadingState(bool force_loading) {

[sandersd (OOO until July 31), 2016/09/21 01:13:39]

Nit: Add _Locked suffix?

[hubbe, 2016/09/21 04:19:22]

Done

   DVLOG(1) << __func__;
+  lock_.AssertAcquired();
   if (assume_fully_buffered())
     return;
   // Update loading state.
   bool is_loading = !!reader_ && reader_->IsLoading();
+  if (read_op_)
+    is_loading = true;

[sandersd (OOO until July 31), 2016/09/21 01:13:39]

This means nothing to me, but I'll believe you.

[hubbe, 2016/09/21 04:19:22]

Well, basically we can't destroy reader_ if there is a read operation pending.
There are a few ways to do this:

1) pretend like we're loading if a read operation is pending. This sort of makes
sense as reading will move the read pointer and cause additional buffering.

2) just do "if(read_op_) return", which just prevents us from going to/from a
loading state while a read is pending. Should also work, but just doesn't feel
as correct to me.

3) Do the state to not loading now, but destroy reader_ later. (Seems a bit more
complicated to me, as the state might change again between now and then...)

   if (force_loading || is_loading != loading_) {
     loading_ = is_loading || force_loading;
 
     if (!loading_ && cancel_on_defer_) {
       reader_.reset(nullptr);
     }
 
     // Callback could kill us, be sure to call it last.
     downloading_cb_.Run(loading_);
   }
@@ skipping 48 matching lines @@
   reader_->SetPinRange(back_buffer, kMaxBufferPreload + kPreloadHighExtra);
 
   if (preload_ == METADATA) {
     reader_->SetPreload(0, 0);
   } else {
     reader_->SetPreload(preload_high, preload);
   }
 }
 
 }  // namespace media