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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

Assignee: nobody → jwwang
Blocks: 1382978
Priority: -- → P3
Attachment #8907384 - Flags: review?(gsquelart)
Attachment #8907385 - Flags: review?(gsquelart)
Comment on attachment 8907384 [details]
Bug 1398711. P1 - remove unused methods.

https://reviewboard.mozilla.org/r/179068/#review184166
Attachment #8907384 - Flags: review?(gsquelart) → review+
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?
Attachment #8907385 - Flags: review?(gsquelart) → review+
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 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.
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/84abfad4ada9
https://hg.mozilla.org/mozilla-central/rev/b420bb292e68
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1428688
No longer blocks: 1428688
You need to log in before you can comment on or make changes to this bug.