1370063 - Crash in mozilla::MozPromise<T>::ChainTo

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-d9f4d911-bf70-4b89-a491-a8f3a0170603.
=============================================================
Crashing Thread (21), Name: VideoChild
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::MozPromise<bool, mozilla::MediaResult, 1>::ChainTo(already_AddRefed<mozilla::MozPromise<bool, mozilla::MediaResult, 1>::Private>, char const*) 	obj-firefox/dist/include/mozilla/MozPromise.h:968
1 	xul.dll 	mozilla::detail::ProxyFunctionRunnable<<lambda_0a668af68202c4c8454dc1e89f116afc>, mozilla::MozPromise<bool, mozilla::MediaResult, 1> >::Run() 	obj-firefox/dist/include/mozilla/MozPromise.h:1488
2 	xul.dll 	mozilla::EventTargetWrapper::Runner::Run() 	xpcom/threads/AbstractThread.cpp:166
3 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1321
4 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:472
5 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:368
6 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
7 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
8 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:501
9 	nss3.dll 	PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:397
10 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:95
11 	ucrtbase.dll 	o__realloc_base 	
12 	kernel32.dll 	BaseThreadInitThunk 	
13 	ntdll.dll 	RtlUserThreadStart

crash reports with this signature are regressing after 55.0a1 build 20170602030204. so far it's only on windows, in the content process and with "MOZ_RELEASE_ASSERT(!IsExclusive || !mHaveRequest)"

this would be the changelog to the day before:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bdb2387396b4a74dfefb7c983733eed3625e906a&tochange=aeb3d0ca558f034cbef1c5a68bd07dd738611494

Comment 1

[Nicholas Nethercote [inactive]]

gerald, any ideas? A MediaResult is involved. This is the #9 topcrash on Windows in Nightly 20170602030204.

Comment 2

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

https://crash-stats.mozilla.com/signature/?product=Firefox&version=55.0a1&signature=mozilla%3A%3AMozPromise%3CT%3E%3A%3AChainTo&date=%3E%3D2017-05-05T06%3A35%3A40.000Z&date=%3C2017-06-05T06%3A35%3A40.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#correlations

(100.0% in signature vs 24.52% overall) Addon "[e-mail]" = true
seems suspicious... What is this addon?

Comment 3

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

http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/dom/media/ipc/VideoDecoderChild.cpp#32-36

Do we also need to reject |mFlushPromise|? Or the call flow ensures it must've been resolved/rejected by the time destructor is called?

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 55 since this looks like a recent crash spike/regression first appearing in Nightly on 6/2.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Probably best to check with jya, but I can't see why we don't need to reject mFlushPromise (or the other new promises).

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> http://searchfox.org/mozilla-central/rev/
> 507743376d1ba753d14ab6b9305b7c6358570be8/dom/media/ipc/VideoDecoderChild.
> cpp#32-36
> 
> Do we also need to reject |mFlushPromise|? Or the call flow ensures it
> must've been resolved/rejected by the time destructor is called?

I wrote that code...

The flush promise should always be resolved in the destructor as the MediaFormatReader will always flush the decoder first, wait for the flush promise to be resolved and then call shutdown.

So there should be no need to resolve the flush promise in the destructor

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #2)
> https://crash-stats.mozilla.com/signature/?product=Firefox&version=55.
> 0a1&signature=mozilla%3A%3AMozPromise%3CT%3E%3A%3AChainTo&date=%3E%3D2017-05-
> 05T06%3A35%3A40.000Z&date=%3C2017-06-05T06%3A35%3A40.
> 000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_colum
> ns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-
> date&page=1#correlations
> 
> (100.0% in signature vs 24.52% overall) Addon "[e-mail]" = true
> seems suspicious... What is this addon?

[e-mail] is the Photon onboarding system addon.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #7)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #2)
> > https://crash-stats.mozilla.com/signature/?product=Firefox&version=55.
> > 0a1&signature=mozilla%3A%3AMozPromise%3CT%3E%3A%3AChainTo&date=%3E%3D2017-05-
> > 05T06%3A35%3A40.000Z&date=%3C2017-06-05T06%3A35%3A40.
> > 000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_colum
> > ns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-
> > date&page=1#correlations
> > 
> > (100.0% in signature vs 24.52% overall) Addon "[e-mail]" = true
> > seems suspicious... What is this addon?
> 
> [e-mail] is the Photon onboarding system addon.

Hi,  Fischer or Photon onboarding team,  is this on your radar?

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> > http://searchfox.org/mozilla-central/rev/
> > 507743376d1ba753d14ab6b9305b7c6358570be8/dom/media/ipc/VideoDecoderChild.
> > cpp#32-36
> > 
> > Do we also need to reject |mFlushPromise|? Or the call flow ensures it
> > must've been resolved/rejected by the time destructor is called?
> 
> I wrote that code...
> 
> The flush promise should always be resolved in the destructor as the
> MediaFormatReader will always flush the decoder first, wait for the flush
> promise to be resolved and then call shutdown.
> 
> So there should be no need to resolve the flush promise in the destructor

Does that mean we could have a patch to fix this according to comment 6?  Mind suggesting how to proceed this bug, Blake?

Comment 10

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

I expect this should have been fixed by jya in some other bug.

Comment 11

[Jean-Yves Avenard [:jya]]

have there been more of those crashes lately?
the crash report above stopped after build 20170606030207 as far as I can tell, which is similar to when bug 1370805 landed.