Closed Bug 733002 Opened 8 years ago Closed 7 years ago

Implement DOM layer for WebRTC dataChannels

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla18

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Whiteboard: [WebRTC], [blocking-webrtc+])

Attachments

(8 files, 31 obsolete files)

28.81 KB, patch
Details | Diff | Splinter Review
2.66 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.62 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.88 KB, patch
smaug
: review-
Details | Diff | Splinter Review
9.94 KB, patch
Details | Diff | Splinter Review
26.14 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1005 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
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.
Depends on: 729512
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.
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.
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
OS: Linux → All
Hardware: x86_64 → All
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
Attached patch Patch (obsolete) — Splinter Review
Attachment #609454 - Attachment is obsolete: true
Attached patch mods to kyle's patch WIP (obsolete) — Splinter Review
Attachment #609754 - Attachment is obsolete: true
Attachment #609855 - Attachment is obsolete: true
Attachment #609877 - Attachment is obsolete: true
Attachment #610019 - Attachment is obsolete: true
Attachment #610676 - Attachment is obsolete: true
Attachment #610676 - Attachment is obsolete: false
Blocks: dc-tests
Attachment #610676 - Attachment is obsolete: true
Attachment #617790 - Attachment is obsolete: true
Attachment #646421 - Attachment is obsolete: true
Attachment #647091 - Attachment is obsolete: true
Attachment #651798 - Attachment is obsolete: true
This builds, no idea yet if it runs
Attachment #652034 - Attachment is obsolete: true
Attachment #652207 - Attachment is obsolete: true
updated a few bits for DTLS
update port number we use for Init()
Attachment #652354 - Attachment is obsolete: true
Attachment #652214 - Attachment is obsolete: true
Also cleans up a couple of warnings
QA Contact: jsmith
Attachment #652362 - Attachment is obsolete: true
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
Attachment #652687 - Flags: feedback?(ekr)
Comment on attachment 652890 [details] [diff] [review]
Implement proper SCTP shutdown on Mozilla exit

This is really part of bug 729511, not DOM
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
Attached patch DataChannel DOM part 1 rollup (obsolete) — Splinter Review
Builds on m-c on top of the latest bug 729511 and bug 729512 patches (plus a disable-dtls patch I need to upload)
Attachment #647090 - Attachment is obsolete: true
Attachment #647100 - Attachment is obsolete: true
Attachment #647182 - Attachment is obsolete: true
Attachment #647868 - Attachment is obsolete: true
Attachment #648255 - Attachment is obsolete: true
Attachment #650593 - Attachment is obsolete: true
Attachment #651817 - Attachment is obsolete: true
Attachment #652687 - Attachment is obsolete: true
Attachment #652687 - Flags: feedback?(ekr)
Attachment #652890 - Attachment is obsolete: true
Whiteboard: [WebRTC], [blocking-webrtc+]
Attachment #655728 - Attachment is obsolete: true
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
Attachment #659983 - Flags: review?(ted.mielczarek)
Attachment #659983 - Flags: review?(mcmanus)
Attachment #659983 - Flags: review?(cbiesinger)
Comment on attachment 659983 [details] [diff] [review]
DataChannel DOM part 1 rollup

probly :smaug or :bz for dom reviewing dom code.
Attachment #659983 - Flags: review?(mcmanus)
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.
Attachment #659983 - Flags: review?(cbiesinger) → review?(bugs)
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.)
Attachment #659983 - Flags: review?(bugs) → review-
Attachment #659983 - Attachment is obsolete: true
Attachment #659983 - Flags: review?(ted.mielczarek)
(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.
Attachment #661000 - Attachment description: DataChannel DOM implementation → DataChannel DOM implementation Part 1
Attachment #661000 - Flags: review?(ted.mielczarek)
Attachment #661000 - Flags: review?(bugs)
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 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 ;)
Attachment #661000 - Flags: review?(bugs) → review-
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.
Attachment #661889 - Flags: review?(bugs)
Attachment #661889 - Flags: review?(bugs) → review+
(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 on attachment 661917 [details] [diff] [review]
readyState -> DOMString, and minor changes per ms2ger's review

Delta patch to address ms2ger's comments
Attachment #661917 - Flags: review?(bugs)
This is the full rollup patch including both deltas; no other changes.
Assignee: nobody → rjesup
Attachment #661000 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #661000 - Flags: review?(ted.mielczarek)
Attachment #661920 - Flags: review?(bugs)
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.)
Attachment #662017 - Flags: review?(bugs)
Attachment #661917 - Flags: review?(bugs) → review+
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.
Attachment #661920 - Flags: review?(bugs) → review-
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.
Attachment #662017 - Flags: review?(bugs) → review-
Attached patch Updated rollup patch (obsolete) — Splinter Review
Attachment #661920 - Attachment is obsolete: true
Attachment #662783 - Flags: review?(bugs)
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).
Attachment #662782 - Attachment is obsolete: true
Updated - modified file from the wrong directory
Attachment #662787 - Attachment is obsolete: true
Attachment #662783 - Attachment is obsolete: true
Attachment #662783 - Flags: review?(bugs)
Attachment #662795 - Flags: review?(bugs)
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.
(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; }
(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.
(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 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?
Attachment #662795 - Flags: review?(bugs) → review+
Yup, realized after I posted it.  I'll remove that line.  Thanks!

I'm working on a solution to the open event issue
Attachment #663130 - Flags: review?(bugs)
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 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
Attachment #663130 - Flags: review?(bugs) → review+
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
The patch adds a new interface so it should be added to test_interfaces.html
Attachment #667764 - Flags: review?(bugs)
Attachment #667764 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/de48716d5b48
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Keywords: verifyme
Going to mark verified given that we've confirmed for months that this is hooked up (confirmed the feature is landed).
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.