Closed Bug 1655509 Opened 3 years ago Closed 1 year ago

Assertion failure: Request::mDisconnected, at /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:447

Categories

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

Product:
Core
Component:
Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
105 Branch
Tracking Flags:
Tracking Status
firefox-esr91 --- wontfix
firefox-esr102 --- wontfix
firefox81 --- wontfix
firefox92 --- wontfix
firefox93 --- wontfix
firefox94 --- wontfix
firefox103 --- disabled
firefox104 --- fixed
firefox105 --- fixed

People

(Reporter: jkratzer, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: crash, nightly-community, testcase, Whiteboard: [bugmon:confirmed])

Crash Data

Attachments

(2 files)

testcase.zip
16.65 KB, application/zip
Details
Bug 1655509 - Reject FaviconDataCallback Promise if it still exists at destruction time. r?nika!
48 bytes, text/x-phabricator-request
diannaS
: approval-mozilla-beta+
Details | Review
Attached file testcase.zip —

Testcase found while fuzzing mozilla-central rev 798bdad605b9.

Assertion failure: Request::mDisconnected, at /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:447

==11972==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7f170bf94507 bp 0x7f166dbf94b0 sp 0x7f166dbf94a0 T34)
==11972==The signal is caused by a WRITE memory access.
==11972==Hint: address points to the zero page.
    #0 0x7f170bf94506 in mozilla::MozPromise<bool, nsresult, false>::ThenValueBase::AssertIsDead() /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:447:9
    #1 0x7f170bf9a1f1 in mozilla::MozPromise<bool, nsresult, false>::AssertIsDead() /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:1036:13
    #2 0x7f170bf99a31 in mozilla::MozPromise<bool, nsresult, false>::~MozPromise() /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:1077:5
    #3 0x7f170bf99db8 in mozilla::MozPromise<bool, nsresult, false>::Private::~Private() /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:257:9
    #4 0x7f1712315838 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:152:3
    #5 0x7f1712315838 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:50:40
    #6 0x7f1712315838 in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:381:36
    #7 0x7f1712315838 in ~RefPtr /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:81:7
    #8 0x7f1712315838 in mozilla::VideoSink::~VideoSink() /builds/worker/checkouts/gecko/dom/media/mediasink/VideoSink.cpp:97:1
    #9 0x7f1712315a7d in mozilla::VideoSink::~VideoSink() /builds/worker/checkouts/gecko/dom/media/mediasink/VideoSink.cpp:93:25
    #10 0x7f1711c8b29e in Release /builds/worker/checkouts/gecko/dom/media/mediasink/MediaSink.h:38:3
    #11 0x7f1711c8b29e in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:50:40
    #12 0x7f1711c8b29e in Release /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:381:36
    #13 0x7f1711c8b29e in assign_assuming_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:69:7
    #14 0x7f1711c8b29e in RefPtr<mozilla::MediaSink>& RefPtr<mozilla::MediaSink>::operator=<mozilla::MediaSink>(already_AddRefed<mozilla::MediaSink>&&) /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:206:5
    #15 0x7f1711c9a007 in mozilla::MediaDecoderStateMachine::ResumeMediaSink() /builds/worker/checkouts/gecko/dom/media/MediaDecoderStateMachine.cpp:3812:14
    #16 0x7f1711ea79a2 in applyImpl<mozilla::MediaDecoderStateMachine, void (mozilla::MediaDecoderStateMachine::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1188:12
    #17 0x7f1711ea79a2 in apply<mozilla::MediaDecoderStateMachine, void (mozilla::MediaDecoderStateMachine::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1194:12
    #18 0x7f1711ea79a2 in mozilla::detail::RunnableMethodImpl<mozilla::MediaDecoderStateMachine*, void (mozilla::MediaDecoderStateMachine::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1240:13
    #19 0x7f170b2a15db in mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() /builds/worker/workspace/obj-build/dist/include/mozilla/TaskDispatcher.h:228:35
    #20 0x7f170b2aea81 in mozilla::TaskQueue::Runner::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskQueue.cpp:158:20
    #21 0x7f170b2dec56 in nsThreadPool::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadPool.cpp:299:14
    #22 0x7f170b2cfa6c in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1234:14
    #23 0x7f170b2da95c in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:513:10
    #24 0x7f170c693094 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:332:5
    #25 0x7f170c572057 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:334:10
    #26 0x7f170c572057 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:327:3
    #27 0x7f170c572057 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:309:3
    #28 0x7f170b2c8417 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:447:10
    #29 0x7f1730780d3e in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #30 0x7f17303c26da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
    #31 0x7f172f3a0a3e in clone /build/glibc-2ORdQG/glibc-2.27/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:447:9 in mozilla::MozPromise<bool, nsresult, false>::ThenValueBase::AssertIsDead()
