1228673 - dom/media fuzzing Assertion failure: !mThenValue || mThenValue->IsDisconnected(), at ../../dist/include/mozilla/MozPromise.h:312

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

Attachment: media-fuzz.patch DO NOT CHECK-IN

STR:
1. Enabling the media fuzzing wrapper (bug 1194518) like the attached patch:

  media.decoder.fuzzing.enabled = true
  media.decoder.fuzzing.video-output-minimum-interval-ms = 1 (assertion requires >= 1 ms)

2. Try mochitest-2 with "try: -b do -p linux,macosx64,win64 -u mochitest-2,web-platform-tests-3 -t none"

RESULT:

MozPromise assertion failures in mochitest-2's dom/media tests! I was unable to reproduce this assertion failure locally. The MozPromise assertion is actually a MOZ_DIAGNOSTIC_ASSERT, so it affects debug builds plus Nightly and Aurora release builds, suggesting this is a serious issue.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f2b86b45c2e

 12:46:18     INFO -  Assertion failure: !mThenValue || mThenValue->IsDisconnected(), at ../../dist/include/mozilla/MozPromise.h:312
 12:47:14     INFO -  #01: nsRunnable::Release() [xpcom/glue/nsThreadUtils.cpp:35]
 12:47:14     INFO -  #02: mozilla::TaskQueue::DispatchLocked(already_AddRefed<nsIRunnable>, mozilla::TaskQueue::DispatchMode, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) [xpcom/threads/TaskQueue.cpp:74]
 12:47:14     INFO -  #03: mozilla::TaskQueue::Dispatch(already_AddRefed<nsIRunnable>, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) [mfbt/AlreadyAddRefed.h:101]
 12:47:14     INFO -  #04: mozilla::MozPromise<bool, bool, true>::ThenValueBase::Dispatch(mozilla::MozPromise<bool, bool, true>*) [mfbt/AlreadyAddRefed.h:101]
 12:47:14     INFO -  #05: mozilla::MozPromise<bool, bool, true>::DispatchAll() [xpcom/threads/MozPromise.h:629]
 12:47:14     INFO -  #06: void mozilla::MozPromise<bool, bool, true>::Private::Reject<bool>(bool&&, char const*) [xpcom/glue/Mutex.h:169]
 12:47:14     INFO -  #07: mozilla::MediaTimer::Destroy() [/usr/include/c++/4.2.1/bits/stl_iterator.h:653]
 12:47:14     INFO -  #08: nsRunnableMethodImpl<void (mozilla::MediaTimer::*)(), false>::Run [xpcom/glue/nsThreadUtils.h:872]
 12:47:14     INFO -  #09: nsThreadPool::Run() [xpcom/glue/nsCOMPtr.h:403]
 12:47:14     INFO -  #10: non-virtual thunk to nsThreadPool::Run() [xpcom/threads/nsThreadPool.cpp:148]
 12:47:14     INFO -  #11: nsThread::ProcessNextEvent(bool, bool*) [xpcom/glue/nsCOMPtr.h:403]
 12:47:14     INFO -  #12: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/glue/nsThreadUtils.cpp:297]
 12:47:14     INFO -  #13: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:355]
 12:47:14     INFO -  #14: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:520]
 12:47:14     INFO -  #15: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:378]
 12:47:17     INFO -  #16: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:215]
 12:47:17     INFO -  #17: libSystem.B.dylib + 0x39fd6

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Interesting that it happens as part of MediaTimer::Destroy().
I'd guess some media element is destroyed early (because some tests may exit early once they have reached their own goals), which destroys timers even if they haven't lapsed yet.
I'll look into it.

Comment 2

[Bobby Holley (:bholley)]

This assertion is basically saying that the dispatch must either succeed (in which case we will invoke ::Run(), and null out mThenValue before the ResolveOrRejectRunnable is destroyed), or the promise must have been disconnected. Technically reading IsDisconnected() in this case is a data race (since mDisconnected is owned by the consumer thread), but I'm pretty sure that the race would only manifest in the case when the caller wasn't properly disconnecting before triggering the operation that caused the failed-dispatch-attempt to run.

Comment 3

