Crash in webrtc::ViEInputManager::RegisterObserver

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
critical
Rank:
15
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: esthermonchari, Assigned: mchiang)

Tracking

({crash})

Trunk
mozilla52
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(crash signature)

Attachments

(2 attachments)

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.
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(mchiang)
Link to all crashes on trunk: http://bit.ly/2dJChSC. Moving to correct component.
Component: Untriaged → WebRTC: Audio/Video
Flags: needinfo?(mozillamarcia.knous)
Product: Firefox → Core
Version: 52 Branch → Trunk
Rank: 15
Priority: -- → P1
Assignee: nobody → mchiang
Flags: needinfo?(mchiang)
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.
Attachment #8799621 - Flags: review?(rjesup) → review+
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?
I don't see how these would be related, but I'll let Munro take a deeper look
Flags: needinfo?(mchiang)
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.
(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.
Flags: needinfo?(mchiang)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb8fca801fc8
protect the critical section accessing device_info_cs_; r=jesup
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bb8fca801fc8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
still found out the symptom
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Adding a related signature to crash stats field.
Crash Signature: [@ webrtc::ViEInputManager::RegisterObserver ] → [@ webrtc::ViEInputManager::RegisterObserver] [@ NS_ReadOptionalObject]
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 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?
Attachment #8801522 - Flags: review?(jib)
I think we still need the patch
Without it, there is some chance to hit race condition.
Attachment #8801522 - Flags: review?(rjesup) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7101334e6c7d
clear DeviceInfoDSSingleton::GetInfo() in DeviceInfoDS dtor to prevent illegal access crash; r=jesup,jib
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7101334e6c7d
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.