Crash in mozilla::PeerConnectionImpl::GetLocalStreams when calling getLocalStreams on a closed peer connection

VERIFIED FIXED in Firefox 48

Status

()

Core
WebRTC
P2
critical
Rank:
22
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Philipp, Assigned: jib)

Tracking

({crash, reproducible, testcase})

45 Branch
mozilla48
crash, reproducible, testcase
Points:
---

Firefox Tracking Flags

(firefox48 verified)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8734720 [details]
crash.txt

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/601.4.4 (KHTML, like Gecko) Version/9.0.3 Safari/601.4.4

Steps to reproduce:

Hello,

When the 'onsignalingstatechange' event handler, is
invoked and the 'iceConnectionState' of the events target is 'closed' I'm trying to stop any localStreams (with 'getLocalStreams'):

peerConnection.onsignalingstatechange = function (e) {
    if (e.target.iceConnectionState === 'closed') {
      var streams = e.target.getLocalStreams();
      ...
    }
}

This causes the browser to crash. (Attached is the crash report).

Firefox: 45.0.1
OS X: 10.11.3


Actual results:

Firefox crashed when invoking 'getLocalStreams'.


Expected results:

I'm guessing it should throw an error message similar to "PeerConnection invalid state".

Updated

2 years ago
Component: Untriaged → WebRTC
Product: Firefox → Core

Comment 1

2 years ago
Could you attach a minimal testcase showing the crash, please.
Flags: needinfo?(mail)
Philipp if you could also type "about:crashes" in a new tab in the Firefox instance that crashed, and send us the top Report ID (we want the one whose time and date matches the crash you're reporting) and paste that here that would be great!
(Reporter)

Comment 3

2 years ago
Hi,

sorry for the delay here is the report ID: bp-81433daa-c992-473d-bc53-7ec0f2160325 (https://crash-stats.mozilla.com/report/index/81433daa-c992-473d-bc53-7ec0f2160325), I'll send a test-case later today.
Flags: needinfo?(mail)
(Reporter)

Comment 4

2 years ago
Here is a minimal testcase, tested with windows version too:

https://jsfiddle.net/stfk1ks4/5/

Cheers

Comment 5

2 years ago
Ty, it's reproducible.

CR FF48:
https://crash-stats.mozilla.com/report/index/7444f578-dee0-48d2-893a-b11a62160329
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozilla::PeerConnectionImpl::GetLocalStreams ]
Ever confirmed: true
Keywords: crash, reproducible, testcase
Summary: Crash when calling getLocalStreams on a closed peer connection → Crash in mozilla::PeerConnectionImpl::GetLocalStreams when calling getLocalStreams on a closed peer connection

Comment 6

2 years ago
Created attachment 8735819 [details]
crash.html

Comment 7

2 years ago
I tested with the prefixed API mozRTCPeerConnection and it crashed too. So not an obvious regression.
Likely there for a Long Time.  media() almost certainly is returning null for a closed PeerConnection.  Some null-checks (here and perhaps elsewhere) and/or state checks should be added.  Likely very easy
Rank: 22
Priority: -- → P2
Assignee: nobody → jib
Created attachment 8736112 [details]
MozReview Request: Bug 1259728 - minimal fix for reentrancy in pc.close().

Review commit: https://reviewboard.mozilla.org/r/43107/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43107/
Attachment #8736112 - Flags: review?(rjesup)
(In reply to Loic from comment #7)
> I tested with the prefixed API mozRTCPeerConnection and it crashed too.

Fyi the prefixed versions test the exact same code.
PeerConnection.js already state-checks getLocalStreams. The problem was a reentrancy issue in pc.close that didn't set the internal _closed flag that these relied on, early enough.

This is a minimal fix, and I tried to be careful not to disturb the order of events, since we have tests that make assumptions about our multiple "closed" states and when they can be tested together.
Comment on attachment 8736112 [details]
MozReview Request: Bug 1259728 - minimal fix for reentrancy in pc.close().

https://reviewboard.mozilla.org/r/43107/#review39691
Attachment #8736112 - Flags: review?(rjesup) → review+
The above patch should take care of the crash.

Not super excited about the event reentrancy, we might want to clean this up in a separate issue, except the spec is unclear. I've filed https://github.com/w3c/webrtc-pc/issues/565 for clarification.

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88df6d5a1f21
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
QA Whiteboard: [good first verify]
Successfully reproduce this bug on Firefox 47.0 ; (Build ID: 20160606114208) on Linux, 64 Bit by following the comment 0 and comment 4.

This Bug is now verified as fixed on Latest Firefox Beta 48.0b6

Build ID: 20160706215822
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
QA Whiteboard: [good first verify] → [good first verify][testday-20160708]

Comment 17

2 years ago
I have reproduced this bug in Firefox releases 47.0.7 with the instruction from comment 0 and windows 8.1 32 bit

Bug is fixed now on Latest Beta 48.0b6(Build ID:20160706215822)
User Agent:Mozilla/5.0 (Windows NT 6.3; rv:48.0) Gecko/20100101 Firefox/48.0

[testday-20160708]

Comment 18

2 years ago
As it verified on both windows (comment 17) and Linux (comment 16) marking it as verified
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.