806375 - PeerConnection DataChannel demo doesn't work a second time, requires restart

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P1)

Description

[(not currently active) Ted Mielczarek]

I'm using an updated copy of the local_data_chat.html demo. It works fine in my Mac nightly (19.0a1 (2012-10-29)), but if I reload the page and try again it doesn't work. Restarting the browser makes it work again.

Comment 1

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

Looks like a pseudo-port reuse issue, though we should be releasing the DataChannelConnection and dropping the mMasterSocket

Comment 2

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

Attachment: cleanup especially of channel close and connection shutdown

WIP

Comment 3

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

Attachment: cleanup especially of channel close and connection shutdown

rebased onto current tip

Comment 4

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

Attachment: Updated testcase

Comment 5

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

Attachment: cleanup especially of channel close and connection shutdown

WIP 2 - fixed almost-every-time crash on shutdown of the connection (don't try to spawn an async runnable with a strong ref from within your destructor).  Still crashes often in pipeline code, unrelated to datachannels

Comment 6

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

Attachment: cleanup especially of channel close and connection shutdown

Working patch that lets me start up and shut down DataChannelConnections multiple times.  Still get crashes on shutdown in the pipeline/Render code if I use video instead of audio, but that's a separate bug and doesn't happen all the time.  Might need a little cleanup of logging/etc for checkin.

Comment 7

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

Attachment: cleanup especially of channel close and connection shutdown

Comment 8

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

Comment on attachment 691858 [details] [diff] [review]
cleanup especially of channel close and connection shutdown

Major cleanup of channel close (didn't really work) and connection shutdown (really didn't work).  Also move connect to a more mozilla-ish paradigm (nsIThread/runnable instead of PR_Thread).

SCTP debugs no longer require rebuilding to use (use datachannel:6).  We could separate it into a separate log ID instead.

I think there's still an issue in the sctp library with sctp releasing ports on socket close (it seems to be, but it still shows as inuse when we try to bind again), but with REUSE_PORT set, it's not an issue for now.

Comment 9

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

The port reuse issue appears to have been an artifact of it only shutting one of the two ports at the time; with the current patch I can disable REUSE_PORT.  We may want to keep it for other reasons (in particular Offer forking/cloning).

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 691858 [details] [diff] [review]
cleanup especially of channel close and connection shutdown

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

this lgtm.. particularly the general necko and sctp bits - there are some internal webrtc bits that are opaque to me. The reuse port stuff should be fine for our application so I don't mind it being there.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/916f6915112d

Comment 12

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

Backout for linux bustage due to anonymous enums causing ambiguous function signatures
https://hg.mozilla.org/integration/mozilla-inbound/rev/0685e3a994e0

Comment 13

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

Re-land after try push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5784e9e50b

Comment 14

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

Randell, can this be tested via an unit test or do we need a mochitest for?

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Florian Quèze [:florian]]

We were hitting this bug for our https://github.com/mozilla/socialapi-demo demo; we worked around it by randomizing the port numbers used for the data channel: https://github.com/mozilla/socialapi-demo/commit/d6ea884a1cc101b10414c7892721df6097a249f9

I've just verified that this bug is fixed by reverting locally our work-around.

I can reproduce the bug with:
Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20121212 Firefox/20.0

I cannot reproduce the bug any more with:
Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20121218 Firefox/20.0

Comment 17

[Jason Smith [:jsmith]]

Verified in 2/4/2013 - I can use the data channel demo after a few reloads.