837421 - WebRTC ConnectDataConnection causes assertion failures if called a second time

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Click the button to reproduce.

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Christoph Diehl [:posidron]]

Tested with m-c changeset: 120354:2cc710018b14

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 4

[Christoph Diehl [:posidron]]

I still get it with m-i changeset: 120658:4466d3ff8ada

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It might be ASAN only? Can you verify that?

Comment 6

[Christoph Diehl [:posidron]]

No, that's a normal assertion failure.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Webrtc: Ignore second call to ConnectDataConnection

Comment 9

[Timothy B. Terriberry (:derf)]

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.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b10cbe334217

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b10cbe334217

Comment 12

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The manifest changes for the crash test don't look ok. Jason, please check this.

Comment 14

[Jason Smith [:jsmith]]

(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.

Comment 15

[Jason Smith [:jsmith]]

(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 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de6ad98785e0