1397528 - Assertion failure: rv, at /home/worker/workspace/build/src/dom/media/MediaManager.cpp:2778

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 20170905-3ecda4678c49.

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: stacktrace.txt

Comment 2

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: dec7cb09336ee273f362ce0550a36ef70d5202d3
INFO: First bad revision: a7e5cdf5c07f2e0b61fb7f5d62a7f080a7df19fb
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=dec7cb09336ee273f362ce0550a36ef70d5202d3&tochange=a7e5cdf5c07f2e0b61fb7f5d62a7f080a7df19fb

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I cannot reproduce this issue. I've tried loading the file from disk, and from a local server.

Also, I'm doubtful the accused bug 1379091 is involved:
That bug is slightly modifying the possible size of MediaCache's in-memory storage.
But the trigger.html code should not even use MediaCache at all! I ran the test page with MOZ_LOG=MediaCache:5, and nothing came out from this component.

The assertion happens in code called from MediaDevices.enumerateDevices(), which is part of WebRTC.
MediaCache is on the playback side, which AFAICT is not happening in trigger.html.

Ryan: How did you get there? Is it reliable?

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Jan-Ivar, you probably have more knowledge about MediaManager&MediaDevices, would you see a link between the assertion in MediaManager and a change in MediaCache? (Or know someone who would know?) Thank you.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Hrm, not as sure as I thought I was. I'm getting different results now vs. what I was getting earlier. Turns out it runs a lot more reliably when opened from disk vs. running it from Bugzilla.

INFO: Last good revision: e3bceb6eb287275e123cb92c487696c6c7325096
INFO: First bad revision: 1fb2d6e0aa2d82c2db246ecd75f7225fecc449ed
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e3bceb6eb287275e123cb92c487696c6c7325096&tochange=1fb2d6e0aa2d82c2db246ecd75f7225fecc449ed

--> Bug 1320994

I was able to let the testcase run on rev e3bceb6eb287275e123cb92c487696c6c7325096 for a few minutes without crashing (when affected builds would typically crash within 10-15sec otherwise). And that culprit definitely looks a lot more plausible. Sorry for the confusion before.

On a side note, this testcase is pure spawn of Satan evil.

Comment 7

[Andreas Pehrson [:pehrsons]]

Thanks Ryan, this is definitely bug 1320994.

Looking at the code, OnNavigation() removes all the window's source listeners, while enumerateDevicesImpl when finished will remove one particular source listener. It's when the latter fails (because the former has already happened) that we fail the assert. Basically, when we navigate while enumerateDevices() is mid-flight.

I think the proper thing to do would be to have OnNavigation() *not* clean up inactive source listeners, and after it has happened it should prevent promoting those inactive ones to active.

jib, thoughts?

Perhaps we can do something more radical, like cancel all pending enumerateDevices?

Comment 8

[Tyson Smith [:tsmith]]

This issue is still being hit by fuzzers.

A Pernosco session can be found here: https://pernos.co/debug/TTZ9JloPY4lcxZg0kJes9g/index.html

Hopefully this will help get the issue resolved.

Comment 9

[Andreas Pehrson [:pehrsons]]

I think we know what's going on here, we just need to get around to fixing it.

I'll steal this, hoping to find more time than jib has over the last 2 years (-:

Comment 10

[Andreas Pehrson [:pehrsons]]

The pernosco recording confirms comment 7.

Comment 11

[Andreas Pehrson [:pehrsons]]

I think the simplest solution here is to mimic what other users of EnumerateDevicesImpl (i.e., GetUserMedia) do and abort the promise with AbortError, should the window have gone away already. We already do this for enumerateDevices if we're in shutdown.

Comment 12

[Andreas Pehrson [:pehrsons]]

Tyson, do you have a newer testcase that works on Nightly, that I could land as a crashtest?

The attachment here does a POST to mozilla.org which the test suite dislikes.

Comment 13

[Tyson Smith [:tsmith]]

Attachment: testcase.html

I removed the calls to open() and send() and the test case works locally.

Comment 14

[Andreas Pehrson [:pehrsons]]

That looks like the wrong testcase.

Comment 15

[Andreas Pehrson [:pehrsons]]

Simply removing the open() and send() calls didn't trigger this for me. I tested loading it from an http server, file://, and as a crashtest. On the other hand, the original testcase doesn't trigger the bug either, when I load it manually. Am I missing some prefs?

I'll upload the fix for now, and get back to the testcase should we find one that works.

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1397528 - Handle cleaned up windows in async enumerateDevices response. r?jib

Comment 17

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Oops you are right. Sorry about that.

Here try this, it uses localhost so when accessed via a local webserver it should trigger the assertion.

Comment 18

[Andreas Pehrson [:pehrsons]]

I am not able to reproduce with the testcase. I'll note that timing here relies on the set of cameras the host machine has available. Even if this would reproduce on try, that would be one brittle crashtest. I think we'll have to land without one.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b14e7bdee7a
Handle cleaned up windows in async enumerateDevices response. r=jib

Comment 20

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9b14e7bdee7a

Comment 21

[Karl Tomlinson (:karlt)]

The failing assertion was https://hg.mozilla.org/mozilla-central/annotate/3ecda4678c49/dom/media/MediaManager.cpp#l2778 (based on the stacktrace and revision).