Open Bug 1719711 Opened 5 years ago Updated 5 years ago

MediaCache sThread is accessed on multiple threads without synchronization

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

People

(Reporter: bryce, Unassigned)

References

(Blocks 1 open bug)

Details

MediaCache::sThread[0] is a thread used by the media cache. At time of writing, it is largely used on the main thread[1]. However, various tasks are dispatched to sThread and then assert they're on that thread[1].

This superficially appears safe, and in practice I'm not aware of any major issues. However, given the complexities and weirdness that can apply around memory ordering, I think we should have something explicitly synchronizing our usage.

[0] https://searchfox.org/mozilla-central/rev/da25888c4495585c532640f0e5efad07b1037621/dom/media/MediaCache.cpp#464
[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla10MediaCache7sThreadE&redirect=false

Blocks: media-cache

This may become less of, or a non-issue once bug 1582575 lands. Once that happens, some of our low level threading guarantees + our usage of sThread may make this safer. Specifically:

  • Threads do not accept new events once we're in the xpcom shutdown threads stage.
  • sThread will not be used off main thread before it's created.
  • sThread will not be mutated on main thread until it is shutdown (bug 1582575 gives us this).
  • sThread is shutdown in response to xpcom shutdown threads.
  • All tasks/events on sThread will be finished by the time shutdown is complete.

What I'm not sure on is if we have guarantees around memory ordering from our threading constructs. The above makes us pretty safe, and in practice we may not run into problems unless we start doing other things with sThread. So this may not be worth chasing further unless we encounter problems that indicate races (or make changes to our sThread usage).

Depends on: 1582575

Lowering severity. I think the actual issues that could happen here are minimal, and may not exist at all depending on the answer to the below.

I'd be interested if there's anything syncing sThread when it's first assigned. It looks unsafe to me that sThread is not atomic and is assigned and then read on different threads.

Severity: S3 → S4
You need to log in before you can comment on or make changes to this bug.