1818283 - A media element can be erroneously garbage-collected when autoplaying an inactive MediaStream

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

This is causing intermittent failures in webrtc/protocol/rtp-demuxing.html and webrtc/protocol/bundle.https.html wpt.

This is particularly prevalent on Linux 18.04 x64 WebRender tsan opt, OS X 10.15 WebRender debug, and Windows 10 x86 2004 WebRender debug.

Comment 1

[Byron Campen [:bwc]]

I know that frames are making it this far:

https://searchfox.org/mozilla-central/rev/75da1dd5d4b9b991f919a41594194eab93cdef62/dom/media/webrtc/transportbridge/MediaPipeline.cpp#1566

And I know that this is getting at least this far:

https://searchfox.org/mozilla-central/rev/75da1dd5d4b9b991f919a41594194eab93cdef62/dom/media/MediaSegment.h#395

For whatever reason, those frames never make it to the video element. Any ideas on what I should be looking for here?

Comment 2

[Byron Campen [:bwc]]

It looks like this happens regardless of whether it is the same stream or not. So all of our wpt that involve more than one video element are flaky for some reason.

Comment 3

[Andreas Pehrson [:pehrsons]]

Do we wire up the right track here?
If yes, do we see the frames on the track's VideoOutput and do they end up in the video element's VideoFrameContainer?

I suppose catching this in rr/pernosco is near impossible?

Comment 4

[Byron Campen [:bwc]]

This bug goes away if I add very much logging, and I really doubt it would persist under rr. I'll see if I can log the stuff you suggest without making the bug disappear.

Comment 5

[Andreas Pehrson [:pehrsons]]

Ugh. Might need a few select statements to pinpoint this then.

Comment 6

[Byron Campen [:bwc]]

I added a MOZ_CRASH at the offending RemoveTrack call, and it is in unlink!

https://treeherder.mozilla.org/logviewer?job_id=407199735&repo=try&lineNumber=112225

Comment 7

[Byron Campen [:bwc]]

This is probably also causing failures (at least on TV) in setParameters-active.https.html.

Comment 8

[Byron Campen [:bwc]]

Not all of the oranges here are due to hitting the MOZ_CRASH mentioned in comment 6, but most of them are. All seem to involve this early unlink, either in webrtc/protocol/rtp-demuxing.html or webrtc/simulcast/setParameters-active.https.html.

https://treeherder.mozilla.org/jobs?repo=try&revision=460c3f0e76975f5c6bdc2933280222f7416d7697

Comment 9

[Andreas Pehrson [:pehrsons]]

The important points here are:

• The media element's srcObject attribute is set to a MediaStream that the application controls
• The MediaStream is inactive
• The media element is paused
• The media element has the autoplay attribute set

This makes it eligible for collection.

But it shouldn't be, because if the application adds a live track to the MediaStream the media element should be playing, and potentially emitting both events and audio.

Probably easiest to add an edge from the MediaStream to the media element. This would make the media element collectable only when the MediaStream is collectable.

Comment 10

[Byron Campen [:bwc]]

I think this might be rearing its head again. I'm again seeing video elements in simulcast tests fail to fire loadedmetadata events, after bug 1870477 landed. If I remove the wait for loadedmetadata, the tests work fine, passing these checks:

https://searchfox.org/mozilla-central/source/dom/media/webrtc/tests/mochitests/test_peerConnection_simulcastOffer.html#80-93

So the video elements are getting frames and know their resolution, they just never fired a loadedmetadata event? This is a fairly easy fix if we want the tests to stop failing, but still, the lack of the event indicates a problem.

Comment 11

[Byron Campen [:bwc]]

Looking at the HTMLMediaElement code, I see multiple places where we can state transition to HAVE_METADATA (or further) without firing the event. Is this intended? A quick glance at the spec indicates that we should fire the event when "readyState is newly equal to HAVE_METADATA or greater for the first time" I don't know if this is why we're having this problem, but it is easy enough to check.

Comment 12

[Byron Campen [:bwc]]

Yeah, that doesn't pan out. I'm going to try adjusting the test case to proceed on 'playing' and 'loadeddata' as well as 'loadedmetadata'.

Comment 13

[Byron Campen [:bwc]]

Those events aren't firing either.

Comment 14

[Byron Campen [:bwc]]

Ok, I think I've found a good workaround. Appending the video elements to the document seems to resolve the problem, which is something we ought to be doing anyway so they're visible.

Comment 15

[Byron Campen [:bwc]]

Attachment: Bug 1818283: Test that GC doesn't collect a video element with a webrtc src stream. r?pehrsons

Comment 16

[Byron Campen [:bwc]]

Attachment: Bug 1818283: Make DOMMediaStream::TrackListener cycle-collected. r?pehrsons

This prevents HTMLMediaElements from being prematurely cycle-collected, and
also makes things a bit safer.

Depends on D205733

Comment 17

[Byron Campen [:bwc]]

Try looks ok:

https://treeherder.mozilla.org/jobs?repo=try&revision=6454a32160c06e54964a0563ddb0a76ee6037377
https://treeherder.mozilla.org/jobs?repo=try&revision=4c44f9804de33637149392d25f81dc0ab7f98778
https://treeherder.mozilla.org/jobs?repo=try&revision=5e6dad5f0cdad2f04f27068d17cb053f650379e3

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae207b1ed783
Test that GC doesn't collect a video element with a webrtc src stream. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/d8a7cdb9f981
Make DOMMediaStream::TrackListener cycle-collected. r=pehrsons

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/45585 for changes under testing/web-platform/tests

Comment 20

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/ae207b1ed783
https://hg.mozilla.org/mozilla-central/rev/d8a7cdb9f981

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot