MSE: Implement SourceBuffer.trackDefaults

MSE: Implement SourceBuffer.trackDefaults

This change implements the SourceBuffer.trackDefaults attribute, which
will be required for compliant SourceBuffer initialization segment
received algorithm implementation and related track management.

BUG=249428
TEST=http/tests/media/media-source/mediasource-sourcebuffer-trackdefaults.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187300

[wolenetz, 2014-12-03 23:15:44]

wolenetz@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[wolenetz, 2014-12-03 23:15:45]

Please take a look at patch set 1:
philipj: everything, OWNERS review.
oilpan-reviews: SourceBuffer.* changes.

Thanks!

(Note: This CL needs https://codereview.chromium.org/702583002 to land first: it
needs TrackDefaultList implemented first.)

[wolenetz, 2014-12-03 23:51:43]

On 2014/12/03 23:15:45, wolenetz wrote:
> Please take a look at patch set 1:
> philipj: everything, OWNERS review.
> oilpan-reviews: SourceBuffer.* changes.
> 
> Thanks!
> 
> (Note: This CL needs https://codereview.chromium.org/702583002 to land first:
it
> needs TrackDefaultList implemented first.)

note: the bot failures are due to the prerequisite CL has not yet landed :/

[sof, 2014-12-04 10:31:29]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-12-04 10:31:30]

oilpan lgtm

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

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-12-04 15:33:10]

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:86: ,
m_trackDefaults(TrackDefaultList::create(HeapVector<Member<TrackDefault>>(),
ASSERT_NO_EXCEPTION))

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]

LGTM, some changes will be needed, but if I'm not responsive on the second round
I trust you'll have done the right thing :)

https://codereview.chromium.org/754463008/diff/1/LayoutTests/http/tests/media...
File
LayoutTests/http/tests/media/media-source/mediasource-sourcebuffer-trackdefaults.html
(right):

https://codereview.chromium.org/754463008/diff/1/LayoutTests/http/tests/media...
LayoutTests/http/tests/media/media-source/mediasource-sourcebuffer-trackdefaults.html:19:
assert_true(sourceBuffer != null, "SourceBuffer returned");
I see there's quite a bit of this in the other tests, but assert_not_equals
would be slightly better. Or nothing, since if it's null an exception will
follow on the next line.

https://codereview.chromium.org/754463008/diff/1/LayoutTests/http/tests/media...
LayoutTests/http/tests/media/media-source/mediasource-sourcebuffer-trackdefaults.html:28:
assert_false(sourceBuffer.trackDefaults == emptyList, "Initial trackDefaults
object differs from new empty list");
assert_not_equals will give better error messages.

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:86: ,
m_trackDefaults(TrackDefaultList::create(HeapVector<Member<TrackDefault>>(),
ASSERT_NO_EXCEPTION))
On 2014/12/04 15:33:09, haraken wrote:
> 
> 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.

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]

Please take a look at patch set 2. Especially, confirm there is indeed no UAF
(or am I misunderstanding something?)

Thanks!

https://codereview.chromium.org/754463008/diff/1/LayoutTests/http/tests/media...
File
LayoutTests/http/tests/media/media-source/mediasource-sourcebuffer-trackdefaults.html
(right):

https://codereview.chromium.org/754463008/diff/1/LayoutTests/http/tests/media...
LayoutTests/http/tests/media/media-source/mediasource-sourcebuffer-trackdefaults.html:19:
assert_true(sourceBuffer != null, "SourceBuffer returned");
On 2014/12/04 16:17:53, philipj wrote:
> I see there's quite a bit of this in the other tests, but assert_not_equals
> would be slightly better. Or nothing, since if it's null an exception will
> follow on the next line.

Done.

https://codereview.chromium.org/754463008/diff/1/LayoutTests/http/tests/media...
LayoutTests/http/tests/media/media-source/mediasource-sourcebuffer-trackdefaults.html:28:
assert_false(sourceBuffer.trackDefaults == emptyList, "Initial trackDefaults
object differs from new empty list");
On 2014/12/04 16:17:53, philipj wrote:
> assert_not_equals will give better error messages.

Done.

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:86: ,
m_trackDefaults(TrackDefaultList::create(HeapVector<Member<TrackDefault>>(),
ASSERT_NO_EXCEPTION))
On 2014/12/04 16:17:53, philipj wrote:
> On 2014/12/04 15:33:09, haraken wrote:
> > 
> > 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.
> 
> 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.

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.)

