From f7c6e743b38f940b0c1cc94f7756aaf00e68b220 Mon Sep 17 00:00:00 2001
From: "stefan@webrtc.org"
 <stefan@webrtc.org@4adac7df-926f-26a2-2b94-8c16560cd09d>
Date: Wed, 29 Jan 2014 10:27:51 +0000
Subject: [PATCH] Fix deadlock in video_receiver.cc.

In webrtc::vcm::VideoReceiver::ResetDecoder(), the lock order is:
1. take _receiveCritSect,
2. take process_crit_sect_

This conflicts with the follow code path:
1. webrtc::vcm::VideoReceiver::Process(), take process_crit_sect_
  call -> webrtc::vcm::VideoReceiver::NackList(),
2.  with nackStats=kNackKeyFrameRequest, take _receiveCritSect

BUG=2861
TEST=trybots
R=sprang@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/7749004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@5456 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 .../main/source/video_receiver.cc             | 77 ++++++++++++-------
 1 file changed, 48 insertions(+), 29 deletions(-)

diff --git a/webrtc/modules/video_coding/main/source/video_receiver.cc b/webrtc/modules/video_coding/main/source/video_receiver.cc
index 68668eae7..bd99311e0 100644
--- a/webrtc/modules/video_coding/main/source/video_receiver.cc
+++ b/webrtc/modules/video_coding/main/source/video_receiver.cc
@@ -121,8 +121,12 @@ int32_t VideoReceiver::Process() {
   // Key frame requests
   if (_keyRequestTimer.TimeUntilProcess() == 0) {
     _keyRequestTimer.Processed();
-    CriticalSectionScoped cs(process_crit_sect_.get());
-    if (_scheduleKeyRequest && _frameTypeCallback != NULL) {
+    bool request_key_frame = false;
+    {
+      CriticalSectionScoped cs(process_crit_sect_.get());
+      request_key_frame = _scheduleKeyRequest && _frameTypeCallback != NULL;
+    }
+    if (request_key_frame) {
       const int32_t ret = RequestKeyFrame();
       if (ret != VCM_OK && returnValue == VCM_OK) {
         returnValue = ret;
@@ -135,16 +139,24 @@ int32_t VideoReceiver::Process() {
   // disabled when NACK is off.
   if (_retransmissionTimer.TimeUntilProcess() == 0) {
     _retransmissionTimer.Processed();
-    CriticalSectionScoped cs(process_crit_sect_.get());
-    if (_packetRequestCallback != NULL) {
-      uint16_t length = max_nack_list_size_;
+    bool callback_registered = false;
+    uint16_t length;
+    {
+      CriticalSectionScoped cs(process_crit_sect_.get());
+      length = max_nack_list_size_;
+      callback_registered = _packetRequestCallback != NULL;
+    }
+    if (callback_registered && length > 0) {
       std::vector<uint16_t> nackList(length);
       const int32_t ret = NackList(&nackList[0], &length);
       if (ret != VCM_OK && returnValue == VCM_OK) {
         returnValue = ret;
       }
-      if (length > 0) {
-        _packetRequestCallback->ResendPackets(&nackList[0], length);
+      if (ret == VCM_OK && length > 0) {
+        CriticalSectionScoped cs(process_crit_sect_.get());
+        if (_packetRequestCallback != NULL) {
+          _packetRequestCallback->ResendPackets(&nackList[0], length);
+        }
       }
     }
   }
@@ -452,7 +464,7 @@ int32_t VideoReceiver::RequestSliceLossIndication(
 
 int32_t VideoReceiver::RequestKeyFrame() {
   TRACE_EVENT0("webrtc", "RequestKeyFrame");
-  CriticalSectionScoped cs(process_crit_sect_.get());
+  CriticalSectionScoped process_cs(process_crit_sect_.get());
   if (_frameTypeCallback != NULL) {
     const int32_t ret = _frameTypeCallback->RequestKeyFrame();
     if (ret < 0) {
@@ -547,6 +559,7 @@ int32_t VideoReceiver::Decode(const VCMEncodedFrame& frame) {
   int32_t ret = _decoder->Decode(frame, clock_->TimeInMilliseconds());
 
   // Check for failed decoding, run frame type request callback if needed.
+  bool request_key_frame = false;
   if (ret < 0) {
     if (ret == VCM_ERROR_REQUEST_SLI) {
       return RequestSliceLossIndication(
@@ -557,52 +570,59 @@ int32_t VideoReceiver::Decode(const VCMEncodedFrame& frame) {
                    VCMId(_id),
                    "Failed to decode frame %u, requesting key frame",
                    frame.TimeStamp());
-      ret = RequestKeyFrame();
+      request_key_frame = true;
     }
   } else if (ret == VCM_REQUEST_SLI) {
     ret = RequestSliceLossIndication(
         _decodedFrameCallback.LastReceivedPictureID() + 1);
   }
   if (!frame.Complete() || frame.MissingFrame()) {
-    CriticalSectionScoped cs(process_crit_sect_.get());
     switch (_keyRequestMode) {
       case kKeyOnKeyLoss: {
         if (frame.FrameType() == kVideoFrameKey) {
-          _scheduleKeyRequest = true;
-          return VCM_OK;
+          request_key_frame = true;
+          ret = VCM_OK;
         }
         break;
       }
       case kKeyOnLoss: {
-        _scheduleKeyRequest = true;
-        return VCM_OK;
+        request_key_frame = true;
+        ret = VCM_OK;
       }
       default:
         break;
     }
   }
+  if (request_key_frame) {
+    CriticalSectionScoped cs(process_crit_sect_.get());
+    _scheduleKeyRequest = true;
+  }
   TRACE_EVENT_ASYNC_END0("webrtc", "Video", frame.TimeStamp());
   return ret;
 }
 
 // Reset the decoder state
 int32_t VideoReceiver::ResetDecoder() {
-  CriticalSectionScoped cs(_receiveCritSect);
-  if (_decoder != NULL) {
-    _receiver.Initialize();
-    _timing.Reset();
-    {
-      CriticalSectionScoped cs(process_crit_sect_.get());
-      _scheduleKeyRequest = false;
+  bool reset_key_request = false;
+  {
+    CriticalSectionScoped cs(_receiveCritSect);
+    if (_decoder != NULL) {
+      _receiver.Initialize();
+      _timing.Reset();
+      reset_key_request = true;
+      _decoder->Reset();
+    }
+    if (_dualReceiver.State() != kPassive) {
+      _dualReceiver.Initialize();
+    }
+    if (_dualDecoder != NULL) {
+      _codecDataBase.ReleaseDecoder(_dualDecoder);
+      _dualDecoder = NULL;
     }
-    _decoder->Reset();
   }
-  if (_dualReceiver.State() != kPassive) {
-    _dualReceiver.Initialize();
-  }
-  if (_dualDecoder != NULL) {
-    _codecDataBase.ReleaseDecoder(_dualDecoder);
-    _dualDecoder = NULL;
+  if (reset_key_request) {
+    CriticalSectionScoped cs(process_crit_sect_.get());
+    _scheduleKeyRequest = false;
   }
   return VCM_OK;
 }
@@ -720,7 +740,6 @@ int32_t VideoReceiver::NackList(uint16_t* nackList, uint16_t* size) {
       return VCM_MEMORY;
     }
     case kNackKeyFrameRequest: {
-      CriticalSectionScoped cs(_receiveCritSect);
       WEBRTC_TRACE(webrtc::kTraceWarning,
                    webrtc::kTraceVideoCoding,
                    VCMId(_id),