1212723 - It seems racy to share mBufferedState among WebMReaders

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[JW Wang [:jwwang] [:jw_wang]]

https://hg.mozilla.org/mozilla-central/file/a45d680f073f/dom/media/webm/WebMReader.cpp#l205

It is possible for multiple WebMReaders to reference to the same WebMBufferedState object. The comment says it should be accessed on the main thread only. However, it is actually on the task queue thread. The usage seems racy and wrong.

Comment 1

[Jean-Yves Avenard [:jya]]

we no longer use WebMReader.

We also no longer access WebMReader on any thread other than the decoder's task queue.
How could multiple WebMReaders shared the WebMBufferedState ? I can't see how that could ever be possible.

It could have been accessed on multiple thread (though via a monitor) with the GetBuffered being on the main thread, but that's no long the case.

So at worse, it's an obsolete comment (and we should remove the monitor there), it will benefit the new WebMDemuxer / MediaFormatReader

I'm fairly sure that I had submitted a patch to do just that a little while ago...

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

It is still possible to use WebMReader by setting "media.format-reader.webm" to false until we completely remove WebMReader.cpp/h.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

https://hg.mozilla.org/mozilla-central/file/a45d680f073f/dom/media/webm/WebMReader.cpp#l205

WebMBufferedState is shared when a non-null aCloneDonor passed to WebMReader::Init(). this->mBufferedState and aCloneDonor->mBufferedState will reference the same object.

Comment 4

[Jean-Yves Avenard [:jya]]

sure the code is there to do so, but when is it ever used in that fashion?

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

https://hg.mozilla.org/mozilla-central/file/a45d680f073f/dom/html/HTMLMediaElement.cpp#l2780

It is used when we are cloning a decoder which happens when 2 media element have the same URI for the 'src' attribute. The decoders will try to share resources when possible. (See MediaResource::CloneData and WebMReader::Init()) for examples.)

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

https://hg.mozilla.org/mozilla-central/file/b68eab795f9d/dom/media/webm/WebMReader.cpp#l806

After fixing bug 1213726, we still have WebMReader::NotifyDataArrivedInternal() calls mBufferedState->NotifyDataArrived. Since WebMBufferedState::NotifyDataArrived() is not protected by a monitor, it will be a data race when multiple WebMReaders share a WebMBufferedState.

Since WebMReader is gonna be deprecated, I think it won't hurt much to disable the sharing in WebMReader::Init(). It also allows me to do more code cleanup.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c187059f2f

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e046bc813397

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1212723. Part 1 - don't share mBufferedState per bug 1212723 comment 6. r=jya.

Bug 1212723. Part 1 - don't share mBufferedState per bug 1212723 comment 6. r=jya.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1212723. Part 2 - remove unused argument aCloneDonor from MediaDecoderReader::Init(). r=jya.

Bug 1212723. Part 2 - remove unused argument aCloneDonor from MediaDecoderReader::Init(). r=jya.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8672885 [details]
MozReview Request: Bug 1212723. Part 1 - don't share mBufferedState per bug 1212723 comment 6. r=jya.

https://reviewboard.mozilla.org/r/21813/#review19585

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8672886 [details]
MozReview Request: Bug 1212723. Part 2 - remove unused argument aCloneDonor from MediaDecoderReader::Init(). r=jya.

https://reviewboard.mozilla.org/r/21815/#review19587

starting to wonder what would happen if we shared the src across multiple media element ; do we have only a single MediaResource shared between decoderreader ?

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

No, it is the underlying MediaCache that is shared.
See: https://hg.mozilla.org/mozilla-central/file/11ff0ccb7d59/dom/media/MediaCache.h#l458

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

In fact, we share only when ChannelMediaResource is used. When it comes to MediaSource, local files or RTSP, there is no way to share.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae763ddaac8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e968320d35b

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ae763ddaac8
https://hg.mozilla.org/mozilla-central/rev/7e968320d35b