1399760 - MediaCacheStream::NotifyDataReceived() might write data to the wrong position when it runs off the main thread

Status: RESOLVED FIXED

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

Description

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

http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/dom/media/MediaCache.cpp#1385,1437

mChannelOffset is updated to a new position before calling CacheClientSeek() at #1437. Since CacheClientSeek() is called outside the lock, it is possible for NotifyDataReceived() to run at the same time and write data to mChannelOffset which is a wrong position for the old channel data.

Comment 1

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

Attachment: Bug 1399760. P1 - remove the CopySegmentToCache() member function.

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

Comment 2

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

Attachment: Bug 1399760. P2 - ensure mCacheStream.NotifyDataStarted() is always called in OnStartRequest().

This keeps us in a good shape that NotifyDataStarted() is always called
before subsequent NotifyDataReceived() calls. This is also required by P3
where we need to set the loadID before NotifyDataReceived().

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

Comment 3

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

Attachment: Bug 1399760. P3 - keep ID of the loading channel so we check whether the data callback is from an old channel.

The load ID works as follows:
1. A load ID is passed to MediaCacheStream::NotifyDataStarted()
   when loading a new channel.
2. Each MediaCacheStream::NotifyDataReceived() call is also associated
   with a load ID from which the data is received.
3. If |mLoadID != aLoadID| tests to be true in NotifyDataReceived(), it means
   the data is from an old channel and should be discarded.
4. MediaCache::Update() reset mLoadID for the stream before calling
   CacheClientSeek() to prevent data from the old channel from being
   stored to the wrong position.

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

Comment 4

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

Attachment: Bug 1399760. P4 - remove Listener::Revoke().

It would be a data race for Listener::OnDataAvailable() to read mResource
off the main thread while Revoke() is happening on the main thread.

We remove Revoke() and apply the same concept of P3 to check if the
loading channels match and continue the call flow.

For OnDataAvailable() which could happen off the main thread, it is not
safe to check mResource->mChannel and continue the call flow. We need
special handling to ensure data from the old channel will not update the
principal or be written to the wrong position (mostly done in P3).

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

Comment 5

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

Comment on attachment 8908535 [details]
Bug 1399760. P1 - remove the CopySegmentToCache() member function.

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

Comment 6

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

Comment on attachment 8908536 [details]
Bug 1399760. P2 - ensure mCacheStream.NotifyDataStarted() is always called in OnStartRequest().

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

Comment 7

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

Comment on attachment 8908537 [details]
Bug 1399760. P3 - keep ID of the loading channel so we check whether the data callback is from an old channel.

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

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8908535 [details]
Bug 1399760. P1 - remove the CopySegmentToCache() member function.

https://reviewboard.mozilla.org/r/180198/#review186326

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8908536 [details]
Bug 1399760. P2 - ensure mCacheStream.NotifyDataStarted() is always called in OnStartRequest().

https://reviewboard.mozilla.org/r/180200/#review186330

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8908537 [details]
Bug 1399760. P3 - keep ID of the loading channel so we check whether the data callback is from an old channel.

https://reviewboard.mozilla.org/r/180202/#review186378

::: dom/media/MediaResource.cpp:485
(Diff revision 2)
> -    nsresult rv = aStream->ReadSegments(CopySegmentToCache, this, count, &read);
> +    nsresult rv = aStream->ReadSegments(
> +      (nsWriteSegmentFun)CopySegmentToCache, &closure, count, &read);

You've added a C-style cast in front of CopySegmentToCache -- Is it necessary? And would it be possible to use a C++ cast instead?

Comment 11

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

Comment on attachment 8908537 [details]
Bug 1399760. P3 - keep ID of the loading channel so we check whether the data callback is from an old channel.

https://reviewboard.mozilla.org/r/180202/#review186880

::: dom/media/MediaResource.cpp:485
(Diff revision 2)
> -    nsresult rv = aStream->ReadSegments(CopySegmentToCache, this, count, &read);
> +    nsresult rv = aStream->ReadSegments(
> +      (nsWriteSegmentFun)CopySegmentToCache, &closure, count, &read);

