MSE-in-Workers: Avoid MediaSource::Trace deadlock

Avoids need to take a lock in ::Trace by changing from an Oilpan-managed
|live_seekable_range_| to plain boolean/double members in MediaSource.

BUG=1162613,878133

Change-Id: Ic437b4b6f1143faad98750bd6c1d2a9791907ac4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617351
Auto-Submit: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Will Cassella <cassew@google.com>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842175}

third_party/blink/renderer/modules/mediasource/media_source.cc

@@ -133,7 +133,9 @@ MediaSource::MediaSource(ExecutionContext* context)
       active_source_buffers_(
           MakeGarbageCollected<SourceBufferList>(GetExecutionContext(),
                                                  async_event_queue_.Get())),
-      live_seekable_range_(MakeGarbageCollected<TimeRanges>()) {
+      has_live_seekable_range_(false),
+      live_seekable_range_start_(0.0),
+      live_seekable_range_end_(0.0) {
   DVLOG(1) << __func__ << " this=" << this;
 
   DCHECK(RuntimeEnabledFeatures::MediaSourceInWorkersEnabled() ||
@@ -667,11 +669,13 @@ void MediaSource::AssertAttachmentsMutexHeldIfCrossThreadForDebugging() const {
 
 void MediaSource::Trace(Visitor* visitor) const {
   visitor->Trace(async_event_queue_);
-  {
-    MutexLocker lock(attachment_link_lock_);
-    visitor->Trace(attachment_tracer_);
-    visitor->Trace(live_seekable_range_);
-  }
+
+  // |attachment_tracer_| is only set when this object is owned by the main
+  // thread and is possibly involved in a SameThreadMediaSourceAttachment.
+  // Therefore, it is thread-safe to access it here without taking the
+  // |attachment_link_lock_|.
+  visitor->Trace(TS_UNCHECKED_READ(attachment_tracer_));
+
   visitor->Trace(source_buffers_);
   visitor->Trace(active_source_buffers_);
   EventTargetWithInlineData::Trace(visitor);
@@ -800,29 +804,26 @@ WebTimeRanges MediaSource::SeekableInternal(
 
     {
       // If cross-thread, protect against concurrent usage of
-      // |live_seekable_range_|, since that member is updated without taking the
+      // |*live_seekable_range*|, since those are updated without taking the
       // attachment's internal |attachment_state_lock_|.
       MutexLocker lock(attachment_link_lock_);
 
       // 1. If live seekable range is not empty:
-      if (live_seekable_range_->length() != 0) {
+      if (has_live_seekable_range_) {
         // 1.1. Let union ranges be the union of live seekable range and the
         //      HTMLMediaElement.buffered attribute.
         // 1.2. Return a single range with a start time equal to the
         //      earliest start time in union ranges and an end time equal to
         //      the highest end time in union ranges and abort these steps.
         if (buffered.empty()) {
-          ranges.emplace_back(
-              live_seekable_range_->start(0, ASSERT_NO_EXCEPTION),
-              live_seekable_range_->end(0, ASSERT_NO_EXCEPTION));
+          ranges.emplace_back(live_seekable_range_start_,
+                              live_seekable_range_end_);
           return ranges;
         }
 
         ranges.emplace_back(
-            std::min(live_seekable_range_->start(0, ASSERT_NO_EXCEPTION),
-                     buffered.front().start),
-            std::max(live_seekable_range_->end(0, ASSERT_NO_EXCEPTION),
-                     buffered.back().end));
+            std::min(live_seekable_range_start_, buffered.front().start),
+            std::max(live_seekable_range_end_, buffered.back().end));
         return ranges;
       }
     }
@@ -1089,10 +1090,12 @@ void MediaSource::setLiveSeekableRange(double start,
     // SeekableInternal simultaneously, if attached fully. Here, for simplicity,
     // we don't need to take the full attachment exclusive
     // |attachment_state_lock_| so long as we
-    // fully protect access to |live_seekable_range_| read/write with
+    // fully protect access to |*live_seekable_range*| read/write with
     // |attachment_link_lock_|.
     MutexLocker lock(attachment_link_lock_);
-    live_seekable_range_ = MakeGarbageCollected<TimeRanges>(start, end);
+    has_live_seekable_range_ = true;
+    live_seekable_range_start_ = start;
+    live_seekable_range_end_ = end;
   }
 }
 
@@ -1116,12 +1119,15 @@ void MediaSource::clearLiveSeekableRange(ExceptionState& exception_state) {
     // If we are cross-thread, then main thread could be running
     // SeekableInternal simultaneously, if attached fully. Here, for simplicity,
     // we don't need to take the full attachment exclusive
-    // |attachment_state_lock_|so long as we
-    // fully protect access to |live_seekable_range_| read/write with
+    // |attachment_state_lock_| so long as we
+    // fully protect access to |*live_seekable_range*| read/write with
     // |attachment_link_lock_|.
     MutexLocker lock(attachment_link_lock_);
-    if (live_seekable_range_->length() != 0)
-      live_seekable_range_ = MakeGarbageCollected<TimeRanges>();
+    if (has_live_seekable_range_) {
+      has_live_seekable_range_ = false;
+      live_seekable_range_start_ = 0.0;
+      live_seekable_range_end_ = 0.0;
+    }
   }
 }
 

third_party/blink/renderer/modules/mediasource/media_source.h

@@ -264,7 +264,7 @@ class MediaSource final : public EventTargetWithInlineData,
 
   // |attachment_link_lock_| protects read/write of |media_source_attachment_|,
   // |attachment_tracer_|, |context_already_destroyed_|, and
-  // |live_seekable_range_|.  It is only truly necessary for
+  // |*live_seekable_range*_|.  It is only truly necessary for
   // CrossThreadAttachment usage of worker MSE, to prevent read/write collision
   // on main thread versus worker thread. Note that |attachment_link_lock_| must
   // be released before attempting CrossThreadMediaSourceAttachment
@@ -289,7 +289,11 @@ class MediaSource final : public EventTargetWithInlineData,
   Member<SourceBufferList> source_buffers_;
   Member<SourceBufferList> active_source_buffers_;
 
-  Member<TimeRanges> live_seekable_range_ GUARDED_BY(attachment_link_lock_);
+  // These are kept as raw data (not an Oilpan managed GC-able TimeRange) to
+  // avoid need to take lock during ::Trace, which could lead to deadlock.
+  bool has_live_seekable_range_ GUARDED_BY(attachment_link_lock_);
+  double live_seekable_range_start_ GUARDED_BY(attachment_link_lock_);
+  double live_seekable_range_end_ GUARDED_BY(attachment_link_lock_);
 };
 
 }  // namespace blink