733002 - Implement DOM layer for WebRTC dataChannels

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect)

Description

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

This is the DOM API and JS objects exposed via the PeerConnection for WebRTC dataChannels, per the evolving (mostly agreed-to) spec in the WebRTC W3C list.

Comment 1

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

An example of the API needed is in 
https://docs.google.com/document/d/16csYCaHxIYP83DzCZJL7relQm2QNxT-qkay4-jLxoKA/edit?ndplr=1
though please ignore the discussion of SDP negotiation; the current proposal by me and others is to have a small wire protocol over SCTP to implement the bidirectional opens and closes.  An IETF draft on this will be submitted today.

Comment 2

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

Another proposal in the tread that starts at
http://lists.w3.org/Archives/Public/public-webrtc/2012Feb/0183.html
Much of the discussion hinges on the handling of "glare" and crossing open requestst.

This is largely the API I expect we'll reach consensus on.

Comment 3

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

Ok, we have an updated draft (though still not at consensus, this should be close):

http://dev.w3.org/2011/webrtc/editor/webrtc.html#peer-to-peer-data-api

Comment 4

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

Attachment: horribly ugly partial rework of WebSockets DOM for DataChannel - emphasize partial

Note: started from WebSockets and did a bunch of query-replaces; the IDL files are at least somewhere in the outfield of the ballpark, the cpp files are still looking for the exit off the freeway

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 6

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

Attachment: mods to kyle's patch WIP

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: mods to jesup's mods to kyle's patch WIP

Comment 8

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

Attachment: fix idl include and QI ordering for the event helpers

Comment 9

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

Attachment: Implement DOM for DataChannels, and modify temporary WebrtcContext to expose it

Comment 10

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

Attachment: Implement DOM for DataChannels - alder/default version

Comment 11

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

Attachment: Implement DOM for DataChannels - alder/default updated

Comment 12

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

Attachment: Implement DOM for DataChannels - part 1, DOMDataChannel

Comment 13

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

Attachment: Implement DOM for DataChannels - part 2, attach to PeerConnection

Comment 14

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

Attachment: Implement DOM for DataChannels - part 2, attach to PeerConnection

Comment 15

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

Attachment: Cleanup to enable SCTP debug, fix incoming PPID

Comment 16

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

Attachment: Fix order-of-build issue with IDLs

Comment 17

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

Attachment: Fix ::Send() to work like WebSockets

Comment 18

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

Attachment: fix shutdown issues for DataChannels

Comment 19

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

Attachment: cleanup DC DOM impl, add window pointer from PeerConnection, js code to deliver DC events

Comment 20

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

Attachment: Update to cleanup DataChannel DOM in PeerConnection and deliver events

Comment 21

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

Attachment: Update to cleanup DataChannel DOM in PeerConnection and deliver events

Comment 22

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

Attachment: Add support for DTLS connections to DataChannels

This builds, no idea yet if it runs

Comment 23

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

Attachment: Add support for DTLS connections to DataChannels

Comment 24

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

Attachment: Add support for DTLS connections to DataChannels

Comment 25

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

Attachment: Add support for DTLS connections to DataChannels

updated a few bits for DTLS

Comment 26

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

Attachment: Add support for DTLS connections to DataChannels

update port number we use for Init()

Comment 27

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

Attachment: Add support for DTLS connections to DataChannels

Comment 28

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

Attachment: Implement proper SCTP shutdown on Mozilla exit

Also cleans up a couple of warnings

Comment 29

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

Comment on attachment 652687 [details] [diff] [review]
Add support for DTLS connections to DataChannels

This already is on alder and works, but I thought you should look over the SCTP<->transport parts at least

Comment 30

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

Comment on attachment 652890 [details] [diff] [review]
Implement proper SCTP shutdown on Mozilla exit

This is really part of bug 729511, not DOM

Comment 31

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

Pushed patch from bug 729512 for Label support and also added DataChannelInit dictionary objects, so you can ask for unreliable channels.  Updated the chat app to used labels and an unreliable channel.  Bug: you have to specify the channel type as an integer (values are shown as constants in PeerConnection); I need to talk to an IDL/DOM person.

This fills in the last major missing parts of the protocol and DOM API

Comment 32

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

Attachment: DataChannel DOM part 1 rollup

Builds on m-c on top of the latest bug 729511 and bug 729512 patches (plus a disable-dtls patch I need to upload)

