Commit 1904e868 authored by miu's avatar miu Committed by Commit bot

[Cast] Sanity-check for unbounded queue growth in PacedSender.

Removes the spammy warning message, and replaces it with a sanity-
check that the queue has not grown past 10 seconds' worth of
packets.  If the sanity-check trips, a process crash dump will be
sent to make us aware, via crash reports, that this is a problem
in-the-wild.

Review URL: https://codereview.chromium.org/539323002

Cr-Commit-Position: refs/heads/master@{#294928}
parent a8248c9e
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
#include "base/big_endian.h" #include "base/big_endian.h"
#include "base/bind.h" #include "base/bind.h"
#include "base/debug/dump_without_crashing.h"
#include "base/message_loop/message_loop.h" #include "base/message_loop/message_loop.h"
#include "media/cast/logging/logging_impl.h" #include "media/cast/logging/logging_impl.h"
...@@ -20,6 +21,12 @@ static const int64 kPacingIntervalMs = 10; ...@@ -20,6 +21,12 @@ static const int64 kPacingIntervalMs = 10;
static const size_t kPacingMaxBurstsPerFrame = 3; static const size_t kPacingMaxBurstsPerFrame = 3;
static const size_t kMaxDedupeWindowMs = 500; static const size_t kMaxDedupeWindowMs = 500;
// "Impossible" upper-bound on the maximum number of packets that should ever be
// enqueued in the pacer. This is used to detect bugs, reported as crash dumps.
static const size_t kHugeQueueLengthSeconds = 10;
static const size_t kRidiculousNumberOfPackets =
kHugeQueueLengthSeconds * (kMaxBurstSize * 1000 / kPacingIntervalMs);
} // namespace } // namespace
DedupInfo::DedupInfo() : last_byte_acked_for_audio(0) {} DedupInfo::DedupInfo() : last_byte_acked_for_audio(0) {}
...@@ -54,6 +61,7 @@ PacedSender::PacedSender( ...@@ -54,6 +61,7 @@ PacedSender::PacedSender(
next_next_max_burst_size_(target_burst_size_), next_next_max_burst_size_(target_burst_size_),
current_burst_size_(0), current_burst_size_(0),
state_(State_Unblocked), state_(State_Unblocked),
has_reached_upper_bound_once_(false),
weak_factory_(this) { weak_factory_(this) {
} }
...@@ -222,6 +230,15 @@ void PacedSender::SendStoredPackets() { ...@@ -222,6 +230,15 @@ void PacedSender::SendStoredPackets() {
return; return;
} }
// If the queue ever becomes impossibly long, send a crash dump without
// actually crashing the process.
if (size() > kRidiculousNumberOfPackets && !has_reached_upper_bound_once_) {
NOTREACHED();
// Please use Cr=Internals-Cast label in bug reports:
base::debug::DumpWithoutCrashing();
has_reached_upper_bound_once_ = true;
}
base::TimeTicks now = clock_->NowTicks(); base::TimeTicks now = clock_->NowTicks();
// I don't actually trust that PostDelayTask(x - now) will mean that // I don't actually trust that PostDelayTask(x - now) will mean that
// now >= x when the call happens, so check if the previous state was // now >= x when the call happens, so check if the previous state was
...@@ -244,14 +261,6 @@ void PacedSender::SendStoredPackets() { ...@@ -244,14 +261,6 @@ void PacedSender::SendStoredPackets() {
size_t max_burst_size = std::min( size_t max_burst_size = std::min(
max_burst_size_, max_burst_size_,
std::max(target_burst_size_, size() / kPacingMaxBurstsPerFrame)); std::max(target_burst_size_, size() / kPacingMaxBurstsPerFrame));
// If the queue is long, issue a warning. Try to limit the number of
// warnings issued by only issuing the warning when the burst size
// grows. Otherwise we might get 100 warnings per second.
if (max_burst_size > next_next_max_burst_size_ && size() > 100) {
LOG(WARNING) << "Packet queue is very long:" << size();
}
current_max_burst_size_ = std::max(next_max_burst_size_, max_burst_size); current_max_burst_size_ = std::max(next_max_burst_size_, max_burst_size);
next_max_burst_size_ = std::max(next_next_max_burst_size_, max_burst_size); next_max_burst_size_ = std::max(next_next_max_burst_size_, max_burst_size);
next_next_max_burst_size_ = max_burst_size; next_next_max_burst_size_ = max_burst_size;
......
...@@ -205,6 +205,8 @@ class PacedSender : public PacedPacketSender, ...@@ -205,6 +205,8 @@ class PacedSender : public PacedPacketSender,
State state_; State state_;
bool has_reached_upper_bound_once_;
// NOTE: Weak pointers must be invalidated before all other member variables. // NOTE: Weak pointers must be invalidated before all other member variables.
base::WeakPtrFactory<PacedSender> weak_factory_; base::WeakPtrFactory<PacedSender> weak_factory_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment