Ignore calls to Reset() when in error state

media/filters/decoder_stream.cc

 // Copyright 2014 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/filters/decoder_stream.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 139 matching lines @@
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::Reset(const base::Closure& closure) {
   FUNCTION_DVLOG(2);
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_NE(state_, STATE_UNINITIALIZED);
   DCHECK(reset_cb_.is_null());
 
   reset_cb_ = closure;
 
   if (!read_cb_.is_null()) {
-    task_runner_->PostTask(FROM_HERE, base::Bind(
-        base::ResetAndReturn(&read_cb_), ABORTED, scoped_refptr<Output>()));
+    task_runner_->PostTask(FROM_HERE,
+                           base::Bind(base::ResetAndReturn(&read_cb_), ABORTED,
+                                      scoped_refptr<Output>()));
   }
 
   ready_outputs_.clear();
 
+  // It's possible to have received a DECODE_ERROR and entered STATE_ERROR right
+  // before a Reset() is executed. See crbug.com/597605 or crbug.com/607454.

[sandersd (OOO until July 31), 2016/05/02 22:57:03]

or -> and.

[tguilbert, 2016/05/03 00:06:41]

Done.

+  if (state_ == STATE_ERROR) {
+    task_runner_->PostTask(FROM_HERE, base::ResetAndReturn(&reset_cb_));

[sandersd (OOO until July 31), 2016/05/02 22:57:03]

Should this be below the |read_cb_| check? It seems that entering an error state
should be responsible for firing that callback, and so here we should be safe to
put the state check first.

[tguilbert, 2016/05/03 00:06:41]

This is something that I considered. The comments above Reset() mention that it
fires any pending read callbacks. I therefore purposely put |reset_cb_| after
|read_cb_|, but that may not be the right thing.

There are two places where STATE_ERROR is set.
- In OnDecodeDone, if we receive a DECODE_ERROR, we set the state and then fire
any |read_cb_|.
- In CompleteDecoderReinitialization(), we set the state, followed by firing
|reset_cb_| and returning if it is present, OR satisfying |read_cb_|.

In either of the cases where we enter STATE_ERROR, I don't think firing or not
firing the of |read_cb_| will cause problems. The |read_cb_| should always be
null when we are in a STATE_ERROR (resulting in a no-op), except in a codepath
that should never happen (queuing a Read() while a Reset() is happening).

+    return;
+  }
+
   // During decoder reinitialization, the Decoder does not need to be and
   // cannot be Reset(). |decrypting_demuxer_stream_| was reset before decoder
   // reinitialization.
   if (state_ == STATE_REINITIALIZING_DECODER)
     return;
 
   // During pending demuxer read and when not using DecryptingDemuxerStream,
   // the Decoder will be reset after demuxer read is returned
   // (in OnBufferReady()).
   if (state_ == STATE_PENDING_DEMUXER_READ && !decrypting_demuxer_stream_)
@@ skipping 527 matching lines @@
   }
 
   ReadFromDemuxerStream();
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::ResetDecoder() {
   FUNCTION_DVLOG(2);
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
-         state_ == STATE_ERROR || state_ == STATE_END_OF_STREAM) << state_;
+         state_ == STATE_END_OF_STREAM)
+      << state_;
   DCHECK(!reset_cb_.is_null());
 
   decoder_->Reset(base::Bind(&DecoderStream<StreamType>::OnDecoderReset,
                              weak_factory_.GetWeakPtr()));
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::OnDecoderReset() {
   FUNCTION_DVLOG(2);
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
-         state_ == STATE_ERROR || state_ == STATE_END_OF_STREAM) << state_;
+         state_ == STATE_END_OF_STREAM)
+      << state_;
   // If Reset() was called during pending read, read callback should be fired
   // before the reset callback is fired.
   DCHECK(read_cb_.is_null());
   DCHECK(!reset_cb_.is_null());
 
   // Make sure we read directly from the demuxer after a reset.
   fallback_buffers_.clear();
   pending_buffers_.clear();
 
   if (state_ != STATE_FLUSHING_DECODER) {
     state_ = STATE_NORMAL;
     active_splice_ = false;
     base::ResetAndReturn(&reset_cb_).Run();
     return;
   }
 
   // The resetting process will be continued in OnDecoderReinitialized().
   ReinitializeDecoder();
 }
 
 template class DecoderStream<DemuxerStream::VIDEO>;
 template class DecoderStream<DemuxerStream::AUDIO>;
 
 }  // namespace media