Randell Jesup [:jesup] (needinfo me) Assignee
	Description • 13 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 1 • 13 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 2 • 13 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 3 • 13 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 4 • 13 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 22 • 12 years ago

This builds, no idea yet if it runs

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 25 • 12 years ago

updated a few bits for DTLS

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 26 • 12 years ago

update port number we use for Init()

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 28 • 12 years ago

Also cleans up a couple of warnings

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 29 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 30 • 12 years ago

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

This is really part of bug 729511, not DOM

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 31 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 32 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 34 • 12 years ago

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

	Patrick McManus [:mcmanus]
	Comment 35 • 12 years ago

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

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 36 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 37 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 39 • 12 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 40 • 12 years ago

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;

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 41 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 43 • 12 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 44 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 46 • 12 years ago

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

Delta patch to address ms2ger's comments

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 47 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 49 • 12 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 50 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 51 • 12 years ago

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.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 54 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 56 • 12 years ago

Updated - modified file from the wrong directory

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 58 • 12 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 59 • 12 years ago

(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; }

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 60 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 61 • 12 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 62 • 12 years ago

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?

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 63 • 12 years ago

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

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 65 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 66 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 67 • 12 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 68 • 12 years ago

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

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 70 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 71 • 12 years ago

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

	Jason Smith [:jsmith]
	Comment 72 • 12 years ago

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