WebRTC ConnectDataConnection causes assertion failures if called a second time

RESOLVED FIXED in mozilla21

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: posidron, Assigned: jesup)

Tracking

(Blocks 1 bug, {crash, testcase})

Trunk
mozilla21
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments)

Posted file testcase
Click the button to reproduce.
Posted file callstack
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.
I still get it with m-i changeset: 120658:4466d3ff8ada
It might be ASAN only? Can you verify that?
No, that's a normal assertion failure.
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+]
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
Closed: 7 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][qa-]
Attachment #710083 - Flags: review?(rjesup)
Attachment #710083 - Flags: review?(rjesup) → review+
The manifest changes for the crash test don't look ok. Jason, please check this.
Keywords: checkin-needed
(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.
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.