Revert "Add ChildMediaLog."

This reverts commit 86ff51d4.

Reason for revert: crbug.com/871587

Original change's description:
> Add ChildMediaLog.
> 
> Users of MediaLog have to be careful about the lifetime of the
> object, since it's passed around as a raw ptr.  For things like
> VdaVideoDecoder, which operate on multiple threads, it requires
> special care that the log isn't used after VdaVideoDecoder is
> no longer allowed to use it.
> 
> ChildMediaLog wraps a MediaLog, and provides an atomic way to
> prevent access to the underlying MediaLog.  For example,
> VdaVideoDecoder can do this when it's Destroy()ed, which is when it
> must stop using the MediaLog that it was given.  Additional calls
> to ChildMediaLog will do nothing.
> 
> The ChildMediaLog itself can be kept around until the other thread
> is definitely done using it.
> 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
> Change-Id: If350b2b78da96bf7ba844f582a96661fd0b15ef0
> Reviewed-on: https://chromium-review.googlesource.com/1139219
> Commit-Queue: Frank Liberato <liberato@chromium.org>
> Reviewed-by: Dan Sanders <sandersd@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#580952}

TBR=sandersd@chromium.org,liberato@chromium.org

Change-Id: Ic7457f610907c8e7d89fd26090b815d281b19664
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1164130Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581109}

content/renderer/media/render_media_log.cc

