Ignore calls to Reset() when in error state

media/filters/decoder_stream.cc

Index: media/filters/decoder_stream.cc
diff --git a/media/filters/decoder_stream.cc b/media/filters/decoder_stream.cc
index e8f590cf9627c66222c27f9de13f220b593c9e07..85a0e42e192bed2e4d6777dbe6e421a7a5499fe6 100644
--- a/media/filters/decoder_stream.cc
+++ b/media/filters/decoder_stream.cc
@@ -157,12 +157,20 @@ void DecoderStream<StreamType>::Reset(const base::Closure& closure) {
   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.
@@ -710,7 +718,8 @@ 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,
@@ -722,7 +731,8 @@ 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());