1545108 - Crash in [@ mozilla::MediaDecoderStateMachine::OnAudioPopped]

Status: RESOLVED WORKSFORME

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

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-f10c2589-c955-4101-b874-738a70190320.

MacOS crash visible in nightly, but may go as far back as FF 65: https://bit.ly/2UYGwR5

The few URLs are https://hangouts.google.com/ and https://www.youtube.com/watch?v=m3sV7daQRls

Top 10 frames of crashing thread:

0 XUL mozilla::MediaDecoderStateMachine::OnAudioPopped clang/include/c++/v1/algorithm:721
1 XUL mozilla::detail::RunnableMethodImpl<mozilla::detail::Listener<RefPtr<mozilla::AudioData> >*, void  xpcom/threads/nsThreadUtils.h:1122
2 XUL mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:197
3 XUL mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:199
4 XUL nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:241
5 XUL non-virtual thunk to nsThreadPool::Run xpcom/threads/nsThreadPool.cpp
6 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1179
7 XUL NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:482
8 XUL mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:333
9 XUL nsThread::ThreadFunc ipc/chromium/src/base/message_loop.cc:315

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Looks like we could be dereferencing a nullptr here. We shouldn't end up with null data in the audio queue, but perhaps there's an edge case we're missing.

Alastor, any idea on what could be going on here?

Comment 2

[Alastor Wu [:alwu]]

Keep NI, will check it later.

Comment 3

[Alastor Wu [:alwu]]

As we've ensured that the samples which are pushed into media queue would always be not a nullptr [1], so I'm looking for if there is any chance the data was freed before popped out.

And I found that the ref-counting for the MediaData is kind of tricky. The AudioQueue holds a raw point to data [2], not a ref counting. So after pushing data into queue, it seems that the only place inside the media queue keeping data alive is here [3].

So after MediaEventListener notifies the event, which would hold the reference to data, there seems no any other places to keep this data alive? (if the data was not popped out during the push event)

In AudioSink case, it would pop out the data when it received the push event [5], which has kept the reference to data. So the strong reference is always kept between pushing and poping data.

However, in DecodedStream case, it won't pop data directly [6], it would pop the data until the DecodedStream::NotifyOutput is called [7]. So, who is keeping the reference between pushing and poping data in this case? I couldn't find it, but I'm sure there is one place to keep the reference, otherwise, DecodedStream could not be able to work (I just couldn't find it)

[1] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/media/MediaQueue.h#45-47
[2] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/media/MediaDecoderStateMachine.h#559
[3] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/media/MediaQueue.h#49
[4] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/media/mediasink/AudioSink.cpp#333
[5] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/media/mediasink/AudioSink.cpp#319
[6] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/media/mediasink/DecodedStream.cpp#539-583
[7] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/media/mediasink/DecodedStream.cpp#809

Comment 4

[Alastor Wu [:alwu]]

Here are some questions,
(1) Is it possible to release data before poping it out from media queue?

• If so, it might be a possible root cause of this crash.

(2) Do you guys know who is keeping the data alive for the DecodedStream case?

• see the comment3, the last paragraph.

(3) Why the AudioQueue and VideoQueue in MDSM only keep the raw pointer, not a strong reference for media data?

---

Hi, Bryce, Jya,
Do you guys have any idea about these questions?
Thank you!

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Discussed a bit on IRC: I think this is handled by the queue manually adding refs here. This also leads to the patterns used when popping from the queue where a RefPtr is returned, but the reference count isn't changed as the queue already added a ref and is now passing that ownership via the returned ptr.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Alastor Wu [:alwu] from comment #3)

As we've ensured that the samples which are pushed into media queue would always be not a nullptr [1], so I'm looking for if there is any chance the data was freed before popped out.

And I found that the ref-counting for the MediaData is kind of tricky. The AudioQueue holds a raw point to data [2], not a ref counting. So after pushing data into queue, it seems that the only place inside the media queue keeping data alive is here [3].

https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/dom/media/MediaQueue.h#46

When the data is held in the queue, AddRef is called on it. It may not be held in a RefPtr, but the refcount is incremented as it should.

MediaQueue override nsDequeue and provides a method of what occurred when the element is removed from the queue:
https://searchfox.org/mozilla-central/source/dom/media/MediaQueue.h#22

Which will call Release on the element, as such, MediaQueue doesn't need to use RefPtr, the handling is the same.

Comment 7

[Alastor Wu [:alwu]]

After some investigation, it seems we have almost no possible to release the sample before the MediaEventListener gets the notification.

In [1], we keep the ref count of the sample is 1 still. And then send it via runnable in order to notify all listeners, we would added another reference in the runnable by creating a new RefPtr [2]. That will be done after doing a type convertion, because the type we sent to MediaEventProducer::Notify() is RefPtr&, but it would be converted to RefPtr later by using Delay() in [4].

That means, when we pop out the sample from queue, we have 2 ref count on the sample. One is for the caller of PopFront() , who owns the orignal ref count, which was tranfered by calling forget() [5]. Another one is for the notify runnable which would ensure the sample alive until all listeners have received the notification.