@@ -57,9 +57,6 @@ RenderMediaLog::RenderMediaLog(
 
 RenderMediaLog::~RenderMediaLog() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  // AddEvent() could be in-flight on some other thread.  Wait for it, and make
-  // sure that nobody else calls it.
-  InvalidateLog();
 
   // There's no further chance to handle this, so send them now. This should not
   // be racy since nothing should have a pointer to the media log on another
@@ -68,8 +65,7 @@ RenderMediaLog::~RenderMediaLog() {
     SendQueuedMediaEvents();
 }
 
-void RenderMediaLog::AddEventLocked(
-    std::unique_ptr<media::MediaLogEvent> event) {
+void RenderMediaLog::AddEvent(std::unique_ptr<media::MediaLogEvent> event) {
   Log(event.get());
 
   // For enforcing delay until it's been a second since the last ipc message was
@@ -78,6 +74,7 @@ void RenderMediaLog::AddEventLocked(
 
   {
     base::AutoLock auto_lock(lock_);
+
     switch (event->type) {
       case media::MediaLogEvent::DURATION_SET:
         // Similar to the extents changed message, this may fire many times for
@@ -128,9 +125,12 @@ void RenderMediaLog::AddEventLocked(
       base::BindOnce(&RenderMediaLog::SendQueuedMediaEvents, weak_this_));
 }
 
-std::string RenderMediaLog::GetErrorMessageLocked() {
+std::string RenderMediaLog::GetErrorMessage() {
+  base::AutoLock auto_lock(lock_);
+
   // Keep message structure in sync with
   // HTMLMediaElement::BuildElementErrorMessage().
+
   std::stringstream result;
   if (last_pipeline_error_)
     result << MediaEventToMessageString(*last_pipeline_error_);
@@ -147,10 +147,8 @@ std::string RenderMediaLog::GetErrorMessageLocked() {
   return result.str();
 }
 
-void RenderMediaLog::RecordRapporWithSecurityOriginLocked(
-    const std::string& metric) {
+void RenderMediaLog::RecordRapporWithSecurityOrigin(const std::string& metric) {
   if (!task_runner_->BelongsToCurrentThread()) {
-    // Note that we don't post back to *Locked.
     task_runner_->PostTask(
         FROM_HERE,
         base::BindOnce(&RenderMediaLog::RecordRapporWithSecurityOrigin,
@@ -167,6 +165,7 @@ void RenderMediaLog::SendQueuedMediaEvents() {
   std::vector<media::MediaLogEvent> events_to_send;
   {
     base::AutoLock auto_lock(lock_);
+
     DCHECK(ipc_send_pending_);
     ipc_send_pending_ = false;
 

content/renderer/media/render_media_log.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/lock.h"
 #include "base/time/time.h"
 #include "content/common/content_export.h"
 #include "media/base/media_log.h"
@@ -38,17 +39,16 @@ class CONTENT_EXPORT RenderMediaLog : public media::MediaLog {
                  scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   ~RenderMediaLog() override;
 
+  // MediaLog implementation.
+  void AddEvent(std::unique_ptr<media::MediaLogEvent> event) override;
+  std::string GetErrorMessage() override;
+  void RecordRapporWithSecurityOrigin(const std::string& metric) override;
+
   // Will reset |last_ipc_send_time_| with the value of NowTicks().
   void SetTickClockForTesting(const base::TickClock* tick_clock);
   void SetTaskRunnerForTesting(
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
 
- protected:
-  // MediaLog implementation.
-  void AddEventLocked(std::unique_ptr<media::MediaLogEvent> event) override;
-  std::string GetErrorMessageLocked() override;
-  void RecordRapporWithSecurityOriginLocked(const std::string& metric) override;
-
  private:
   // Posted as a delayed task on |task_runner_| to throttle ipc message
   // frequency.
@@ -62,9 +62,7 @@ class CONTENT_EXPORT RenderMediaLog : public media::MediaLog {
   // |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.
+  // GetErrorMessage().
   mutable base::Lock lock_;
   const base::TickClock* tick_clock_;
   base::TimeTicks last_ipc_send_time_;

media/base/media_log.cc

@@ -8,7 +8,6 @@
 
 #include "base/atomic_sequence_num.h"
 #include "base/json/json_writer.h"
-#include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 
@@ -169,58 +168,17 @@ std::string MediaLog::BufferingStateToString(BufferingState state) {
   return "";
 }
 
-MediaLog::MediaLog() : MediaLog(new ParentLogRecord(this)) {}
-
-MediaLog::MediaLog(scoped_refptr<ParentLogRecord> parent_log_record)
-    : parent_log_record_(std::move(parent_log_record)),
-      id_(g_media_log_count.GetNext()) {}
-
-MediaLog::~MediaLog() {
-  // If we are the underlying log, then somebody should have called
-  // InvalidateLog before now.  Otherwise, there could be concurrent calls into
-  // this after we're destroyed.  Note that calling it here isn't really much
-  // better, since there could be concurrent calls into the now destroyed
-  // derived class.
-  //
-  // However, we can't DCHECK on it, since lots of folks create a base Medialog
-  // implementation temporarily.  So, the best we can do is invalidate the log.
-  // We could get around this if we introduce a new NullMediaLog that handles
-  // log invalidation, so we could dcheck here.  However, that seems like a lot
-  // of boilerplate.
-  if (parent_log_record_->media_log == this)
-    InvalidateLog();
-}
+MediaLog::MediaLog() : id_(g_media_log_count.GetNext()) {}
 
-void MediaLog::AddEvent(std::unique_ptr<MediaLogEvent> event) {
-  base::AutoLock auto_lock(lock());
-  // Forward to the parent log's implementation.
-  if (parent_log_record_->media_log)
-    parent_log_record_->media_log->AddEventLocked(std::move(event));
-}
+MediaLog::~MediaLog() = default;
 
-void MediaLog::AddEventLocked(std::unique_ptr<MediaLogEvent> event) {}
+void MediaLog::AddEvent(std::unique_ptr<MediaLogEvent> event) {}
 
 std::string MediaLog::GetErrorMessage() {
-  base::AutoLock auto_lock(lock());
-  // Forward to the parent log's implementation.
-  if (parent_log_record_->media_log)
-    return parent_log_record_->media_log->GetErrorMessageLocked();
-
-  return "";
-}
-
-std::string MediaLog::GetErrorMessageLocked() {
   return "";
 }
 
 void MediaLog::RecordRapporWithSecurityOrigin(const std::string& metric) {
-  base::AutoLock auto_lock(lock());
-  // Forward to the parent log's implementation.
-  if (parent_log_record_->media_log)
-    parent_log_record_->media_log->RecordRapporWithSecurityOriginLocked(metric);
-}
-
-void MediaLog::RecordRapporWithSecurityOriginLocked(const std::string& metric) {
   DVLOG(1) << "Default MediaLog doesn't support rappor reporting.";
 }
 
@@ -348,11 +306,6 @@ void MediaLog::SetBooleanProperty(
   AddEvent(std::move(event));
 }
 
-std::unique_ptr<MediaLog> MediaLog::Clone() {
-  // Protected ctor, so we can't use std::make_unique.
-  return base::WrapUnique(new MediaLog(parent_log_record_));
-}
-
 // static
 std::string MediaLog::TruncateUrlString(std::string log_string) {
   if (log_string.length() > kMaxUrlLength) {
@@ -366,28 +319,11 @@ std::string MediaLog::TruncateUrlString(std::string log_string) {
   return log_string;
 }
 
-MediaLog::ParentLogRecord::ParentLogRecord(MediaLog* log) : media_log(log) {}
-MediaLog::ParentLogRecord::~ParentLogRecord() = default;
-
-void MediaLog::InvalidateLog() {
-  base::AutoLock auto_lock(lock());
-  // It's almost certainly unintentional to invalidate a parent log.
-  DCHECK(parent_log_record_->media_log == nullptr ||
-         parent_log_record_->media_log == this);
-
-  parent_log_record_->media_log = nullptr;
-  // Keep |parent_log_record_| around, since the lock must keep working.
-}
-
 LogHelper::LogHelper(MediaLog::MediaLogLevel level, MediaLog* media_log)
     : level_(level), media_log_(media_log) {
   DCHECK(media_log_);
 }
 
-LogHelper::LogHelper(MediaLog::MediaLogLevel level,
-                     const std::unique_ptr<MediaLog>& media_log)
-    : LogHelper(level, media_log.get()) {}
-
 LogHelper::~LogHelper() {
   media_log_->AddLogEvent(level_, stream_.str());
 }

media/base/media_log.h

@@ -14,7 +14,6 @@
 
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
 #include "media/base/buffering_state.h"
 #include "media/base/media_export.h"
 #include "media/base/media_log_event.h"
@@ -26,14 +25,8 @@ namespace media {
 
 // Interface for media components to log to chrome://media-internals log.
 //
-// To provide a logging implementation, derive from MediaLog instead.
-//
-// Implementations only need to implement AddEventLocked(), which must be thread
-// safe in the sense that it may be called from multiple threads, though it will
-// not be called concurrently.  See below for more details.
-//
-// Implementations should also call InvalidateLog during destruction, to signal
-// to any child logs that the underlying log is no longer available.
+// Implementations only need to implement AddEvent(), which must be thread-safe.
+// AddEvent() is expected to be called from multiple threads.
 class MEDIA_EXPORT MediaLog {
  public:
   enum MediaLogLevel {
@@ -76,23 +69,21 @@ class MEDIA_EXPORT MediaLog {
   MediaLog();
   virtual ~MediaLog();
 
-  // Add an event to this log.  Inheritors should override AddEventLocked to
-  // do something.
-  void AddEvent(std::unique_ptr<MediaLogEvent> event);
+  // Add an event to this log. Overriden by inheritors to actually do something
+  // with it.
+  virtual void AddEvent(std::unique_ptr<MediaLogEvent> event);
 
   // Returns a string usable as the contents of a MediaError.message.
   // This method returns an incomplete message if it is called before the
   // pertinent events for the error have been added to the log.
   // Note: The base class definition only produces empty messages. See
   // RenderMediaLog for where this method is meaningful.
-  // Inheritors should override GetErrorMessageLocked().
-  std::string GetErrorMessage();
+  virtual std::string GetErrorMessage();
 
   // Records the domain and registry of the current frame security origin to a
   // Rappor privacy-preserving metric. See:
   //   https://www.chromium.org/developers/design-documents/rappor
-  // Inheritors should override RecordRapportWithSecurityOriginLocked().
-  void RecordRapporWithSecurityOrigin(const std::string& metric);
+  virtual void RecordRapporWithSecurityOrigin(const std::string& metric);
 
   // Helper methods to create events and their parameters.
   std::unique_ptr<MediaLogEvent> CreateEvent(MediaLogEvent::Type type);
@@ -131,59 +122,8 @@ class MEDIA_EXPORT MediaLog {
   // event with a specific media playback.
   int32_t id() const { return id_; }
 
-  // Provide a MediaLog which can have a separate lifetime from this one, but
-  // still write to the same log.  It is not guaranteed that this will log
-  // forever; it might start silently discarding log messages if the original
-  // log is closed by whoever owns it.
-  virtual std::unique_ptr<MediaLog> Clone();
-
- protected:
-  // Methods that may be overridden by inheritors.  All calls may arrive on any
-  // thread, but will be synchronized with respect to any other *Locked calls on
-  // any other thread, and with any parent log invalidation.
-  //
-  // Please see the documentation for the corresponding public methods.
-  virtual void AddEventLocked(std::unique_ptr<MediaLogEvent> event);
-  virtual std::string GetErrorMessageLocked();
-  virtual void RecordRapporWithSecurityOriginLocked(const std::string& metric);
-
-  // Notify all child logs that they should stop working.  This should be called
-  // to guarantee that no further calls into AddEvent should be allowed.
-  // Further, since calls into this log may happen on any thread, it's important
-  // to call this while the log is still in working order.  For example, calling
-  // it immediately during destruction is a good idea.
-  void InvalidateLog();
-
-  struct ParentLogRecord : base::RefCountedThreadSafe<ParentLogRecord> {
-    ParentLogRecord(MediaLog* log);
-
-    // |lock_| protects the rest of this structure.
-    base::Lock lock;
-
-    // Original media log, or null.
-    MediaLog* media_log = nullptr;
-
-   protected:
-    friend class base::RefCountedThreadSafe<ParentLogRecord>;
-    virtual ~ParentLogRecord();
-
-    DISALLOW_COPY_AND_ASSIGN(ParentLogRecord);
-  };
-
-  // Use |parent_log_record| instead of making a new one.
-  MediaLog(scoped_refptr<ParentLogRecord> parent_log_record);
-
  private:
-  // Return a lock that will be taken during InvalidateLog on the parent log,
-  // and before calls to the *Locked methods.
-  base::Lock& lock() { return parent_log_record_->lock; }
-
-  // The underlying media log.
-  scoped_refptr<ParentLogRecord> parent_log_record_;
-
   friend class MediaLogTest;
-  FRIEND_TEST_ALL_PREFIXES(MediaLogTest, EventsAreForwarded);
-  FRIEND_TEST_ALL_PREFIXES(MediaLogTest, EventsAreNotForwardedAfterInvalidate);
 
   enum : size_t {
     // Max length of URLs in Created/Load events. Exceeding triggers truncation.
@@ -197,6 +137,7 @@ class MEDIA_EXPORT MediaLog {
 
   // A unique (to this process) id for this MediaLog.
   int32_t id_;
+
   DISALLOW_COPY_AND_ASSIGN(MediaLog);
 };
 
@@ -204,8 +145,6 @@ class MEDIA_EXPORT MediaLog {
 class MEDIA_EXPORT LogHelper {
  public:
   LogHelper(MediaLog::MediaLogLevel level, MediaLog* media_log);
-  LogHelper(MediaLog::MediaLogLevel level,
-            const std::unique_ptr<MediaLog>& media_log);
   ~LogHelper();
 
   std::ostream& stream() { return stream_; }

media/base/media_log_unittest.cc

@@ -7,11 +7,8 @@
 
 #include "base/macros.h"
 #include "media/base/media_log.h"
-#include "media/base/mock_media_log.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using testing::_;
-
 namespace media {
 
 // Friend class of MediaLog for access to internal constants.
@@ -79,22 +76,4 @@ TEST_F(MediaLogTest, TruncateLongUrlStrings) {
             0);
 }
 
-TEST_F(MediaLogTest, EventsAreForwarded) {
-  // Make sure that |root_log_| receives events.
+}  // namespace media
\ No newline at end of file
-  std::unique_ptr<MockMediaLog> root_log(std::make_unique<MockMediaLog>());
-  std::unique_ptr<MediaLog> child_media_log(root_log->Clone());
-  EXPECT_CALL(*root_log, DoAddEventLogString(_)).Times(1);
-  child_media_log->AddLogEvent(MediaLog::MediaLogLevel::MEDIALOG_ERROR, "test");
-}
-
-TEST_F(MediaLogTest, EventsAreNotForwardedAfterInvalidate) {
-  // Make sure that |root_log_| doesn't forward things after we invalidate the
-  // underlying log.
-  std::unique_ptr<MockMediaLog> root_log(std::make_unique<MockMediaLog>());
-  std::unique_ptr<MediaLog> child_media_log(root_log->Clone());
-  EXPECT_CALL(*root_log, DoAddEventLogString(_)).Times(0);
-  root_log.reset();
-  child_media_log->AddLogEvent(MediaLog::MediaLogLevel::MEDIALOG_ERROR, "test");
-}
-
-}  // namespace media

media/base/mock_media_log.h

@@ -43,7 +43,7 @@ class MockMediaLog : public MediaLog {
   // Trampoline method to workaround GMOCK problems with std::unique_ptr<>.
   // Also simplifies tests to be able to string match on the log string
   // representation on the added event.
-  void AddEventLocked(std::unique_ptr<MediaLogEvent> event) override {
+  void AddEvent(std::unique_ptr<MediaLogEvent> event) override {
     DoAddEventLogString(MediaEventToLogString(*event));
   }
 

media/formats/mp4/mp4_box_reader_fuzzer.cc

@@ -17,7 +17,7 @@ class NullMediaLog : public media::MediaLog {
   NullMediaLog() = default;
   ~NullMediaLog() override = default;
 
-  void AddEventLocked(std::unique_ptr<media::MediaLogEvent> event) override {}
+  void AddEvent(std::unique_ptr<media::MediaLogEvent> event) override {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NullMediaLog);

media/gpu/ipc/service/vda_video_decoder.cc

@@ -104,7 +104,7 @@ std::unique_ptr<VdaVideoDecoder, std::default_delete<VideoDecoder>>
 VdaVideoDecoder::Create(
     scoped_refptr<base::SingleThreadTaskRunner> parent_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
-    std::unique_ptr<MediaLog> media_log,
+    MediaLog* media_log,
     const gfx::ColorSpace& target_color_space,
     const gpu::GpuPreferences& gpu_preferences,
     const gpu::GpuDriverBugWorkarounds& gpu_workarounds,
@@ -114,9 +114,8 @@ VdaVideoDecoder::Create(
   // TODO(sandersd): Extend base::WrapUnique() to handle this.
   std::unique_ptr<VdaVideoDecoder, std::default_delete<VideoDecoder>> ptr(
       new VdaVideoDecoder(
-          std::move(parent_task_runner), std::move(gpu_task_runner),
-          std::move(media_log), target_color_space,
-          base::BindOnce(&PictureBufferManager::Create),
+          std::move(parent_task_runner), std::move(gpu_task_runner), media_log,
+          target_color_space, base::BindOnce(&PictureBufferManager::Create),
           base::BindOnce(&CreateCommandBufferHelper, std::move(get_stub_cb)),
           base::BindOnce(&CreateAndInitializeVda, gpu_preferences,
                          gpu_workarounds),
@@ -129,7 +128,7 @@ VdaVideoDecoder::Create(
 VdaVideoDecoder::VdaVideoDecoder(
     scoped_refptr<base::SingleThreadTaskRunner> parent_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
-    std::unique_ptr<MediaLog> media_log,
+    MediaLog* media_log,
     const gfx::ColorSpace& target_color_space,
     CreatePictureBufferManagerCB create_picture_buffer_manager_cb,
     CreateCommandBufferHelperCB create_command_buffer_helper_cb,
@@ -137,7 +136,7 @@ VdaVideoDecoder::VdaVideoDecoder(
     const VideoDecodeAccelerator::Capabilities& vda_capabilities)
     : parent_task_runner_(std::move(parent_task_runner)),
       gpu_task_runner_(std::move(gpu_task_runner)),
-      media_log_(std::move(media_log)),
+      media_log_(media_log),
       target_color_space_(target_color_space),
       create_command_buffer_helper_cb_(
           std::move(create_command_buffer_helper_cb)),
@@ -185,7 +184,6 @@ void VdaVideoDecoder::DestroyOnGpuThread() {
   // don't call back into |vda_| during its destruction.
   gpu_weak_vda_factory_ = nullptr;
   vda_ = nullptr;
-  media_log_ = nullptr;
 
   delete this;
 }
@@ -310,7 +308,7 @@ void VdaVideoDecoder::InitializeOnGpuThread() {
 
   // Create and initialize the VDA.
   vda_ = std::move(create_and_initialize_vda_cb_)
-             .Run(command_buffer_helper, this, media_log_.get(), vda_config);
+             .Run(command_buffer_helper, this, this, vda_config);
   if (!vda_) {
     parent_task_runner_->PostTask(
         FROM_HERE, base::BindOnce(&VdaVideoDecoder::InitializeDone,
@@ -690,6 +688,31 @@ void VdaVideoDecoder::ReusePictureBuffer(int32_t picture_buffer_id) {
   vda_->ReusePictureBuffer(picture_buffer_id);
 }
 
+void VdaVideoDecoder::AddEvent(std::unique_ptr<MediaLogEvent> event) {
+  DVLOG(1) << __func__;
+
+  if (parent_task_runner_->BelongsToCurrentThread()) {
+    if (!parent_weak_this_)
+      return;
+    AddEventOnParentThread(std::move(event));
+    return;
+  }
+
+  // Hop to the parent thread to be sure we don't call into |media_log_| after
+  // Destroy() returns.
+  parent_task_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&VdaVideoDecoder::AddEventOnParentThread,
+                                parent_weak_this_, std::move(event)));
+}
+
+void VdaVideoDecoder::AddEventOnParentThread(
+    std::unique_ptr<MediaLogEvent> event) {
+  DVLOG(1) << __func__;
+  DCHECK(parent_task_runner_->BelongsToCurrentThread());
+
+  media_log_->AddEvent(std::move(event));
+}
+
 void VdaVideoDecoder::EnterErrorState() {
   DVLOG(1) << __func__;
   DCHECK(parent_task_runner_->BelongsToCurrentThread());

media/gpu/ipc/service/vda_video_decoder.h

@@ -38,7 +38,8 @@ namespace media {
 // Implements the VideoDecoder interface backed by a VideoDecodeAccelerator.
 // This class expects to run in the GPU process via MojoVideoDecoder.
 class VdaVideoDecoder : public VideoDecoder,
-                        public VideoDecodeAccelerator::Client {
+                        public VideoDecodeAccelerator::Client,
+                        public MediaLog {
  public:
   using GetStubCB = base::RepeatingCallback<gpu::CommandBufferStub*()>;
   using CreatePictureBufferManagerCB =
@@ -68,7 +69,7 @@ class VdaVideoDecoder : public VideoDecoder,
   static std::unique_ptr<VdaVideoDecoder, std::default_delete<VideoDecoder>>
   Create(scoped_refptr<base::SingleThreadTaskRunner> parent_task_runner,
          scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
-         std::unique_ptr<MediaLog> media_log,
+         MediaLog* media_log,
          const gfx::ColorSpace& target_color_space,
          const gpu::GpuPreferences& gpu_preferences,
          const gpu::GpuDriverBugWorkarounds& gpu_workarounds,
@@ -79,7 +80,8 @@ class VdaVideoDecoder : public VideoDecoder,
   //     MediaService task runner).
   // |gpu_task_runner|: Task runner that GPU command buffer methods must be
   //     called on (should be the GPU main thread).
-  // |media_log|: MediaLog object to log to.
+  // |media_log|: MediaLog object to log to; must live at least until
+  //     Destroy() returns.
   // |target_color_space|: Color space of the output device.
   // |create_picture_buffer_manager_cb|: PictureBufferManager factory.
   // |create_command_buffer_helper_cb|: CommandBufferHelper factory.
@@ -89,7 +91,7 @@ class VdaVideoDecoder : public VideoDecoder,
   VdaVideoDecoder(
       scoped_refptr<base::SingleThreadTaskRunner> parent_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner,
-      std::unique_ptr<MediaLog> media_log,
+      MediaLog* media_log,
       const gfx::ColorSpace& target_color_space,
       CreatePictureBufferManagerCB create_picture_buffer_manager_cb,
       CreateCommandBufferHelperCB create_command_buffer_helper_cb,
@@ -112,6 +114,9 @@ class VdaVideoDecoder : public VideoDecoder,
   bool CanReadWithoutStalling() const override;
   int GetMaxDecodeRequests() const override;
 
+  // media::MediaLog implementation.
+  void AddEvent(std::unique_ptr<MediaLogEvent> event) override;
+
  private:
   void Destroy() override;
 
@@ -164,7 +169,7 @@ class VdaVideoDecoder : public VideoDecoder,
   //
   scoped_refptr<base::SingleThreadTaskRunner> parent_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner_;
-  std::unique_ptr<MediaLog> media_log_;
+  MediaLog* media_log_;
   gfx::ColorSpace target_color_space_;
   scoped_refptr<PictureBufferManager> picture_buffer_manager_;
   CreateCommandBufferHelperCB create_command_buffer_helper_cb_;

media/gpu/ipc/service/vda_video_decoder_unittest.cc

@@ -110,7 +110,7 @@ class VdaVideoDecoderTest : public testing::Test {
     EXPECT_CALL(*vda_, Destroy());
 
     vdavd_.reset(new VdaVideoDecoder(
-        task_runner, task_runner, media_log_.Clone(), gfx::ColorSpace(),
+        task_runner, task_runner, &media_log_, gfx::ColorSpace(),
         base::BindOnce(&VdaVideoDecoderTest::CreatePictureBufferManager,
                        base::Unretained(this)),
         base::BindOnce(&VdaVideoDecoderTest::CreateCommandBufferHelper,

media/mojo/services/gpu_mojo_media_client.cc

@@ -127,7 +127,7 @@ std::unique_ptr<VideoDecoder> GpuMojoMediaClient::CreateVideoDecoder(
   }
 #endif  // defined(OS_WIN)
   return VdaVideoDecoder::Create(
-      task_runner, gpu_task_runner_, media_log->Clone(), target_color_space,
+      task_runner, gpu_task_runner_, media_log, target_color_space,
       gpu_preferences_, gpu_workarounds_,
       base::BindRepeating(&GetCommandBufferStub, media_gpu_channel_manager_,
                           command_buffer_id->channel_token,

media/mojo/services/mojo_media_log.cc

@@ -20,13 +20,9 @@ MojoMediaLog::MojoMediaLog(mojom::MediaLogAssociatedPtrInfo remote_media_log,
 
 MojoMediaLog::~MojoMediaLog() {
   DVLOG(1) << __func__;
-  // Note that we're not invalidating the remote side.  We're only invalidating
-  // anything that was cloned from us.  Effectively, we're a log that just
-  // happens to operate via mojo.
-  InvalidateLog();
 }
 
-void MojoMediaLog::AddEventLocked(std::unique_ptr<MediaLogEvent> event) {
+void MojoMediaLog::AddEvent(std::unique_ptr<MediaLogEvent> event) {
   DVLOG(1) << __func__;
   DCHECK(event);
 
@@ -41,7 +37,12 @@ void MojoMediaLog::AddEventLocked(std::unique_ptr<MediaLogEvent> event) {
     return;
   }
 
-  // From other threads, we have little choice.
+  // From other threads, it's okay to post without worrying about losing a
+  // message.  This is because any message that's causally related to the object
+  // (and thus MediaLog) being destroyed hopefully posts the result back to the
+  // same sequence as |task_runner_| after we do.  Of course, async destruction
+  // (e.g., the renderer destroys a MojoVideoDecoder) can still lose messages,
+  // but that's really a race.
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&MojoMediaLog::AddEvent, weak_this_, std::move(event)));

media/mojo/services/mojo_media_log.h

@@ -16,7 +16,6 @@
 
 namespace media {
 
-// Client side for a MediaLog via mojo.
 class MojoMediaLog final : public MediaLog {
  public:
   // TODO(sandersd): Template on Ptr type to support non-associated.
@@ -24,10 +23,9 @@ class MojoMediaLog final : public MediaLog {
                         scoped_refptr<base::SequencedTaskRunner> task_runner);
   ~MojoMediaLog() final;
 
- protected:
   // MediaLog implementation.  May be called from any thread, but will only
   // use |remote_media_log_| on |task_runner_|.
-  void AddEventLocked(std::unique_ptr<MediaLogEvent> event) override;
+  void AddEvent(std::unique_ptr<MediaLogEvent> event) override;
 
  private:
   mojom::MediaLogAssociatedPtr remote_media_log_;