Fix a reentrance issue in DecryptingDemuxerStream.

media/filters/decrypting_demuxer_stream.cc

 // Copyright (c) 2012 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/decrypting_demuxer_stream.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 25 matching lines @@
     const SetDecryptorReadyCB& set_decryptor_ready_cb)
     : message_loop_(message_loop),
       weak_factory_(this),
       state_(kUninitialized),
       demuxer_stream_(NULL),
       set_decryptor_ready_cb_(set_decryptor_ready_cb),
       decryptor_(NULL),
       key_added_while_decrypt_pending_(false) {
 }
 
-void DecryptingDemuxerStream::Initialize(
-    DemuxerStream* stream,
-    const PipelineStatusCB& status_cb) {
-  DVLOG(2) << "Initialize()";
+void DecryptingDemuxerStream::Initialize(DemuxerStream* stream,
+                                         const PipelineStatusCB& status_cb) {
+  DVLOG(2) << __FUNCTION__;
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kUninitialized) << state_;
 
   DCHECK(!demuxer_stream_);
   weak_this_ = weak_factory_.GetWeakPtr();
   demuxer_stream_ = stream;
-  init_cb_ = status_cb;
+  init_cb_ = BindToCurrentLoop(status_cb);
 
   InitializeDecoderConfig();
 
   state_ = kDecryptorRequested;
   set_decryptor_ready_cb_.Run(
       BIND_TO_LOOP(&DecryptingDemuxerStream::SetDecryptor));
 }
 
 void DecryptingDemuxerStream::Read(const ReadCB& read_cb) {
-  DVLOG(3) << "Read()";
+  DVLOG(3) << __FUNCTION__;
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kIdle) << state_;
   DCHECK(!read_cb.is_null());
   CHECK(read_cb_.is_null()) << "Overlapping reads are not supported.";
 
-  read_cb_ = read_cb;
+  read_cb_ = BindToCurrentLoop(read_cb);
   state_ = kPendingDemuxerRead;
   demuxer_stream_->Read(
       base::Bind(&DecryptingDemuxerStream::DecryptBuffer, weak_this_));
 }
 
 void DecryptingDemuxerStream::Reset(const base::Closure& closure) {
-  DVLOG(2) << "Reset() - state: " << state_;
+  DVLOG(2) << __FUNCTION__ << " - state: " << state_;
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(state_ != kUninitialized && state_ != kDecryptorRequested) << state_;
   DCHECK(init_cb_.is_null());  // No Reset() during pending initialization.
   DCHECK(reset_cb_.is_null());
 
   reset_cb_ = BindToCurrentLoop(closure);
 
   decryptor_->CancelDecrypt(GetDecryptorStreamType());
 
   // Reset() cannot complete if the read callback is still pending.
@@ skipping 29 matching lines @@
 
 DemuxerStream::Type DecryptingDemuxerStream::type() {
   DCHECK(state_ != kUninitialized && state_ != kDecryptorRequested) << state_;
   return demuxer_stream_->type();
 }
 
 void DecryptingDemuxerStream::EnableBitstreamConverter() {
   demuxer_stream_->EnableBitstreamConverter();
 }
 
-DecryptingDemuxerStream::~DecryptingDemuxerStream() {}
+DecryptingDemuxerStream::~DecryptingDemuxerStream() {
+  DVLOG(2) << __FUNCTION__;
+  if (!set_decryptor_ready_cb_.is_null()) {
+    base::ResetAndReturn(&set_decryptor_ready_cb_).Run(DecryptorReadyCB());
+    set_decryptor_ready_cb_.Reset();

[whywhat, 2013/09/17 01:00:40]

Hm, so you reset the callback, then call it and reset it again before it's
destructed? isn't it too much?

[xhwang, 2013/09/17 01:13:00]

Good catch. Done.

+  }
+}
 
 void DecryptingDemuxerStream::SetDecryptor(Decryptor* decryptor) {
-  DVLOG(2) << "SetDecryptor()";
+  DVLOG(2) << __FUNCTION__;
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kDecryptorRequested) << state_;
   DCHECK(!init_cb_.is_null());
   DCHECK(!set_decryptor_ready_cb_.is_null());
 
   set_decryptor_ready_cb_.Reset();
 
   if (!decryptor) {
+    state_ = kUninitialized;
     base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);

[xhwang, 2013/09/17 00:53:49]

The caller could destroy |this| and create a new one here. If the new one's
address is the same as the old one, the state will be set to kUninitialized,
which triggers a DCHECK because we have a funny state.

[whywhat, 2013/09/17 01:00:40]

I'd at least leave a comment in the code saying that Run() should be the last
thing to happen before return (or post it to the message loop instead of relying
on fragile ordering).

[xhwang, 2013/09/17 01:13:00]

I am forcing the post. See line 55 in the new code. But still setting state
before posting callback is a better style.

On 2013/09/17 01:00:40, whywhat wrote:

-    state_ = kUninitialized;
     return;
   }
 
   decryptor_ = decryptor;
 
   decryptor_->RegisterNewKeyCB(
       GetDecryptorStreamType(),
       BIND_TO_LOOP(&DecryptingDemuxerStream::OnKeyAdded));
 
   state_ = kIdle;
   base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK);
 }
 
 void DecryptingDemuxerStream::DecryptBuffer(
     DemuxerStream::Status status,
     const scoped_refptr<DecoderBuffer>& buffer) {
-  DVLOG(3) << "DecryptBuffer()";
+  DVLOG(3) << __FUNCTION__;
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kPendingDemuxerRead) << state_;
   DCHECK(!read_cb_.is_null());
   DCHECK_EQ(buffer.get() != NULL, status == kOk) << status;
 
   if (!reset_cb_.is_null()) {
     base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
     DoReset();
     return;
   }
@@ skipping 35 matching lines @@
   DCHECK_EQ(state_, kPendingDecrypt) << state_;
   decryptor_->Decrypt(
       GetDecryptorStreamType(),
       pending_buffer_to_decrypt_,
       BIND_TO_LOOP(&DecryptingDemuxerStream::DeliverBuffer));
 }
 
 void DecryptingDemuxerStream::DeliverBuffer(
     Decryptor::Status status,
     const scoped_refptr<DecoderBuffer>& decrypted_buffer) {
-  DVLOG(3) << "DeliverBuffer() - status: " << status;
+  DVLOG(3) << __FUNCTION__ << " - status: " << status;
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kPendingDecrypt) << state_;
   DCHECK_NE(status, Decryptor::kNeedMoreData);
   DCHECK(!read_cb_.is_null());
   DCHECK(pending_buffer_to_decrypt_.get());
 
   bool need_to_try_again_if_nokey = key_added_while_decrypt_pending_;
   key_added_while_decrypt_pending_ = false;
 
   if (!reset_cb_.is_null()) {
@@ skipping 98 matching lines @@
       break;
     }
 
     default:
       NOTREACHED();
       return;
   }
 }
 
 }  // namespace media