1547381 - Crash in [@ mozilla::GetUserMediaWindowListener::StopRawID]

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-6d1f4d70-878c-4b82-874d-7b4800190421.

Seen while looking at nightly crash stats: https://bit.ly/2ZEEq8b - started in 20190416220148. The address looks similar to ones in Bug 1540434, and the regression range seems to match the fix in that bug. 16 crashes/11 installs so far.

One comment "just joined a w3c teleconference on mit.webex.com"

Top 10 frames of crashing thread:

0 xul.dll void mozilla::GetUserMediaWindowListener::StopRawID dom/media/MediaManager.cpp:4633
1 xul.dll static void mozilla::MediaManager::IterateWindowListeners<`lambda at z:/task_1555448790/build/src/dom/media/MediaManager.cpp:2221:27'> dom/media/MediaManager.cpp:3949
2 xul.dll static void mozilla::MediaManager::OnDeviceChange::<unnamed-tag>::operator dom/media/MediaManager.cpp:2219
3 xul.dll void mozilla::MozPromise<bool, RefPtr<mozilla::MediaMgrError>, 1>::ThenValue<`lambda at z:/task_1555448790/build/src/dom/media/MediaManager.cpp:2187:17', `lambda at z:/task_1555448790/build/src/dom/media/MediaManager.cpp:2228:17'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:717
4 xul.dll nsresult mozilla::MozPromise<RefPtr<AudioDeviceInfo>, nsresult, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:393
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1180
6 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
7 xul.dll void mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:110
8 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
9 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290

Comment 1

[Andreas Pehrson [:pehrsons]]

I'll be away for the next few weeks. Paul, can you give this some traction in the meantime?

Comment 2

[Paul Adenot (:padenot)]

jib, I don't understand why we're suddenly using UniquePtr here, shouldn't this be RefPtr as well for mVideoDeviceState ?

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The UniquePtr is to DeviceState, an intermediary struct Andreas added. It's not refcountable. The only reason it's not a flat member embedded in the class seems to be for constructor semantics since it's initialized late in Allocate().

The VideoDevice is still a RefPtr inside DeviceState in mDevice.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

We loop over mActiveListeners here in StopRawID:

void GetUserMediaWindowListener::StopRawID(const nsString& removedDeviceID) {
  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread");

  for (auto& source : mActiveListeners) {
    if (source->GetAudioDevice()) {
      nsString id;
      source->GetAudioDevice()->GetRawId(id);
      if (removedDeviceID.Equals(id)) {
        source->StopTrack(kAudioTrack);
      }
    }
=>  if (source->GetVideoDevice()) {
      nsString id;
      source->GetVideoDevice()->GetRawId(id);
      if (removedDeviceID.Equals(id)) {
        source->StopTrack(kVideoTrack);
      }
    }
  }
}

Two things strike me:

1. StopTrack() calls Stop() which sometimes removes itself from the array we're iterating over. How does nsTArray react to that?
2. The docs on StopTrack says "any caller will need to keep its own hard ref"—which we're not doing here.

I suspect #2, that the first StopTrack() above in the audio part removed the last refcount on the SourceListener, leaving source pointing at freed memory on the next line. A UAF. Seems hard to exploit, but hiding it just in case.

The only thing I don't get is why this wouldn't happen more often. Maybe a special situation is needed for the refcount to be near 0? Or some code changed elsewhere making the overall refcount lower than before?

Comment 5

[Paul Adenot (:padenot)]

Can we try to fix either or both of those and see what's the result?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1547381 - Call StopTrack() in the right order.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/cf38c251e2e643abd7b4bbc994f141a5dd174c5b
https://hg.mozilla.org/mozilla-central/rev/cf38c251e2e6