Closed Bug 1468241 Opened Last year Closed Last year

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

Categories

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

61 Branch
Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: marcia, Assigned: jya)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

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

=============================================================
Jean-Yves what do you think of this one? A recent regression?
Rank: 15
Flags: needinfo?(jyavenard)
Priority: -- → P2
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...
Flags: needinfo?(jyavenard)
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
Assignee: nobody → jyavenard
Useful when using promise chaining in combination with MozPromise::ResolveOrRejectValue parameter.

MozReview-Commit-ID: F8qMh7yFnHQ
Amend comments to clarify how some members are accessed.

MozReview-Commit-ID: FeEIRap3zvn

Depends on D1726
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
Attachment #8986442 - Flags: review+
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
Attachment #8986444 - Flags: review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d039520dd5a
P1. Add MozPromise::CreateAndResolveOrReject method r=gerald
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/686fc84d4ffb
P2. Ensure RemoteVideoDecoder::mSeekTarget is only accessed on task queue. r=padenot
https://hg.mozilla.org/mozilla-central/rev/3d039520dd5a
https://hg.mozilla.org/mozilla-central/rev/686fc84d4ffb
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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 :).
You need to log in before you can comment on or make changes to this bug.