[1] https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/dom/media/MediaQueue.h#59
[2] https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/ipc/chromium/src/base/task.h#293
[3] https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/dom/media/MediaEventSource.h#438
[4] https://searchfox.org/mozilla-central/rev/ d143f8ce30d1bcfee7a1227c27bf876a85f8cede/dom/media/MediaEventSource.h#139
[5] https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/dom/media/MediaQueue.h#63

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

We have MOZ_ASSERTs to guard against nulls at various points. If this case is rare enough, could we be missing it in assert enabled builds, and occasionally placing null data into our queue in release builds? Then we only see it when we do the bad deref?

We could use a diagnostic assert here to see if this is happening and to get a better idea what is giving us null data.

Adding leave-open so we can land such a patch and keep an eye on this.

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1545108 - Use a diagnostic assert to check for nullptr when pushing into MediaQueue. r?alwu

We're rarely getting nullptrs out of MediaQueues. It's not clear where these are
coming from, as we have many guards against them. Upgrade this assert to a
diagnostic to help us track the source and determine if the value is null before
entering the queue.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9bd90dd38bf
Use a diagnostic assert to check for nullptr when pushing into MediaQueue. r=alwu

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/b9bd90dd38bf

Comment 12

[Patricia Lawless]

Hi Bryce, is the diagnostic assert showing anything helpful here? (tracking for 68). Thanks!

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(In reply to Patricia Lawless from comment #12)

Hi Bryce, is the diagnostic assert showing anything helpful here? (tracking for 68). Thanks!

Kind of. I've been keeping an eye on crashes, and it looks like we're not hitting that assert. It's possible that we'd hit it with such a low rate that we need to keep watching it for the case where it happens. On the other hand, us not hitting the assert could suggest the data is being nulled after we push into the queue and that we've overlooked something above.

We've had two crashes with this sig since the assert was added, but they're both in old builds, and don't look like the null deref I suspect (I'm not sure what to make of these new ones yet). My inclination is to keep watching this and see if we get more crashes.

Comment 14

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Most recent crash against this signature is interesting. It is happening in a build that should have the diagnostic assert added above. This suggests that this is not a null being pushed into the MediaQueue. If this were the case the diagnostic assert should have fired before we could reach this later failure.

So we may need to take another look at if our sample could somehow be nulled while in the queue.

Comment 15

[BugBot [:suhaib / :marco]]

The leave-open keyword is there and there is no activity for 6 months.
:bryce, maybe it's time to close this bug?

Comment 16

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

I'm not sure, bot. We still have some recent reports of this (like this). However, the rates this is happening at are very low.

I figure we leave this open and keep and eye on it.

Comment 17

[BugBot [:suhaib / :marco]]

The leave-open keyword is there and there is no activity for 6 months.
:bryce, maybe it's time to close this bug?

Comment 18

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

As comment 16.

Comment 19

[Alastor Wu [:alwu]]

After discussed with bryce offline, we think it's worth to use RefPtr in the MediaQueue directly to prevent any possible UAF.

Comment 20

[Alastor Wu [:alwu]]

I wonder if there is any specific reason why we don't use RefPtr directly in MediaQueue?

Comment 21

[Alastor Wu [:alwu]]

Attachment: Bug 1545108 - use diagnostic assertion to ensure the sample is valid to be accessed.

This patch will do :

• add MOZ_DIAGNOSTIC_ASSERT to ensure the sample is valid to be accessed

The advantage :

• to ensure the sample is still valid when we push it to the queue or pop it out the queue

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0009cfbd79cb
use diagnostic assertion to ensure the sample is valid to be accessed. r=bryce

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0009cfbd79cb

Comment 24

[Jean-Yves Avenard [:jya]]

(In reply to Alastor Wu [:alwu] from comment #20)

I wonder if there is any specific reason why we don't use RefPtr directly in MediaQueue?

What do you mean?

MediaQueue does use raw pointer, but it only ever add that pointers after increasing the addRef.
And the pointer is only released when the pointed is removed from the queue.

Here is push()
https://searchfox.org/mozilla-central/source/dom/media/MediaQueue.h#50-62

It takes an already_AddRefed; and add it to the queue, the object isn't released, in total the refcount got incremented.
When when popping an object:
https://searchfox.org/mozilla-central/source/dom/media/MediaQueue.h#64-72
The pointer is put into a RefPtr, keeping the refcount intact and an already_addRefed is returned.

Why it was done that way I'm not sure.

Using RefPtr while it would make the code clearer to read, will no fix what you think it will. This is not a life-cycle issue.
The MediaQueue owns the pointers that it contains.

Comment 25

[BugBot [:suhaib / :marco]]

The leave-open keyword is there and there is no activity for 6 months.
:alwu, maybe it's time to close this bug?

Comment 26

[BugBot [:suhaib / :marco]]

The leave-open keyword is there and there is no activity for 6 months.
:alwu, maybe it's time to close this bug?

Comment 27

[Alastor Wu [:alwu]]

This issue still exists but we still don't have a clear idea about why it happens. So set it stalled and lower its priority because of its low crash rate.

Comment 28

[BugBot [:suhaib / :marco]]

Closing because no crashes reported for 12 weeks.

Comment 29

[BugBot [:suhaib / :marco]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.