Closed
Bug 1259728
Opened 8 years ago
Closed 8 years ago
Crash in mozilla::PeerConnectionImpl::GetLocalStreams when calling getLocalStreams on a closed peer connection
Categories
(Core :: WebRTC, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | verified |
People
(Reporter: mail, Assigned: jib)
Details
(Keywords: crash, reproducible, testcase)
Crash Data
Attachments
(3 files)
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•8 years ago
|
Component: Untriaged → WebRTC
Product: Firefox → Core
Could you attach a minimal testcase showing the crash, please.
Flags: needinfo?(mail)
Assignee | ||
Comment 2•8 years ago
|
||
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
I tested with the prefixed API mozRTCPeerConnection and it crashed too. So not an obvious regression.
Comment 8•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → jib
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Assignee | ||
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/88df6d5a1f21
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
QA Whiteboard: [good first verify]
Comment 16•8 years ago
|
||
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•8 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•8 years ago
|
||
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.
Description
•