1681025 - Firefox crashes when watching/expanding stopped RTCRtpTransceiver

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[edvinv]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.66 Safari/537.36

Steps to reproduce:

When debugging webrtc application with devtools, I enter following in console:

pc.getTransceivers()

All transceivers are listed correctly but if I try to expand transceiver with property stopped=true, firefox crashes. Expanding not stopped transceivers works ok. This issue is 100% repeatable.

Actual results:

Firefox crashes with message:
Your tab just crashed.
We can help!

Choose Restore This Tab to reload the page. etc..

Expected results:

Stopped transceiver should be normally watched/expanded.

Comment 1

[Daniel Bodea [:danibodea]]

I am trying to reproduce your issue by opening a webrtc room, like whereby or talky and pasting "pc.getTransceivers()" into the devetools console.
Unfortunately, I get the error that "ReferenceError: pc is not defined". Can you help me reproduce it?
How exactly do you do it?

Comment 2

[edvinv]

Sorry, maybe explanation was not very good. You must already have valid pc (peer connection) with some stopped transceivers. So here is complete code to reproduce:

async function test() {
	try {
		const stream = await navigator.mediaDevices.getUserMedia({ 'video': true, 'audio': true });
		console.log('Got MediaStream:', stream);
		const configuration = { 'iceServers': [{ 'urls': 'stun:stun.l.google.com:19302' }] }
		const peerConnection = new RTCPeerConnection(configuration);
		peerConnection.addTrack(stream.getVideoTracks()[0], stream);
		peerConnection.addTrack(stream.getAudioTracks()[0], stream);

		const transceivers = peerConnection.getTransceivers();
		console.log('Got transceivers:', transceivers);
		transceivers[1].stop();

	} catch (error) {
		console.error('Error accessing media devices.', error);
	}
}

test();

Paste code in console, allow cam/mic access. You will get transceivers array with two transceivers, where second is stopped. If you expand first one it will be ok, expanding second will crash firefox.

Comment 3

[Daniel Bodea [:danibodea]]

I have succeeded in reproducing the issue described with the following steps:

1. Open browser.
2. Open Consonle.
3. Paste the code above in console.
4. Allow the appearing permission door-hanger.
5. Expend the... "Got transceivers: Array [ RTCRtpTransceiver, RTCRtpTransceiver ]"
6. Expand the... "1: RTCRtpTransceiver { mid: null, stopped: true, direction: "sendrecv", … }"
   Bad result: The tab crashes.
   Good result: The expansion was correctly displayed and there was no crash.

Mozregression results:

2021-01-04T22:05:29: DEBUG : Found commit message:
Bug 1661739 - Adjust merge_dlldata to widl output. r=nalexander
widl output for dlldata has #defines immediately followed by #includes,
so looking for empty lines when we observer a #define doesn't work. We
instead look for #defines.
Differential Revision: https://phabricator.services.mozilla.com/D88938
2021-01-04T22:05:29: DEBUG : Did not find a branch, checking all integration branches
2021-01-04T22:05:29: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=00a15ff99b87cc88718646c76b48a5ea54943c52&full=1
2021-01-04T22:05:30: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=00a15ff99b87cc88718646c76b48a5ea54943c52&full=1
2021-01-04T22:05:30: DEBUG : Didn't find 00a15ff99b87cc88718646c76b48a5ea54943c52 in mozilla-inbound
2021-01-04T22:05:30: DEBUG : Repo 'autoland' seems to have the earliest push
2021-01-04T22:05:30: INFO : ************* Switching to autoland by process of elimination (no branch detected in commit message)

Then continued bisection and gave this result:

2021-01-04T22:14:19: DEBUG : Found commit message:
Bug 1654399 - pt4 - better close_notify support during renegotiation. r=bwc
Similar to changes for Bug 1303867 to make sure we destroy the NrIceMediaStream after removing the transport.
Depends on D85204
Differential Revision: https://phabricator.services.mozilla.com/D88924
2021-01-04T22:14:19: DEBUG : Did not find a branch, checking all integration branches
2021-01-04T22:14:19: INFO : The bisection is done.
2021-01-04T22:14:19: INFO : Stopped

Comment 4

[Dan Minor [:dminor]]

Michael, looks like this might be fallout from one of your bugs from a few months ago.

Comment 5

[Michael Froman [:mjf]]

Good find, and nice STR in comment 3! I think the actual patch to cause the issue is pt2 (not pt4 as id'd in comment 3), but definitely my bug. I should have a fix shortly.

Comment 6

[Michael Froman [:mjf]]

Attachment: Bug 1681025 - don't call GetDtlsTransport on transceiver if RTCRtpReceiver has been shutdown. r?ng!

Comment 7

[Michael Froman [:mjf]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83f3726c41f9
don't call GetDtlsTransport on transceiver if RTCRtpReceiver has been shutdown. r=ng

Comment 9

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

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

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/83f3726c41f9

Comment 11

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

Upstream PR merged by moz-wptsync-bot

Comment 12

[Michael Froman [:mjf]]

Comment on attachment 9195720 [details]
Bug 1681025 - don't call GetDtlsTransport on transceiver if RTCRtpReceiver has been shutdown. r?ng!

Beta/Release Uplift Approval Request

• User impact if declined: Potential crash on webrtc sites that use RTCRtpReceiver::GetTransport.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very small patch that checks for nullptr.
• String changes made/needed: n/a

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9195720 [details]
Bug 1681025 - don't call GetDtlsTransport on transceiver if RTCRtpReceiver has been shutdown. r?ng!

approved for 85.0b7

Comment 15

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37cb0224bb98

Comment 16

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Hello! Reproduced the issue using Firefox 86.0a1 (20210104172545) on Windows 10x64 using the STR from comment 3. Thank you Dani.
The issue is verified fixed with Firefox 86.0a1 (20210110213430) and 85.0b7 (20210110185809) on Windows 10x64, macOS 10.12 and Ubuntu 18.04. No crashes are encountered after following the above steps.