Fix a data race in AudioEncoderMutableImpl and derived classes

webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h

Index: webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h
diff --git a/webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h b/webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h
index 8d355d8e35a11fe4ab01bd075e8e19527adfb2ee..f760b01c3d9d8a4fb12b2a0816b77424e8fa98b5 100644
--- a/webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h
+++ b/webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h
@@ -12,7 +12,10 @@
 #define WEBRTC_MODULES_AUDIO_CODING_CODECS_AUDIO_ENCODER_MUTABLE_IMPL_H_
 
 #include "webrtc/base/scoped_ptr.h"
+#include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/audio_coding/codecs/audio_encoder.h"
+#include "webrtc/system_wrappers/interface/critical_section_wrapper.h"
+#include "webrtc/system_wrappers/interface/thread_wrapper.h"
 
 namespace webrtc {
 
@@ -44,32 +47,46 @@ class AudioEncoderMutableImpl : public P {
                                                   const int16_t* audio,
                                                   size_t max_encoded_bytes,
                                                   uint8_t* encoded) override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->EncodeInternal(rtp_timestamp, audio, max_encoded_bytes,
                                     encoded);
   }
-  int SampleRateHz() const override { return encoder_->SampleRateHz(); }
-  int NumChannels() const override { return encoder_->NumChannels(); }
+  int SampleRateHz() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
+    return encoder_->SampleRateHz();
+  }
+  int NumChannels() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
+    return encoder_->NumChannels();
+  }
   size_t MaxEncodedBytes() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->MaxEncodedBytes();
   }
   int RtpTimestampRateHz() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->RtpTimestampRateHz();
   }
   int Num10MsFramesInNextPacket() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->Num10MsFramesInNextPacket();
   }
   int Max10MsFramesInAPacket() const override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     return encoder_->Max10MsFramesInAPacket();
   }
   void SetTargetBitrate(int bits_per_second) override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     encoder_->SetTargetBitrate(bits_per_second);
   }
   void SetProjectedPacketLossRate(double fraction) override {
+    CriticalSectionScoped cs(encoder_lock_.get());
     encoder_->SetProjectedPacketLossRate(fraction);
   }
 
  protected:
-  explicit AudioEncoderMutableImpl(const typename T::Config& config) {
+  explicit AudioEncoderMutableImpl(const typename T::Config& config)
+      : encoder_lock_(CriticalSectionWrapper::CreateCriticalSection()) {
     Reconstruct(config);
   }
 
@@ -77,16 +94,23 @@ class AudioEncoderMutableImpl : public P {
     if (!config.IsOk())
       return false;
     config_ = config;
+    CriticalSectionScoped cs(encoder_lock_.get());

[Jelena, 2015/06/13 09:47:50]

Actually, lock needs to protect the config_ as well, so move it one line up.

[kwiberg-webrtc, 2015/06/13 20:27:49]

I don't think it does, actually. If I'm right, all AudioEncoder(Mutable)
accesses are from one thread, and all AudioDecoder accesses are from one thread,
so the only possible race is between those two threads. And the decoder thread
will never touch config_.

That said, since this is a stopgap solution, I'm not at all opposed to following
your advice. Just make config_ GUARDED_BY(encoder_lock_) in that case.

[hlundin-webrtc, 2015/06/15 09:38:10]

Moved the critsect as suggested, and added GUARDED_BY. However, the latter
caused some collateral damage.

     encoder_.reset(new T(config_));
     return true;
   }
 
   const typename T::Config& config() const { return config_; }

[Jelena, 2015/06/13 09:47:50]

Since config_ can be modified on another thread, you need to obtain a lock
first. For the same reason, it is unsafe to return a reference, so this method
must return a copy of config_.

[kwiberg-webrtc, 2015/06/13 20:27:49]

Same comment as on line 97.

[hlundin-webrtc, 2015/06/15 09:38:10]

Done.

-  T* encoder() { return encoder_.get(); }
-  const T* encoder() const { return encoder_.get(); }
+  T* encoder() EXCLUSIVE_LOCKS_REQUIRED(encoder_lock_) {
+    return encoder_.get();
+  }
+  const T* encoder() const EXCLUSIVE_LOCKS_REQUIRED(encoder_lock_) {
+    return encoder_.get();
+  }
+
+  const rtc::scoped_ptr<CriticalSectionWrapper> encoder_lock_;
 
  private:
-  rtc::scoped_ptr<T> encoder_;
+  rtc::scoped_ptr<T> encoder_ GUARDED_BY(encoder_lock_);
   typename T::Config config_;
 };