MSE: Implement SourceBuffer.trackDefaults

Source/modules/mediasource/SourceBuffer.cpp

Index: Source/modules/mediasource/SourceBuffer.cpp
diff --git a/Source/modules/mediasource/SourceBuffer.cpp b/Source/modules/mediasource/SourceBuffer.cpp
index ec9727e5d46cc6943af3caa095d8e6a7fbf6c6a7..073e55ed5f350a7c872eb75d44824efae15f7e4a 100644
--- a/Source/modules/mediasource/SourceBuffer.cpp
+++ b/Source/modules/mediasource/SourceBuffer.cpp
@@ -83,6 +83,7 @@ SourceBuffer::SourceBuffer(PassOwnPtr<WebSourceBuffer> webSourceBuffer, MediaSou
     : ActiveDOMObject(source->executionContext())
     , m_webSourceBuffer(webSourceBuffer)
     , m_source(source)
+    , m_trackDefaults(TrackDefaultList::create(HeapVector<Member<TrackDefault>>(), ASSERT_NO_EXCEPTION))

[haraken, 2014/12/04 15:33:09]

This looks unsafe to me. The HeapVector is created on stack and thus destructed
at the end of the SourceBuffer's constructor. However, TrackDefaultList keeps
referencing the HeapVector, which will cause use-after-free.

I'd propose to change the parameter type to 'HeapVector<Member<TrackDefault>>*'
and pass 'new HeapVector<Member<TrackDefault>>()' here.

[philipj_slow, 2014/12/04 16:17:53]

Alternatively, change the spec to let trackDefaults be nullable, since an empty
list by default isn't very useful. It would also make clearing the track default
by setting them to null more natural, as opposed to setting them to an empty
list.

[wolenetz, 2014/12/12 22:15:06]

UAF/unsafety: TrackDefaultList::create passes through the const-ref arg to its
ctor, which makes a copy during ctor initializer. Therefore, while the original
vector is indeed on stack, m_trackDefaults internally has a copy of the empty
original HeapVector contents.

nullable/spec change: I'll file a w3c bug, but do not want to block landing this
on getting spec changed to allow null here (along with spec language to skip the
default {language,label,kinds} algorithms if this attribute is null). Such
change will not disallow passing an empty list (as is currently required at
minimum, and implemented here as the initial value of this attribute.)

[philipj_slow, 2014/12/15 09:02:38]

AFAICT, this is safe, the TrackDefaultList constructor takes a const
HeapVector<Member<TrackDefault>>& but the m_trackDefaults member is a const
HeapVector<Member<TrackDefault>>, so the HeapVector copy constructor will be
invoked, which (without stepping) I think ends up in
TypeOperations::uninitializedCopy(other.begin(), other.end(), begin());

haraken, is there something subtle going on here specific to HeapVector?

[wolenetz, 2014/12/15 23:49:20]

With patch set 3, this becomes academic: I'm still curious if there really was
UAF/unsafety in PS1-2, but PS3 now uses a simpler way to initialize
|m_trackDefaults|.

     , m_asyncEventQueue(asyncEventQueue)
     , m_mode(segmentsKeyword())
     , m_updating(false)
@@ -366,6 +367,22 @@ void SourceBuffer::remove(double start, double end, ExceptionState& exceptionSta
     m_removeAsyncPartRunner.runAsync();
 }
 
+void SourceBuffer::setTrackDefaults(TrackDefaultList* trackDefaults, ExceptionState& exceptionState)
+{
+    // Per 02 Dec 2014 Editor's Draft
+    // http://w3c.github.io/media-source/#widl-SourceBuffer-trackDefaults
+    // 1. If this object has been removed from the sourceBuffers attribute of
+    //    the parent media source, then throw an InvalidStateError exception
+    //    and abort these steps.
+    // 2. If the updating attribute equals true, then throw an InvalidStateError
+    //    exception and abort these steps.
+    if (throwExceptionIfRemovedOrUpdating(isRemoved(), m_updating, exceptionState))
+        return;
+
+    // 3. Update the attribute to the new value.
+    m_trackDefaults = trackDefaults;
+}
+
 void SourceBuffer::abortIfUpdating()
 {
     // Section 3.2 abort() method step 3 substeps.
@@ -746,6 +763,7 @@ void SourceBuffer::trace(Visitor* visitor)
 {
     visitor->trace(m_source);
     visitor->trace(m_stream);
+    visitor->trace(m_trackDefaults);
     EventTargetWithInlineData::trace(visitor);
 }