[Chris Peterson [:cpeterson]]

P2 because this assertion failure is only reproducible when media fuzzing is enabled.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1228673-fix.patch

Clean up 'Then' before ~MediaTimer.

Destroying an unlapsed timer will trigger an assertion in its 'Then' request.
So in case the Fuzzing Wrapper is flushed or errors when there are pending
delayed frames, the 'Then' request attached to the MediaTimer is now
disconnected before being let go.
This also needs to be done when shutting down.

Try with the fuzzing wrapper enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=636d504495ff

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8697894 [details] [diff] [review]
1228673-fix.patch

Review of attachment 8697894 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/platforms/wrappers/FuzzingWrapper.cpp
@@ +262,2 @@
>    RefPtr<DecoderCallbackFuzzingWrapper> self = this;
> +  mDelayedOutputRequest =

The convention is to use a MozPromiseRequestHolder to hold the result of Then().

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1228673-fix.patch v2

(In reply to JW Wang [:jwwang] from comment #5)
> Comment on attachment 8697894 [details] [diff] [review]
> 1228673-fix.patch
> 
> Review of attachment 8697894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp
> @@ +262,2 @@
> >    RefPtr<DecoderCallbackFuzzingWrapper> self = this;
> > +  mDelayedOutputRequest =
> 
> The convention is to use a MozPromiseRequestHolder to hold the result of
> Then().

Thank you JW for the tip.

Updated patch to use MozPromiseRequestHolder.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8693092 [details]
media-fuzz.patch DO NOT CHECK-IN

Making sure this testing patch won't get checked-in by mistake when bug is ready to land.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8697926 [details] [diff] [review]
1228673-fix.patch v2

Review of attachment 8697926 [details] [diff] [review]:
-----------------------------------------------------------------

I've never seed FuzzingWrapper before, so I think jya should probably review this.

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8697926 [details] [diff] [review]
1228673-fix.patch v2

Review of attachment 8697926 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: dom/media/platforms/wrappers/FuzzingWrapper.cpp
@@ +262,2 @@
>    RefPtr<DecoderCallbackFuzzingWrapper> self = this;
> +  mDelayedOutputRequest.Begin(

Don't you have the potential that the decoder is being destroyed prior the output timer kicking in?

this would cause an assert of mDelayedOutputRequest destructor (all promises must be disconnected first)

@@ +270,5 @@
> +               self->mDelayedOutputRequest.Complete();
> +               self->OutputDelayedFrame();
> +             }
> +           },
> +           [self] () -> void { self->ClearDelayedOutput(); }));

even if rejected you have to call complete() to prevent assert on destruction or new Begin()

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1228673-fix.patch v3

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> Comment on attachment 8697926 [details] [diff] [review]
> 1228673-fix.patch v2
> 
> Review of attachment 8697926 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed
> 
> ::: dom/media/platforms/wrappers/FuzzingWrapper.cpp
> @@ +262,2 @@
> >    RefPtr<DecoderCallbackFuzzingWrapper> self = this;
> > +  mDelayedOutputRequest.Begin(
> 
> Don't you have the potential that the decoder is being destroyed prior the
> output timer kicking in?
> 
> this would cause an assert of mDelayedOutputRequest destructor (all promises
> must be disconnected first)

As discussed, Shutdown will always be called before destruction; Shutdown() calls `mCallbackWrapper->Shutdown()`, which calls `ClearDelayedOutput(); mTaskQueue->AwaitIdle();`, so there won't be any connected promise at destruction time.

> @@ +270,5 @@
> > +               self->mDelayedOutputRequest.Complete();
> > +               self->OutputDelayedFrame();
> > +             }
> > +           },
> > +           [self] () -> void { self->ClearDelayedOutput(); }));
> 
> even if rejected you have to call complete() to prevent assert on
> destruction or new Begin()

Very low chance of rejection happening, and `ClearDelayedOutput()` should disconnect promises. But just in case I've added a Complete() as well, to make sure it happens quickly.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try with fuzzing wrapper enabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdabd092762a
Try without: (Just to verify that nothing else is impacted)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d8025b577ac

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc346dea130e

Comment 13

[Carsten Book [:Tomcat]]

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