1398711 - Data race in accessing mClosed in MediaCacheStream::Seek()

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

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

http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/dom/media/MediaCache.cpp#2115

mClosed is updated on the main thread without holding the lock. It is a data race to read mClosed off the main thread in Seek().

http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/dom/media/MediaCache.cpp#2286

Comment 1

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

Attachment: Bug 1398711. P1 - remove unused methods.

Review commit: https://reviewboard.mozilla.org/r/179068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179068/

Comment 2

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

Attachment: Bug 1398711. P2 - write to mClosed only when the cache monitor is held.

This fixes the data race when Seek() read mClosed off the main thread.

Review commit: https://reviewboard.mozilla.org/r/179070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179070/

Comment 3

[Gerald Squelart (he/him)]

Comment on attachment 8907384 [details]
Bug 1398711. P1 - remove unused methods.

https://reviewboard.mozilla.org/r/179068/#review184166

Comment 4

[Gerald Squelart (he/him)]

Comment on attachment 8907385 [details]
Bug 1398711. P2 - write to mClosed only when the cache monitor is held.

https://reviewboard.mozilla.org/r/179070/#review184168

::: dom/media/MediaCache.h:225
(Diff revision 1)
>    void SetTransportSeekable(bool aIsTransportSeekable);
>    // This must be called (and return) before the ChannelMediaResource
>    // used to create this MediaCacheStream is deleted.
>    void Close();
>    // This returns true when the stream has been closed
>    bool IsClosed() const { return mClosed; }

AFAICS IsClosed() is only called from ChannelMediaResource::IsClosed(), which you nuked in P1, so you could get rid of this one too; that's one fewer instance of mClosed to worry about.

::: dom/media/MediaCache.h:227
(Diff revision 1)
>    bool IsAvailableForSharing() const
>    {
>      return !mClosed && !mIsPrivateBrowsing &&

What about this access?

Comment 5

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

Comment on attachment 8907385 [details]
Bug 1398711. P2 - write to mClosed only when the cache monitor is held.

https://reviewboard.mozilla.org/r/179070/#review184168

> AFAICS IsClosed() is only called from ChannelMediaResource::IsClosed(), which you nuked in P1, so you could get rid of this one too; that's one fewer instance of mClosed to worry about.

It is used here: http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/dom/media/MediaCache.cpp#243

It is safe for ResourceStreamIterator to call IsClosed() only after P2 is landed.

> What about this access?

IsAvailableForSharing() is called only on the main thread. I will add a main-thread assertion so it is easier to read and reason about the code.

Comment 6

[Gerald Squelart (he/him)]

Comment on attachment 8907385 [details]
Bug 1398711. P2 - write to mClosed only when the cache monitor is held.

https://reviewboard.mozilla.org/r/179070/#review184168

> It is used here: http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/dom/media/MediaCache.cpp#243
> 
> It is safe for ResourceStreamIterator to call IsClosed() only after P2 is landed.

Ah you're right, sorry -- Searchfox didn't find it!

> IsAvailableForSharing() is called only on the main thread. I will add a main-thread assertion so it is easier to read and reason about the code.

Thanks.

Comment 7

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

Thanks for the review!

Comment 8

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

Comment on attachment 8907384 [details]
Bug 1398711. P1 - remove unused methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179068/diff/1-2/

Comment 9

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

Comment on attachment 8907385 [details]
Bug 1398711. P2 - write to mClosed only when the cache monitor is held.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179070/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84abfad4ada9
P1 - remove unused methods. r=gerald
https://hg.mozilla.org/integration/autoland/rev/b420bb292e68
P2 - write to mClosed only when the cache monitor is held. r=gerald

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/84abfad4ada9
https://hg.mozilla.org/mozilla-central/rev/b420bb292e68