Guard BatchingMediaLog::event_handlers_ with lock

It seems that despite MediaLog::OnWebMediaPlayerDestroyed and
MediaLog::AddLogRecord both grabbing a lock,
BatchingMediaLog::AddLogRecordLocked can escape the lock handle by
posting BatchingMediaLog::SendQueuedMediaEvents, causing a race.

When the addition of an event is interrupted by the deletion of a player
due to player culling in MediaInspectorContextImpl, a UAF can occur.

R=dalecurtis

Bug: 1295786
Change-Id: I77df94988f806e4d98924669d27860e50455299d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3451494
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Ted (Chromium) Meyer <tmathmeyer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#970815}

content/renderer/media/batching_media_log.cc

@@ -52,9 +52,9 @@ BatchingMediaLog::BatchingMediaLog(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
     std::vector<std::unique_ptr<EventHandler>> event_handlers)
     : task_runner_(std::move(task_runner)),
-      event_handlers_(std::move(event_handlers)),
       tick_clock_(base::DefaultTickClock::GetInstance()),
       last_ipc_send_time_(tick_clock_->NowTicks()),
+      event_handlers_(std::move(event_handlers)),
       ipc_send_pending_(false),
       logged_rate_limit_warning_(false) {
   // Pre-bind the WeakPtr on the right thread since we'll receive calls from
@@ -76,6 +76,7 @@ BatchingMediaLog::~BatchingMediaLog() {
 }
 
 void BatchingMediaLog::OnWebMediaPlayerDestroyedLocked() {
+  base::AutoLock lock(lock_);
   for (const auto& handler : event_handlers_)
     handler->OnWebMediaPlayerDestroyed();
 }
@@ -204,32 +205,30 @@ std::string BatchingMediaLog::MediaEventToMessageString(
 
 void BatchingMediaLog::SendQueuedMediaEvents() {
   DCHECK(task_runner_->BelongsToCurrentThread());
+  base::AutoLock auto_lock(lock_);
 
-  std::vector<media::MediaLogRecord> events_to_send;
-  {
-    base::AutoLock auto_lock(lock_);
-    DCHECK(ipc_send_pending_);
-    ipc_send_pending_ = false;
-
-    if (last_duration_changed_event_) {
-      queued_media_events_.push_back(*last_duration_changed_event_);
-      last_duration_changed_event_.reset();
-    }
+  DCHECK(ipc_send_pending_);
+  ipc_send_pending_ = false;
 
-    if (last_buffering_state_event_) {
-      queued_media_events_.push_back(*last_buffering_state_event_);
-      last_buffering_state_event_.reset();
-    }
+  if (last_duration_changed_event_) {
+    queued_media_events_.push_back(*last_duration_changed_event_);
+    last_duration_changed_event_.reset();
+  }
 
-    queued_media_events_.swap(events_to_send);
-    last_ipc_send_time_ = tick_clock_->NowTicks();
+  if (last_buffering_state_event_) {
+    queued_media_events_.push_back(*last_buffering_state_event_);
+    last_buffering_state_event_.reset();
   }
 
-  if (events_to_send.empty())
+  last_ipc_send_time_ = tick_clock_->NowTicks();
+
+  if (queued_media_events_.empty())
     return;
 
   for (const auto& handler : event_handlers_)
-    handler->SendQueuedMediaEvents(events_to_send);
+    handler->SendQueuedMediaEvents(queued_media_events_);
+
+  queued_media_events_.clear();
 }
 
 void BatchingMediaLog::SetTickClockForTesting(

content/renderer/media/batching_media_log.h

@@ -66,29 +66,31 @@ class CONTENT_EXPORT BatchingMediaLog : public media::MediaLog {
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
-  // impl for sending queued events.
-  std::vector<std::unique_ptr<EventHandler>> event_handlers_;
-
   // |lock_| protects access to all of the following member variables.  It
   // allows any render process thread to AddEvent(), while preserving their
   // sequence for throttled send on |task_runner_| and coherent retrieval by
   // GetErrorMessage().  This is needed in addition to the synchronization
   // guarantees provided by MediaLog, since SendQueuedMediaEvents must also
   // be synchronized with respect to AddEvent.
   mutable base::Lock lock_;
-  const base::TickClock* tick_clock_;
-  base::TimeTicks last_ipc_send_time_;
-  std::vector<media::MediaLogRecord> queued_media_events_;
+  const base::TickClock* tick_clock_ GUARDED_BY(lock_);
+  base::TimeTicks last_ipc_send_time_ GUARDED_BY(lock_);
+  std::vector<media::MediaLogRecord> queued_media_events_ GUARDED_BY(lock_);
+
+  // impl for sending queued events.
+  std::vector<std::unique_ptr<EventHandler>> event_handlers_ GUARDED_BY(lock_);
 
   // For enforcing max 1 pending send.
-  bool ipc_send_pending_;
+  bool ipc_send_pending_ GUARDED_BY(lock_);
 
   // True if we've logged a warning message about exceeding rate limits.
-  bool logged_rate_limit_warning_;
+  bool logged_rate_limit_warning_ GUARDED_BY(lock_);
 
   // Limits the number of events we send over IPC to one.
-  absl::optional<media::MediaLogRecord> last_duration_changed_event_;
-  absl::optional<media::MediaLogRecord> last_buffering_state_event_;
+  absl::optional<media::MediaLogRecord> last_duration_changed_event_
+      GUARDED_BY(lock_);
+  absl::optional<media::MediaLogRecord> last_buffering_state_event_
+      GUARDED_BY(lock_);
 
   // Holds the earliest MEDIA_ERROR_LOG_ENTRY event added to this log. This is
   // most likely to contain the most specific information available describing