Thread T34 (MediaDe~hine #1) created by T0 (file:// Content) here:
    #0 0x55e7fa577a1a in pthread_create /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:209:3
    #1 0x7f17307711e5 in _PR_CreateThread /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:458:14
    #2 0x7f173076215e in PR_CreateThread /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:533:12
    #3 0x7f170b2cb0f7 in nsThread::Init(nsTSubstring<char> const&) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:659:8
    #4 0x7f170b2d95ba in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadManager.cpp:629:12
    #5 0x7f170b2e475a in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, already_AddRefed<nsIRunnable>, unsigned int) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:161:57
    #6 0x7f170b2dd51d in NS_NewNamedThread /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:152:10
    #7 0x7f170b2dd51d in nsThreadPool::PutEvent(already_AddRefed<nsIRunnable>, unsigned int) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadPool.cpp:115:17
    #8 0x7f170b2dfd4e in nsThreadPool::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadPool.cpp:350:5
    #9 0x7f170b2ad786 in mozilla::TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>&, unsigned int, mozilla::AbstractThread::DispatchReason) /builds/worker/checkouts/gecko/xpcom/threads/TaskQueue.cpp:65:26
    #10 0x7f170b2ece4a in mozilla::TaskQueue::Dispatch(already_AddRefed<nsIRunnable>, mozilla::AbstractThread::DispatchReason) /builds/worker/workspace/obj-build/dist/include/mozilla/TaskQueue.h:86:14
    #11 0x7f170b2a0ff3 in mozilla::AutoTaskDispatcher::DispatchTaskGroup(mozilla::UniquePtr<mozilla::AutoTaskDispatcher::PerThreadTaskGroup, mozilla::DefaultDelete<mozilla::AutoTaskDispatcher::PerThreadTaskGroup> >) /builds/worker/workspace/obj-build/dist/include/mozilla/TaskDispatcher.h:276:20
    #12 0x7f170b2a020b in mozilla::AutoTaskDispatcher::~AutoTaskDispatcher() /builds/worker/workspace/obj-build/dist/include/mozilla/TaskDispatcher.h:122:7
    #13 0x7f170b2a2608 in mozilla::Maybe<mozilla::AutoTaskDispatcher>::reset() /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:652:19
    #14 0x7f170b29cafc in AfterProcessNextEvent /builds/worker/checkouts/gecko/xpcom/threads/AbstractThread.cpp:130:5
    #15 0x7f170b29cafc in non-virtual thunk to mozilla::XPCOMThreadWrapper::AfterProcessNextEvent(nsIThreadInternal*, bool) /builds/worker/checkouts/gecko/xpcom/threads/AbstractThread.cpp
    #16 0x7f170b2cffdd in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1258:3
    #17 0x7f170b2da95c in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:513:10
    #18 0x7f170c69130f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:87:21
    #19 0x7f170c572057 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:334:10
    #20 0x7f170c572057 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:327:3
    #21 0x7f170c572057 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:309:3
    #22 0x7f17138b1ab8 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:137:27
    #23 0x7f171747aa06 in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:913:20
    #24 0x7f170c572057 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:334:10
    #25 0x7f170c572057 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:327:3
    #26 0x7f170c572057 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:309:3
    #27 0x7f1717479fef in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:744:34
    #28 0x55e7fa5bff53 in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #29 0x55e7fa5bff53 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:303:18
    #30 0x7f172f2a0b96 in __libc_start_main /build/glibc-2ORdQG/glibc-2.27/csu/../csu/libc-start.c:310
Flags: in-testsuite?
Crash Signature: [@ mozilla::MozPromise<T>::ThenValueBase::AssertIsDead ]
Keywords: bugmon
Whiteboard: [bugmon:confirm] → [bugmon:confirmed]
Bugmon Analysis:
Unable to reproduce bug using the following builds:
> mozilla-central 20200803094100-84b257d07031
> mozilla-central 20200727033000-56082fc4acfa
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

This testcase now triggers the following assertion:

Assertion failure: mSegment.GetDuration() <= buffering, at /builds/worker/checkouts/gecko/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:872

    #0 0x7f0fd15f5b46 in mozilla::AudioInputProcessing::Pull(mozilla::MediaTrackGraphImpl*, long, long, long, mozilla::AudioSegment*, bool, bool*) /builds/worker/checkouts/gecko/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:872:5
    #1 0x7f0fd15f92b5 in mozilla::AudioInputTrack::ProcessInput(long, long, unsigned int) /builds/worker/checkouts/gecko/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:1272:21
    #2 0x7f0fd12313dc in mozilla::MediaTrackGraphImpl::Process(mozilla::AudioMixer*) /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:1291:15
    #3 0x7f0fd123246d in mozilla::MediaTrackGraphImpl::OneIterationImpl(long, long, mozilla::AudioMixer*) /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:1415:3
    #4 0x7f0fd1018a09 in mozilla::GraphRunner::Run() /builds/worker/checkouts/gecko/dom/media/GraphRunner.cpp:116:32
    #5 0x7f0fcda06003 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1152:16
    #6 0x7f0fcda0c36a in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:548:10
    #7 0x7f0fce32bedd in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:302:20
    #8 0x7f0fce296643 in MessageLoop::RunInternal() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:335:10
    #9 0x7f0fce29655d in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:328:3
    #10 0x7f0fce29655d in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:310:3
    #11 0x7f0fcda02726 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:391:10
    #12 0x7f0fe48e1cdb in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #13 0x7f0fe4e53608 in start_thread /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8
Severity: normal → S4
Priority: -- → P3

The recent crash volume for early beta in v103 and v104, e.g. bp-6c407d54-125a-4ad4-b328-b8d570220727, aligns with the landing of bug 1354248 which looks related to the stack:

mozilla::MozPromise<CopyableTArray<bool>, RefPtr<mozilla::MediaMgrError>, 1>::ThenValueBase::AssertIsDead()
mozilla::MozPromise<mozilla::dom::fs::FileSystemGetRootResponse, nsresult, 0>::AssertIsDead()
mozilla::MozPromise<mozilla::places::FaviconMetadata, nsresult, 0>::~MozPromise()
mozilla::MozPromise<mozilla::places::FaviconMetadata, nsresult, 0>::Private::~Private()
mozilla::places::PageIconProtocolHandler::NewChannelInternal(nsIURI*, nsILoadInfo*, nsIChannel**)
mozilla::places::PageIconProtocolHandler::NewChannel(nsIURI*, nsILoadInfo*, nsIChannel**)

80-220 crashes per beta version but dividing by 4 looks reasonable, ~60% with 32-bit builds.

Flags: needinfo?(mconley)

So I took a quick look and I have a hypothesis, but I'm wondering if, nika, you could confirm or refute my hypothesis on what's going wrong here.

We have this method in PageIconProtocolHandler called GetFaviconData which returns a RefPtr<FaviconMetadataPromise> which is used in two places. In both of those places, the Promise returned by GetFaviconData isn't actually held on to - it just kinda gets thrown away:

https://searchfox.org/mozilla-central/rev/23bf1890e07f780ba70e075bc8f46ffb85d1128c/toolkit/components/places/PageIconProtocolHandler.cpp#254
https://searchfox.org/mozilla-central/rev/23bf1890e07f780ba70e075bc8f46ffb85d1128c/toolkit/components/places/PageIconProtocolHandler.cpp#353

Now, granted, there are two Then's for that MozPromise that (I had assumed) would keep it alive. Maybe that assumption is wrong? Is it possible that the MozPromise returned by GetFaviconData is getting destroyed after it goes out of scope, and that's hitting our assertion?

I feel like that's the sort of thing we would have caught in testing, since if I needed to hold a reference to the MozPromise to keep it alive while we wait for it to resolve, I would have expected it to crash immediately upon exiting the GetFaviconData method... so I must be missing something.

What do you think, nika?

Flags: needinfo?(mconley) → needinfo?(nika)

I've found another instance where we seem to throw away a reference to a MozPromise returned by a method while we wait for it's Then's to be invoked: https://searchfox.org/mozilla-central/rev/23bf1890e07f780ba70e075bc8f46ffb85d1128c/docshell/base/CanonicalBrowsingContext.cpp#696

So I guess there's an established pattern here... so maybe there's something else going wrong?

The Then callbacks should be keeping it alive I believe. IIRC the more likely cause of the issue is actually that the private side of the promise (the one used to resolve it) was discarded without ever resolving or rejecting the promise. This could happen if the FaviconDataCallback was destroyed without rejecting the promise in mPromiseHolder (https://searchfox.org/mozilla-central/rev/2bbb0c0a90df20702df8c8011a8996536a83cb75/toolkit/components/places/PageIconProtocolHandler.cpp#119-168).

Perhaps adding an explicit destructor which calls mPromiseHolder.RejectIfExists(...); in that function would be good to make sure that the destructor doesn't completely fail to be called in some cases?

Flags: needinfo?(nika)

Sounds good, thanks Nika!

Assignee: nobody → mconley
Status: NEW → ASSIGNED
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4354360429c
Reject FaviconDataCallback Promise if it still exists at destruction time. r=nika

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox105: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch

The patch landed in nightly and beta is affected.
:mconley, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox104 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(mconley)

Comment on attachment 9289064 [details]
Bug 1655509 - Reject FaviconDataCallback Promise if it still exists at destruction time. r?nika!

Beta/Release Uplift Approval Request

  • User impact if declined: Potential for crashes due to MOZ_DIAGNOSTIC_ASSERTS.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a speculative fix for the crash being reported here, but it's very straight-forward: we reject a Promise upon destruction of a thing holding onto that Promise. This speculative fix is based on the advice of someone who really knows our Promise infrastructure well, so I have high confidence in this fix.
  • String changes made/needed: None.
  • Is Android affected?: Unknown
Flags: needinfo?(mconley)
Attachment #9289064 - Flags: approval-mozilla-beta?

:mconley since the crash seems to only happen in early beta and we only have b9 left, should we just let this ride the 105 train?

Flags: needinfo?(mconley)

I don't exactly know what the ramifications are of silently failing that assertion. If we're not seeing stability problems in later betas or when this hits release, then sure, letting it ride the trains is probably fine.

Flags: needinfo?(mconley)

Comment on attachment 9289064 [details]
Bug 1655509 - Reject FaviconDataCallback Promise if it still exists at destruction time. r?nika!

Ok, thanks! It's probably best to take it this case then. I'll keep an eye on it when 105 goes to beta.

Approved for 104.0b9

Attachment #9289064 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.