1971116 - Crash in [@ mozilla::detail::InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::GetUserMediaWindowListener::MuteOrUnmuteCameras] [Android]

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/98c5a835-2a31-4fcb-8d6c-65b000250607

MOZ_CRASH Reason:

ElementAt(aIndex = 2, aLength = 0)

Top 10 frames:

0  libmozglue.so  MOZ_CrashSequence(void*, long)  mfbt/Assertions.h:253
0  libmozglue.so  MOZ_Crash(char const*, int, char const*)  mfbt/Assertions.h:381
0  libmozglue.so  mozilla::detail::InvalidArrayIndex_CRASH(unsigned long, unsigned long)  mfbt/Assertions.cpp:50
1  libxul.so  nsTArray_Impl<RefPtr<mozilla::DeviceListener>, nsTArrayInfallibleAllocator>::...  xpcom/ds/nsTArray.h:1225
1  libxul.so  mozilla::ArrayIterator<RefPtr<mozilla::DeviceListener>&, nsTArray_Impl<RefPtr...  xpcom/ds/ArrayIterator.h:107
1  libxul.so  mozilla::GetUserMediaWindowListener::MuteOrUnmuteCameras(bool)  dom/media/MediaManager.cpp:4770
2  libxul.so  mozilla::MediaManager::OnCameraMute(bool)  dom/media/MediaManager.cpp:3572
3  libxul.so  mozilla::MediaManager::Observe(nsISupports*, char const*, char16_t const*)  dom/media/MediaManager.cpp:0
4  libxul.so  nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*)  xpcom/ds/nsObserverList.cpp:71
4  libxul.so  nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*)  xpcom/ds/nsObserverService.cpp:288

27 of these crashes in the last week, all on Fenix. We're crashing on this line which doesn't make any sense:
for (auto& l : mActiveListeners) {

Could this be a sign of a race, where another thread is modifying this array?

Comment 1

[Andrew McCreight [:mccr8]]

This is the only crash signature I see in the last month with GetUserMediaWindowListener in it, so there's something specifically going wrong with this function. Although maybe it just gets toggled vastly more often because it happens any time the browser goes in the background.

Comment 2

[Andrew McCreight [:mccr8]]

In most, but not all, cases, the length is 0, so maybe something else cleared out the array?

Comment 3

[Andrew McCreight [:mccr8]]

Could it also be that some code we run in this loop causes us to run content JS or otherwise spin the event loop, causing mActiveListeners to get cleared?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

It is expected that we can crash when doing iterator operations over a nsTArray, as we intentionally use the index getters so that we safely crash in this way if the array is mutated while we're iterating (rather than potentially UAF-ing: https://searchfox.org/mozilla-central/rev/b6a437e0e51dbbfa65f07ee4e1db3864624ab5ea/xpcom/ds/ArrayIterator.h#101-108).

The nsTArray<...> in this code should probably be a nsTObserverArray, with some thought put into what behaviour you want when elements are added/removed during the run (picking between ForwardRange() (will fire on newly-added listeners), EndLimitedRange() (will not fire on newly-added listeners), or BackwardRange() (runs backwards, doesn't include newly-added listeners)).

https://searchfox.org/mozilla-central/rev/b6a437e0e51dbbfa65f07ee4e1db3864624ab5ea/xpcom/ds/nsTObserverArray.h#74

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Another thing to note is that nsTObserverArray's iterators also do not keep the current entry alive while iterating, so you'll want to create a RefPtr<...> to the entry in the loop, and not just hold a reference.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(As a note, another option is to copy the array onto the stack and then iterate over the local copy, which fixes both problems without needing nsTObserverArray).

Comment 7

[Andreas Pehrson [:pehrsons]]

The problem seems to be that the path in question will synchronously dispatch an event, which will run JS, and e.g. MediaStreamTrack::Stop will synchronously remove a track from the array we are crashing on.

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1971116 - For global mute events, iterate on copies of containers. r?dbaker

Mute/unmute events are fired synchronously to content, which if it stops an
(event target) track in the event handler, may call back into and mutate the
containers we're iterating over.

Comment 9

[Andreas Pehrson [:pehrsons]]

Iterating over local copies is a pattern we already use, so sticking with that.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9072b0a5b23f
https://hg.mozilla.org/integration/autoland/rev/f50011f5fabd
For global mute events, iterate on copies of containers. r=dbaker

Comment 11

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

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

Comment 12

[Daniel Veditz [:dveditz]]

I see this crash signature going back to 115 versions, but there are no ESR-branches for Android so we can skip backporting the fix.