[wolenetz, 2014-12-12 22:42:06]

On 2014/12/12 22:15:06, wolenetz wrote:
..
> 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.)
I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=27599 to track
allowing nullable SourceBuffer.trackDefaults, though as I noted in the bug, such
a change might break existing apps.

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

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:86: ,
m_trackDefaults(TrackDefaultList::create(HeapVector<Member<TrackDefault>>(),
ASSERT_NO_EXCEPTION))
On 2014/12/12 22:15:06, wolenetz wrote:
> On 2014/12/04 16:17:53, philipj wrote:
> > On 2014/12/04 15:33:09, haraken wrote:
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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 19:36:35]

haraken: Friendly ping :)

[wolenetz, 2014-12-15 23:27:49]

On 2014/12/15 19:36:35, wolenetz wrote:
> haraken: Friendly ping :)

Per sof's comment on the prereq CL at
https://codereview.chromium.org/702583002/diff/120001/Source/modules/mediasou...,
I'll be updating TrackDefaultList to have another ::create() method that
simplifies the default initial "empty" trackDefaults_ creation in this CL. I'll
update both CLs shortly with this simplification.

[wolenetz, 2014-12-15 23:49:21]

Please take a look at patch set 3.
haraken, sof: especially.
philipj: fyi

Thanks!

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/754463008/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:86: ,
m_trackDefaults(TrackDefaultList::create(HeapVector<Member<TrackDefault>>(),
ASSERT_NO_EXCEPTION))
On 2014/12/15 09:02:38, philipj wrote:
> On 2014/12/12 22:15:06, wolenetz wrote:
> > On 2014/12/04 16:17:53, philipj wrote:
> > > On 2014/12/04 15:33:09, haraken wrote:
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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?

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|.

[haraken, 2014-12-16 01:16:16]

LGTM with a comment.

https://codereview.chromium.org/754463008/diff/40001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/754463008/diff/40001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:86: ,
m_trackDefaults(TrackDefaultList::create())

Do we need to create an empty vector? Can we just use nullptr?

[sof, 2014-12-16 07:24:36]

lgtm

https://codereview.chromium.org/754463008/diff/40001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/754463008/diff/40001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:86: ,
m_trackDefaults(TrackDefaultList::create())
On 2014/12/16 01:16:16, haraken wrote:
> 
> Do we need to create an empty vector? Can we just use nullptr?

Don't think the IDL allows for that in practice, an empty TrackListDefault
should be returned.

[philipj_slow, 2014-12-16 09:55:48]

https://codereview.chromium.org/754463008/diff/40001/Source/modules/mediasour...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/754463008/diff/40001/Source/modules/mediasour...
Source/modules/mediasource/SourceBuffer.cpp:86: ,
m_trackDefaults(TrackDefaultList::create())
On 2014/12/16 07:24:36, sof wrote:
> On 2014/12/16 01:16:16, haraken wrote:
> > 
> > Do we need to create an empty vector? Can we just use nullptr?
> 
> Don't think the IDL allows for that in practice, an empty TrackListDefault
> should be returned.

Right, that is the topic of the spec bug Matthew filed:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27599

[wolenetz, 2014-12-16 21:19:19]

On 2014/12/16 09:55:48, philipj wrote:
>
https://codereview.chromium.org/754463008/diff/40001/Source/modules/mediasour...
> File Source/modules/mediasource/SourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/754463008/diff/40001/Source/modules/mediasour...
> Source/modules/mediasource/SourceBuffer.cpp:86: ,
> m_trackDefaults(TrackDefaultList::create())
> On 2014/12/16 07:24:36, sof wrote:
> > On 2014/12/16 01:16:16, haraken wrote:
> > > 
> > > Do we need to create an empty vector? Can we just use nullptr?
> > 
> > Don't think the IDL allows for that in practice, an empty TrackListDefault
> > should be returned.
> 
> Right, that is the topic of the spec bug Matthew filed:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=27599

Correct, that w3 bug may change the interface, but I suspect it won't be to
allow nullable, but rather add a no-argument TrackDefaultList constructor that
implicitly creates an empty TrackDefaultList.

Thank you all for reviews.
The prerequisite CL (https://codereview.chromium.org/702583002) is in CQ
currently. Once it lands, I'll send this CL to CQ.

[wolenetz, 2014-12-16 22:10:03]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-12-16 22:10:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/754463008/40001

[commit-bot: I haz the power, 2014-12-16 23:31:30]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187300