Data race in reading mResource in ChannelMediaResource::Listener::OnDataAvailable()

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

Reading mResource (which might happen off the main thread) is a data race if Revoke() is happening on the main thread at the same time. We will remove Revoke() and constify mResource so it is safe to read mResource on all threads.

[1] http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/dom/media/ChannelMediaResource.cpp#95
[2] http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/dom/media/ChannelMediaResource.h#130
Assignee: nobody → jwwang
Blocks: 1382978
Priority: -- → P3
Attachment #8910572 - Flags: review?(gsquelart)
Attachment #8910573 - Flags: review?(gsquelart)
Comment on attachment 8910572 [details]
Bug 1401461. P1 - protect access to ChannelMediaResource::Listener::mResource.

https://reviewboard.mozilla.org/r/182022/#review187368
Attachment #8910572 - Flags: review?(gsquelart) → review+
Comment on attachment 8910573 [details]
Bug 1401461. P2 - don't call mChannelStatistics.AddBytes() if the data is from an old channel.

https://reviewboard.mozilla.org/r/182024/#review187370
Attachment #8910573 - Flags: review?(gsquelart) → review+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ceb7e5789f4
P1 - remove ChannelMediaResource::Listener::Revoke(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/ddfa978c27f7
P2 - don't call mChannelStatistics.AddBytes() if the data is from an old channel. r=gerald
https://treeherder.mozilla.org/logviewer.html#?job_id=132418075&repo=autoland&lineNumber=45025

[5300, Main Thread] ###!!! ASSERTION: Failed Dispatch after xpcom-shutdown-threads: 'false', file z:/build/build/src/xpcom/threads/ThreadEventTarget.cpp, line 136
#01: mozilla::ThreadEventTarget::Dispatch(already_AddRefed<nsIRunnable>,unsigned int) [xpcom/threads/ThreadEventTarget.cpp:136]
#02: nsThread::Dispatch(already_AddRefed<nsIRunnable>,unsigned int) [xpcom/threads/nsThread.cpp:646]
#03: mozilla::FileBlockCache::Close() [dom/media/FileBlockCache.cpp:203]
#04: mozilla::FileBlockCache::~FileBlockCache() [dom/media/FileBlockCache.cpp:177]
#05: mozilla::MediaBlockCacheBase::Release() [dom/media/MediaBlockCacheBase.h:39]
#06: mozilla::MediaCache::~MediaCache() [dom/media/MediaCache.cpp:304]
#07: mozilla::MediaCacheStream::~MediaCacheStream() [dom/media/MediaCache.cpp:2093]
#08: mozilla::ChannelMediaResource::~ChannelMediaResource() [dom/media/ChannelMediaResource.cpp:52]
#09: mozilla::MediaResource::Destroy() [dom/media/MediaResource.cpp:57]
#10: mozilla::MediaResource::Release() [dom/media/MediaResource.cpp:68]
#11: mozilla::ChannelMediaResource::Listener::~Listener() [dom/media/ChannelMediaResource.h:116]
#12: nsCOMPtr<nsIStreamListener>::~nsCOMPtr<nsIStreamListener>() [xpcom/base/nsCOMPtr.h:426]
#13: mozilla::dom::MediaDocumentStreamListener::Release() [dom/html/MediaDocument.cpp:40]
#14: mozilla::dom::VideoDocument::~VideoDocument()

It is too late for ~Listener() to drop its reference to ChannelMediaResource. We still need Revoke() to manually break the reference cycle.
Flags: needinfo?(jwwang)
Summary: Remove ChannelMediaResource::Listener::Revoke() → Data race in reading mResource in ChannelMediaResource::Listener::OnDataAvailable()
Attachment #8910572 - Flags: review+ → review?(gsquelart)
Comment on attachment 8910572 [details]
Bug 1401461. P1 - protect access to ChannelMediaResource::Listener::mResource.

https://reviewboard.mozilla.org/r/182022/#review187716

r+ as-is. Suggestion for another bug:

::: dom/media/ChannelMediaResource.h:135
(Diff revision 2)
> +    Mutex mMutex;
> +    // mResource should only be modified on the main thread with the lock.
> +    // So it can be read without lock on the main thread or on other threads
> +    // with the lock.
>      RefPtr<ChannelMediaResource> mResource;

Not an actual issue for this bug, just thinking out loud:
I've seen this pattern in a few places. Would it be worth having a templated class that ensures this kind of variable is correctly accessed?
E.g.:
template<typename T>
class OnlyWrittenOnMutexedMainThread
{
public:
  // special member functions as needed...
  // Write on main thread with lock.
  T& Ref(MutexAutoLock& aProofOfLock) { MOZ_ASSERT(NS_IsMainThread();) return mVar; }
  // Read on main thread without lock.
  const T& Get() { MOZ_ASSERT(NS_IsMainThread();) return mVar; }
  // Read elsewhere with lock.
  const T& Get(MutexAutoLock& aProofOfLock) { return mVar; }
private:
  T mVar;
}
Attachment #8910572 - Flags: review?(gsquelart) → review+
Comment on attachment 8910573 [details]
Bug 1401461. P2 - don't call mChannelStatistics.AddBytes() if the data is from an old channel.

https://reviewboard.mozilla.org/r/182024/#review187718

Still r+.
Comment on attachment 8910572 [details]
Bug 1401461. P1 - protect access to ChannelMediaResource::Listener::mResource.

https://reviewboard.mozilla.org/r/182022/#review187716

> Not an actual issue for this bug, just thinking out loud:
> I've seen this pattern in a few places. Would it be worth having a templated class that ensures this kind of variable is correctly accessed?
> E.g.:
> template<typename T>
> class OnlyWrittenOnMutexedMainThread
> {
> public:
>   // special member functions as needed...
>   // Write on main thread with lock.
>   T& Ref(MutexAutoLock& aProofOfLock) { MOZ_ASSERT(NS_IsMainThread();) return mVar; }
>   // Read on main thread without lock.
>   const T& Get() { MOZ_ASSERT(NS_IsMainThread();) return mVar; }
>   // Read elsewhere with lock.
>   const T& Get(MutexAutoLock& aProofOfLock) { return mVar; }
> private:
>   T mVar;
> }

Yeah, I thought about that too. But I stopped immediately for I couldn't find a good name for this class, or for below members. http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/dom/media/MediaCache.h#444-467 I believe the idea that if you can't find a good name for a function or class, it probably means you haven't though it through.
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3c36a62b5b1
P1 - protect access to ChannelMediaResource::Listener::mResource. r=gerald
https://hg.mozilla.org/integration/autoland/rev/20a0000f97bc
P2 - don't call mChannelStatistics.AddBytes() if the data is from an old channel. r=gerald
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c5cd3ceffc28
Backed out 2 changesets for being the wrong patches
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0a767cd8383
P1 - protect access to ChannelMediaResource::Listener::mResource. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c8bf13603933
P2 - don't call mChannelStatistics.AddBytes() if the data is from an old channel. r=gerald
https://hg.mozilla.org/mozilla-central/rev/d0a767cd8383
https://hg.mozilla.org/mozilla-central/rev/c8bf13603933
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1428688
No longer blocks: 1428688
You need to log in before you can comment on or make changes to this bug.