828828 - Assertion failure: i < Length() (invalid array index) and crash [@mozilla::MediaEngineWebRTCAudioSource::Process]

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Christoph Diehl [:posidron]]

Attachment: TraceSDP

I am seeing this assertion failure before the crash:

Assertion failure: i < Length() (invalid array index), at ../../../dist/include/nsTArray.h:611

Tested with m-c changeset: 118260:f60b87eed1ac and the patch of bug 786236 https://bugzilla.mozilla.org/attachment.cgi?id=698334

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Christoph Diehl [:posidron]]

Attachment: SDP

Comment 3

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

Attachment: Use monitor around all accesses to stream array

Error I strongly suspect was due to 'mSources[i]' in Process, and that Stop() and other functions modifying mSources don't lock it

Comment 4

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

Comment on attachment 700222 [details] [diff] [review]
Use monitor around all accesses to stream array

FYI, mSources in MediaEngineWebRTCVideo.cpp doesn't need this treatment since mSources for video isn't accessed from threads other than the MediaManager thread.

Comment 5

[Timothy B. Terriberry (:derf)]

Comment on attachment 700222 [details] [diff] [review]
Use monitor around all accesses to stream array

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

::: content/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +89,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  {
> +    ReentrantMonitorAutoEnter enter(mMonitor);

So, part of the problem here is that mMonitor doesn't document what it's supposed to be guarding access to. Please add some comments to MediaEngineWebRTC.h documenting what has to be accessed with the monitor held.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c557309182

Comment 7

[Ed Morley [:emorley]]

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

Comment 8

[Ed Morley [:emorley]]

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

Comment 9

[Ed Morley [:emorley]]

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

Comment 10

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

Comment on attachment 700222 [details] [diff] [review]
Use monitor around all accesses to stream array

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 811757

User impact if declined: random crashes or worse. Blocking for production release.

Testing completed (on m-c, etc.): on m-c, local testing.  As problem was found in ASAN, we can ask Christoph to report if the crashes are still happening.

Risk to taking this patch (and alternatives if risky): Very low, as this merely extends the use of an existing lock in the structure to account for the new cross-thread usage introduced by bug 811757.  There are no deadlock issues.

String or UUID changes made by this patch: none

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 700222 [details] [diff] [review]
Use monitor around all accesses to stream array

Approving for uplift to Aurora, given this impacts getUserMedia (first released in Firefox 20).

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a6bf87c333e

Comment 13

[Maire Reavy [:mreavy]]

Fixed in Fx20+, removing webrtc-uplift flag