1663736 - Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Sink was not explicitly removed), at /builds/worker/workspace/obj-build/dist/include/mozilla/dom/MediaStreamTrack.h:297

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.zip

Testcase found while fuzzing mozilla-central rev fb9c01b719fa (built with --enable-debug). Testcase must be served over HTTP.

Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Sink was not explicitly removed), at /builds/worker/workspace/obj-build/dist/include/mozilla/dom/MediaStreamTrack.h:297

    #0 0x7f0dee86dc9e in mozilla::dom::MediaStreamTrackSource::OverrideEnded() /builds/worker/checkouts/gecko/dom/media/MediaStreamTrack.h:297:9
    #1 0x7f0df0bc43a6 in applyImpl<mozilla::dom::HTMLMediaElement::MediaElementTrackSource, void (mozilla::dom::HTMLMediaElement::MediaElementTrackSource::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1188:12
    #2 0x7f0df0bc43a6 in apply<mozilla::dom::HTMLMediaElement::MediaElementTrackSource, void (mozilla::dom::HTMLMediaElement::MediaElementTrackSource::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1194:12
    #3 0x7f0df0bc43a6 in mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::dom::HTMLMediaElement::MediaElementTrackSource>, void (mozilla::dom::HTMLMediaElement::MediaElementTrackSource::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1240:13
    #4 0x7f0ded652212 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/checkouts/gecko/xpcom/threads/SchedulerGroup.cpp:146:20
    #5 0x7f0ded657b5f in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:242:16
    #6 0x7f0ded655bda in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:512:26
    #7 0x7f0ded654d34 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:371:15
    #8 0x7f0ded654ee7 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:168:36
    #9 0x7f0ded65c896 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:83:37
    #10 0x7f0ded65c896 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_4>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:577:5
    #11 0x7f0ded66fc7f in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1234:14
    #12 0x7f0ded67562a in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:513:10
    #13 0x7f0dedf6b076 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:87:21
    #14 0x7f0dedede433 in MessageLoop::RunInternal() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:334:10
    #15 0x7f0dedede34d in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:327:3
    #16 0x7f0dedede34d in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:309:3
    #17 0x7f0df1b53668 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:137:27
    #18 0x7f0df3322053 in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:913:20
    #19 0x7f0dedf6be39 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:237:9
    #20 0x7f0dedede433 in MessageLoop::RunInternal() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:334:10
    #21 0x7f0dedede34d in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:327:3
    #22 0x7f0dedede34d in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:309:3
    #23 0x7f0df3321c2c in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:744:34
    #24 0x556bd78bf79b in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #25 0x556bd78bf79b in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:303:18
    #26 0x7f0e01cb40b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16
    #27 0x556bd789d549 in _start (/home/worker/builds/m-c-20200828153126-fuzzing-debug/firefox-bin+0x17549)

Comment 1

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Unable to reproduce bug using the following builds:
> mozilla-central 20200908215255-dc90a7a18c07
> mozilla-central 20200908030802-80ac8d8c74d5
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 2

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/sofb_kfUpIeBTVGm-2nSNw/index.html

Comment 3

[Andreas Pehrson [:pehrsons]]

This is due to the array Clone() in OverrideEnded(). We loop over two sinks. On the first we call OverrideEnded(), which causes both to be removed, but we have not yet iterated over the second sink in the cloned array. When we do, its weak ptr is now null and it tries to remove itself from mSinks (which it can't!).

Let's improve this check.

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1663736 - Also check mSinks for explicitly removed sinks. r?jib!

Comment 5

[Andreas Pehrson [:pehrsons]]

I was not able to reproduce this in a crashtest fwiw. It's very timing (and GC) dependent.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The patch here only seems to improve the assert, but doesn't seem to attempt to fix the problem. Are more patches forthcoming?

(In reply to Andreas Pehrson [:pehrsons] from comment #3)

This is due to the array Clone() in OverrideEnded().

Cloning an array before walking it while removing entries in it seems like a good pattern.

We loop over two sinks. On the first we call OverrideEnded(), which causes both to be removed, but we have not yet iterated over the second sink in the cloned array. When we do, its weak ptr is now null and it tries to remove itself from mSinks (which it can't!).

The code seems re-entrant, maybe because HTMLMediaElement::MediaElementTrackSource is both a source and a sink?

1. MediaStreamTrackSource::OverrideEnded() calls sink->OverrideEnded(), which is...
2. HTMLMediaElement::MediaElementTrackSource::OverrideEnded, which in turn calls...
3. MediaStreamTrackSource::OverrideEnded() again.

Makes the code a bit hard to reason about, but encountering null weakptrs here seems normal for mSinks.Length > 1?

Comment 7

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #6)

The patch here only seems to improve the assert, but doesn't seem to attempt to fix the problem. Are more patches forthcoming?

The assert is the problem.

(In reply to Andreas Pehrson [:pehrsons] from comment #3)

This is due to the array Clone() in OverrideEnded().

Cloning an array before walking it while removing entries in it seems like a good pattern.

Yes, if there's a chance the calls made during the iteration will modify the array it makes a lot of sense. But for the same reason we need to validate the invariant against the original array, not against the clone, since the aforementioned modifications during the iteration are made to the original array.

We loop over two sinks. On the first we call OverrideEnded(), which causes both to be removed, but we have not yet iterated over the second sink in the cloned array. When we do, its weak ptr is now null and it tries to remove itself from mSinks (which it can't!).

The code seems re-entrant, maybe because HTMLMediaElement::MediaElementTrackSource is both a source and a sink?

1. MediaStreamTrackSource::OverrideEnded() calls sink->OverrideEnded(), which is...
2. HTMLMediaElement::MediaElementTrackSource::OverrideEnded, which in turn calls...
3. MediaStreamTrackSource::OverrideEnded() again.

A HTMLMediaElement playing a MediaStream with a track A, that is captured via mozCaptureStream, will return another MediaStream with a track B forwarded from A. Thus B is a sink to A, and A is a source to B (somewhat simplified because the class hierarchy is different). So I don't think this is re-entrant in the way you mean, because in your step 1 A's OverrideEnded() calls B's OverrideEnded() in step 3.

Makes the code a bit hard to reason about, but encountering null weakptrs here seems normal for mSinks.Length > 1?

We have an invariant that sinks must unregister themselves, so that Stop() and SinkEnabledStateChanged() are timely. Really the only reason we use WeakPtr here is so we can avoid doing something stupid with a rawptr in a corner case.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a08383b8236
Also check mSinks for explicitly removed sinks. r=jib

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7a08383b8236