Closed Bug 828828 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: WebRTC, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox20 --- fixed
firefox21 --- fixed

People

(Reporter: posidron, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, Whiteboard: [getUserMedia][blocking-gum+][qa-])

Crash Data

Attachments

(4 files)

Attached file 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
Attached file callstack
Attached file SDP
Crash Signature: [@ mozilla::MediaEngineWebRTCAudioSource::Process]
Error I strongly suspect was due to 'mSources[i]' in Process, and that Stop() and other functions modifying mSources don't lock it
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.
Attachment #700222 - Flags: review?(tterribe)
Assignee: nobody → rjesup
Priority: -- → P1
Whiteboard: [getUserMedia][blocking-gum+][webrtc-uplift]
OS: Mac OS X → All
Hardware: x86_64 → All
Status: NEW → ASSIGNED
Keywords: assertion
Summary: WebRTC crash [@mozilla::MediaEngineWebRTCAudioSource::Process] → Assertion failure: i < Length() (invalid array index) and crash [@mozilla::MediaEngineWebRTCAudioSource::Process]
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.
Attachment #700222 - Flags: review?(tterribe) → review+
https://hg.mozilla.org/mozilla-central/rev/c3c557309182
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #700222 - Flags: approval-mozilla-aurora?
Whiteboard: [getUserMedia][blocking-gum+][webrtc-uplift] → [getUserMedia][blocking-gum+][webrtc-uplift][qa-]
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).
Attachment #700222 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite-
Fixed in Fx20+, removing webrtc-uplift flag
Whiteboard: [getUserMedia][blocking-gum+][webrtc-uplift][qa-] → [getUserMedia][blocking-gum+][qa-]
You need to log in before you can comment on or make changes to this bug.