802982 - Crash with excessive getUserMedia calls [crashtest checkin]

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Jesse Ruderman]

Attachment: testcase (usually crashes)

With:
  user_pref("media.navigator.enabled", true);

The testcases causes a bunch of invalid frees, and usually crashes.

(Also: so many threads!)

Comment 1

[Jesse Ruderman]

Attachment: stack (b malloc_error_break)

Nightly: bp-3547f59f-8322-4069-9ab8-0d6472121018

Comment 2

[Jesse Ruderman]

Hmm, this testcase ended up pretty similar to the one in bug 798829.

Comment 3

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

The threading thing is already covered by bug 798323. But looks like that you hit the crash I was looking for. I will add it as dependency.

Comment 4

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

I still can't get it to crash with the latest nightly build on OS X.

Comment 5

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

I see some of these on a linux build:

322660160[7f4213161150]: WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8007000E: file nsThreadUtils.cpp, line 58
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8007000E: file nsThreadUtils.cpp, line 58
322660160[7f4213161150]: WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8007000E: file ../../../dom/media/MediaManager.cpp, line 875
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8007000E: file ../../../dom/media/MediaManager.cpp, line 875
322660160[7f4213161150]: WARNING: NS_ENSURE_TRUE(thread) failed: file ../../../xpcom/threads/nsThreadPool.cpp, line 83
WARNING: NS_ENSURE_TRUE(thread) failed: file ../../../xpcom/threads/nsThreadPool.cpp, line 83

when repeatedly reloading the testcase.  Which isn't horribly surprising.  When reopening and FF loaded it as the last page loaded, I got a string of these from the hash code:

-333572352[7f9d078fe570]: ###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.cpp, line 574
###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.cpp, line 574
-585353472[7f9cfe5c06a0]: ###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.cpp, line 574
###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.cpp, line 574

The backtrace for those is:
#0  NS_DebugBreak_P (aSeverity=1, aStr=0x7ffff50181cb "RECURSION_LEVEL(table_) > 0", aExpr=
    0x7ffff50181b0 "RECURSION_LEVEL(table) > 0", aFile=0x7ffff501811a "pldhash.cpp", aLine=669)
    at ../../../xpcom/base/nsDebugImpl.cpp:286
#1  0x00007ffff390bed0 in PL_DHashTableOperate (table=0x7fffde156d50, key=0x7fff5bb36a00, op=
    PL_DHASH_LOOKUP) at pldhash.cpp:669
#2  0x00007ffff2da0ef3 in nsTHashtable<nsBaseHashtableET<nsStringHashKey, nsRefPtr<mozilla::MediaEngineWebRTCAudioSource> > >::GetEntry (this=0x7fffde156d50, aKey=...)
    at ../../../dist/include/nsTHashtable.h:148
#3  0x00007ffff2da0a27 in nsRefPtrHashtable<nsStringHashKey, mozilla::MediaEngineWebRTCAudioSource>::Get (this=0x7fffde156d50, aKey=..., pRefPtr=0x7fff5bb36ba0)
    at ../../../dist/include/nsRefPtrHashtable.h:83
#4  0x00007ffff2da05ff in mozilla::MediaEngineWebRTC::EnumerateAudioDevices (this=0x7fffde156d00, 
    aASources=0x7fff5bb36c30) at ../../../../content/media/webrtc/MediaEngineWebRTC.cpp:165
#5  0x00007ffff2aaa76d in mozilla::GetUserMediaDevicesRunnable::Run (this=0x7fffdee98130)
    at ../../../dom/media/MediaManager.cpp:623
#6  0x00007ffff397a408 in nsThread::ProcessNextEvent (this=0x7fffba2ed7a0, mayWait=true, result=
    0x7fff5bb36def) at ../../../xpcom/threads/nsThread.cpp:612
#7  0x00007ffff3909b6e in NS_ProcessNextEvent_P (thread=0x7fffba2ed7a0, mayWait=true)
    at nsThreadUtils.cpp:220
#8  0x00007ffff39792ce in nsThread::ThreadFunc (arg=0x7fffba2ed7a0)
    at ../../../xpcom/threads/nsThread.cpp:256
#9  0x00007ffff7ae70c9 in _pt_root (arg=0x7fffba2e4020)
    at ../../../../../nsprpub/pr/src/pthreads/ptthread.c:156
#10 0x0000003151a07b41 in start_thread (arg=0x7fff5bb37700) at pthread_create.c:305
#11 0x00000031516e0e6d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115

This appears to be a lack of locking for access to EnumerateAudioDevices/EnumerateVideoDevices.  I'll spawn a normal bug for that.


None more on reload, and eventually we hit the thread failures.

Also saw a few errors from nsContentUtils.cpp nsCxPusher::Push()

Comment 6

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

Spawned lock issue as bug 803093

Comment 7

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

Jesse - please retest with the bug 803093 patch (currently on inbound).  It's still leaking threads, but I'm interested if the invalid frees go away.  Thanks

Comment 8

[Jesse Ruderman]

Yeah, this testcase is doing okay now that bug 780790 and bug 803093 are fixed.

I'm still seeing lots of leaks.  And once, after reloading the testcase many times and quitting, I managed to hit:

Assertion failure: 0 == rv, at nsprpub/pr/src/pthreads/ptsynch.c:162

But I'm expecting bug 798323 to take care of that.

Comment 9

[Jason Smith [:jsmith]]

Marking verified per Jesse's comment in comment 8.

Comment 10

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 11

[Jason Smith [:jsmith]]

Comment on attachment 687659 [details] [diff] [review]
Crashtest v1

This is an exact copy of Jesse's testcase as a crashtest.

Comment 12

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

Comment on attachment 687659 [details] [diff] [review]
Crashtest v1

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

Just one note. Please stick to the same coding style as the other tests have. That makes it easier for contributors and lessens the confusion. Thanks.

Comment 13

[Jesse Ruderman]

If there are coding styles for crashtests, I'd like to know what they are!

Comment 14

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

(In reply to Jesse Ruderman from comment #13)
> If there are coding styles for crashtests, I'd like to know what they are!

It's most likely off-topic here but just in short.  There are no specific coding styles for crashtests but for js code as you know. I know we enforce this only for core code but it would help a lot if at least we could take care of and make it easier for contributors to step in.

Comment 15

[Jason Smith [:jsmith]]

The only thing I see consistent style-wise on these tests I think I already did, which was:

1. Add a reference to the crash bug number in a comment
2. Use a lot of the same boilerplate HTML
3. Use a useful name for the test

Other things (like indentation) even vary by the test. I'm not inclined to think this is critical for a crashtest (a mochitest it would be, however).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10a3208b7a9d

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/10a3208b7a9d