All users were logged out of Bugzilla on October 13th, 2018

WebRTC ConnectDataConnection causes assertion failures if called a second time

RESOLVED FIXED in mozilla21

Status

()

--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: posidron, Assigned: jesup)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla21
x86_64
Mac OS X
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webrtc][blocking-webrtc+][qa-], crash signature)

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
Created attachment 709408 [details]
testcase

Click the button to reproduce.
(Reporter)

Comment 1

6 years ago
Created attachment 709409 [details]
callstack
(Reporter)

Comment 2

6 years ago
Tested with m-c changeset: 120354:2cc710018b14
This patch landed on on January 30th on m-c. Do you still see it with todays nightly? I can't reproduce it on OS X.
(Reporter)

Comment 4

6 years ago
I still get it with m-i changeset: 120658:4466d3ff8ada
It might be ASAN only? Can you verify that?
(Reporter)

Comment 6

6 years ago
No, that's a normal assertion failure.
(Assignee)

Comment 7

6 years ago
Ok, the problem (caught by the assertion) is that ConnectDataConnection is only meant to be called once (and is supposed to be a temporary interface until we can get the proper negotiation and configuration written).
Assignee: nobody → rjesup
Summary: WebRTC Assertion failure: !mMasterSocket, at netwerk/sctp/datachannel/DataChannel.cpp:190 and crash [@mozilla::DataChannelConnection::~DataChannelConnection] → WebRTC ConnectDataConnection causes assertion failures if called a second time
Whiteboard: [webrtc][blocking-webrtc+]
(Assignee)

Comment 8

6 years ago
Created attachment 709441 [details] [diff] [review]
Webrtc: Ignore second call to ConnectDataConnection
(Assignee)

Updated

6 years ago
Attachment #709441 - Flags: review?(tterribe)
Comment on attachment 709441 [details] [diff] [review]
Webrtc: Ignore second call to ConnectDataConnection

Review of attachment 709441 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +623,5 @@
>  #ifdef MOZILLA_INTERNAL_API
> +  if (mDataConnection) {
> +    CSFLogError(logTag,"%s DataConnection already connected",__FUNCTION__);
> +    // Ignore the request to connect when already connected.  This entire
> +    // implementation is temporary.

If aNumstreams is more than the amount of streams we have now, we should probably request more here. Since actually doing that looks like work, at least leave a note here about it.
Attachment #709441 - Flags: review?(tterribe) → review+
https://hg.mozilla.org/mozilla-central/rev/b10cbe334217
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Flags: in-testsuite?
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][qa-]

Updated

6 years ago
Attachment #710083 - Flags: review?(rjesup)
(Assignee)

Updated

6 years ago
Attachment #710083 - Flags: review?(rjesup) → review+

Updated

6 years ago
Keywords: checkin-needed
The manifest changes for the crash test don't look ok. Jason, please check this.
Keywords: checkin-needed
Attachment #710083 - Flags: feedback-
(In reply to Henrik Skupin (:whimboo) from comment #13)
> The manifest changes for the crash test don't look ok. Jason, please check
> this.

Umm...I don't see a problem here. The changes are adding in the crashtest file and reordering the one crashtest such that the numbers are in order. So I have to disagree.

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Attachment #710083 - Flags: feedback-
(In reply to Jason Smith [:jsmith] from comment #14)
> (In reply to Henrik Skupin (:whimboo) from comment #13)
> > The manifest changes for the crash test don't look ok. Jason, please check
> > this.
> 
> Umm...I don't see a problem here. The changes are adding in the crashtest
> file and reordering the one crashtest such that the numbers are in order. So
> I have to disagree.

So in short, there's no issues with the manifest file in itself with doing the small fix on the reordering.
Comment on attachment 710083 [details] [diff] [review]
Crashtest v1

Review of attachment 710083 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for that. It's totally true. Landed as:

https://hg.mozilla.org/integration/mozilla-inbound/rev/de6ad98785e0
Attachment #710083 - Flags: checkin+
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.