1308792 - Crash in webrtc::ViEInputManager::RegisterObserver

Status: RESOLVED FIXED

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

Description

[Esther Monchari]

This bug was filed from the Socorro interface and is 
report bp-2eac13dc-a8ae-41bc-b6e0-1fdd12161008.

This was noted while looking at nightly crash stats.As far as I found out the crash associated multiple users. The crash started on 2016-10-08 14:10:22 with build id 20161007030207 and is still continuing.

adding ni on :marcia maybe help figure out how to place this in the right component

adding ni on :Munro Mengjue in case he has some ideas,since it appears he worked in that area.

Comment 1

[Marcia Knous [:marcia]]

Link to all crashes on trunk: http://bit.ly/2dJChSC. Moving to correct component.

Comment 2

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1308792 - protect the critical section accessing device_info_cs_;

Review commit: https://reviewboard.mozilla.org/r/84762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84762/

Comment 3

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

Comment on attachment 8799621 [details]
Bug 1308792 - protect the critical section accessing device_info_cs_;

https://reviewboard.mozilla.org/r/84762/#review83548

::: media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc:376
(Diff revision 1)
>      CriticalSectionScoped cs(observer_cs_.get());
>      if (observer_) {
>        LOG_F(LS_ERROR) << "Observer already registered.";
>        return -1;
>      }
>      observer_ = observer;
>    }
>  
> +  CriticalSectionScoped cs(device_info_cs_.get());
>    if (!GetDeviceInfo())
>      return -1;
>  
>    if (capture_device_info_ != NULL)
>      capture_device_info_->RegisterVideoInputFeedBack(*this);

Do we really need to have both critial sections held at once?  Can't we release one, then grab the other?  (I.e. must the two updates be effectively atomic?)  I always am very hesitant to write code that requires two semaphores/critical-sections/etc at the same time.

That said, this does appear to be safe; the critical sections are obtained in the same order.

Comment 4

[Marcia Knous [:marcia]]

I also see another signature that looks to be related - [@ webrtc::ViECaptureImpl::GetCaptureDevice] - will the patch address this crash as well: http://bit.ly/2dZiz3v?

Comment 5

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

I don't see how these would be related, but I'll let Munro take a deeper look

Comment 6

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8799621 [details]
Bug 1308792 - protect the critical section accessing device_info_cs_;

https://reviewboard.mozilla.org/r/84762/#review83548

> Do we really need to have both critial sections held at once?  Can't we release one, then grab the other?  (I.e. must the two updates be effectively atomic?)  I always am very hesitant to write code that requires two semaphores/critical-sections/etc at the same time.
> 
> That said, this does appear to be safe; the critical sections are obtained in the same order.

{
  CriticalSectionScoped cs(observer_cs_.get());
  if (observer_) {
    LOG_F(LS_ERROR) << "Observer already registered.";
    return -1;
  }
  observer_ = observer;
}

Yes, in this patch, we release one, then grab the other.
The first critical section is enclosed in braces.
CriticalSectionScoped dtor will leave the critical section automatically.

Comment 7

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Marcia Knous [:marcia - use ni] from comment #4)
> I also see another signature that looks to be related - [@
> webrtc::ViECaptureImpl::GetCaptureDevice] - will the patch address this
> crash as well: http://bit.ly/2dZiz3v?

This should be different issues.
It may be related to bug 1308114. 
I have filed bug 1309456 to trace this symptom.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb8fca801fc8
protect the critical section accessing device_info_cs_; r=jesup

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

still found out the symptom

Comment 11

[Marcia Knous [:marcia]]

Adding a related signature to crash stats field.

Comment 12

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

The crash occurs at the second time of creating DeviceInfoDS.

In bug 1300468 (https://hg.mozilla.org/mozilla-central/rev/18722be58f18), we use DeviceInfoDSSingleton to store the pointer of DeviceInfoDS object.
However, we forgot to set DeviceInfoDSSingleton::GetInfo() to NULL in DeviceInfoDS dtor when the DeviceInfoDS object is destroyed by ViEInputManager.

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc#52

So the crash occurs when we create DeviceInfoDS again since DeviceInfoDSSingleton::GetInfo() will return an address which has been deleted.

I suspect all recent crashes is due to this bug.

Comment 13

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug-1308792-clear-DeviceInfoDSSingleton-GetInfo.patch

Comment 14

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

Comment on attachment 8801522 [details] [diff] [review]
Bug-1308792-clear-DeviceInfoDSSingleton-GetInfo.patch

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

Should the previous patch be backed out, or was it something that was required?

Comment 15

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

I think we still need the patch
Without it, there is some chance to hit race condition.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7101334e6c7d
clear DeviceInfoDSSingleton::GetInfo() in DeviceInfoDS dtor to prevent illegal access crash; r=jesup,jib

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7101334e6c7d