1150271 - WebRTC session crashes in mozilla::MediaEngineGonkVideoSource::StartImpl()

Status: RESOLVED FIXED

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

Description

[Diego Wilson [:diego]]

There's a hard to reproduce null dereference crash in MediaEngineGonkVideoSource.

My best guess is it's we need to null check mCameraControl [1]

[1] https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineGonkVideoSource.cpp#473

Comment 1

[Diego Wilson [:diego]]

[Blocking Requested - why for this release]:

Comment 2

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.116
Moz BuildID: 20150323002504
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.116.xml?h=release
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=7f367fc98ffdd183f21d2cdfe20556ab877ece34
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=3970b1e517b6447237421c1f9e7845c00bde0838
Patches: bug 1133398, bug 1143694, bug 1146987, bug 1145724, bug 1147646, bug 1133147, bug 1142770

Comment 3

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.116

Comment 4

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.116

Comment 5

[Diego Wilson [:diego]]

Sotaro,

Looks like there's a race condition somewhere here. Does the mCameraControl null check make sense?

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #5)
> Looks like there's a race condition somewhere here. Does the mCameraControl
> null check make sense?

I just looked the source, the implementation seems very tricky. I am going to look into more.

Comment 7

[Kai-Chih Hu [:khu]]

Could we know if there is any update? Thanks.

Comment 8

[Sotaro Ikeda [:sotaro]]

The following functions could be related to the crash. They have a common characteristics. They are called on media thread(MediaManager's thread) and actual task is done on main thread. And They synchronously wait the task's completion.

- MediaEngineGonkVideoSource::Allocate()
    - allocate ICameraControl
- MediaEngineGonkVideoSource::Start()
    - Trigger MediaEngineGonkVideoSource::StartImpl() call.
- MediaEngineGonkVideoSource::Deallocate()
    - Clear ICameraControl pointer.

Comment 9

[Sotaro Ikeda [:sotaro]]

ICameraControl does not call valid callback function until ICameraControl::Start() is called. Therefore, it seems not possible that ICameraControl is cleared between MediaEngineGonkVideoSource::Start() and MediaEngineGonkVideoSource::StartImpl().

Instead it seems possible that ICameraControl is nullptr when MediaEngineGonkVideoSource::Start() is called. MediaEngineGonkVideoSource::Start() does not check ICameraControl pointer.

Comment 10

[Sotaro Ikeda [:sotaro]]

FYI: The following is a diagram around MediaManager on gonk.

https://github.com/sotaroikeda/firefox-diagrams/blob/master/media/dom_media_MediaManager_FirefoxOS_2_2.pdf

Comment 11

[Sotaro Ikeda [:sotaro]]

Comment 9 is just one possibility of the crash. Therefore I am going to create a new bug for it.

Comment 12

[Sotaro Ikeda [:sotaro]]

Diego, can you confirm if the problem still happens?

Comment 13

[Diego Wilson [:diego]]

Attachment: 0001-Bug-1150271-WebRTC-session-crashes-in-mozilla-MediaE.patch

Sotaro,

We are currently patching gecko to avoid this crash

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #13)
> Created attachment 8592308 [details] [diff] [review]
> 0001-Bug-1150271-WebRTC-session-crashes-in-mozilla-MediaE.patch
> 
> Sotaro,
> 
> We are currently patching gecko to avoid this crash

diego, can it be removed? If the code of MediaEngineGonkVideoSource works as expected, this bugs fix should prevent the crash.

Comment 15

[Diego Wilson [:diego]]

OK, I'll remove the patch from our builds and wait and see if the crash comes up again.

Comment 16

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 17

[Diego Wilson [:diego]]

OK. I removed the patch from our builds. I'll mark this bug as resolved and reopen if I see the crash again. Thanks for your help sotaro!

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #17)
> OK. I removed the patch from our builds. I'll mark this bug as resolved and
> reopen if I see the crash again. Thanks for your help sotaro!

No problem :-)