Add event cache size limit to media devtools

This adds a cache for both total number of players as well as total
number of events kept by the each individual player, in order to prevent
long-living pages from creating an otherwise unbounded number of events
and creating memory pressure.

There is some logic for selecting which players to prune, but no logic
to do anything special for events yet.

Events, Messages, and Errors are all subject to the same cache size
limits, but the Properties can't grow unbounded so they aren't.

R=dalecurtis

Bug: 1145392
Change-Id: Iab39207276b6179b9f8828df7ced210154ab97dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2517928
Commit-Queue: Ted Meyer <tmathmeyer@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827070}

content/renderer/media/inspector_media_event_handler.cc

@@ -108,6 +108,7 @@ void InspectorMediaEventHandler::SendQueuedMediaEvents(
 
 void InspectorMediaEventHandler::OnWebMediaPlayerDestroyed() {
   video_player_destroyed_ = true;
+  inspector_context_->DestroyPlayer(player_id_);
 }
 
 }  // namespace content

content/renderer/media/inspector_media_event_handler_unittest.cc

@@ -22,6 +22,12 @@ class MockMediaInspectorContext : public blink::MediaInspectorContext {
 
   blink::WebString CreatePlayer() override { return "TestPlayer"; }
 
+  void DestroyPlayer(const blink::WebString& playerId) override {
+    MockDestroyPlayer();
+  }
+  void IncrementActiveSessionCount() override { active_players_++; }
+  void DecrementActiveSessionCount() override { active_players_--; }
+
   void NotifyPlayerEvents(blink::WebString id,
                           const blink::InspectorPlayerEvents& events) override {
     MockNotifyPlayerEvents(events);
@@ -48,6 +54,11 @@ class MockMediaInspectorContext : public blink::MediaInspectorContext {
   MOCK_METHOD1(MockSetPlayerProperties, void(blink::InspectorPlayerProperties));
   MOCK_METHOD1(MockNotifyPlayerErrors, void(blink::InspectorPlayerErrors));
   MOCK_METHOD1(MockNotifyPlayerMessages, void(blink::InspectorPlayerMessages));
+  MOCK_METHOD0(MockDestroyPlayer, void());
+  MOCK_METHOD0(MockIncrementActiveSessionCount, void());
+  MOCK_METHOD0(MockDecrementActiveSessionCount, void());
+
+  size_t active_players_ = 0;
 };
 
 class InspectorMediaEventHandlerTest : public testing::Test {

third_party/blink/public/web/web_media_inspector.h

@@ -43,7 +43,10 @@ class MediaInspectorContext {
  public:
   virtual WebString CreatePlayer() = 0;
 
-  // These methods DCHECK if the player id is invalid.
+  virtual void DestroyPlayer(const WebString& playerId) = 0;
+  virtual void IncrementActiveSessionCount() = 0;
+  virtual void DecrementActiveSessionCount() = 0;
+
   virtual void NotifyPlayerEvents(WebString player_id,
                                   const InspectorPlayerEvents&) = 0;
   virtual void NotifyPlayerErrors(WebString player_id,

third_party/blink/renderer/core/inspector/inspector_media_agent.cc

@@ -96,7 +96,8 @@ void InspectorMediaAgent::RegisterAgent() {
   instrumenting_agents_->AddInspectorMediaAgent(this);
   auto* cache = MediaInspectorContextImpl::From(
       *local_frame_->DomWindow()->GetExecutionContext());
-  Vector<WebString> players = cache->AllPlayerIds();
+  Vector<WebString> players = cache->AllPlayerIdsAndMarkSent();
+  cache->IncrementActiveSessionCount();
   PlayersCreated(players);
   for (const auto& player_id : players) {
     const auto& media_player = cache->MediaPlayerFromId(player_id);
@@ -124,6 +125,9 @@ protocol::Response InspectorMediaAgent::disable() {
     return protocol::Response::Success();
   enabled_.Clear();
   instrumenting_agents_->RemoveInspectorMediaAgent(this);
+  auto* cache = MediaInspectorContextImpl::From(
+      *local_frame_->DomWindow()->GetExecutionContext());
+  cache->DecrementActiveSessionCount();
   return protocol::Response::Success();
 }
 

third_party/blink/renderer/core/inspector/inspector_media_context_impl.cc

@@ -49,11 +49,11 @@ void MediaInspectorContextImpl::Trace(Visitor* visitor) const {
   visitor->Trace(players_);
 }
 
-Vector<WebString> MediaInspectorContextImpl::AllPlayerIds() {
+Vector<WebString> MediaInspectorContextImpl::AllPlayerIdsAndMarkSent() {
   Vector<WebString> existing_players;
-  existing_players.ReserveCapacity(players_.size());
-  for (const auto& player_id : players_.Keys())
-    existing_players.push_back(player_id);
+  const auto& keys = players_.Keys();
+  existing_players.AppendRange(keys.begin(), keys.end());
+  unsent_players_.clear();
   return existing_players;
 }
 
@@ -64,21 +64,93 @@ const MediaPlayer& MediaInspectorContextImpl::MediaPlayerFromId(
   return *player->value;
 }
 
+bool MediaInspectorContextImpl::IsConnected() {
+  return !!active_session_count_;
+}
+
+void MediaInspectorContextImpl::IncrementActiveSessionCount() {
+  active_session_count_++;
+  DCHECK_GT(active_session_count_, 0lu);
+}
+
+void MediaInspectorContextImpl::DecrementActiveSessionCount() {
+  active_session_count_--;
+}
+
 WebString MediaInspectorContextImpl::CreatePlayer() {
   String next_player_id =
       String::FromUTF8(base::UnguessableToken::Create().ToString());
   players_.insert(next_player_id, MakeGarbageCollected<MediaPlayer>());
   probe::PlayersCreated(GetSupplementable(), {next_player_id});
+  if (!IsConnected())
+    unsent_players_.push_back(next_player_id);
   return next_player_id;
 }
 
+void MediaInspectorContextImpl::RemovePlayer(WebString playerId) {
+  const auto& player = players_.find(playerId);
+  DCHECK(player != players_.end());
+  total_event_count_ -=
+      (player->value->errors.size() + player->value->events.size() +
+       player->value->messages.size());
+  players_.erase(playerId);
+}
+
+void MediaInspectorContextImpl::CullPlayers() {
+  // Erase all the dead players, but only erase the required number of others.
+  for (const auto& playerId : dead_players_)
+    RemovePlayer(playerId);
+  dead_players_.clear();
+
+  if (total_event_count_ <= kMaxCachedPlayerEvents)
+    return;
+
+  for (const auto& playerId : expendable_players_) {
+    if (total_event_count_ <= kMaxCachedPlayerEvents)
+      return;
+    RemovePlayer(playerId);
+    expendable_players_.EraseAt(expendable_players_.Find(playerId));
+  }
+
+  for (const auto& playerId : unsent_players_) {
+    if (total_event_count_ <= kMaxCachedPlayerEvents)
+      return;
+    RemovePlayer(playerId);
+    unsent_players_.EraseAt(unsent_players_.Find(playerId));
+  }
+
+  // As a last resort, we'll just remove the first player.
+  // TODO(tmathmeyer) keep last event time stamps for players to remove the
+  // most stale one.
+  for (const auto& playerId : players_.Keys()) {
+    if (total_event_count_ <= kMaxCachedPlayerEvents)
+      return;
+    RemovePlayer(playerId);
+  }
+}
+
+void MediaInspectorContextImpl::DestroyPlayer(const WebString& playerId) {
+  if (unsent_players_.Contains(String(playerId))) {
+    // unsent players become dead when destroyed.
+    unsent_players_.EraseAt(unsent_players_.Find(String(playerId)));
+    dead_players_.push_back(playerId);
+    players_.erase(playerId);
+  } else {
+    expendable_players_.push_back(playerId);
+  }
+}
+
 // Convert public version of event to protocol version, and send it.
 void MediaInspectorContextImpl::NotifyPlayerErrors(
     WebString playerId,
     const InspectorPlayerErrors& errors) {
   const auto& player = players_.find(playerId);
-  DCHECK_NE(player, players_.end());
-  player->value->errors.AppendRange(errors.begin(), errors.end());
+  if (player != players_.end()) {
+    player->value->errors.AppendRange(errors.begin(), errors.end());
+    total_event_count_ += errors.size();
+    if (total_event_count_ > kMaxCachedPlayerEvents)
+      CullPlayers();
+  }
 
   Vector<InspectorPlayerError> vector =
       Iter2Vector<InspectorPlayerError>(errors);
@@ -89,8 +161,12 @@ void MediaInspectorContextImpl::NotifyPlayerEvents(
     WebString playerId,
     const InspectorPlayerEvents& events) {
   const auto& player = players_.find(playerId);
-  DCHECK_NE(player, players_.end());
-  player->value->events.AppendRange(events.begin(), events.end());
+  if (player != players_.end()) {
+    player->value->events.AppendRange(events.begin(), events.end());
+    total_event_count_ += events.size();
+    if (total_event_count_ > kMaxCachedPlayerEvents)
+      CullPlayers();
+  }
 
   Vector<InspectorPlayerEvent> vector =
       Iter2Vector<InspectorPlayerEvent>(events);
@@ -101,9 +177,10 @@ void MediaInspectorContextImpl::SetPlayerProperties(
     WebString playerId,
     const InspectorPlayerProperties& props) {
   const auto& player = players_.find(playerId);
-  DCHECK_NE(player, players_.end());
-  for (const auto& property : props)
-    player->value->properties.insert(property.name, property);
+  if (player != players_.end()) {
+    for (const auto& property : props)
+      player->value->properties.insert(property.name, property);
+  }
 
   Vector<InspectorPlayerProperty> vector =
       Iter2Vector<InspectorPlayerProperty>(props);
@@ -114,8 +191,12 @@ void MediaInspectorContextImpl::NotifyPlayerMessages(
     WebString playerId,
     const InspectorPlayerMessages& messages) {
   const auto& player = players_.find(playerId);
-  DCHECK_NE(player, players_.end());
-  player->value->messages.AppendRange(messages.begin(), messages.end());
+  if (player != players_.end()) {
+    player->value->messages.AppendRange(messages.begin(), messages.end());
+    total_event_count_ += messages.size();
+    if (total_event_count_ > kMaxCachedPlayerEvents)
+      CullPlayers();
+  }
 
   Vector<InspectorPlayerMessage> vector =
       Iter2Vector<InspectorPlayerMessage>(messages);

third_party/blink/renderer/core/inspector/inspector_media_context_impl.h

@@ -5,6 +5,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_INSPECTOR_INSPECTOR_MEDIA_CONTEXT_IMPL_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_INSPECTOR_INSPECTOR_MEDIA_CONTEXT_IMPL_H_
 
+#include "build/build_config.h"
 #include "third_party/blink/public/web/web_media_inspector.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/platform/supplementable.h"
@@ -13,6 +14,13 @@
 
 namespace blink {
 
+#if defined(OS_ANDROID)
+// Players are cached per tab.
+constexpr size_t kMaxCachedPlayerEvents = 128;
+#else
+constexpr size_t kMaxCachedPlayerEvents = 512;
+#endif
+
 class ExecutionContext;
 
 struct MediaPlayer final : public GarbageCollected<MediaPlayer> {
@@ -38,6 +46,10 @@ class CORE_EXPORT MediaInspectorContextImpl final
 
   // MediaInspectorContext methods.
   WebString CreatePlayer() override;
+  void DestroyPlayer(const WebString& playerId) override;
+  void IncrementActiveSessionCount() override;
+  void DecrementActiveSessionCount() override;
+
   void NotifyPlayerErrors(WebString playerId,
                           const InspectorPlayerErrors&) override;
   void NotifyPlayerEvents(WebString playerId,
@@ -50,11 +62,35 @@ class CORE_EXPORT MediaInspectorContextImpl final
   // GarbageCollected methods.
   void Trace(Visitor*) const override;
 
-  Vector<WebString> AllPlayerIds();
+  Vector<WebString> AllPlayerIdsAndMarkSent();
   const MediaPlayer& MediaPlayerFromId(const WebString&);
 
  private:
+  // When a player is added, its ID is stored in |unsent_players_| if no
+  // connections are open. When an unsent player is destroyed, its ID is moved
+  // to |dead_players_| and is first to be deleted if there is memory pressure.
+  // If it has already been sent when it is destroyed, it gets moved to
+  // |expendable_players_|, which is the second group of players to be deleted
+  // on memory pressure.
+
+  // If there are no dead or expendable players when it's time to start removing
+  // players, then a player from |unsent_players_| will be removed. As a last
+  // resort, remaining unended, already-sent players will be removed from
+  // |players_| until the total event size is within the limit.
+
+  // All events will be sent to any open clients regardless of players existing
+  // because the clients can handle dead players and may have their own cache.
+  void CullPlayers();
+  bool IsConnected();
+  void RemovePlayer(WebString playerId);
+
   HeapHashMap<String, Member<MediaPlayer>> players_;
+  Vector<String> unsent_players_;
+  Vector<String> dead_players_;
+  Vector<String> expendable_players_;
+
+  size_t total_event_count_ = 0;
+  size_t active_session_count_ = 0;
 };
 
 }  // namespace blink