1468241 - Crash in mozilla::Maybe<T>::value const

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-82d26354-68c1-4638-9d2a-ef3fe0180601.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20180531101443: https://bit.ly/2t0tsue

MOZ_RELEASE_ASSERT(mIsSome) is the crash reason. Maybe this is expected.

Top 10 frames of crashing thread:

0 libxul.so mozilla::Maybe<mozilla::media::TimeUnit>::value const mfbt/Maybe.h:525
1 libxul.so mozilla::RemoteVideoDecoder::IsUsefulData dom/media/platforms/android/RemoteDataDecoder.cpp:250
2 libxul.so mozilla::RemoteDataDecoder::UpdateOutputStatus dom/media/platforms/android/RemoteDataDecoder.cpp:683
3 libxul.so decltype  xpcom/threads/nsThreadUtils.h:1165
4 libxul.so mozilla::detail::RunnableMethodImpl<mozilla::RemoteDataDecoder*, void  xpcom/threads/nsThreadUtils.h:1171
5 libxul.so mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:243
6 libxul.so nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:229
7 libxul.so non-virtual thunk to nsThreadPool::Run xpcom/threads/nsThreadPool.cpp
8 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1088
9 libxul.so NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519

=============================================================

Comment 1

[Nils Ohlmeier [:drno]]

Jean-Yves what do you think of this one? A recent regression?

Comment 2

[Jean-Yves Avenard [:jya]]

It's a recent regression in that Maybe<T>::value() was changed to use MOZ_DIAGNOSTIC_ASSERT to make sure the Maybe contains a value.

The crash however is nonsensical because one line above:
https://hg.mozilla.org/mozilla-central/annotate/763f30c3421233a45ef9e67a695c5c241a2c8a3a/dom/media/platforms/android/RemoteDataDecoder.cpp#l247

we check that the value actually contains something.

So I don't see how that crash could ever happen with the code as it is.
You can't get to Maybe<mozilla::media::TimeUnit>::value if value isn't set...

unless the compiler for some reason optimised out the test...

Comment 3

[Jean-Yves Avenard [:jya]]

Oh I see the reason..

the code is racy:
https://hg.mozilla.org/mozilla-central/annotate/e0595117ff5bda3a63a72ad7b3b8754fec4fb4f0/dom/media/platforms/android/RemoteDataDecoder.cpp#l214

Flush would reset mSeekTarget, but does so in the reader's taskqueue, while IsUsefulData is called on the decoder's task queue

Comment 4

[Phabricator Automation]

Attachment: Bug 1468241 - P1. Add MozPromise::CreateAndResolveOrReject method

Useful when using promise chaining in combination with MozPromise::ResolveOrRejectValue parameter.

MozReview-Commit-ID: F8qMh7yFnHQ

Comment 5

[Phabricator Automation]

Attachment: Bug 1468241 - P2. Ensure RemoteVideoDecoder::mSeekTarget is only accessed on task queue.

Amend comments to clarify how some members are accessed.

MozReview-Commit-ID: FeEIRap3zvn

Depends on D1726

Comment 6

[Phabricator Automation]

Comment on attachment 8986442 [details]
Bug 1468241 - P1. Add MozPromise::CreateAndResolveOrReject method

Gerald Squelart [:gerald] has approved the revision.

https://phabricator.services.mozilla.com/D1726

Comment 7

[Phabricator Automation]

Comment on attachment 8986444 [details]
Bug 1468241 - P2. Ensure RemoteVideoDecoder::mSeekTarget is only accessed on task queue.

Paul Adenot (:padenot) has approved the revision.

https://phabricator.services.mozilla.com/D1727

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d039520dd5a
P1. Add MozPromise::CreateAndResolveOrReject method r=gerald

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/686fc84d4ffb
P2. Ensure RemoteVideoDecoder::mSeekTarget is only accessed on task queue. r=padenot

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/3d039520dd5a
https://hg.mozilla.org/mozilla-central/rev/686fc84d4ffb

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Looks like the racy code goes back to 57 or thereabouts? Either way, not convinced this is something we need to track for backporting anywhere, but let me know if you feel strongly otherwise :).