From 159a2fe9da7194492d688f6b46f6e665821ddb8a Mon Sep 17 00:00:00 2001
From: stefan <stefan@webrtc.org>
Date: Mon, 18 Jul 2016 04:14:11 -0700
Subject: [PATCH] Fix crash which happens when there's reordering in the
 beginning of a call.

The added unittest triggers this CHECK:
https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e8122ef34901519a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc#146

It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (greater than last seq num + 2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks.

The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now.

BUG=

Review-Url: https://codereview.webrtc.org/2157843002
Cr-Commit-Position: refs/heads/master@{#13496}
---
 .../remote_estimator_proxy.cc                 | 11 +++++++++
 .../remote_estimator_proxy.h                  |  2 +-
 .../remote_estimator_proxy_unittest.cc        | 24 ++++++++++++++++++-
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
index 524985c490..194ef56b25 100644
--- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
+++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
@@ -89,7 +89,18 @@ void RemoteEstimatorProxy::Process() {
 
 void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number,
                                            int64_t arrival_time) {
+  // TODO(holmer): We should handle a backwards wrap here if the first
+  // sequence number was small and the new sequence number is large. The
+  // SequenceNumberUnwrapper doesn't do this, so we should replace this with
+  // calls to IsNewerSequenceNumber instead.
   int64_t seq = unwrapper_.Unwrap(sequence_number);
+  if (seq > window_start_seq_ + 0xFFFF / 2) {
+    LOG(LS_WARNING) << "Skipping this sequence number (" << sequence_number
+                    << ") since it likely is reordered, but the unwrapper"
+                       "failed to handle it. Feedback window starts at "
+                    << window_start_seq_ << ".";
+    return;
+  }
 
   if (packet_arrival_times_.lower_bound(window_start_seq_) ==
       packet_arrival_times_.end()) {
diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h
index 66373e2977..127886300d 100644
--- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h
+++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h
@@ -54,7 +54,7 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator {
  private:
   void OnPacketArrival(uint16_t sequence_number, int64_t arrival_time)
       EXCLUSIVE_LOCKS_REQUIRED(&lock_);
-  bool BuildFeedbackPacket(rtcp::TransportFeedback* feedback_packetket);
+  bool BuildFeedbackPacket(rtcp::TransportFeedback* feedback_packet);
 
   Clock* const clock_;
   PacketRouter* const packet_router_;
diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
index 62dd2c3956..d999525be0 100644
--- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
+++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
@@ -48,7 +48,7 @@ class RemoteEstimatorProxyTest : public ::testing::Test {
   }
 
   SimulatedClock clock_;
-  MockPacketRouter router_;
+  testing::StrictMock<MockPacketRouter> router_;
   RemoteEstimatorProxy proxy_;
 
   const size_t kDefaultPacketSize = 100;
@@ -225,6 +225,28 @@ TEST_F(RemoteEstimatorProxyTest, SendsFragmentedFeedback) {
   Process();
 }
 
+TEST_F(RemoteEstimatorProxyTest, GracefullyHandlesReorderingAndWrap) {
+  const int64_t kDeltaMs = 1000;
+  const uint16_t kLargeSeq = 62762;
+  IncomingPacket(kBaseSeq, kBaseTimeMs);
+  IncomingPacket(kLargeSeq, kBaseTimeMs + kDeltaMs);
+
+  EXPECT_CALL(router_, SendFeedback(_))
+      .Times(1)
+      .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) {
+        packet->Build();
+        EXPECT_EQ(kBaseSeq, packet->GetBaseSequence());
+        EXPECT_EQ(kMediaSsrc, packet->GetMediaSourceSsrc());
+
+        std::vector<int64_t> delta_vec = packet->GetReceiveDeltasUs();
+        EXPECT_EQ(1u, delta_vec.size());
+        EXPECT_EQ(kBaseTimeMs, (packet->GetBaseTimeUs() + delta_vec[0]) / 1000);
+        return true;
+      }));
+
+  Process();
+}
+
 TEST_F(RemoteEstimatorProxyTest, ResendsTimestampsOnReordering) {
   IncomingPacket(kBaseSeq, kBaseTimeMs);
   IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2);