Revert "Add event cache size limit to media devtools"

This reverts commit 79e55e3e.

Reason for revert: Fix failed to land in time.

Original change's description:
> 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}

TBR=dalecurtis@chromium.org,caseq@chromium.org,tkent@chromium.org,tmathmeyer@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1145392
Change-Id: I40b2631948aadcb3643595681b222a5b6805a6dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2547403Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828775}

content/renderer/media/inspector_media_event_handler.cc

@@ -108,7 +108,6 @@ 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,12 +22,6 @@ 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);
@@ -54,11 +48,6 @@ 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,10 +43,7 @@ class MediaInspectorContext {
  public:
   virtual WebString CreatePlayer() = 0;
 
-  virtual void DestroyPlayer(const WebString& playerId) = 0;
-  virtual void IncrementActiveSessionCount() = 0;
-  virtual void DecrementActiveSessionCount() = 0;
-
+  // These methods DCHECK if the player id is invalid.
   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,8 +96,7 @@ void InspectorMediaAgent::RegisterAgent() {
   instrumenting_agents_->AddInspectorMediaAgent(this);
   auto* cache = MediaInspectorContextImpl::From(
       *local_frame_->DomWindow()->GetExecutionContext());
-  Vector<WebString> players = cache->AllPlayerIdsAndMarkSent();
-  cache->IncrementActiveSessionCount();
+  Vector<WebString> players = cache->AllPlayerIds();
   PlayersCreated(players);
   for (const auto& player_id : players) {
     const auto& media_player = cache->MediaPlayerFromId(player_id);
@@ -125,9 +124,6 @@ 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::AllPlayerIdsAndMarkSent() {
+Vector<WebString> MediaInspectorContextImpl::AllPlayerIds() {
   Vector<WebString> existing_players;
-  const auto& keys = players_.Keys();
-  existing_players.AppendRange(keys.begin(), keys.end());
-  unsent_players_.clear();
+  existing_players.ReserveCapacity(players_.size());
+  for (const auto& player_id : players_.Keys())
+    existing_players.push_back(player_id);
   return existing_players;
 }
 
@@ -64,93 +64,21 @@ 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);
-  if (player != players_.end()) {
-    player->value->errors.AppendRange(errors.begin(), errors.end());
-    total_event_count_ += errors.size();
-    if (total_event_count_ > kMaxCachedPlayerEvents)
-      CullPlayers();
-  }
+  DCHECK_NE(player, players_.end());
+  player->value->errors.AppendRange(errors.begin(), errors.end());
 
   Vector<InspectorPlayerError> vector =
       Iter2Vector<InspectorPlayerError>(errors);
@@ -161,12 +89,8 @@ void MediaInspectorContextImpl::NotifyPlayerEvents(
     WebString playerId,
     const InspectorPlayerEvents& events) {
   const auto& player = players_.find(playerId);
-  if (player != players_.end()) {
-    player->value->events.AppendRange(events.begin(), events.end());
-    total_event_count_ += events.size();
-    if (total_event_count_ > kMaxCachedPlayerEvents)
-      CullPlayers();
-  }
+  DCHECK_NE(player, players_.end());
+  player->value->events.AppendRange(events.begin(), events.end());
 
   Vector<InspectorPlayerEvent> vector =
       Iter2Vector<InspectorPlayerEvent>(events);
@@ -177,10 +101,9 @@ void MediaInspectorContextImpl::SetPlayerProperties(
     WebString playerId,
     const InspectorPlayerProperties& props) {
   const auto& player = players_.find(playerId);
-  if (player != players_.end()) {
-    for (const auto& property : props)
-      player->value->properties.insert(property.name, property);
-  }
+  DCHECK_NE(player, players_.end());
+  for (const auto& property : props)
+    player->value->properties.insert(property.name, property);
 
   Vector<InspectorPlayerProperty> vector =
       Iter2Vector<InspectorPlayerProperty>(props);
@@ -191,12 +114,8 @@ void MediaInspectorContextImpl::NotifyPlayerMessages(
     WebString playerId,
     const InspectorPlayerMessages& messages) {
   const auto& player = players_.find(playerId);
-  if (player != players_.end()) {
-    player->value->messages.AppendRange(messages.begin(), messages.end());
-    total_event_count_ += messages.size();
-    if (total_event_count_ > kMaxCachedPlayerEvents)
-      CullPlayers();
-  }
+  DCHECK_NE(player, players_.end());
+  player->value->messages.AppendRange(messages.begin(), messages.end());
 
   Vector<InspectorPlayerMessage> vector =
       Iter2Vector<InspectorPlayerMessage>(messages);

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

@@ -5,7 +5,6 @@
 #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"
@@ -14,13 +13,6 @@
 
 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> {
@@ -46,10 +38,6 @@ 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,
@@ -62,35 +50,11 @@ class CORE_EXPORT MediaInspectorContextImpl final
   // GarbageCollected methods.
   void Trace(Visitor*) const override;
 
-  Vector<WebString> AllPlayerIdsAndMarkSent();
+  Vector<WebString> AllPlayerIds();
   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