807012 - WebRTC possible data race with CSF::CallControlManagerImpl::notifyDeviceEventObservers vs. sipcc::PeerConnectionCtx::Initialize

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

media/webrtc/signaling/src/callcontrol/CallControlManagerImpl.cpp:463

void CallControlManagerImpl::notifyDeviceEventObservers (ccapi_device_event_e deviceEvent, CC_DevicePtr devicePtr, CC_DeviceInfoPtr info)
{
    mozilla::MutexAutoLock lock(m_lock);
    set<CC_Observer*>::const_iterator it = ccObservers.begin();
    for ( ; it != ccObservers.end(); it++ )
    {
        (*it)->onDeviceEvent(deviceEvent, devicePtr, info);
    }
}

media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:83

  mCCM->addCCObserver(this);
  mDevice = mCCM->getActiveDevice();
  NS_ENSURE_TRUE(mDevice.get(), NS_ERROR_FAILURE);
  ChangeSipccState(PeerConnectionImpl::kStarting);
  return NS_OK;



Tested with m-c changeset: 111684:e19e170d2f6d

Comment 1

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

Ethan - can you verify if there's a real race here?  I doubt it, but you'd be better at being sure.

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: Signaling - reorder initiialization of CallControlManagerImpl

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 693032 [details] [diff] [review]
Signaling - reorder initiialization of CallControlManagerImpl


I don't think this is a particularly dangerous race on initialization since things work either way, but it can be avoided by setting the device in the ctx before adding it as an observer.

Comment 4

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac511266b2c

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2ac511266b2c