1394724 - Data race in ChannelMediaResource when HTTP OMT data delivery is enabled

Status: RESOLVED FIXED

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

Description

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

http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/dom/media/MediaResource.cpp#154
It is a data race to read mResource off the main thread while Listener::Revoke() [1] is happening on the main thread.

[1] http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/dom/media/MediaResource.h#520

Comment 1

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

Attachment: Bug 1394724. P1 - assert the channel is already closed in ~ChannelMediaResource().

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

Comment 2

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

Attachment: Bug 1394724. P2 - mListener->Revoke() should happen after mChannel->Cancel() to avoid data race.

http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/netwerk/base/nsIRequest.idl#56
All OnDataAvailable() calls should happen before Cancel().
It is safe to modify mResource after a call to Cancel().

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

Comment 3

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

Attachment: Bug 1394724. P3 - fix data race in reading mClosed in MediaCacheStream::NotifyDataReceived().

1. mCacheStream.Close() should happen after CloseChannel() to avoid data race
   in reading mClosed in MediaCacheStream::NotifyDataReceived().
2. MediaCache::Update() and CloseStreamsForPrivateBrowsing() should call
   ChannelMediaResource::Close() to ensure mCacheStream.Close() happens
   after CloseChannel().

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

Comment 4

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

Attachment: Bug 1394724. P4 - merge MediaCacheStream::Close() and CloseInternal().

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

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8902548 [details]
Bug 1394724. P1 - assert the channel is already closed in ~ChannelMediaResource().

https://reviewboard.mozilla.org/r/174152/#review179720

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8902549 [details]
Bug 1394724. P2 - mListener->Revoke() should happen after mChannel->Cancel() to avoid data race.

https://reviewboard.mozilla.org/r/174154/#review179704

::: commit-message-b2d67:1
(Diff revision 1)
> +Bug 1394724. P2 - mListener->Revoke() should happen after mChannel->Caancel() to avoid data race.

s/Caancel/Cancel/

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8902550 [details]
Bug 1394724. P3 - fix data race in reading mClosed in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/174156/#review179722

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8902551 [details]
Bug 1394724. P4 - merge MediaCacheStream::Close() and CloseInternal().

https://reviewboard.mozilla.org/r/174158/#review179726

Comment 9

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

Thanks for the review!

Comment 10

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

Comment on attachment 8902549 [details]
Bug 1394724. P2 - mListener->Revoke() should happen after mChannel->Cancel() to avoid data race.

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

Comment 11

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

Comment on attachment 8902550 [details]
Bug 1394724. P3 - fix data race in reading mClosed in MediaCacheStream::NotifyDataReceived().

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

Comment 12

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

Comment on attachment 8902551 [details]
Bug 1394724. P4 - merge MediaCacheStream::Close() and CloseInternal().

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a81e995c2a6a
P1 - assert the channel is already closed in ~ChannelMediaResource(). r=cpearce
https://hg.mozilla.org/integration/autoland/rev/4e9acbce2e29
P2 - mListener->Revoke() should happen after mChannel->Cancel() to avoid data race. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/a91707ac4a3a
P3 - fix data race in reading mClosed in MediaCacheStream::NotifyDataReceived(). r=cpearce
https://hg.mozilla.org/integration/autoland/rev/87c1afc748cc
P4 - merge MediaCacheStream::Close() and CloseInternal(). r=cpearce

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/a81e995c2a6a
https://hg.mozilla.org/mozilla-central/rev/4e9acbce2e29
https://hg.mozilla.org/mozilla-central/rev/a91707ac4a3a
https://hg.mozilla.org/mozilla-central/rev/87c1afc748cc