Closed Bug 1259728 Opened 4 years ago Closed 4 years ago

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

Categories

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

45 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox48 --- verified

People

(Reporter: mail, Assigned: jib)

Details

(Keywords: crash, reproducible, testcase)

Crash Data

Attachments

(3 files)

Attached file 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".
Component: Untriaged → WebRTC
Product: Firefox → Core
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!
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)
Here is a minimal testcase, tested with windows version too:

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

Cheers
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
Summary: Crash when calling getLocalStreams on a closed peer connection → Crash in mozilla::PeerConnectionImpl::GetLocalStreams when calling getLocalStreams on a closed peer connection
Attached file crash.html
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
(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.
https://hg.mozilla.org/mozilla-central/rev/88df6d5a1f21
Status: NEW → RESOLVED
Closed: 4 years ago
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]
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]
As it verified on both windows (comment 17) and Linux (comment 16) marking it as verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.