The signature of nsWriteSegmentFun is
nsresult(*)(nsIInputStream*, void*, const char*, uint32_t, uint32_t, uint32_t*) while CopySegmentToCache has the signature of
nsresult(*)(nsIInputStream*, Closure*, const char*, uint32_t, uint32_t, uint32_t*).

Explicit casting is needed to pass CopySegmentToCache to aStream->ReadSegments().

However, https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type says such casting is undefined for void* is not compatible with Closure*.

I think I should change the signature of CopySegmentToCache to be the same as nsWriteSegmentFun.

Comment 12

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

Comment on attachment 8908537 [details]
Bug 1399760. P3 - keep ID of the loading channel so we check whether the data callback is from an old channel.

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

Comment 13

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

Thanks for the review!

Comment 14

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1b4a443bc32d -d 6b2618acffea: rebasing 421462:1b4a443bc32d "Bug 1399760. P1 - remove the CopySegmentToCache() member function. r=gerald"
merging dom/media/MediaResource.cpp
merging dom/media/MediaResource.h
warning: conflicts while merging dom/media/MediaResource.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/media/MediaResource.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 15

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

Attachment: Bug 1399760. P1 - remove the CopySegmentToCache() member function.

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

Comment 16

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

Attachment: Bug 1399760. P2 - ensure mCacheStream.NotifyDataStarted() is always called in OnStartRequest().

This keeps us in a good shape that NotifyDataStarted() is always called
before subsequent NotifyDataReceived() calls. This is also required by P3
where we need to set the loadID before NotifyDataReceived().

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

Comment 17

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

Attachment: Bug 1399760. P3 - keep ID of the loading channel so we check whether the data callback is from an old channel.

The load ID works as follows:
1. A load ID is passed to MediaCacheStream::NotifyDataStarted()
   when loading a new channel.
2. Each MediaCacheStream::NotifyDataReceived() call is also associated
   with a load ID from which the data is received.
3. If |mLoadID != aLoadID| tests to be true in NotifyDataReceived(), it means
   the data is from an old channel and should be discarded.
4. MediaCache::Update() reset mLoadID for the stream before calling
   CacheClientSeek() to prevent data from the old channel from being
   stored to the wrong position.

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

Comment 18

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

Comment on attachment 8910112 [details]
Bug 1399760. P1 - remove the CopySegmentToCache() member function.

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

Comment 19

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

Comment on attachment 8910113 [details]
Bug 1399760. P2 - ensure mCacheStream.NotifyDataStarted() is always called in OnStartRequest().

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

Comment 20

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

Comment on attachment 8910114 [details]
Bug 1399760. P3 - keep ID of the loading channel so we check whether the data callback is from an old channel.

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

Comment 21

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

It looks like the review history is lost after rebase.

Hi Gerald,
Would you mind r+ing the patches again? Thanks!

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8910112 [details]
Bug 1399760. P1 - remove the CopySegmentToCache() member function.

https://reviewboard.mozilla.org/r/181594/#review186918

Comment 23

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8910113 [details]
Bug 1399760. P2 - ensure mCacheStream.NotifyDataStarted() is always called in OnStartRequest().

https://reviewboard.mozilla.org/r/181596/#review186920

Comment 24

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8910114 [details]
Bug 1399760. P3 - keep ID of the loading channel so we check whether the data callback is from an old channel.

https://reviewboard.mozilla.org/r/181598/#review186922

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/083932453cfc
P1 - remove the CopySegmentToCache() member function. r=gerald
https://hg.mozilla.org/integration/autoland/rev/723726a9d8c3
P2 - ensure mCacheStream.NotifyDataStarted() is always called in OnStartRequest(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/58c5b9d3eab3
P3 - keep ID of the loading channel so we check whether the data callback is from an old channel. r=gerald

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/083932453cfc
https://hg.mozilla.org/mozilla-central/rev/723726a9d8c3
https://hg.mozilla.org/mozilla-central/rev/58c5b9d3eab3