1396419 - Crash in mozilla::PeerConnectionImpl::ExecuteStatsQuery_s

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-de4a6ebb-1724-4ab4-bef6-874ad0170903.
=============================================================
Crashing Thread (11), Name: Socket Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::PeerConnectionImpl::ExecuteStatsQuery_s(mozilla::RTCStatsQuery*) 	media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3688
1 	xul.dll 	mozilla::EverySecondTelemetryCallback_s 	media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:228
2 	xul.dll 	mozilla::runnable_args_func<void (*)(nsAutoPtr<std::deque<mozilla::TransportLayer*, std::allocator<mozilla::TransportLayer*> > >), nsAutoPtr<std::deque<mozilla::TransportLayer*, std::allocator<mozilla::TransportLayer*> > > >::Run() 	media/mtransport/runnable_utils.h:121
3 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1446
4 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:480
5 	xul.dll 	mozilla::net::nsSocketTransportService::Run() 	netwerk/base/nsSocketTransportService2.cpp:976
6 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1446
7 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:480
8 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:339
9 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:319
10 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:299
11 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:506
12 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:397
13 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:95
14 	ucrtbase.dll 	thread_start<unsigned int (__stdcall*)(void*)> 	
15 	kernel32.dll 	BaseThreadInitThunk 	
16 	mozglue.dll 	patched_BaseThreadInitThunk 	mozglue/build/WindowsDllBlocklist.cpp:815
17 	ntdll.dll 	__RtlUserThreadStart 	
18 	ntdll.dll 	_RtlUserThreadStart

crashes with this signature seem to be regressing in firefox 55 and their volume is getting worse during the 56.0b cycle.

Comment 1

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 2

[Michael Froman [:mjf]]

I'll take a look.

Comment 3

[Michael Froman [:mjf]]

Attachment: Bug 1396419 - adding null checks to avoid intermittent crash.

Review commit: https://reviewboard.mozilla.org/r/181484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181484/

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8910007 [details]
Bug 1396419 - adding null checks to avoid intermittent crash.

https://reviewboard.mozilla.org/r/181484/#review186750

Any idea how we get into the situation where the pipeline or it's conduit are gone?

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8910007 [details]
Bug 1396419 - adding null checks to avoid intermittent crash.

https://reviewboard.mozilla.org/r/181484/#review186762

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3716
(Diff revision 1)
>    ASSERT_ON_THREAD(query->iceCtx->thread());
>  
>    // Gather stats from pipelines provided (can't touch mMedia + stream on STS)
>  
>    for (size_t p = 0; p < query->pipelines.Length(); ++p) {
> +    MOZ_ASSERT(query->pipelines[p]);

Drive-by, unrequested comment that this will still crash in debug builds because of the asserts. Is that desirable?

Comment 6

[Michael Froman [:mjf]]

Comment on attachment 8910007 [details]
Bug 1396419 - adding null checks to avoid intermittent crash.

https://reviewboard.mozilla.org/r/181484/#review186750

No, and that was my reason for adding the asserts (see below reply to Nico).

Comment 7

[Michael Froman [:mjf]]

Comment on attachment 8910007 [details]
Bug 1396419 - adding null checks to avoid intermittent crash.

https://reviewboard.mozilla.org/r/181484/#review186762

> Drive-by, unrequested comment that this will still crash in debug builds because of the asserts. Is that desirable?

I will remove them if you feel strongly, but because I don't know why we're getting into this state I'd like a debug build to assert to potentially catch it.  I suspect this is a teardown timing issue, but have nothing to base that on but gut feeling.

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8910007 [details]
Bug 1396419 - adding null checks to avoid intermittent crash.

https://reviewboard.mozilla.org/r/181484/#review186762

> I will remove them if you feel strongly, but because I don't know why we're getting into this state I'd like a debug build to assert to potentially catch it.  I suspect this is a teardown timing issue, but have nothing to base that on but gut feeling.

Just double checking the intent. Dropping.

Comment 9

[Michael Froman [:mjf]]

After looking at the distribution in the crash reports, it looks like we've had only 2 crashes in 57 (~2%), ~36% in 55, and the remaining ~62% in 56.  I see 56 is marked as won't fix, but should we reconsider that?

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

56 RC was cut on Monday (Sept. 18th).

Comment 11

[Michael Froman [:mjf]]

Comment on attachment 8910007 [details]
Bug 1396419 - adding null checks to avoid intermittent crash.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181484/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/648e06c8b1b5
adding null checks to avoid intermittent crash. r=drno

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/648e06c8b1b5

Comment 14

[[:philipp]]

crashes with this signature are still ongoing in 57beta - do we need to reopen this bug or should a new one be filed?

Comment 15

[Michael Froman [:mjf]]

I've been watching this - we have a trio of what I'm beginning to suspect are related.  This bug, Bug 1389793, and a new one, Bug 1405940.  I'm starting with 1405940 now.

Comment 16

[Nils Ohlmeier [:drno]]

Here is a another one which apparently has the new asserts and continue statements in it, but still crashed at the old place: https://crash-stats.mozilla.com/report/index/ae6e78c7-c50c-43fc-846d-783ef0171205#tab-details

I guess that means something destroys the conduit while the stats query is going it's loop.

I'm also wondering if this has the same root cause as bug 1389793.

Michael should we re-open this bug, or do we continue over in bug 1389793?

Comment 17

[Michael Froman [:mjf]]

I think continuing in Bug 1389793 is probably sufficient.