Comment 33

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

Attachment: DataChannel DOM part 1 rollup

Comment 34

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

Comment on attachment 659983 [details] [diff] [review]
DataChannel DOM part 1 rollup

DOM for Datachannels (not including how it's accessed via PeerConnection) up for review.  Strongly based on nsWebSockets.cpp.

Ted: Trivial makesystem changes

Comment 35

[Patrick McManus [:mcmanus]]

Comment on attachment 659983 [details] [diff] [review]
DataChannel DOM part 1 rollup

probly :smaug or :bz for dom reviewing dom code.

Comment 36

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

Comment on attachment 659983 [details] [diff] [review]
DataChannel DOM part 1 rollup

Sorry, too late at night when I marked it for review.  I meant to hit the DOM guys.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 659983 [details] [diff] [review]
DataChannel DOM part 1 rollup

>diff --git a/content/base/public/nsIDOMDataChannel.idl b/content/base/public/nsIDOMDataChannel.idl
>new file mode 100644
>--- /dev/null
>+++ b/content/base/public/nsIDOMDataChannel.idl
>@@ -0,0 +1,39 @@
>+#include "domstubs.idl"
>+
>+interface nsIDOMEventListener;
Do you need this?

>+interface nsIVariant;
>+
>+[scriptable, builtinclass, uuid(fb7a8ec4-c1eb-4d9f-b927-fbb8b4493e6d)]
>+interface nsIDOMDataChannel : nsISupports
>+{
>+  readonly attribute DOMString label;
>+  readonly attribute boolean reliable;
>+  readonly attribute boolean ordered;
>+
>+  const unsigned short CONNECTING = 0;
>+  const unsigned short OPEN = 1;
>+  const unsigned short CLOSING = 2;
>+  const unsigned short CLOSED = 3;
>+
>+  readonly attribute unsigned short readyState;
>+  readonly attribute unsigned long bufferedAmount;
>+
>+  [implicit_jscontext] attribute jsval onopen;
>+  [implicit_jscontext] attribute jsval onerror;
>+  [implicit_jscontext] attribute jsval onclose;
>+  [implicit_jscontext] attribute jsval onmessage;
So, nsIDOMDataChannel is apparently event target. Then it should inherit nsIDOMEventTarget.


>+
>+	class nsDOMDataChannel : public nsDOMEventTargetHelper,
>+                         public nsIDOMDataChannel,
>+                         public mozilla::DataChannelListener
Indentation is odd here. Looks like you have \t before 'class' 


>+  // Get msg info out of JS variable being sent (string, arraybuffer, blob)
>+  nsresult GetSendParams(nsIVariant *aData, nsCString &aStringOut,
>+                         nsCOMPtr<nsIInputStream> &aStreamOut,
>+                         bool &aIsBinary, uint32_t &aOutgoingLength,
>+                         JSContext *aCx);
>+
>+  nsresult CreateResponseBlob(const nsACString& aData, JSContext *aCx,
>+                              jsval &jsData);
Nit, please be consistent where * and & go.
(They should be with the type.)



>+
>+  // Owning reference
>+  nsAutoPtr<mozilla::DataChannel> mDataChannel;
>+  nsString  mUTF16Origin;
I guess this could be just mOrigin


>+
>+  // XXX any need to CheckInnerWindowCorrectness() like WebSockets?
>+  // It's only an issue if the PeerConnection can likewise leak, which I think it can't.
>+  // See bug 696085
Yeas, please add a check for innerwindowcorrectness



>+  // Do we need to observe for window destroyed or frozen?  (same bug)
Well, what should happen when the window/document goes to bfcache?
And what should happen when the page goes out from bfcache?
I think it might be ok to disable bfcache if we have webrtc active.
Add something to nsDocument::CanSavePresentation


>+NS_IMETHODIMP
>+nsDOMDataChannel::Send(nsIVariant *aData, JSContext *aCx)
nsIVariant*

>+nsresult
>+nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData,
>+                                       bool isBinary)
aIsBinary



>+  if (isBinary) {
>+    if (mBinaryType == DC_BINARY_TYPE_BLOB) {
>+      rv = CreateResponseBlob(aData, cx, jsData);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    } else if (mBinaryType == DC_BINARY_TYPE_ARRAYBUFFER) {
>+      JSObject *arrayBuf;
JSObject*


>+nsresult
>+nsDOMDataChannel::OnChannelConnected(nsISupports* aContext)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv = NS_NewDOMEvent(getter_AddRefs(event), nullptr, nullptr);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  rv = event->InitEvent(NS_LITERAL_STRING("open"), false, false);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  event->SetTrusted(true);
>+
>+  LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
>+
>+  return DispatchDOMEvent(nullptr, event, nullptr, nullptr);
>+}
>+
>+nsresult
>+nsDOMDataChannel::OnChannelClosed(nsISupports* aContext)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv = NS_NewDOMEvent(getter_AddRefs(event), nullptr, nullptr);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  rv = event->InitEvent(NS_LITERAL_STRING("close"), false, false);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  event->SetTrusted(true);
>+
>+  LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
>+
>+  return DispatchDOMEvent(nullptr, event, nullptr, nullptr);
>+}

You could dispatch events the following way:
  return GetOwner() ? nsContentUtils::DispatchTrustedEvent(GetOwner()->GetExtantDoc(), this, NS_LITERAL_STRING(< you event name >), false, false) : NS_ERROR_FAILURE;
Though, it is not clear to me whether we want to propagate the error. Perhaps NS_OK would be ok too.




>+NS_NewDOMDataChannel(mozilla::DataChannel* dataChannel,
>+                     nsPIDOMWindow* aWindow,
>+                     nsIDOMDataChannel** domDataChannel)
Parameters should be in form aParameterName



The interface will be converted to use webidl, but that can be done later in a followup.
(I'll need to get some more experience with webidl, so that I could start reviewing such patches.)

Comment 38

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

Attachment: DataChannel DOM implementation Part 1

Comment 39

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

(In reply to Olli Pettay [:smaug] from comment #37)
> >+  [implicit_jscontext] attribute jsval onopen;
> >+  [implicit_jscontext] attribute jsval onerror;
> >+  [implicit_jscontext] attribute jsval onclose;
> >+  [implicit_jscontext] attribute jsval onmessage;
> So, nsIDOMDataChannel is apparently event target. Then it should inherit
> nsIDOMEventTarget.

Yup, done.

> >+
> >+	class nsDOMDataChannel : public nsDOMEventTargetHelper,
> >+                         public nsIDOMDataChannel,
> >+                         public mozilla::DataChannelListener
> Indentation is odd here. Looks like you have \t before 'class' 

Oops, typo.

> 
> >+  // Get msg info out of JS variable being sent (string, arraybuffer, blob)
> >+  nsresult GetSendParams(nsIVariant *aData, nsCString &aStringOut,
> >+                         nsCOMPtr<nsIInputStream> &aStreamOut,
> >+                         bool &aIsBinary, uint32_t &aOutgoingLength,
> >+                         JSContext *aCx);
> >+
> >+  nsresult CreateResponseBlob(const nsACString& aData, JSContext *aCx,
> >+                              jsval &jsData);
> Nit, please be consistent where * and & go.
> (They should be with the type.)

Yeah, my fingers have 27 years of C/C++ doing it the other way...  But in any case the style needs to match the rest of the code it's with.  (Ditto for the other similar instances)  We're nowhere near consistent in the tree, but as this is new code (even if much of it is copied from nsWebSocket), I'll normalize.

> >+
> >+  // Owning reference
> >+  nsAutoPtr<mozilla::DataChannel> mDataChannel;
> >+  nsString  mUTF16Origin;
> I guess this could be just mOrigin

Done.

> >+
> >+  // XXX any need to CheckInnerWindowCorrectness() like WebSockets?
> >+  // It's only an issue if the PeerConnection can likewise leak, which I think it can't.
> >+  // See bug 696085
> Yeas, please add a check for innerwindowcorrectness

Well, reading that bug, I think there are differences between WebSockets and DataChannels here that indicate that the problem in bug 696085 can't happen in the same way (DataChannels are attached to PeerConnections via DataChannelConnection objects).  However, *maybe* the PeerConnection this is hung off of (which is new'd in JS) might be able to leak in a similar way (probably tougher, perhaps impossible, but better to be safe).
 
> >+  // Do we need to observe for window destroyed or frozen?  (same bug)
> Well, what should happen when the window/document goes to bfcache?
> And what should happen when the page goes out from bfcache?
> I think it might be ok to disable bfcache if we have webrtc active.
> Add something to nsDocument::CanSavePresentation

A very good point; PeerConnections should die on navigation (and that will ripple down to the DataChannels attached to it).  Perhaps they should add themselves as unload or beforeunload listeners to the document, which will let them kill themselves as well as block CanSavePresentation. Per Olli, it would need to a system event handler, so it can't be blocked (e.g. AddSystemEventListener())

(This patch is being reviewed in a slight vacuum, since PeerConnection isn't ready - I plan to lash it into the DOM in a temporary place until PeerConnection lands).  If we ever decide to use DataChannels without PeerConnection (which I doubt due to all the NAT traversal and negotiation issues), we'd need to deal with this directly.

Revised comment:
  // See bug 696085
  // We don't need to observe for window destroyed or frozen; but PeerConnection needs
  // to not allow itself to be bfcached (and get destroyed on navigation).


> You could dispatch events the following way:
>   return GetOwner() ?
> nsContentUtils::DispatchTrustedEvent(GetOwner()->GetExtantDoc(), this,
> NS_LITERAL_STRING(< you event name >), false, false) : NS_ERROR_FAILURE;
> Though, it is not clear to me whether we want to propagate the error.
> Perhaps NS_OK would be ok too.

Tried this;  I ended up going back to DispatchDOMEvent() (though cleaned up some) because the target isn't 'this', and I wasn't sure what to use (nullptr works, but complains and some stuff doesn't seem to work entirely).

> >+NS_NewDOMDataChannel(mozilla::DataChannel* dataChannel,
> >+                     nsPIDOMWindow* aWindow,
> >+                     nsIDOMDataChannel** domDataChannel)
> Parameters should be in form aParameterName

Done

> The interface will be converted to use webidl, but that can be done later in
> a followup.
> (I'll need to get some more experience with webidl, so that I could start
> reviewing such patches.)

I wrote a WebIDL version at one point, though it's not in the patch.

Comment 40

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 661000 [details] [diff] [review]
DataChannel DOM implementation Part 1

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

::: content/base/public/nsIDOMDataChannel.idl
@@ +15,5 @@
> +  const unsigned short OPEN = 1;
> +  const unsigned short CLOSING = 2;
> +  const unsigned short CLOSED = 3;
> +
> +  readonly attribute unsigned short readyState;

readyState should be a DOMString

::: content/base/src/Makefile.in
@@ +145,5 @@
> +LOCAL_INCLUDES += \
> +		-I$(topsrcdir)/netwerk/sctp/datachannel \
> +		-I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source \
> +		$(NULL)
> +endif

ifdef MOZ_WEBRTC
EXPORTS += \
  nsDOMDataChannel.h \
  $(NULL)
CPPSRCS += \
  nsDOMDataChannel.cpp \
  $(NULL)
LOCAL_INCLUDES += \
  -I$(topsrcdir)/netwerk/sctp/datachannel \
  -I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source \
  $(NULL)
endif

(Definitely no tabs.)

::: content/base/src/nsDOMDataChannel.cpp
@@ +44,5 @@
> +                         public mozilla::DataChannelListener
> +{
> +public:
> +  nsDOMDataChannel(mozilla::DataChannel* aDataChannel) : mDataChannel(aDataChannel),
> +                                                         mBinaryType(DC_BINARY_TYPE_BLOB)

nsDOMDataChannel(mozilla::DataChannel* aDataChannel)
  : mDataChannel(aDataChannel)
  , mBinaryType(DC_BINARY_TYPE_BLOB)

@@ +222,5 @@
> +    mBinaryType = DC_BINARY_TYPE_ARRAYBUFFER;
> +  } else if (aBinaryType.EqualsLiteral("blob")) {
> +    mBinaryType = DC_BINARY_TYPE_BLOB;
> +  } else  {
> +    return NS_ERROR_INVALID_ARG;

Don't throw

@@ +255,5 @@
> +  nsresult rv = GetSendParams(aData, msgString, msgStream, isBinary, msgLen, aCx);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Always increment outgoing buffer len, even if closed
> +  //mOutgoingBufferedAmount += msgLen;

Bah

@@ +281,5 @@
> +}
> +
> +// XXX Exact clone of nsWebSocketChannel::GetSendParams() - find a way to share!
> +nsresult
> +nsDOMDataChannel::GetSendParams(nsIVariant* aData, nsCString& aStringOut,

You really need to use WebIDL.

@@ +359,5 @@
> +// Initial implementation: only stores to RAM, not file
> +// TODO: bug 704447: large file support
> +nsresult
> +nsDOMDataChannel::CreateResponseBlob(const nsACString& aData, JSContext* aCx,
> +                                     jsval& jsData)

JS::Value* aBlob

@@ +362,5 @@
> +nsDOMDataChannel::CreateResponseBlob(const nsACString& aData, JSContext* aCx,
> +                                     jsval& jsData)
> +{
> +  uint32_t blobLen = aData.Length();
> +  void* blobData = PR_Malloc(blobLen);

PR_Malloc?

@@ +378,5 @@
> +nsresult
> +nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData,
> +                                       bool isBinary)
> +{
> +  nsresult rv;

Declare lower

@@ +379,5 @@
> +nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData,
> +                                       bool isBinary)
> +{
> +  nsresult rv;
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");

MOZ_ASSERT throughout

@@ +415,5 @@
> +    }
> +  } else {
> +    NS_ConvertUTF8toUTF16 utf16data(aData);
> +    JSString* jsString;
> +    jsString = JS_NewUCStringCopyN(cx, utf16data.get(), utf16data.Length());

Same line

@@ +492,5 @@
> +nsDOMDataChannel::OnChannelClosed(nsISupports* aContext)
> +{
> +  LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
> +
> +

One line suffices

@@ +521,5 @@
> +NS_NewDOMDataChannel(mozilla::DataChannel* aDataChannel,
> +                     nsPIDOMWindow* aWindow,
> +                     nsIDOMDataChannel** aDomDataChannel)
> +{
> +  nsresult rv;

Lower

@@ +528,5 @@
> +
> +  rv = domdc->Init(aWindow);
> +  NS_ENSURE_SUCCESS(rv,rv);
> +
> +  return CallQueryInterface(domdc, aDomDataChannel);

domdc.forget(aDomDataChannel);
return NS_OK;

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 661000 [details] [diff] [review]
DataChannel DOM implementation Part 1


>+#include "domstubs.idl"
>+
>+#include "nsIEventTarget.idl"
>+
>+interface nsIVariant;
>+
>+[scriptable, builtinclass, uuid(fb7a8ec4-c1eb-4d9f-b927-fbb8b4493e6d)]
>+interface nsIDOMDataChannel : nsIEventTarget

nsIDOMEventTarget. nsIEventTarget is something very different in Gecko ;)

Comment 42

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

Attachment: Switch nsIEventTarget -> nsIDOMEventTarget per review

Comment 43

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

Comment on attachment 661889 [details] [diff] [review]
Switch nsIEventTarget -> nsIDOMEventTarget per review

Delta diff for response to last review comment on nsIEventTarget.  Update including this and ms2ger's comments forthcoming.

Comment 44

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

(In reply to :Ms2ger from comment #40)
> Comment on attachment 661000 [details] [diff] [review]
> DataChannel DOM implementation Part 1
> 
> Review of attachment 661000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/public/nsIDOMDataChannel.idl
> @@ +15,5 @@
> > +  const unsigned short OPEN = 1;
> > +  const unsigned short CLOSING = 2;
> > +  const unsigned short CLOSED = 3;
> > +
> > +  readonly attribute unsigned short readyState;
> 
> readyState should be a DOMString

Ok. The spec changed this recently to strings with no discussion.  Since we're trying to emulate (and be largely plug-competive with) WebSockets, and WebSockets uses integers, I want to check with people on the webrtc list.  I'll make the change under the assumption this will stick.

> ::: content/base/src/Makefile.in
> @@ +145,5 @@
> > +LOCAL_INCLUDES += \
> > +		-I$(topsrcdir)/netwerk/sctp/datachannel \
> > +		-I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source \
> > +		$(NULL)
> > +endif
> 
> ifdef MOZ_WEBRTC
> EXPORTS += \
>   nsDOMDataChannel.h \
>   $(NULL)
> CPPSRCS += \
>   nsDOMDataChannel.cpp \
>   $(NULL)
> LOCAL_INCLUDES += \
>   -I$(topsrcdir)/netwerk/sctp/datachannel \
>   -I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source \
>   $(NULL)
> endif
> 
> (Definitely no tabs.)

I understand that's current Make style.  I normally avoid making a small mod to a large existing file enforce style different than the style within the file (which my mods match).  If you want to convert style on the entire file, ok, but that's a different issue/bug.

> ::: content/base/src/nsDOMDataChannel.cpp
> @@ +44,5 @@
> > +                         public mozilla::DataChannelListener
> > +{
> > +public:
> > +  nsDOMDataChannel(mozilla::DataChannel* aDataChannel) : mDataChannel(aDataChannel),
> > +                                                         mBinaryType(DC_BINARY_TYPE_BLOB)
> 
> nsDOMDataChannel(mozilla::DataChannel* aDataChannel)
>   : mDataChannel(aDataChannel)
>   , mBinaryType(DC_BINARY_TYPE_BLOB)

Sure.

> @@ +222,5 @@
> > +    mBinaryType = DC_BINARY_TYPE_ARRAYBUFFER;
> > +  } else if (aBinaryType.EqualsLiteral("blob")) {
> > +    mBinaryType = DC_BINARY_TYPE_BLOB;
> > +  } else  {
> > +    return NS_ERROR_INVALID_ARG;
> 
> Don't throw

Tell WebSockets that ;-)
We're trying to emulate as far as reasonable the WebSocket API (and impl).

> @@ +255,5 @@
> > +  nsresult rv = GetSendParams(aData, msgString, msgStream, isBinary, msgLen, aCx);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  // Always increment outgoing buffer len, even if closed
> > +  //mOutgoingBufferedAmount += msgLen;
> 
> Bah

Sorry, cruft and we haven't decided exactly how stats like this will be handled.

> 
> @@ +281,5 @@
> > +}
> > +
> > +// XXX Exact clone of nsWebSocketChannel::GetSendParams() - find a way to share!
> > +nsresult
> > +nsDOMDataChannel::GetSendParams(nsIVariant* aData, nsCString& aStringOut,
> 
> You really need to use WebIDL.

It wasn't ready when I started this work.  I do have a webidl description, but I've never tried it, and it's unclear to me how much I'd need to change the impl to match.

> @@ +359,5 @@
> > +// Initial implementation: only stores to RAM, not file
> > +// TODO: bug 704447: large file support
> > +nsresult
> > +nsDOMDataChannel::CreateResponseBlob(const nsACString& aData, JSContext* aCx,
> > +                                     jsval& jsData)
> 
> JS::Value* aBlob

Copied from WebSockets, and changing it breaks the nsContentUtils::WrapNative() call; that takes a jsval.  I suspect I can transform JS::Value to jsval, but I don't see what that buys us here.
 
> @@ +362,5 @@
> > +nsDOMDataChannel::CreateResponseBlob(const nsACString& aData, JSContext* aCx,
> > +                                     jsval& jsData)
> > +{
> > +  uint32_t blobLen = aData.Length();
> > +  void* blobData = PR_Malloc(blobLen);
> 
> PR_Malloc?

Ditto -- and correct!  nsDOMMemoryFile assumes PR_Malloc'd memory, as it uses PR_Free() to release it.

> @@ +378,5 @@
> > +nsresult
> > +nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData,
> > +                                       bool isBinary)
> > +{
> > +  nsresult rv;
> 
> Declare lower

Ok.

> @@ +379,5 @@
> > +nsDOMDataChannel::DoOnMessageAvailable(const nsACString& aData,
> > +                                       bool isBinary)
> > +{
> > +  nsresult rv;
> > +  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
> 
> MOZ_ASSERT throughout

Matches WebSocket - will change, almost all were MOZ_ASSERT already

> @@ +415,5 @@
> > +    }
> > +  } else {
> > +    NS_ConvertUTF8toUTF16 utf16data(aData);
> > +    JSString* jsString;
> > +    jsString = JS_NewUCStringCopyN(cx, utf16data.get(), utf16data.Length());
> 
> Same line

ok

> 
> @@ +492,5 @@
> > +nsDOMDataChannel::OnChannelClosed(nsISupports* aContext)
> > +{
> > +  LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
> > +
> > +
> 
> One line suffices

typo

> 
> @@ +521,5 @@
> > +NS_NewDOMDataChannel(mozilla::DataChannel* aDataChannel,
> > +                     nsPIDOMWindow* aWindow,
> > +                     nsIDOMDataChannel** aDomDataChannel)
> > +{
> > +  nsresult rv;
> 
> Lower

ok

> @@ +528,5 @@
> > +
> > +  rv = domdc->Init(aWindow);
> > +  NS_ENSURE_SUCCESS(rv,rv);
> > +
> > +  return CallQueryInterface(domdc, aDomDataChannel);
> 
> domdc.forget(aDomDataChannel);
> return NS_OK;

Looking over other DOM NS_New*() functions, this sort of signature and returning CallQueryInterface() is the norm.  Why should we be doing it differently here?  Is this mishandling the refcnt's?

Comment 45

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

Attachment: readyState -> DOMString, and minor changes per ms2ger's review

Comment 46

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

Comment on attachment 661917 [details] [diff] [review]
readyState -> DOMString, and minor changes per ms2ger's review

Delta patch to address ms2ger's comments

Comment 47

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

Attachment: Rollup patch of datachannel code and DOM code including nsIDOMEventTarget and ms2ger patches

This is the full rollup patch including both deltas; no other changes.

Comment 48

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

Attachment: fire onopen if set after we're already in open state to solve application race conditions

Comment 49

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

Comment on attachment 662017 [details] [diff] [review]
fire onopen if set after we're already in open state to solve application race conditions

(unlike WebSockets) when you get a DataChannel handed to you by ondatachannel it may be open already or will be very soon, by the time the app sets the onopen handler we may have already transitioned to open.

With a default implementation of onopen, the app never gets notified and may wait forever for onopen to start sending.

This fires onopen if it's set when we're already in the 'open' state.

It might not be a bad idea to consider this sort of patch for WebSockets (or if we started doing this generally for state-transition-events).

(This is just the DOM portion; we added a ResendOpen() method to DataChannel as well.)

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 661920 [details] [diff] [review]
Rollup patch of datachannel code and DOM code including nsIDOMEventTarget and ms2ger patches


>+  nsresult
>+  DoOnMessageAvailable(const nsACString& message, bool isBinary);
Paramaters should be in form
aParameterName
Same also elsewhere.


>+nsDOMDataChannel::SetBinaryType(const nsAString& aBinaryType)
>+{
>+  if (aBinaryType.EqualsLiteral("arraybuffer")) {
>+    mBinaryType = DC_BINARY_TYPE_ARRAYBUFFER;
>+  } else if (aBinaryType.EqualsLiteral("blob")) {
>+    mBinaryType = DC_BINARY_TYPE_BLOB;
>+  } else  {
>+    return NS_ERROR_INVALID_ARG;
>+  }
I doubt NS_ERROR_INVALID_ARG is the right error per the spec.
See either WebRTC spec or WebIDL spec for the right error.

>+// XXX Exact clone of nsWebSocketChannel::GetSendParams() - find a way to share!
Uh, add a method to nsContentUtils and use it in both places? Or add a new 
DataSender class or some such which both inherit.
Would help also with reviewing.

Could I still a new patch with that fixed, please.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 662017 [details] [diff] [review]
fire onopen if set after we're already in open state to solve application race conditions

If this is really needed, that sounds like a spec bug.
Web page shouldn't get access to un-opened channel, or there should be
some clear state that it is opened and open event shouldn't have this special
case.

Also, this is anyway wrong if someone happens to set
onopen multiple times. The event should be dispatched only once.

Comment 52

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

Attachment: Delta patch in response to last review

Comment 53

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

Attachment: Updated rollup patch

Comment 54

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

Comment on attachment 662782 [details] [diff] [review]
Delta patch in response to last review

These are the changes since the last rollup.

GetSendParams is no longer shared with WebSockets (since it doesn't use it anymore).  We do share the former GetResponseBlob() (now nsContentUtils::GetBlobBuffer())

SetBinaryType() uses the same error return as WebSockets, and none of this is in the spec (we're driving the spec, and patterning on WebSockets).

Comment 55

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

Attachment: Delta patch in response to last review

Comment 56

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

Attachment: Delta patch in response to last review

Updated - modified file from the wrong directory

Comment 57

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

Attachment: Updated rollup patch

Comment 58

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

So, the reason I was trying to fire 'open' if it's set after it's already open is that I wasn't seeing the 'open' events in the app, even though I know they were dispatched from DataChannel after 'datachannel' was dispatched.

On the sending side, we get onopen for all events.  On the receiving side, none (or maybe one).

The sequence on the receiving side is:
1) OpenRequest message received
2) OpenResponse message sent
3) ON_CHANNEL_CREATED sent to PeerConnection (via NS_DispatchToMainThread)
4) PeerConnectionImpl->NotifyDataChannel() is called and creates nsDOMDataChannel
5) runnable dispatched to PeerConnectionObserver (which is in JS)
6) NotifyDataChannel (in JS) runs and calls the onCallBack in the PeerConnection
7) the ondatachannel function in the app sets onopen, etc.

Somewhere after this (or more likely perhaps during 5 or 6 above):
A) OpenResponseAck received
B) DataChannel changes state to OPEN
C) ON_CHANNEL_OPEN sent to nsDOMDataChannel (via NS_DispatchToMainThread)
D) OnChannelConnected is called and called DispatchDOMEvent() for "open".

So, *perhaps* the issue is the multiple trips through the main thread event queue for ondatachannel, while the open event makes less trips and thus (maybe) can 'jump' ahead of the final ondatachannel runnable event.  Maybe that's not the reason, but we don't get the open event in the app in any case.

If we wait for the JS to acknowledge the datachannel event in some way and queue the open event (and any data messages), obviously we're good.  Or don't send OpenResponse until the datachannel is set up and has notified the low-level code.

Comment 59

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Randell Jesup [:jesup] from comment #54)
> SetBinaryType() uses the same error return as WebSockets, and none of this
> is in the spec (we're driving the spec, and patterning on WebSockets).

It doesn't. WebSocket has

void SetBinaryType(dom::BinaryType aData) { mBinaryType = aData; }

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Randell Jesup [:jesup] from comment #58)
> If we wait for the JS to acknowledge the datachannel event in some way and
> queue the open event (and any data messages), obviously we're good.  Or
> don't send OpenResponse until the datachannel is set up and has notified the
> low-level code.
Either way sounds ok.

Comment 61

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

(In reply to :Ms2ger from comment #59)
> (In reply to Randell Jesup [:jesup] from comment #54)
> > SetBinaryType() uses the same error return as WebSockets, and none of this
> > is in the spec (we're driving the spec, and patterning on WebSockets).
> 
> It doesn't. WebSocket has
> 
> void SetBinaryType(dom::BinaryType aData) { mBinaryType = aData; }

My apologies, WebSockets *had* the exact same implementation until a week or so ago.

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 662795 [details] [diff] [review]
Updated rollup patch

>+// XXX Exact clone of nsWebSocketChannel::GetSendParams() - find a way to share!
So this comment isn't valid anymore?

Comment 63

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

Yup, realized after I posted it.  I'll remove that line.  Thanks!

I'm working on a solution to the open event issue

Comment 64

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

Attachment: delta patch to solve open/message event races against datachannel events

Comment 65

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

Grr, hit return instead of tab - smaug, the patch here works with another patch I'll add to bug 729512 (and mods to PeerConnectionImpl.cpp to invoke NS_DataChannelAppReady()).

Basically, I queue both CONNECTING->OPEN state changes and incoming data messages until we get positive confirmation that we've delivered the datachannel event to the PeerConnection object.

My test is much happier.

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 663130 [details] [diff] [review]
delta patch to solve open/message event races against datachannel events


>+  virtual void
>+  AppReady();
could be just one line.

>+// Tell DataChannel it's ok to deliver open and message events
>+void NS_DataChannelAppReady(nsIDOMDataChannel* domDataChannel);
aDOMDataChannel

Comment 67

[Ryan VanderMeulen [:RyanVM]]

This landed on inbound (without a changeset link posted here, apparently...), but had to be backed out due to mochitest-3 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/94279b84aee8

https://tbpl.mozilla.org/php/getParsedLog.php?id=15791309&tree=Mozilla-Inbound

13366 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected interface name in global scope: DataChannel

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

The patch adds a new interface so it should be added to test_interfaces.html

Comment 69

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

Attachment: add test_interfaces.html entry for DataChannel

Comment 70

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

reland:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37ac46f7dd40
https://hg.mozilla.org/integration/mozilla-inbound/rev/de48716d5b48

Comment 71

[Ed Morley [:emorley]]

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

Comment 72

[Jason Smith [:jsmith]]

Going to mark verified given that we've confirmed for months that this is hooked up (confirmed the feature is landed).