837037 - Frequent Windows test_peerConnection_basicAudio.html | application crashed [@ 0x1cf3c0] [@ 0x12f91a] [@ 0x2ff968] [@ 0x12fa1d] [@ sipcc::PeerConnectionImpl::IceGatheringCompleted_m]

Status: RESOLVED FIXED

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

Description

[Phil Ringnalda (:philor)]

+++ This bug was initially created as a clone of Bug #837026 +++

Almost certainly the Windows flavor of the Mac/Linux bug 837026.

Stacks are all ugly and random looking above sipcc::PeerConnectionImpl::IceGatheringCompleted_m

https://tbpl.mozilla.org/php/getParsedLog.php?id=19345804&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=19345920&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=19342506&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=19342582&tree=Mozilla-Inbound

PROCESS-CRASH | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudio.html | application crashed [@ 0x1cf3c0]
Crash dump filename: c:\users\cltbld\appdata\local\temp\tmpb3gssn\minidumps\8dcd512c-c692-42ab-b922-ac8070af4d88.dmp
Operating system: Windows NT
                  6.1.7600 
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0x2

Thread 0 (crashed)
 0  0x1cf3c0
    eip = 0x001cf3c0   esp = 0x001cf3a8   ebp = 0x259baf08   ebx = 0x6c46e0f0
    esi = 0x00000002   edi = 0x013caf98   eax = 0x00000000   ecx = 0x00000008
    edx = 0x6bbc15b1   efl = 0x00210246
    Found by: given as instruction pointer in context
 1  xul.dll!sipcc::PeerConnectionImpl::IceGatheringCompleted_m(mozilla::NrIceCtx *) [PeerConnectionImpl.cpp:fbcff2d6edc8 : 1289 + 0x27]
    eip = 0x6bbc4b82   esp = 0x001cf3c8   ebp = 0x259baf08
    Found by: stack scanning
 2  MSVCP100D.dll + 0x1d1b
    eip = 0x72ee1d1c   esp = 0x001cf3e0   ebp = 0x259baf08
    Found by: stack scanning
 3  xul.dll!BloatEntry::AccountRefs() [nsTraceRefcntImpl.cpp:fbcff2d6edc8 : 275 + 0x12]
    eip = 0x6ba0ad33   esp = 0x001cf428   ebp = 0x259baf08
    Found by: stack scanning
 4  MSVCP100D.dll + 0x1b77
    eip = 0x72ee1b78   esp = 0x001cf42c   ebp = 0x259baf08
    Found by: stack scanning
 5  nspr4.dll + 0xe89f
    eip = 0x734de8a0   esp = 0x001cf490   ebp = 0x259baf08
    Found by: stack scanning
 6  xul.dll!mozilla::runnable_args_m_1<nsRefPtr<sipcc::PeerConnectionImpl>,tag_nsresult ( sipcc::PeerConnectionImpl::*)(mozilla::NrIceCtx *),mozilla::NrIceCtx *>::Run() [runnable_utils_generated.h:fbcff2d6edc8 : 122 + 0x15]
    eip = 0x6bbc16b7   esp = 0x001cf4ac   ebp = 0x259baf08
    Found by: stack scanning
 7  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:fbcff2d6edc8 : 627 + 0xd]
    eip = 0x6b9fe0a5   esp = 0x001cf4bc   ebp = 0x259baf08
    Found by: stack scanning
 8  xul.dll!NS_ProcessNextEvent_P(nsIThread *,bool) [nsThreadUtils.cpp:fbcff2d6edc8 : 238 + 0xc]
    eip = 0x6b9a991b   esp = 0x001cf4f0   ebp = 0x259baf08

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is not a null deref crash and sounds security sensitive to me.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Crash address is 0x2; that's a null-pointer deref, but some of the other linked crashes have bizzarro addresses.

I *think* I know the issue: the nricectx signals SignalGatheringCompleted(this), which then gets proxied to pcmedia's SignalIceGatheringCompleted, and to PeerConnectionImpl's IceGatheringCompleted.

That ships off a runnable (async) with a refptr to the PeerCOnnectionImpl - but not the the icectx or the PeerConnectionMedia holding it.  I believe (due to checks elsewhere) that PeerConnectionMedia can go away without the PeerConnection going away.  and if the runnable is still outstanding...

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry but I didn't wanted to reference the 0x2 address with my last comment but the other those from the other reports given in comment 0 like:

Crash reason:  EXCEPTION_PRIV_INSTRUCTION
Crash address: 0x12fa1d

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: hold refptr to NrIceCtx in IceGathering runnables

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 709316 [details] [diff] [review]
hold refptr to NrIceCtx in IceGathering runnables

I think holding the refptr is a good idea here... Though the targeted functions don't seem to do anything with aCtx other than write debugs (but those may be what's causing the crash!).

ekr - is my analysis reasonable?

Comment 6

[Eric Rescorla (:ekr)]

jesup:

1. Rather than trying to keep the NrIceCtx alive, I think it would be better to just remove the argument, since,
as you say, we're not doing anything.

2. I'm unconvinced that this is the source of the problem. We don't have a line number, but as far as I can
tell, the only thing that IceCompleted_m does with aCtx is log its value. As far as I know, it's generally
safe to print the value of pointers that don't currently point to valid memory [I suspect it's technically
underfined behavior, but safe in most implementations.]

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

yeah.  The crashes went away with the Clobber, which concerns me, but a bunch of weird stuff in this area went away after the clobber.  I was looking at this to try to blame it on needing-clobber, but when I looked I realized that we were passing a referenced pointer without a reference.  If we have no intention of using it, removing it is good; if we think we will at time time in the future, we could make it a RefPtr as in the patch.  Either is good by me, but we should do one.

Comment 8

[Eric Rescorla (:ekr)]

I do not believe we need it. Let's remove it.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: hold refptr to NrIceCtx in IceGathering runnables

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 709446 [details] [diff] [review]
hold refptr to NrIceCtx in IceGathering runnables

Removed Ctx from the main-thread runnables

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: hold refptr to NrIceCtx in IceGathering runnables

Comment 12

[Eric Rescorla (:ekr)]

Comment on attachment 709488 [details] [diff] [review]
hold refptr to NrIceCtx in IceGathering runnables

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

Please be sure to change the commit message, since the title of this patch is inaccurate.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/153ec028905a

Comment 14

[Jason Smith [:jsmith]]

Marking in-testsuite+ - existing mochitest automation will already cover this.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/153ec028905a