Closed Bug 745283 Opened 12 years ago Closed 10 years ago

Expose a client UDP datagram socket API to web applications

Categories

(Core :: DOM: Device Interfaces, defect, P3)

defect

Tracking

()

VERIFIED FIXED
2.1 S3 (29aug)
feature-b2g 2.1

People

(Reporter: asuth, Assigned: schien)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [FT:Stream3][dependency: marketplace-partners][2.1-feature-qa+])

Attachments

(4 files, 54 obsolete files)

17.08 KB, patch
schien
: review+
Details | Diff | Splinter Review
3.65 KB, patch
schien
: review+
Details | Diff | Splinter Review
49.49 KB, patch
schien
: review+
Details | Diff | Splinter Review
48.49 KB, patch
schien
: review+
schien
: superreview+
Details | Diff | Splinter Review
Similar to bug 733573, it would be beneficial to expose a UDP API to content.

The motivating use case is DNS resolution for retrieval of MX records for the e-mail app's autoconfiguration mechanism.  The motivating discussion thread can be found here:
http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/34007ffac7c1ca32/cbf2179ca8fe8e28

Note that TCP can also be used for DNS, so this is not the highest priority.  Also, it is expected that the API will have uses beyond DNS.
See Also: → 733573
A UDP API which included broadcast support would also be beneficial for boot2gecko apps which wish to consume network discoverable services.

An example of this is an app which connects to a iTunes music share via Bonjour.

My motivation in this case is to connect to network discoverable 3D printers from a boot2gecko device for print monitoring and control.

Unlike the DNS example, TCP cannot be used for the use case of network discoverable services.
I think we can base this on the code written in bug 839757 and create a JS api to send UDP messages to a specific address, not just as an answer to an incoming packet.  I think simply changing the server (the name of the interface) to be just a UDP socket (that it actually is..) and add a pure sendMessage method on it is what you need.
Depends on: 839757
To add another use case, I'm working on a library for SSDP (http://en.wikipedia.org/wiki/Simple_Service_Discovery_Protocol) that operates on UDP port 1900.
I try to provide ipdl interface for WebRTC e10s in bug 869869. We can implement mozUDP API based on that patch.
I'm not working on this so I'll release it in case someone else wants to take it
Assignee: dpreston → nobody
Status: ASSIGNED → NEW
Assignee: nobody → pwang
Whiteboard: RN5/29
Whiteboard: RN5/29 → RN6/21
Attached patch WIP: Part 1 - WebIDL (obsolete) — Splinter Review
First WIP interface.. simply copied from http://raw-sockets.sysapps.org/
If we have platform resources to spend on the raw-sockets API family, assuming the e-mail app is the only Gaia app motivating this work, our top-most priority would be getting startTLS support as tracked on bug 784816 since it (at least theoretically) prevents us from supporting all IMAP servers out there.
I'm interested in implementing PPSPP[1] directly in ffox, I'd love to have a UDP family available directly to make this easier. Please ping me dch*jsonified.com.

[1]: http://datatracker.ietf.org/doc/draft-ietf-ppsp-peer-protocol/
Status: NEW → ASSIGNED
Hi All,

Yet another use case (although it's quite close to what was described in comment #4) would be mDNS Service discovery.

While i'm at it, I'm a bit curious about the current progress on the implementation of the IDL provided in attachment #784336 [details] [diff] [review]. Is anyone currently actively working on this or are we waiting on bug #869869 to be completed first ?

On my end I currently have a patch, that is a few months old and which together with bug #869869 enables mDNS discovery using UDP multicast (it would enable SSDP too I guess, 869869 is needed to get the ability of setting the source udp port).

I'd like to share a few implementation issues I have encountered that could make getting thejoinMulticast and leaveMulticast methods to work as specified in the IDL complex.

Currently in NSPR calling SetSocketOption with the AddMember / DropMember options requires a target interface to be specified (interface is selected by providing an ip address) Not specifying any interface (0.0.0.0) will result in the first available interface being selected, which is platform dependent, this is highly problematic if you have multiple network interfaces (think phone here).
Problem is, there is currently no cross-platform way in Necko (that I know of) to list all interfaces / IPs in NSPR, and therefore no way to reliably select all interfaces, or a select specific interface such as Wifi only. Worse even, the only method I've found around this (without modifying NSPR's behaviour) is a hack that only works in windows, which consists in resolving the local host name through DNS, which effectively lists all configured local IP addresses.

There is actually a feature request (bug #461543) to add interface listing capabilities to NSPR, however it's not been active for a few years, and it is probably quite challenging to implement this properly in a portable way across all NSPR supported platforms.

As my employer recently agreed to it, I could share my current patch, it's quite modest (it only adds a multicast method to the current UDPServerSocket) but we'd be happy to share if this can help moving forward with the inclusion of a more comprehensive UDP API in Necko.

Hope this helps,

Florian
Our app has a need for this.  Our use case is the same as Rob Gilson describes - network discovery using broadcst UDP.  I have time to work on it now but was unable to re-assign.  Do I need permissions?
Assignee: pwang → nobody
Assignee: nobody → pwork70
Patrick, when implementing this, it would be awesome if you could make it work in workers right away. That means you'll have to do it in C++ and use PBackground to proxy calls to the parent process.

I think that nsIUDPSocket only works from the main thread still, which sadly means that you'll have to proxy calls between the PBackground thread and the main thread. But there's not much we can do about that for now.
Whiteboard: RN6/21
This is a work in progress - it supports the following scenario: 

var udpSocket = navigator.mozUDPSocket;
udpSocket.onmessage = function (e) { console.log("received: " + e.data); };
udpSocket.send("some message", "111.222.333.444", portNumber);
Peter, please see comment 12. This API needs to be implemented in C++ because we want to make it available in workers.
Jonas, Peter knows it needs to be implemented in C++ -- I asked him to attach his WIP js implementation so that it is not lost.
We planned to support DLNA for the TV project based on the UDPSocket API. I would like this bug land before the end of June. @Peter, I can jump in at any time, if you need some help.
Whiteboard: [stingray]
Shih-Chiang, unfortunately at this time I'm on a different project, so that would be great if you could help out with it.  Do you want the bug?  At this point I just implemented enough functionality in JS to support our use case which was UDP broadcast & receive replies.  I understand it needs to be re-written in C++ so that was my next plan.
Yeah sure, I can start the c++ implementation immediately.
Assignee: pwork70 → schien
This is a compilable stub code for UDP socket API. I'd like to have some feedback for WebIDL before diving into the implementation.
Attachment #784336 - Attachment is obsolete: true
Attachment #8391384 - Attachment is obsolete: true
Attachment #8404647 - Flags: feedback?(josh)
Attachment #8404647 - Flags: feedback?(jonas)
Comment on attachment 8404647 [details] [diff] [review]
[WIP] Part 1 - udp-socket-webidl.patch

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

You should add UDPMessageEvent.webidl to the generated event webidl list, and then you don't need to write UDPMessageEvent.cpp/h.

::: dom/network/src/moz.build
@@ +12,5 @@
>      'TCPSocketChild.h',
>      'TCPSocketParent.h',
>      'Types.h',
> +    'UDPMessageEvent.h',
> +    'UDPSocket.h',

If you export this under mozilla/dom you don't need a HeaderFile annotation.

::: dom/webidl/UDPMessageEvent.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent
> + */
> +
> +[NoInterfaceObject, HeaderFile="mozilla/dom/network/UDPMessageEvent.h"]

HeaderFile shouldn't be necessary, and why NoInterfaceObject?

::: dom/webidl/UDPSocket.webidl
@@ +25,5 @@
> +  "halfclosed"
> +};
> +
> +[Constructor (optional UDPOptions options),
> + HeaderFile="mozilla/dom/network/UDPSocket.h"]

This shouldn't be necessary.
Attachment #8404647 - Flags: feedback?(josh) → feedback+
(In reply to Josh Matthews [:jdm] from comment #21)
> Comment on attachment 8404647 [details] [diff] [review]
> [WIP] Part 1 - udp-socket-webidl.patch
> 
> Review of attachment 8404647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should add UDPMessageEvent.webidl to the generated event webidl list,
> and then you don't need to write UDPMessageEvent.cpp/h.
I tried but the generated code won't compile because of the NoInterfaceObject annotation. The detailed reason is in below.
> 
> ::: dom/network/src/moz.build
> @@ +12,5 @@
> >      'TCPSocketChild.h',
> >      'TCPSocketParent.h',
> >      'Types.h',
> > +    'UDPMessageEvent.h',
> > +    'UDPSocket.h',
> 
> If you export this under mozilla/dom you don't need a HeaderFile annotation.
> 
> ::: dom/webidl/UDPMessageEvent.webidl
> @@ +6,5 @@
> > + * The origin of this IDL file is
> > + * http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent
> > + */
> > +
> > +[NoInterfaceObject, HeaderFile="mozilla/dom/network/UDPMessageEvent.h"]
> 
> HeaderFile shouldn't be necessary, and why NoInterfaceObject?
NoInterfaceObject is annotated in the current spec (http://www.w3.org/TR/raw-sockets/#idl-def-UDPMessageEvent). My guess is the spec editor don't allow people to manually create a fake UDP message and dispatch it in client-side JS. I did some search on w3c spec and our code base, however I couldn't find any other Event interface is defined like this. Should we raise this question to the spec editor?
> 
> ::: dom/webidl/UDPSocket.webidl
> @@ +25,5 @@
> > +  "halfclosed"
> > +};
> > +
> > +[Constructor (optional UDPOptions options),
> > + HeaderFile="mozilla/dom/network/UDPSocket.h"]
> 
> This shouldn't be necessary.
UDPSocket.h is under mozilla/dom/network, generated code will not compiled because it expect the header file is under mozilla/dom. Should I expose UDPSocket.h to mozilla/dom instead?
Flags: needinfo?(josh)
Yes, you can change the dom/network moz.build file to export UDPSocket.h under the mozilla/dom namespace. What's the error you get that's caused by NoInterfaceObject?
Flags: needinfo?(josh)
Attached patch use event codegen, v1 (obsolete) — Splinter Review
The super class |MessageEvent| doesn't use codegen and not follow the convention in the code generator.

error message
=============
> 0:37.17 In file included from /Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dom/bindings/UnifiedBindings31.cpp:114:
> 0:37.17 ./UDPMessageEvent.cpp:40:5: error: no matching constructor for initialization of 'mozilla::dom::MessageEvent'
> 0:37.17   : MessageEvent(aOwner)
> 0:37.17     ^            ~~~~~~
> 0:37.17 ../../dist/include/mozilla/dom/MessageEvent.h:29:7: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'mozilla::dom::EventTarget *' to 'const mozilla::dom::MessageEvent' for 1st argument
> 0:37.17 class MessageEvent : public Event,
> 0:37.17       ^
> 0:37.17 ../../dist/include/mozilla/dom/MessageEvent.h:33:3: note: candidate constructor not viable: requires 3 arguments, but 1 was provided
> 0:37.17   MessageEvent(EventTarget* aOwner,
> 0:37.17   ^
> 0:37.17 1 error generated.
> 0:37.18 In the directory  /Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dom/bindings
Attachment #8405148 - Flags: feedback?(josh)
BTW, do I still need to modify js/xpconnect/src/event_impl_gen.conf.in if I don't want UDPMessageEvent to be created in JS?
Attached patch use event codegen, v2 (obsolete) — Splinter Review
I fix the compile error by adding the corresponding |MessageEvent(EventTarget*)| constructor, but I'm not sure if this modification really make sense.
Attachment #8405148 - Attachment is obsolete: true
Attachment #8405148 - Flags: feedback?(josh)
Attachment #8405214 - Flags: feedback?(josh)
Comment on attachment 8405214 [details] [diff] [review]
use event codegen, v2

The MessageEvent change seems fine to me, since all other users always pass nullptr for those anyways. I think it's worth it to avoid writing another event class by hand.
Attachment #8405214 - Flags: feedback?(josh) → feedback+
support join/leave multicast group.
support send string/ArrayBuffer, receive, join/leave multicast group.

test case: http://people.mozilla.org/~schien/udp-test.html

TODO:
1. support send blob
2. support content process
3. support suspend/resume/ondrain
Attachment #8404647 - Attachment is obsolete: true
Attachment #8405214 - Attachment is obsolete: true
Attachment #8404647 - Flags: feedback?(jonas)
Attachment #8417986 - Attachment description: Part 2 - support socket options/multicast functionality in PUDPSocket.ipdl → [WIP] Part 2 - support socket options/multicast functionality in PUDPSocket.ipdl
Summary of changes:
1. make socket open API synchronous
2. simplify send data API
3. introduce udp-socket permission

TODO: support send blob over IPDL
Attachment #8417986 - Attachment is obsolete: true
Attachment #8419341 - Flags: feedback?(josh)
I'm following this with great interest but I'm not clear atm if it's possible to build firefox with these patches, or a particular branch. Any advice, even if it's just to wait a bit, is welcomed. If there are daily builds or something I can use directly that would be amazing. Thanks & sorry for the noise.
Attached patch [WIP] Part 3 - UDPSocket webidl (obsolete) — Splinter Review
summary of changes:
1. support open/close/joinMulticast/leaveMulticast
2. support send string/ArrayBuffer/ArrayBufferView

TODOs:
1. support send blob
2. support suspend/resume
3. support ondrain event

The API is changed to a Promise-type API in the latest editor's draft[1]. Should I follow the latest editor's draft at the stage?

[1] http://raw-sockets.sysapps.org/
Attachment #8417987 - Attachment is obsolete: true
Attachment #8419345 - Flags: feedback?(josh)
Attachment #8419341 - Attachment description: Part 2 - support socket options/multicast functionality in PUDPSocket.ipdl → [WIP] Part 2 - support socket options/multicast functionality in PUDPSocket.ipdl
(In reply to dch from comment #34)
> I'm following this with great interest but I'm not clear atm if it's
> possible to build firefox with these patches, or a particular branch. Any
> advice, even if it's just to wait a bit, is welcomed. If there are daily
> builds or something I can use directly that would be amazing. Thanks & sorry
> for the noise.

Hi, you can apply these 3 patches I've uploaded in sequence on top of mozilla-central code base. If you don't have a Firefox build environment yet, you can follow the instruction in [1] to create one.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #36)
> (In reply to dch from comment #34)
> > I'm following this with great interest but I'm not clear atm if it's
> > possible to build firefox with these patches, or a particular branch. Any
> > advice, even if it's just to wait a bit, is welcomed. If there are daily
> > builds or something I can use directly that would be amazing. Thanks & sorry
> > for the noise.
> 
> Hi, you can apply these 3 patches I've uploaded in sequence on top of
> mozilla-central code base. If you don't have a Firefox build environment
> yet, you can follow the instruction in [1] to create one.
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions

I pushed my patches to try server and you can download the development build from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-6103e2f0dcc4/
support multicast loopback, reuse_addr, send input stream in nsIUDPSocket
Attachment #8417985 - Attachment is obsolete: true
Attachment #8420069 - Flags: feedback?(jduell.mcbugs)
Update: support input stream.
Attachment #8419341 - Attachment is obsolete: true
Attachment #8419341 - Flags: feedback?(josh)
Attachment #8420070 - Flags: feedback?(josh)
Attached patch [WIP] Part 3 - UDPSocket webidl (obsolete) — Splinter Review
Update: support send blob
Attachment #8419345 - Attachment is obsolete: true
Attachment #8419345 - Flags: feedback?(josh)
Attachment #8420071 - Flags: feedback?(josh)
Comment on attachment 8420069 [details] [diff] [review]
Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v1

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

Looks good to me, but I'd like Honza's opinion on whether allowing port reuse by default is a good idea, and on the SendBinaryStream changes.
Attachment #8420069 - Flags: feedback?(jduell.mcbugs)
Attachment #8420069 - Flags: feedback?(honzab.moz)
Attachment #8420069 - Flags: feedback+
The more I think about it, the more it seems unlikely we'd want to allow UDP port reuse by multiple sockets by default.  schien, is there a reason clients want that?
Flags: needinfo?(schien)
(In reply to Jason Duell (:jduell) from comment #42)
> The more I think about it, the more it seems unlikely we'd want to allow UDP
> port reuse by multiple sockets by default.  schien, is there a reason
> clients want that?
I simply keep the "reuse by default" behavior for backward compatibility. There are three udp-socket users in Mozilla code base, PushService.jsm, SimpleServiceDiscovery.jsm, and WebRTC. I think SimpleServiceDiscovery.jsm and WebRTC are expecting reuse by default. However, there are some add-ons use udp-socket and I'm afraid to break it again since it is out of our control, see bug 869869 comment #125.
Flags: needinfo?(schien)
Comment on attachment 8420069 [details] [diff] [review]
Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v1

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

It was at true before, tcp server has the same setting.  Now we have an option here, yay!  Yeah, I'm OK with that..
Attachment #8420069 - Flags: feedback?(honzab.moz) → feedback+
support send huge buffer by multiple packet.
Attachment #8420069 - Attachment is obsolete: true
Comment on attachment 8420070 [details] [diff] [review]
[WIP] Part 2 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl

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

The changes here seem reasonable; I'm not sure entirely what kind of feedback you're looking for. It's sad that we need to make the socket constructor synchronous.

::: dom/network/src/UDPSocketChild.cpp
@@ +150,5 @@
> +                                 nsIInputStream* aStream)
> +{
> +  NS_ENSURE_ARG(aStream);
> +
> +  nsAutoPtr<OptionalInputStreamParams> stream(new OptionalInputStreamParams());

This doesn't look like it needs to be a heap allocation or nsAutoPtr.

@@ +153,5 @@
> +
> +  nsAutoPtr<OptionalInputStreamParams> stream(new OptionalInputStreamParams());
> +  nsTArray<mozilla::ipc::FileDescriptor> fds;
> +  SerializeInputStream(aStream, *stream, fds);
> +  

nit: whitespace

::: dom/network/src/UDPSocketParent.cpp
@@ +82,2 @@
>  
> +  

nit: whitespace

::: netwerk/ipc/NeckoParent.cpp
@@ +455,3 @@
>                                         const nsCString& aFilter)
>  {
> +  // XXX do nothing?

This method should go away if this is true.
Attachment #8420070 - Flags: feedback?(josh) → feedback+
Comment on attachment 8420071 [details] [diff] [review]
[WIP] Part 3 - UDPSocket webidl

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

This all looks pretty reasonable so far.

::: browser/app/profile/firefox.js
@@ +1566,5 @@
>  // Whether experiments are supported by the current application profile.
>  pref("experiments.supported", true);
> +
> +//XXX for dev only
> +pref("dom.udpsocket.enabled", true);

Get rid of this before pushing, obviously.

::: dom/network/src/UDPSocket.cpp
@@ +160,5 @@
> +}
> +
> +UDPSocket::~UDPSocket()
> +{
> +  if (mSocket) {

Perhaps call Close here instead?

@@ +194,5 @@
> +void
> +UDPSocket::Suspend()
> +{
> +  //TODO
> +  mSuspended = true;

Assert !mSuspended.

@@ +201,5 @@
> +void
> +UDPSocket::Resume()
> +{
> +  //TODO
> +  mSuspended = false;

Assert mSuspended.

@@ +208,5 @@
> +void
> +UDPSocket::JoinMulticastGroup(const nsAString& aMulticastGroupAddress,
> +                              ErrorResult& aRv)
> +{
> +  // If readyState is "closed", throw InvalidStateError.

This comment doesn't add anything to the code right now.

@@ +216,5 @@
> +  }
> +
> +  nsCString address = NS_ConvertUTF16toUTF8(aMulticastGroupAddress);
> +  if (mSocket) {
> +    nsresult rv = mSocket->JoinMulticast(address, EmptyCString());

Why do we pass an empty string here in all cases? Should the iface parameter be removed from the api instead?

@@ +232,5 @@
> +void
> +UDPSocket::LeaveMulticastGroup(const nsAString& aMulticastGroupAddress,
> +                               ErrorResult& aRv)
> +{
> +  // If readyState is "closed", throw InvalidStateError.

Same comment complaint as earlier.

@@ +258,5 @@
> +                const Optional<nsAString >& aRemoteAddress,
> +                const Optional<Nullable<uint16_t > >& aRemotePort,
> +                ErrorResult& aRv)
> +{
> +  // If readyState is "closed", throw InvalidStateError.

Same comment complaint.

@@ +264,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return false;
> +  }
> +
> +  // If the type of the data is not compatible any expected type, throw InvalidAccessError.

compatible with any

@@ +271,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> +    return false;
> +  }
> +
> +  // If remote address and port was not specified in constructor and argument,

were not

@@ +347,5 @@
> +                       fallibleArray.Elements(), fallibleArray.Length(), &count);
> +  } else if (mSocketChild) {
> +    rv = mSocketChild->Send(remoteAddress, remotePort,
> +                            fallibleArray.Elements(), fallibleArray.Length());
> +    count = fallibleArray.Length();

What is the purpose of this? Should we be doing something with count after this point?

@@ +353,5 @@
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return false;
> +  }
> +
> +  if(NS_FAILED(rv)) {

nit: space before (

@@ +410,5 @@
> +      do_CreateInstance("@mozilla.org/network/udp-socket;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (aLocalAddress.IsEmpty()) {
> +    rv = sock->Init(aLocalPort, /* loopback = */ false, aAddressReuse, /* optionalArgc = */ 1);

Why do we ignore aLoopback here?

@@ +460,5 @@
> +
> +  nsIScriptContext* scriptContext = sgo->GetContext();
> +  NS_ENSURE_TRUE(scriptContext, NS_ERROR_FAILURE);
> +
> +  AutoPushJSContext cx(scriptContext->GetNativeContext());

We may want to enter a compartment here. Make sure someone who knows JSAPI stuff looks at this patch.

@@ +473,5 @@
> +
> +  memcpy(data, aData, aDataLength);
> +
> +  JS::Rooted<JS::Value> jsData(cx);
> +  jsData = OBJECT_TO_JSVAL(arrayBuf);

ObjectValue(*arrayBuf)

@@ +483,5 @@
> +                                              jsData);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  //XXX multiple inheritance: Event -> nsISupports & nsIDOMMessageEvent -> nsISupports
> +  nsCOMPtr<nsIDOMEvent> event = do_QueryObject(udpEvent);

I don't understand what's happening here. Why can't you just call udpEvent->SetTrusted and pass udpEvent to DispatchDOMEvent?

@@ +501,5 @@
> +  if (NS_FAILED(rv)) {
> +    return NS_OK;
> +  }
> +
> +  // Create appropriate JS object for message

This comment doesn't make sense here.

@@ +546,5 @@
> +    init.mBubbles = false;
> +    nsRefPtr<ErrorEvent> event = ErrorEvent::Constructor(this,
> +                                                         NS_LITERAL_STRING("error"),
> +                                                         init);
> +    return DispatchDOMEvent(nullptr, event, nullptr, nullptr);;

nit: extra semicolon

::: dom/network/src/UDPSocket.h
@@ +39,5 @@
> +            const uint16_t& aRemotePort);
> +
> +  ~UDPSocket();
> +
> +  // WebIDL

This comment doesn't add anything.

::: dom/webidl/UDPMessageEvent.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent
> + */
> +
> +[NoInterfaceObject,

Why this?

@@ +10,5 @@
> +[NoInterfaceObject,
> + Pref="dom.udpsocket.enabled"]
> +interface UDPMessageEvent : MessageEvent {
> +  readonly    attribute DOMString      remoteAddress;
> +  readonly    attribute unsigned short remotePort;

Nit: I don't think the extra spaces here are necessary.

::: dom/webidl/UDPSocket.webidl
@@ +34,5 @@
> +  readonly    attribute unsigned short? remotePort;
> +  readonly    attribute boolean         addressReuse;
> +  readonly    attribute boolean         loopback;
> +  readonly    attribute unsigned long   bufferedAmount;
> +  readonly    attribute ReadyState      readyState;

Nit: I don't think the spaces between readonly and attribute are necessary.

@@ +42,5 @@
> +              attribute EventHandler    onmessage;
> +
> +  void    close ();
> +  void    suspend ();
> +  void    resume ();

Nit: no need for extra spaces here.

@@ +48,5 @@
> +  [Throws]
> +  void    joinMulticastGroup (DOMString multicastGroupAddress);
> +
> +  [Throws]
> +  void    leaveMulticastGroup (DOMString multicastGroupAddress);

Nit: same here and for joinMulticastGroup

@@ +51,5 @@
> +  [Throws]
> +  void    leaveMulticastGroup (DOMString multicastGroupAddress);
> +
> +  [Throws]
> +  boolean send ((DOMString or Blob or ArrayBuffer or ArrayBufferView) data,

Nit: get rid of the space before (
Attachment #8420071 - Flags: feedback?(josh) → feedback+
(In reply to Josh Matthews [:jdm] from comment #47)
> Comment on attachment 8420070 [details] [diff] [review]
> [WIP] Part 2 - support socket options/multicast/input stream functionality
> in PUDPSocket.ipdl
> 
> Review of attachment 8420070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes here seem reasonable; I'm not sure entirely what kind of
> feedback you're looking for. It's sad that we need to make the socket
> constructor synchronous.

> 
> ::: dom/network/src/UDPSocketChild.cpp
> @@ +150,5 @@
> > +                                 nsIInputStream* aStream)
> > +{
> > +  NS_ENSURE_ARG(aStream);
> > +
> > +  nsAutoPtr<OptionalInputStreamParams> stream(new OptionalInputStreamParams());
> 
> This doesn't look like it needs to be a heap allocation or nsAutoPtr.
Yes, using a local variable is enough. I took the code from WebSocketChild without thinking deeply.
> 
> @@ +153,5 @@
> > +
> > +  nsAutoPtr<OptionalInputStreamParams> stream(new OptionalInputStreamParams());
> > +  nsTArray<mozilla::ipc::FileDescriptor> fds;
> > +  SerializeInputStream(aStream, *stream, fds);
> > +  
> 
> nit: whitespace
done
> 
> ::: dom/network/src/UDPSocketParent.cpp
> @@ +82,2 @@
> >  
> > +  
> 
> nit: whitespace
done
> 
> ::: netwerk/ipc/NeckoParent.cpp
> @@ +455,3 @@
> >                                         const nsCString& aFilter)
> >  {
> > +  // XXX do nothing?
> 
> This method should go away if this is true.
I'm going to move the UDP packet filter initialization to UDPSocketParent::Init and invoke it here.
(In reply to Josh Matthews [:jdm] from comment #48)
> Comment on attachment 8420071 [details] [diff] [review]
> [WIP] Part 3 - UDPSocket webidl
> @@ +216,5 @@
> > +  }
> > +
> > +  nsCString address = NS_ConvertUTF16toUTF8(aMulticastGroupAddress);
> > +  if (mSocket) {
> > +    nsresult rv = mSocket->JoinMulticast(address, EmptyCString());
> 
> Why do we pass an empty string here in all cases? Should the iface parameter
> be removed from the api instead?
The iface parameter is already declared as optional in IDL, but the generated c++ function doesn't create corresponding signature. That's the reason we need to pass an empty string here.
> @@ +347,5 @@
> > +                       fallibleArray.Elements(), fallibleArray.Length(), &count);
> > +  } else if (mSocketChild) {
> > +    rv = mSocketChild->Send(remoteAddress, remotePort,
> > +                            fallibleArray.Elements(), fallibleArray.Length());
> > +    count = fallibleArray.Length();
> 
> What is the purpose of this? Should we be doing something with count after
> this point?
> 
I'll use the count to implement ondrain event in my next version of patch.
> @@ +410,5 @@
> > +      do_CreateInstance("@mozilla.org/network/udp-socket;1", &rv);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  if (aLocalAddress.IsEmpty()) {
> > +    rv = sock->Init(aLocalPort, /* loopback = */ false, aAddressReuse, /* optionalArgc = */ 1);
> 
> Why do we ignore aLoopback here?
The loopback parameter for nsUDPSocket::Init is for binding socket on loopback interface. The aLoopback means that data you send is looped back to your host and is been used later in nsUDPSocket::SetMulticastLoopback.
> @@ +483,5 @@
> > +                                              jsData);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  //XXX multiple inheritance: Event -> nsISupports & nsIDOMMessageEvent -> nsISupports
> > +  nsCOMPtr<nsIDOMEvent> event = do_QueryObject(udpEvent);
> 
> I don't understand what's happening here. Why can't you just call
> udpEvent->SetTrusted and pass udpEvent to DispatchDOMEvent?
udpEvent->SetTrusted is ok. Passing udpEvent to DispatchDOMEvent still get compile error because UDPMessageEvent has two super classes (Event and nsIDOMMessageEvent) inherit nsIDOMEvent interface. I can fix the problem by using do_QueryObject or static_cast<Event*>.
> 
> ::: dom/webidl/UDPMessageEvent.webidl
> @@ +6,5 @@
> > + * The origin of this IDL file is
> > + * http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent
> > + */
> > +
> > +[NoInterfaceObject,
> 
> Why this?
> 
The WebIDL is from raw socket spec. I guess the original purpose is not allowing UDPMessageEvent to be created by client-side Javascript. All the event interfaces defined in raw socket spec are declared as NoInterfaceObject.
summary of changes:
1. support addressReuse parameter
2. expose local address as nsINetAddr
3. support send input stream
4. update xpcshell test case
Attachment #8426131 - Attachment is obsolete: true
Attachment #8428541 - Flags: review?(jduell.mcbugs)
Attachment #8428541 - Flags: review?(honzab.moz)
summary of changes:
1. introduce udp-socket permission
2. support send input stream and multicast operations
3. make PUDPSocket sync
4. move UDPPacketFilter initialization to UDPSocketParent
5. corresponding change in NrSocketIpc
Attachment #8420070 - Attachment is obsolete: true
Attachment #8428545 - Flags: review?(josh)
Attachment #8428545 - Flags: review?(fabrice)
Attachment #8428545 - Flags: review?(ekr)
Attached patch Part 3 - UDPSocket webidl (obsolete) — Splinter Review
summary of changes:
1. implement UDPSocket WebIDL
2. update test_interfaces.html

NOTE: UDPSocket.send will never return false without exception, because packet will be sent ASAP and in-order delivery is not guaranteed. Thus, ondrain event will never be fired in this implementation.
Attachment #8420071 - Attachment is obsolete: true
Attachment #8428549 - Flags: superreview?(jonas)
Attachment #8428549 - Flags: review?(josh)
Attached patch Part 3 - UDPSocket webidl (obsolete) — Splinter Review
summary of changes:
1. implement UDPSocket WebIDL
2. update test_interfaces.html

NOTE: UDPSocket.send will never return false without exception, because packet will be sent ASAP and in-order delivery is not guaranteed. Thus, ondrain event will never be fired in this implementation.
Attachment #8428549 - Attachment is obsolete: true
Attachment #8428549 - Flags: superreview?(jonas)
Attachment #8428549 - Flags: review?(josh)
Attachment #8428553 - Flags: superreview?(jonas)
Attachment #8428553 - Flags: review?(josh)
Attached patch Part 4 - test case for UDPSocket (obsolete) — Splinter Review
summary of changes:
1. test for dom.udpsocket.enabled
2. test for UDPSocket functionality
Attachment #8426134 - Attachment is obsolete: true
Attachment #8428555 - Flags: review?(josh)
Comment on attachment 8428553 [details] [diff] [review]
Part 3 - UDPSocket webidl

@khuey, can you take a look at JSAPI usage in this patch?
Attachment #8428553 - Flags: feedback?(khuey)
Before we start reviewing the details of this patch, what's the assumed security
model? You can't allow content to send arbitrary UDP packets to nonconsenting
targets any more than you can allow it to send arbitary TCP data to nonconsenting
targets. This is a basic assumption of the Web security model and is why we went
to so much effort to filter out these packets for WebRTC so that they had to be STUN
consent verified.

Two questions:
1. Is your intention to make this available to Web content?
2. How do you expect the user to intelligently decide whether this is safe?
This should not be exposed to Web content, the fact that attachment 8428553 [details] [diff] [review] doesn't prevent that is a bug.
Comment on attachment 8428553 [details] [diff] [review]
Part 3 - UDPSocket webidl

You need to hide this API behind a permission in addition to hiding it behind a pref.  Right now, setting the pref to true means that this API will be exposed to Web content.
Attachment #8428553 - Flags: review-
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #58)
> This should not be exposed to Web content, the fact that attachment 8428553 [details] [diff] [review]
> [details] [diff] [review] doesn't prevent that is a bug.

1. That's a start. But we went to a lot of trouble to restrict content processes
in general from sending generic UDP datagrams (https://bugzilla.mozilla.org/show_bug.cgi?id=870660).
Is that concern no longer inoperative? If so, we should remove that code.

2. Is the intent here to prompt the user for this permission or just to set it
for some apps? There's no way for the user to make an intelligent decision
about this.
(In reply to comment #60)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #58)
> > This should not be exposed to Web content, the fact that attachment 8428553 [details] [diff] [review]
> > [details] [diff] [review] doesn't prevent that is a bug.
> 
> 1. That's a start. But we went to a lot of trouble to restrict content
> processes
> in general from sending generic UDP datagrams
> (https://bugzilla.mozilla.org/show_bug.cgi?id=870660).
> Is that concern no longer inoperative? If so, we should remove that code.

I'm not sure, I'm not familiar with that code and why we added it.

> 2. Is the intent here to prompt the user for this permission or just to set it
> for some apps? There's no way for the user to make an intelligent decision
> about this.

We do not prompt for tcp-socket <http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#52>, I'm not sure why we would want to prompt for udp-socket.  And I definitely agree that this is not a good question to ask the user about.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #61)
> (In reply to comment #60)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #58)
> > > This should not be exposed to Web content, the fact that attachment 8428553 [details] [diff] [review]
> > > [details] [diff] [review] doesn't prevent that is a bug.
> > 
> > 1. That's a start. But we went to a lot of trouble to restrict content
> > processes
> > in general from sending generic UDP datagrams
> > (https://bugzilla.mozilla.org/show_bug.cgi?id=870660).
> > Is that concern no longer inoperative? If so, we should remove that code.
> 
> I'm not sure, I'm not familiar with that code and why we added it.

It was added after discussion with Sicking, who was concerned about the
implications of allowing content processes to send arbitrary UDP.



> > 2. Is the intent here to prompt the user for this permission or just to set it
> > for some apps? There's no way for the user to make an intelligent decision
> > about this.
> 
> We do not prompt for tcp-socket
> <http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.
> jsm#52>, I'm not sure why we would want to prompt for udp-socket.  And I
> definitely agree that this is not a good question to ask the user about.

OK
Flags: needinfo?(jonas)
One thing worth noting is that for TCPSocket we were considering letting the manifest more explicitly scope the TCP connections that could be made.  While the security model still depends on the app store reviewers doing a good job, scoping potentially helps them do a better job.

For example, for the email app, we really only need to issue TCP connections on the standard mail-related ports.  Likewise, when email starts doing DNS resolution itself for autoconfig, we'll only need TCP and/or UDP on port 53.  We absolutely do not need to be able to talk UPnP over UDP port 1900.  This doesn't work for everything (people like to run SSH on arbitrary ports or support crazy port knocking), but the ability for specific white/black-listing to occur, especially if certain more dangerous ports are blacklisted by default (tftp?) and must be whitelisted, still seems generally useful.

Domain-based restriction seem less critical, but could still be handy for the cases where WebSockets aren't being used for some reason.
(In reply to Andrew Sutherland (:asuth) from comment #63)
> One thing worth noting is that for TCPSocket we were considering letting the
> manifest more explicitly scope the TCP connections that could be made. 
> While the security model still depends on the app store reviewers doing a
> good job, scoping potentially helps them do a better job.
> 
> For example, for the email app, we really only need to issue TCP connections
> on the standard mail-related ports.  Likewise, when email starts doing DNS
> resolution itself for autoconfig, we'll only need TCP and/or UDP on port 53.
> We absolutely do not need to be able to talk UPnP over UDP port 1900.  This
> doesn't work for everything (people like to run SSH on arbitrary ports or
> support crazy port knocking), but the ability for specific
> white/black-listing to occur, especially if certain more dangerous ports are
> blacklisted by default (tftp?) and must be whitelisted, still seems
> generally useful.

It's pretty hard to distinguish between the ports that are useful and
the ports that are unsafe. Aren't those mostly the same?


> Domain-based restriction seem less critical, but could still be handy for
> the cases where WebSockets aren't being used for some reason.

What do you have in mind for domain-based restriction? Recall that the
nameserver can return internal IP addreses.
The security model I have in mind is the UDPSocket can only be created by webapps that have udp-socket permission. This is the same as the security model we have for TCPSocket. The packet filtering mechanism is only applied for socket that created by WebRTC because webapps and pages don't need any permission. As @asuth mentioned in comment #63, the security model depends on app store reviewer.

For the SSDP use case, webapps might need to send UDP packet to other endpoint in the local network. Domain-based restriction doesn't applied on this scenario because webapps need to list all the possible local IP addresses in manifest.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #65)
> The security model I have in mind is the UDPSocket can only be created by
> webapps that have udp-socket permission. This is the same as the security
> model we have for TCPSocket. The packet filtering mechanism is only applied
> for socket that created by WebRTC because webapps and pages don't need any
> permission. As @asuth mentioned in comment #63, the security model depends
> on app store reviewer.
> 
> For the SSDP use case, webapps might need to send UDP packet to other
> endpoint in the local network. Domain-based restriction doesn't applied on
> this scenario because webapps need to list all the possible local IP
> addresses in manifest.

Where are these restrictions enforced, the child or the parent process?
(In reply to Eric Rescorla (:ekr) from comment #66)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #65)
> > The security model I have in mind is the UDPSocket can only be created by
> > webapps that have udp-socket permission. This is the same as the security
> > model we have for TCPSocket. The packet filtering mechanism is only applied
> > for socket that created by WebRTC because webapps and pages don't need any
> > permission. As @asuth mentioned in comment #63, the security model depends
> > on app store reviewer.
> > 
> > For the SSDP use case, webapps might need to send UDP packet to other
> > endpoint in the local network. Domain-based restriction doesn't applied on
> > this scenario because webapps need to list all the possible local IP
> > addresses in manifest.
> 
> Where are these restrictions enforced, the child or the parent process?

In parent process (UDPSocketParent::RecvBind). You can see the code in UDPSocketParent.cpp in Attachment 8428545 [details] [diff].
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #67)
> (In reply to Eric Rescorla (:ekr) from comment #66)
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #65)
> > > The security model I have in mind is the UDPSocket can only be created by
> > > webapps that have udp-socket permission. This is the same as the security
> > > model we have for TCPSocket. The packet filtering mechanism is only applied
> > > for socket that created by WebRTC because webapps and pages don't need any
> > > permission. As @asuth mentioned in comment #63, the security model depends
> > > on app store reviewer.
> > > 
> > > For the SSDP use case, webapps might need to send UDP packet to other
> > > endpoint in the local network. Domain-based restriction doesn't applied on
> > > this scenario because webapps need to list all the possible local IP
> > > addresses in manifest.
> > 
> > Where are these restrictions enforced, the child or the parent process?
> 
> In parent process (UDPSocketParent::RecvBind). You can see the code in
> UDPSocketParent.cpp in Attachment 8428545 [details] [diff].

OK. Sounds reasonable.
(In reply to Eric Rescorla (:ekr) from comment #64)
> It's pretty hard to distinguish between the ports that are useful and
> the ports that are unsafe. Aren't those mostly the same?

Yes, but only a port-scanning app would need to access all the ports from within a single app.  So to re-state, an option would be to require all target ports < 1024 to be explicitly enumerated in the manifest plus any high ports we're particularly concerned about (like UPnP poking holes in the firewall).  Ports above 1024 could also be manually enumerated or have a "highports" option that just means we expect to connect to randomly bound ports so we need to be ready to talk to any port.

We'd protect TCPSocket with this too.
 
> What do you have in mind for domain-based restriction? Recall that the
> nameserver can return internal IP addreses.

The assumption for a protection like this is that we aren't dealing with an initially-attacker authored-app and are instead limiting the damage if that could happen if the app is compromised/gets some bad data.  So the domain(s)/wildcards provided wouldn't be something easily hijacked (ex: datafeeds.somebank.com).

On https://wiki.mozilla.org/Security/Reviews/TCPSocket we did discuss triggering explicit prompting or requiring a specific manifest opt-in to access local-net IPs.


Of the two families of protection, requiring the declaration of ports seems most useful and likely to be used.
Once a child-process has been given the upd-socket permission, which we should only do to processes that run privileged apps (i.e. packaged apps that are signed by mozilla), then I do think that we should allow arbitrary UDP datagrams yes.

But child processes that are running just generic web content should not have permission to send arbitrary UDP datagrams in my opinion.
Flags: needinfo?(jonas)
Comment on attachment 8428545 [details] [diff] [review]
Part 2 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl

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

Clearing review flag since there's no hope I can take a look at this in the next 3 weeks. I'm fine with others reviewing!
Attachment #8428545 - Flags: review?(fabrice)
Attached patch Part 2 - udp-socket permission (obsolete) — Splinter Review
extract the change for "udp-socket" permission into another patch.
Attachment #8429732 - Flags: review?(jonas)
move the change for "udp-socket" permission to another patch.
Attachment #8428545 - Attachment is obsolete: true
Attachment #8428545 - Flags: review?(josh)
Attachment #8428545 - Flags: review?(ekr)
Attachment #8429734 - Flags: review?(josh)
Attachment #8429734 - Flags: review?(ekr)
Attached patch Part 4 - UDPSocket webidl (obsolete) — Splinter Review
update the permission control for non-oop frame and merge test case into one patch.
Attachment #8428553 - Attachment is obsolete: true
Attachment #8428555 - Attachment is obsolete: true
Attachment #8428553 - Flags: superreview?(jonas)
Attachment #8428553 - Flags: review?(josh)
Attachment #8428553 - Flags: feedback?(khuey)
Attachment #8428555 - Flags: review?(josh)
Attachment #8429735 - Flags: review?(josh)
Attachment #8429735 - Flags: feedback?(ehsan)
Comment on attachment 8428541 [details] [diff] [review]
Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v2

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

Looks OK to me but I'd like Honza to review as well.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +160,5 @@
>  
>      /**
> +     * sendBinaryStream
> +     *
> +     * Send out the datagrame to specified remote address and port.

datagram (no 'e')

@@ +172,5 @@
> +
> +    /**
> +     * sendBinaryStreamWithAddress
> +     *
> +     * Send out the datagrame to specified remote address and port.

datagram
Attachment #8428541 - Flags: review?(jduell.mcbugs) → review+
Shih-Chiang, what parts of the patch would you like me to provide feedback on?  Thanks!
Flags: needinfo?(schien)
Comment on attachment 8429735 [details] [diff] [review]
Part 4 - UDPSocket webidl

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

Mostly looked at the WebIDL changes, please flag me again if you'd like me to look at the rest of the patch.

::: b2g/app/b2g.js
@@ +972,5 @@
>  // Enable mapped array buffer
>  pref("dom.mapped_arraybuffer.enabled", true);
> +
> +// UDPSocket API
> +pref("dom.udpsocket.enabled", true);

What is the goal of this pref?  What you currently have enables the API for all web pages on b2g, which is definitely not what we want.  If you want to use the pref as a way to disable the API in case there is a security/stability issue with the implementation, I think you can remove this from b2g.js, and in all.js either set the pref to false if you'd like to land this disabled by default, or set it to true if you feel confident about landing this enabled by default.  But like I said in the other part of my comment, you should use [CheckPermissions] for the access check to the API entry point.

::: dom/webidl/UDPMessageEvent.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent
> + */
> +
> +[NoInterfaceObject,

Hmm, do we really want to keep this [NoInterfaceObject]?  I think we should consider raising a spec issue, and give this a constructor, etc.  Please check with smaug.

@@ +7,5 @@
> + * http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent
> + */
> +
> +[NoInterfaceObject,
> + Pref="dom.udpsocket.enabled"]

You need to use [CheckPermissions] here as well.

::: dom/webidl/UDPSocket.webidl
@@ +25,5 @@
> +  "halfclosed"
> +};
> +
> +[Constructor (optional UDPOptions options),
> + Pref="dom.udpsocket.enabled"]

This needs to use [CheckPermissions] as well.  I'm assuming that the pref is currently used to disable the implementation altogether.

@@ +26,5 @@
> +};
> +
> +[Constructor (optional UDPOptions options),
> + Pref="dom.udpsocket.enabled"]
> +interface UDPSocket : EventTarget {

Nit: please paste these definitions directly from the spec, so that in the future we can re-paste from newer versions of the spec and diff quickly to see what has changed.
Attachment #8429735 - Flags: feedback?(ehsan)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #76)
> Shih-Chiang, what parts of the patch would you like me to provide feedback
> on?  Thanks!

Hi Ehsan, I'd like see your feedback on the permission part. The permission model is copied from TCPSocket. I didn't realize there is a [CheckPermissions] for webidl and it is good to make UDPSocket not available instead of crash after use it. Maybe we should also use [CheckPermissions] on TCPSocket.

BTW, I do have permission check in my current patch for both OOP and non-OOP use case (in UDPSocket.cpp and UDPSocketParent.cpp). Web page will see this API but will get an exception or force quit after using UDPSocket.
Flags: needinfo?(schien)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #77)
> Comment on attachment 8429735 [details] [diff] [review]
> Part 4 - UDPSocket webidl
> @@ +26,5 @@
> > +};
> > +
> > +[Constructor (optional UDPOptions options),
> > + Pref="dom.udpsocket.enabled"]
> > +interface UDPSocket : EventTarget {
> 
> Nit: please paste these definitions directly from the spec, so that in the
> future we can re-paste from newer versions of the spec and diff quickly to
> see what has changed.

Hi Ehsan, in comment #48 @jdm ask me to remove the extra white space in webidl. May I know the latest convention for webidl?
Flags: needinfo?(ehsan)
Comment on attachment 8428541 [details] [diff] [review]
Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v2

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

Fix also UDPSocketParent call arguments.

I've checked the stream is copied out to the socket in one loop of nsAStreamCopier::Process() so the stream content cannot be in anyway mixed up.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +51,3 @@
>       */
> +    [optional_argc]
> +    void init(in long aPort, in boolean aLoopbackOnly, [optional] in boolean aAddressReuse);

all on one line

@@ +164,5 @@
> +     * Send out the datagrame to specified remote address and port.
> +     *
> +     * @param host The remote host name.
> +     * @param port The remote port.
> +     * @param stream The input stream to be sent.

document what are the requirements/options for the stream: blocking? async? buffered?

@@ +175,5 @@
> +     *
> +     * Send out the datagrame to specified remote address and port.
> +     *
> +     * @param addr The remote host address.
> +     * @param stream The input stream to be sent.

as well here.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +1021,5 @@
> +  PR_InitializeNetAddr(PR_IpAddrAny, 0, &prAddr);
> +  NetAddrToPRNetAddr(aAddr, &prAddr);
> +
> +  nsRefPtr<nsUDPOutputStream> os = new nsUDPOutputStream(this, mFD, prAddr);
> +  return NS_AsyncCopy(aStream, os, mSts, NS_ASYNCCOPY_VIA_READSEGMENTS,

Note that this means the input stream needs to be buffered.
Attachment #8428541 - Flags: review?(honzab.moz) → review+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #78)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #76)
> > Shih-Chiang, what parts of the patch would you like me to provide feedback
> > on?  Thanks!
> 
> Hi Ehsan, I'd like see your feedback on the permission part. The permission
> model is copied from TCPSocket. I didn't realize there is a
> [CheckPermissions] for webidl and it is good to make UDPSocket not available
> instead of crash after use it. Maybe we should also use [CheckPermissions]
> on TCPSocket.

Yes, we should!  FWIW permission checks used to be written as [Func] attributes which checked the permission manually, [CheckPermissions] is a new attribute that we recently added in bug 952486 to make it possible to write these permission checks declaratively.

> BTW, I do have permission check in my current patch for both OOP and non-OOP
> use case (in UDPSocket.cpp and UDPSocketParent.cpp). Web page will see this
> API but will get an exception or force quit after using UDPSocket.

That is not the correct way to expose an API.  We should only make the API visible to pages which will be allowed to call into this API, which is why I asked you to add the [CheckPermissions] bits in the IDL.  With that, you should be able to remove the permission checks in the implementation of the API, because only pages which have the API visible would be able to call into it.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #79)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #77)
> > Comment on attachment 8429735 [details] [diff] [review]
> > Part 4 - UDPSocket webidl
> > @@ +26,5 @@
> > > +};
> > > +
> > > +[Constructor (optional UDPOptions options),
> > > + Pref="dom.udpsocket.enabled"]
> > > +interface UDPSocket : EventTarget {
> > 
> > Nit: please paste these definitions directly from the spec, so that in the
> > future we can re-paste from newer versions of the spec and diff quickly to
> > see what has changed.
> 
> Hi Ehsan, in comment #48 @jdm ask me to remove the extra white space in
> webidl. May I know the latest convention for webidl?

Hmm, I guess different people have different ideas on what we should do here.  :-)  I don't care about this very strongly, as it's just a stylistic issue, so please do what Josh asked you.  Thanks!
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #81)
> > Hi Ehsan, in comment #48 @jdm ask me to remove the extra white space in
> > webidl. May I know the latest convention for webidl?
> 
> Hmm, I guess different people have different ideas on what we should do
> here.  :-)  I don't care about this very strongly, as it's just a stylistic
> issue, so please do what Josh asked you.  Thanks!

If it's a direct copy and paste, I don't care.
Comment on attachment 8429732 [details] [diff] [review]
Part 2 - udp-socket permission

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

Please get someone else to review the test part. I don't know the infrastructure there well enough.
Attachment #8429732 - Flags: review?(jonas) → review+
Comment on attachment 8429732 [details] [diff] [review]
Part 2 - udp-socket permission

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

Hi @baku, can you help review the test case for "udp-socket" permission?

NOTE: I'll remove the change in marketplace_privileged_app.webapp because it is unnecessary.
Attachment #8429732 - Flags: review?(amarchesini)
Comment on attachment 8429734 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl

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

::: dom/network/src/UDPSocketParent.cpp
@@ +68,5 @@
>  {
> +  // We don't have browser actors in xpcshell, and hence can't run automated
> +  // tests without this loophole.
> +  if (net::UsingNeckoIPCSecurity() &&
> +      (mFilter || !AssertAppProcessPermission(Manager()->Manager(), "udp-socket"))) {

The correct statement for checking permission should be |if(net::UsingNeckoIPCSecurity() && !mFilter && !AssertAppProcessPermission(Manager()->Manager(), "udp-socket"))|. Will fix in next version.
Attachment #8429734 - Flags: review?(josh)
Attachment #8429734 - Flags: review?(ekr)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #77)
> Comment on attachment 8429735 [details] [diff] [review]
> Part 4 - UDPSocket webidl
> 
> ::: dom/webidl/UDPMessageEvent.webidl
> @@ +6,5 @@
> > + * The origin of this IDL file is
> > + * http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent
> > + */
> > +
> > +[NoInterfaceObject,
> 
> Hmm, do we really want to keep this [NoInterfaceObject]?  I think we should
> consider raising a spec issue, and give this a constructor, etc.  Please
> check with smaug.
Hi @smaug, UDPMessageEvent is annotate with [NoInterfaceObject] in current version of working draft. Do you think it is a bug in spec?
IMO we should keep this attribute because we shouldn't allow JS code to create a fake UDP package and inject it to a socket listener.
Flags: needinfo?(bugs)
Comment on attachment 8429735 [details] [diff] [review]
Part 4 - UDPSocket webidl

cancel the review request for now and I'll upload another version that uses [CheckPermissions] in webidl
Attachment #8429735 - Flags: review?(josh)
Attachment #8429732 - Flags: review?(amarchesini) → review+
> Hi @smaug, UDPMessageEvent is annotate with [NoInterfaceObject] in current
> version of working draft. Do you think it is a bug in spec?
> IMO we should keep this attribute because we shouldn't allow JS code to
> create a fake UDP package and inject it to a socket listener.

Er, what? Does some method take UDPMessageEvent as parameter?
I'm not aware of any other [NoInterfaceObject] event interface.
Looks like a spec bug to me.
One can always use !event.isTrusted to see if event is created by content JS.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (vacation-ish May 26-30) from comment #88)
> > Hi @smaug, UDPMessageEvent is annotate with [NoInterfaceObject] in current
> > version of working draft. Do you think it is a bug in spec?
> > IMO we should keep this attribute because we shouldn't allow JS code to
> > create a fake UDP package and inject it to a socket listener.
> 
> Er, what? Does some method take UDPMessageEvent as parameter?
> I'm not aware of any other [NoInterfaceObject] event interface.
> Looks like a spec bug to me.
> One can always use !event.isTrusted to see if event is created by content JS.

Only the onmessage callback function take UDPMessageEvent as parameter. What's the next step to do if this is a spec bug?
UDPMessageEvent is removed in the latest editor's draft [1]. Do we still need to file a spec bug for it?

[1] http://www.w3.org/2012/sysapps/tcp-udp-sockets/#interface-udpsocket
Flags: needinfo?(bugs)
If there is no such interface anymore, no need to file a spec bug :)
Flags: needinfo?(bugs)
I spent quit a lot of time rewrite UDPMessageEvent.webidl but there are too many codegen errors need to fix. The main reason is that UDPMessageEvent is inherited from MessageEvent and MessageEvent is not codegen from WebIDL.
I start to think it is not valuable to fix the [NoInterfaceObject] issue because there is no UDPMessageEvent in next version of UDPSocket API [1]. @jdm, do you think we need to block on it?

[1] http://www.w3.org/2012/sysapps/tcp-udp-sockets/#dictionary-udpmessage
Flags: needinfo?(josh)
update according to review comment and rebase to latest m-c, carry r+.
Attachment #8428541 - Attachment is obsolete: true
Attachment #8437551 - Flags: review+
Attached patch Part 2 - udp-socket permission (obsolete) — Splinter Review
rebase to latest m-c, carry r+.
Attachment #8429732 - Attachment is obsolete: true
Attachment #8437552 - Flags: review+
Attached patch Part 4 - UDPSocket webidl, v2 (obsolete) — Splinter Review
update according to review comment.
Attachment #8429735 - Attachment is obsolete: true
Attachment #8437553 - Attachment description: support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v2 → Part3, support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v2
Attached patch Part 4 - UDPSocket webidl, v2 (obsolete) — Splinter Review
Ask @khuey to review JSAPI usage first. Will add r? to jdm if he think I can go with [NoInterfaceObject].
Attachment #8437555 - Attachment is obsolete: true
Attachment #8439064 - Flags: review?(khuey)
Since there's no UDPMessageEvent, why are we implementing it in place of the UDPMessage dictionary?
Flags: needinfo?(josh)
(In reply to Josh Matthews [:jdm] from comment #98)
> Since there's no UDPMessageEvent, why are we implementing it in place of the
> UDPMessage dictionary?

Incoming packet is still delivered by event listener in this version of API but in next version it changes to use ReadableStream.read() and ReadableStream.wait(). According to Bug 891286, It's not like we are going to implement Stream API any soon. I still need to use UDPMessageEvent in this version.
I found two OS dependent behavior during my test.

1. receive buffer size: OSX 10.6, Windows XP, and Windows 7 have smaller receive buffer size (8k in windows and 40k in OSX 10.6). Sending a ArrayBuffer/Blob larger than the receive buffer size will cause packet dropped at receiver side. Do we need to provide a minimal guaranteed size of receive buffer across all platforms?

2. address reuse: OSX, Linux, Windows XP, and Windows 7/8 define different strategies of reuse a socket[1]. Setting |addressReuse = true| in JS doesn't mean you can share the socket on all platforms. Do we need to put an note on spec about the OS-dependent behavior?

[1] http://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-so-reuseport-how-do-they-differ-do-they-mean-t
Flags: needinfo?(josh)
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8437553 [details] [diff] [review]
Part3, support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v2

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

::: dom/network/src/UDPSocketParent.cpp
@@ +149,5 @@
> +
> +    // Sending unallowed data, kill content.
> +    NS_ENSURE_SUCCESS(rv, false);
> +
> +    if(!allowed) {

nit: space before (

@@ +179,2 @@
>    uint32_t count;
> +  switch(aAddr.type()) {

Nit: according to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style, this (and all other switches in this patch) should follow
switch (aAddr.type()) {
  case ...: {
    ...
    break;
  }
}

@@ +181,5 @@
> +  case UDPSocketAddr::TUDPAddressInfo:
> +    {
> +      const UDPAddressInfo& addrInfo(aAddr.get_UDPAddressInfo());
> +      rv = mSocket->Send(addrInfo.addr(), addrInfo.port(),
> +                           aData.Elements(), aData.Length(), &count);

nit: indentation

@@ +188,5 @@
> +  case UDPSocketAddr::TNetAddr:
> +    {
> +      const NetAddr& addr(aAddr.get_NetAddr());
> +      rv = mSocket->SendWithAddress(&addr, aData.Elements(),
> +                                      aData.Length(), &count);

nit: indentation

@@ +240,3 @@
>  {
> +  nsresult rv = mSocket->JoinMulticast(aMulticastAddress, aInterface);
> +  NS_ENSURE_SUCCESS(rv, false);

Do we really want to terminate the child process if this operation fails?

@@ +252,1 @@
>    NS_ENSURE_SUCCESS(rv, false);

Same question about process termination.
Attachment #8437553 - Flags: review?(josh) → review+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #99)
> (In reply to Josh Matthews [:jdm] from comment #98)
> > Since there's no UDPMessageEvent, why are we implementing it in place of the
> > UDPMessage dictionary?
> 
> Incoming packet is still delivered by event listener in this version of API
> but in next version it changes to use ReadableStream.read() and
> ReadableStream.wait(). According to Bug 891286, It's not like we are going
> to implement Stream API any soon. I still need to use UDPMessageEvent in
> this version.

Ok, I guess NoInterfaceObject is fine in that case. Do we have a plan and/or timeframe to implement what's in the spec?

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #100)
> I found two OS dependent behavior during my test.
> 
> 1. receive buffer size: OSX 10.6, Windows XP, and Windows 7 have smaller
> receive buffer size (8k in windows and 40k in OSX 10.6). Sending a
> ArrayBuffer/Blob larger than the receive buffer size will cause packet
> dropped at receiver side. Do we need to provide a minimal guaranteed size of
> receive buffer across all platforms?
> 
> 2. address reuse: OSX, Linux, Windows XP, and Windows 7/8 define different
> strategies of reuse a socket[1]. Setting |addressReuse = true| in JS doesn't
> mean you can share the socket on all platforms. Do we need to put an note on
> spec about the OS-dependent behavior?
> 
> [1]
> http://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-
> so-reuseport-how-do-they-differ-do-they-mean-t

I think fzzzy is probably in a better position to answer this than me.
Flags: needinfo?(josh) → needinfo?(dpreston)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #100)
> I found two OS dependent behavior during my test.
> 
> 1. receive buffer size: OSX 10.6, Windows XP, and Windows 7 have smaller
> receive buffer size (8k in windows and 40k in OSX 10.6). Sending a
> ArrayBuffer/Blob larger than the receive buffer size will cause packet
> dropped at receiver side. Do we need to provide a minimal guaranteed size of
> receive buffer across all platforms?

Requiring packets to be smaller is just one of the pitfalls of using UDP instead of TCP. I think we should put a note in the documentation recommending using small packet sizes (No bigger than 8k). Doing extra work to increase buffer size across platforms is not worth it, in my opinion.

> 2. address reuse: OSX, Linux, Windows XP, and Windows 7/8 define different
> strategies of reuse a socket[1]. Setting |addressReuse = true| in JS doesn't
> mean you can share the socket on all platforms. Do we need to put an note on
> spec about the OS-dependent behavior?
> 
> [1]
> http://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-
> so-reuseport-how-do-they-differ-do-they-mean-t

This is really unfortunate. Since the incompatibility is at the OS level and we can't do much about it, I do think we need to put a note in the spec mentioning that the behavior will be different depending on the underlying os.
Flags: needinfo?(dpreston)
(In reply to Josh Matthews [:jdm] from comment #102)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #99)
> > (In reply to Josh Matthews [:jdm] from comment #98)
> > > Since there's no UDPMessageEvent, why are we implementing it in place of the
> > > UDPMessage dictionary?
> > 
> > Incoming packet is still delivered by event listener in this version of API
> > but in next version it changes to use ReadableStream.read() and
> > ReadableStream.wait(). According to Bug 891286, It's not like we are going
> > to implement Stream API any soon. I still need to use UDPMessageEvent in
> > this version.
> 
> Ok, I guess NoInterfaceObject is fine in that case. Do we have a plan and/or
> timeframe to implement what's in the spec?

I don't know there is any plan to implement Stream API. Jonas, do we have a plan to implement Stream API?
Flags: needinfo?(jonas)
Honza, can you answer comment 100?
Flags: needinfo?(honzab.moz)
Flags: needinfo?(jduell.mcbugs)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #100)
> I found two OS dependent behavior during my test.
> 
> 1. receive buffer size: OSX 10.6, Windows XP, and Windows 7 have smaller
> receive buffer size (8k in windows and 40k in OSX 10.6). Sending a
> ArrayBuffer/Blob larger than the receive buffer size will cause packet
> dropped at receiver side. Do we need to provide a minimal guaranteed size of
> receive buffer across all platforms?

Hm.. this is (AFAIK) disadvantage of UDP.  When sending data with TCP, on fill of the target rwin (the read buffer) sources stops and waits for ack.  UDP doesn't do anything like this unless it's implemented on the application level - mimicking TCP actually.

So, depends on how large data you want to be sending between peers, enlarging the buffer may be wise.  

However, you cannot ensure or be sure that the recv buffer of your peer is always at least N bytes available.  So, it's just a half solution anyway.

Also, when you send a UDP datagram larger then 1 MTU it will fragment to ethernet (or other type/smaller) frames and thus raise the chance one of the frames gets lost and the whole datagram is not re-built in peer's system buffer.

We should send by chunks of something<1MTU.  I.e. send by some 1400kB pieces (most common MTU).  I though we already did that.


Patrick McManus may know even more details here.

> 
> 2. address reuse: OSX, Linux, Windows XP, and Windows 7/8 define different
> strategies of reuse a socket[1]. Setting |addressReuse = true| in JS doesn't
> mean you can share the socket on all platforms. Do we need to put an note on
> spec about the OS-dependent behavior?
> 
> [1]
> http://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-
> so-reuseport-how-do-they-differ-do-they-mean-t

Hmm.. We, I believe, only support REUSEADDRES (not PORT) in NSPR.  Then, what we probably want to resolve with this is the TIME_WAIT state.  But that doesn't seem to apply to UDP unless there are multiple listeners to a single multicast [2].  But there you need REUSEPORT (on linux) anyway, that NSPR doesn't know and is implemented on every platform a little bit different way.

So, thinking about this more, do we need this turned on by default for UDP?  Do we need it at all?  Should we default to not reuse on single cast binding and reuse on multicast?

[2] http://stackoverflow.com/questions/12540449/so-reuseaddr-with-udp-sockets-on-linux-is-it-necessary
Flags: needinfo?(honzab.moz) → needinfo?(mcmanus)
(In reply to Honza Bambas (:mayhemer) from comment #106)
> > 
> > 2. address reuse: OSX, Linux, Windows XP, and Windows 7/8 define different
> > strategies of reuse a socket[1]. Setting |addressReuse = true| in JS doesn't
> > mean you can share the socket on all platforms. Do we need to put an note on
> > spec about the OS-dependent behavior?
> > 
> > [1]
> > http://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-
> > so-reuseport-how-do-they-differ-do-they-mean-t
> 
> Hmm.. We, I believe, only support REUSEADDRES (not PORT) in NSPR.  Then,
> what we probably want to resolve with this is the TIME_WAIT state.  But that
> doesn't seem to apply to UDP unless there are multiple listeners to a single
> multicast [2].  But there you need REUSEPORT (on linux) anyway, that NSPR
> doesn't know and is implemented on every platform a little bit different way.

NSPR actually does now support REUSEPORT[1] (added 1 month ago).  But yes, as you say, the meaning is slightly different per platform.  Also, it's a relatively new addition to Linux, only added in Linux 3.9, which means it is not yet available on many current b2g devices (as an example).

[1]: http://hg.mozilla.org/mozilla-central/rev/fbab35dfe7e3
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #100)
> I found two OS dependent behavior during my test.
> 
> 1. receive buffer size: OSX 10.6, Windows XP, and Windows 7 have smaller
> receive buffer size (8k in windows and 40k in OSX 10.6). Sending a
> ArrayBuffer/Blob larger than the receive buffer size will cause packet
> dropped at receiver side. Do we need to provide a minimal guaranteed size of
> receive buffer across all platforms?

with UDP the only minimum you can guarantee is going to be 548 bytes. (The IP minimum of 576 - 28 bytes of UDP/IP headers). Even if by spec you mandate something higher for implementers of this web-udp spec you can't say anything about the infrastructure that carries the IP. So be careful what you document about guarantees :)

dns is limited to 512 bytes of udp for that reason.

Also, actually using anything over 1500 at scale is begging for trouble because it often results in IP fragmentation.. and the kernel then has to reassemble the IP fragments into the original UDP message before delivering it to userspace.. this is a traditional DoS attack on servers (because clients are not authenticated even via a handshake) and they mitigate it by severely restricting the amount of reassembly buffering available and just dropping any overflow. Making it pretty useless at scale.

The good news is UDP applicaton developers generally know this and keep things small - so having an effective 8k limit shouldn't be a big problem.

I would document the reality - 548 is what you can count on.

And then I would silently set the receive buffer on every platform to at least 32KB to make as much stuff just-work as possible.

> 
> 2. address reuse: OSX, Linux, Windows XP, and Windows 7/8 define different
> strategies of reuse a socket[1]. Setting |addressReuse = true| in JS doesn't
> mean you can share the socket on all platforms. Do we need to put an note on
> spec about the OS-dependent behavior?
> 

yes - I think that's what you're going to need to do
Flags: needinfo?(mcmanus)
Comment on attachment 8439064 [details] [diff] [review]
Part 4 - UDPSocket webidl, v2

start the WebAPI review.
Attachment #8439064 - Flags: review?(josh)
How would I test the patches out?  I applied the patches to mozilla-central then built the b2g desktop client, enabled permissions & preferences, but UDPSocket() is undefined.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #104)
> I don't know there is any plan to implement Stream API. Jonas, do we have a
> plan to implement Stream API?

We do as soon as there's somewhat of a consensus of what it looks like. I think that will take a long time still, so we shouldn't wait for it. For now we should implement this API without relying on Streams I think.
Flags: needinfo?(jonas)
Update according to review comment.

@ekr, please review nrsocket and packet filter related modification. Thanks.
Attachment #8437553 - Attachment is obsolete: true
Attachment #8437553 - Flags: review?(ekr)
Attachment #8446480 - Flags: review?(ekr)
Attached patch Part 4 - UDPSocket webidl, v3 (obsolete) — Splinter Review
Update according to latest AutoJSAPI change in Bug 1025476.
Attachment #8439064 - Attachment is obsolete: true
Attachment #8439064 - Flags: review?(khuey)
Attachment #8439064 - Flags: review?(josh)
Attachment #8446481 - Flags: superreview?(jonas)
Attachment #8446481 - Flags: review?(khuey)
Attachment #8446481 - Flags: review?(josh)
Comment on attachment 8446481 [details] [diff] [review]
Part 4 - UDPSocket webidl, v3

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

Does a UDPSocket that we never call Close() on get GCd?  I kind of expect it just leaks based on this patch.

::: dom/network/src/UDPSocket.cpp
@@ +22,5 @@
> +namespace dom {
> +
> +namespace {
> +
> +class UDPMessageEventInternal : public UDPMessageEvent

If you do the constructor thing I mentioned you won't need this.

@@ +87,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(UDPSocket, DOMEventTargetHelper)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSocket)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSocketChild)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSuspendMessages)

None of these objects are cycle collected, so this doesn't actually do anything.

@@ +482,5 @@
> +    return NS_OK;
> +  }
> +
> +  AutoJSAPI jsapi;
> +  if (NS_WARN_IF(!jsapi.InitUsingWin(GetOwner()))) {

This was renamed to just Init the other day.

@@ +498,5 @@
> +
> +  memcpy(data, aData, aDataLength);
> +
> +  JS::Rooted<JS::Value> jsData(cx);
> +  jsData = JS::ObjectValue(*arrayBuf);

Why can't you use ArrayBuffer::Create (from TypedArray.h) here?

@@ +585,5 @@
> +                                    uint16_t aRemotePort,
> +                                    uint8_t* aData,
> +                                    uint32_t aDataLength)
> +{
> +  NS_ABORT_IF_FALSE(aType.EqualsLiteral("ondata"), "Wrong event type while invoking ReceivedData callback");

Just use MOZ_ASSERT.

@@ +595,5 @@
> +  return HandleReceivedData(aRemoteAddress, aRemotePort, aData, aDataLength);
> +}
> +
> +NS_IMETHODIMP
> +UDPSocket::UpdateReadyState(const nsACString & readyState)

& to the left

::: dom/network/src/UDPSocket.h
@@ +11,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/UDPSocketBinding.h"
> +#include "mozilla/dom/UnionTypes.h"

Forward declare whatever you need from UDPSocketBinding.h and UnionTypes.h, or move it into the .cpp.

@@ +15,5 @@
> +#include "mozilla/dom/UnionTypes.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsIUDPSocket.h"
> +#include "nsIUDPSocketChild.h"
> +#include "nsWrapperCache.h"

nsWrapperCache.h is included in DOMEventTargetHelper.h (because it inherits from that.

@@ +29,5 @@
> +                          , public nsIUDPSocketInternal
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(UDPSocket, DOMEventTargetHelper)

You can drop the SCRIPT_HOLDER part, you don't have any JS to trace.

@@ +39,5 @@
> +  UDPSocket(nsPIDOMWindow* aOwner,
> +            const Nullable<nsString>& aRemoteAddress,
> +            const Nullable<uint16_t>& aRemotePort);
> +
> +  ~UDPSocket();

dtors of refcounted objects should be private (or protected if they have subclasses).

@@ +41,5 @@
> +            const Nullable<uint16_t>& aRemotePort);
> +
> +  ~UDPSocket();
> +
> +  nsPIDOMWindow* GetParentObject() const

Preferred style in dom code is

return type
FunctionName(args)
{
...

@@ +114,5 @@
> +                           ErrorResult& aRv);
> +
> +  bool Send(const StringOrBlobOrArrayBufferOrArrayBufferView& aData,
> +            const Optional<nsAString >& aRemoteAddress,
> +            const Optional<Nullable<uint16_t > >& aRemotePort,

no spaces before > please

::: dom/webidl/UDPMessageEvent.webidl
@@ +8,5 @@
> + */
> +
> +[NoInterfaceObject,
> + Pref="dom.udpsocket.enabled"]
> +interface UDPMessageEvent : MessageEvent {

This should not inherit from MessageEvent.  Inherit from Event directly and add a data member to UDPMessageEvent.  Also drop the NoInterfaceObject and add a constructor with a dictionary like the other events.  And add the CheckPermission bit here too, no?
Attachment #8446481 - Flags: review?(khuey) → review-
Component: General → DOM: Device Interfaces
Product: Firefox OS → Core
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #114)
> Comment on attachment 8446481 [details] [diff] [review]
> Part 4 - UDPSocket webidl, v3
> 
> Review of attachment 8446481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does a UDPSocket that we never call Close() on get GCd?  I kind of expect it
> just leaks based on this patch.
> 
Dtor for unclosed UDPSocket is invoked while exiting Firefox, so yes there is a memory leak issue on this patch. Sadly bloatview shows no leakage on it. I tried make UDPSocket inherit nsSupportsWeakReference like WebSocket and XHR but the leakage still exists. I guess there are more things to do for making UDPSocket weak referenced by other no-CCed object.
> 
> ::: dom/webidl/UDPMessageEvent.webidl
> @@ +8,5 @@
> > + */
> > +
> > +[NoInterfaceObject,
> > + Pref="dom.udpsocket.enabled"]
> > +interface UDPMessageEvent : MessageEvent {
> 
> This should not inherit from MessageEvent.  Inherit from Event directly and
> add a data member to UDPMessageEvent.  Also drop the NoInterfaceObject and
> add a constructor with a dictionary like the other events.  And add the
> CheckPermission bit here too, no?
Agree, inheriting from MessageEvent is non-sense because UDPSocket doesn't really use the MessagePort. Is it ok to change the webidl copied from spec? BTW, CheckPermission is missed and will add it in next version.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #115)
> Dtor for unclosed UDPSocket is invoked while exiting Firefox, so yes there
> is a memory leak issue on this patch. Sadly bloatview shows no leakage on
> it. I tried make UDPSocket inherit nsSupportsWeakReference like WebSocket
> and XHR but the leakage still exists. I guess there are more things to do
> for making UDPSocket weak referenced by other no-CCed object.

Well UDPSocket holds Necko stuff, which also holds references back to the UDPSocket.  That cycle needs to be broken.  I would suggest using an inner-window-destroyed observer to close all the outstanding UDPSockets in a Window.  Make sure you use a weak observer reference so that the observer itself doesn't cause a leak :P

> Agree, inheriting from MessageEvent is non-sense because UDPSocket doesn't
> really use the MessagePort. Is it ok to change the webidl copied from spec?

Yes, absolutely!  Usually the specs are written without writing an implementation, and all sorts of problems are uncovered when trying to write code for them.
Attached patch Part 4 - UDPSocket webidl, v4 (obsolete) — Splinter Review
update according to review comments.

summary of changes:
1. UDPMessageEvent now inherited from Event directly and provides constructor.
2. UDPSocket will close itself while inner window destroyed.
3. Clean up nits.
Attachment #8446481 - Attachment is obsolete: true
Attachment #8446481 - Flags: superreview?(jonas)
Attachment #8446481 - Flags: review?(josh)
Attachment #8447859 - Flags: superreview?(jonas)
Attachment #8447859 - Flags: review?(khuey)
Attachment #8447859 - Flags: review?(josh)
Comment on attachment 8447859 [details] [diff] [review]
Part 4 - UDPSocket webidl, v4

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

::: dom/network/src/UDPSocket.h
@@ +21,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +enum class ReadyState : uint32_t;

Hmm...Windows and Emulator doesn't like enum class forward declaration. Cancel this review and will fix it in next revision.
Attachment #8447859 - Flags: superreview?(jonas)
Attachment #8447859 - Flags: review?(khuey)
Attachment #8447859 - Flags: review?(josh)
Attached patch Part 4 - UDPSocket webidl, v5 (obsolete) — Splinter Review
Update for fixing WindowsXP and B2G ICS build error. Typed enum is declared differently on these two platforms. I cannot use forward declaration in UDPSocket.h but include UDPSocketBinding.h.
Attachment #8447859 - Attachment is obsolete: true
Attachment #8448469 - Flags: superreview?(jonas)
Attachment #8448469 - Flags: review?(khuey)
Attachment #8448469 - Flags: review?(josh)
feature-b2g: --- → 2.1
Whiteboard: [stingray] → [FT:Stream3]
Comment on attachment 8448469 [details] [diff] [review]
Part 4 - UDPSocket webidl, v5

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

There are a lot of comments here, but I trust you to make them, and don't want to hold you up any longer. r=me

::: dom/network/src/UDPSocket.cpp
@@ +28,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(UDPSocket, DOMEventTargetHelper)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mSocket)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mSocketChild)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mSuspendMessages)

If you want to do this you should probably call Close() instead of unlinking mSocket and mChild, to make sure that you call the Close() methods on those pointers.

@@ +56,5 @@
> +  bool addressReuse =
> +      aOptions.mAddressReuse.WasPassed() ? aOptions.mAddressReuse.Value()
> +                                         : true;
> +  bool loopback = aOptions.mLoopback.WasPassed() ? aOptions.mLoopback.Value()
> +                                                 : false;

I think this would be easier if you just set default values on the dictionary for these properties.

@@ +104,5 @@
> +  SetIsDOMBinding();
> +  MOZ_ASSERT(aOwner);
> +  MOZ_ASSERT(aOwner->IsInnerWindow());
> +
> +  if (NS_IsMainThread()) {

This can't ever be created off the main thread.

@@ +142,5 @@
> +  mSuspendMessages.SetLength(0);
> +}
> +
> +void
> +UDPSocket::Suspend()

Is it really not an API error to suspend twice, or resume when you haven't suspended, etc?  I would kind of expect that to throw exceptions.

@@ +154,5 @@
> +  if (mSuspended) {
> +    for (uint32_t i = 0; i < mSuspendMessages.Length(); ++i) {
> +      mSuspendMessages[i]->PostDOMEvent();
> +    }
> +    mSuspendMessages.SetLength(0);

.Clear()

@@ +218,5 @@
> +  }
> +
> +  // If the type of the data is not compatible with any expected type, throw InvalidAccessError.
> +  if (!aData.IsString() && !aData.IsBlob() &&
> +      !aData.IsArrayBuffer() && !aData.IsArrayBufferView()) {

What else would you expect it to be?  WebIDL should be checking the types for you, so this does nothing.

@@ +224,5 @@
> +    return false;
> +  }
> +
> +  // If remote address and port were not specified in constructor and argument,
> +  // throw InvalidAccessError.

If the remote address and port were not specified in the constructor or as arguments throw InvalidAccessError.

@@ +271,5 @@
> +
> +    if (aData.IsString()) {
> +      const nsACString& data = NS_ConvertUTF16toUTF8(aData.GetAsString());
> +      count = data.Length();
> +      rv = strStream->SetData(data.BeginReading(), data.Length());

Review this harder.

@@ +291,5 @@
> +    }
> +
> +    stream = strStream;
> +  } else {
> +    // should never enter this block

Yeah, you're guaranteed that, so there's no need to have this here.

@@ +423,5 @@
> +    return NS_OK;
> +  }
> +
> +  AutoJSAPI jsapi;
> +  if (NS_WARN_IF(!jsapi.InitUsingWin(GetOwner()))) {

Again, this was renamed to just Init.  You should rebase ;)

@@ +429,5 @@
> +  }
> +
> +  JSContext* cx = jsapi.cx();
> +
> +  // Copy packet dat to ArrayBuffer

data

@@ +495,5 @@
> +UDPSocket::CallListenerError(const nsACString& type,
> +                             const nsACString& message,
> +                             const nsACString& filename,
> +                             uint32_t lineNumber,
> +                             uint32_t columnNumber)

nits: aType, aMessage, etc

@@ +511,5 @@
> +                                                         NS_LITERAL_STRING("error"),
> +                                                         init);
> +    return DispatchDOMEvent(nullptr, event, nullptr, nullptr);;
> +  }
> +  return NS_OK;

nit: \n after }

@@ +531,5 @@
> +  return HandleReceivedData(aRemoteAddress, aRemotePort, aData, aDataLength);
> +}
> +
> +NS_IMETHODIMP
> +UDPSocket::UpdateReadyState(const nsACString& readyState)

aReadyState

@@ +536,5 @@
> +{
> +  if (readyState.EqualsLiteral("closed")) {
> +    Close();
> +  }
> +  return NS_OK;

\n

@@ +548,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!strcmp(aTopic, "inner-window-destroyed")) {
> +    Close();
> +  }

This gets fired whenever every inner window gets destroyed.  You need to compare aSubject ...

::: dom/network/src/UDPSocket.h
@@ +10,5 @@
> +#include "mozilla/AsyncEventDispatcher.h"
> +#include "mozilla/Attributes.h"
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/UDPSocketBinding.h"

bah enums

@@ +11,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/UDPSocketBinding.h"
> +#include "nsCycleCollectionParticipant.h"

nsCycleCollectionParticipant.h is included in DOMEventTargetHelper.h, so you don't need it here.
Attachment #8448469 - Flags: review?(khuey) → review+
Attached patch Part 4 - UDPSocket webidl, v6 (obsolete) — Splinter Review
Update according to @khuey's review comment.
Attachment #8448469 - Attachment is obsolete: true
Attachment #8448469 - Flags: superreview?(jonas)
Attachment #8448469 - Flags: review?(josh)
Attachment #8454290 - Flags: superreview?(jonas)
Attachment #8454290 - Flags: review?(josh)
I've updated my patch except for the following two comments. @khuey, please let me know if you want me to change anything.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #120)
> Comment on attachment 8448469 [details] [diff] [review]
> Part 4 - UDPSocket webidl, v5
> @@ +271,5 @@
> > +
> > +    if (aData.IsString()) {
> > +      const nsACString& data = NS_ConvertUTF16toUTF8(aData.GetAsString());
> > +      count = data.Length();
> > +      rv = strStream->SetData(data.BeginReading(), data.Length());
> 
> Review this harder.
> 
> ::: dom/network/src/UDPSocket.h
> @@ +10,5 @@
> > +#include "mozilla/AsyncEventDispatcher.h"
> > +#include "mozilla/Attributes.h"
> > +#include "mozilla/DOMEventTargetHelper.h"
> > +#include "mozilla/ErrorResult.h"
> > +#include "mozilla/dom/UDPSocketBinding.h"
> 
> bah enums
>
> > Review this harder.

That was a note to myself that I forgot to remove.

> > bah enums

And this is me complaining :P

sicking is on vacation for the next couple weeks so we should find a different sr.  Let's try smaug?
Attachment #8454290 - Flags: superreview?(jonas) → superreview?(bugs)
So if the new draft http://www.w3.org/2012/sysapps/tcp-udp-sockets/ doesn't have
UDPMessageEvent, why do we want to implement such?

And the patch doesn't even implement it per
http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent but has different kind of interface.
(Since event codegen supports only interfaces inheriting Event, or inheriting other
 codegen'ed interface, one would probably have to implement UDPMessageEvent manually.)

So we should either (1) implement the new draft, or (2) the old one, or (3) get the old one fixed so
that UDPMessageEvent doesn't inherit MessageEvent in the spec.
Has the working group discussed about (3)? Or is the wg focusing on (1) only now?

If we want to take the webidl defined in attachment 8454290 [details] [diff] [review], UDPMessageEvent.webidl certainly should
explain why it doesn't follow the spec the file links to.
(I agree UDPMessageEvent inheriting MessageEvent is a bit silly.)

But I'd like to get some feedback from people following UDPSocket standardization.
/me sees several people from Mozilla in http://www.w3.org/2000/09/dbwg/details?group=58119&public=1
The new draft requires streams which we don't have, and the old draft sucks, so we're making this up as we go along.
And I wouldn't expect the working group to be interested in improving an obsolete draft.
And we need the API before we'll have Streams?
Comment on attachment 8454290 [details] [diff] [review]
Part 4 - UDPSocket webidl, v6

>+  // If the options.localAddress argument is absent then bind the
>+  // socket to the IPv4/6 address of the default local interface.
>+  nsString localAddress = NS_LITERAL_STRING("");
Strings are empty by default. (and never use NS_LITERAL_STRING("") - there is EmptyString() )

>+UDPSocket::UDPSocket(nsPIDOMWindow* aOwner,
>+                     const Nullable<nsString>& aRemoteAddress,
>+                     const Nullable<uint16_t>& aRemotePort)
>+  : DOMEventTargetHelper(aOwner)
>+  , mRemoteAddress(aRemoteAddress)
>+  , mRemotePort(aRemotePort)
>+  , mBufferedAmount(0)
>+  , mReadyState(mozilla::dom::ReadyState::Open)
>+  , mSuspended(false)
>+{
>+  SetIsDOMBinding();
>+  MOZ_ASSERT(aOwner);
>+  MOZ_ASSERT(aOwner->IsInnerWindow());
>+
>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+  if (obs) {
>+    obs->AddObserver(this, "inner-window-destroyed", true);
>+  }
Observing inner-window-destroyed feels odd.
You could just override DOMEventTargetHelper's DisconnectFromOwner, 
similar to EventSource for example.

>+  nsCOMPtr<nsIUDPSocket> sock =
>+      do_CreateInstance("@mozilla.org/network/udp-socket;1", &rv);
2 space indentation please

>+UDPSocket::CallListenerError(const nsACString& aType,
>+                             const nsACString& aMessage,
>+                             const nsACString& aFilename,
>+                             uint32_t aLineNumber,
>+                             uint32_t aColumnNumber)
>+{
>+  if (aType.EqualsLiteral("onerror")) {
>+    // close socket if data cannot be sent
>+    Close();
>+
>+    AutoJSContext cx;
>+    RootedDictionary<ErrorEventInit> init(cx);
>+    init.mMessage = NS_LITERAL_STRING("NetworkError");
>+    init.mCancelable = false;
>+    init.mBubbles = false;
>+
>+    nsRefPtr<ErrorEvent> event =
>+      ErrorEvent::Constructor(this, NS_LITERAL_STRING("error"), init);
>+
>+    return DispatchDOMEvent(nullptr, event, nullptr, nullptr);;
So you dispatch an untrusted event here.
You want to use DispatchTrustedEvent(event)

>+UDPSocket::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aData)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  if (!strcmp(aTopic, "inner-window-destroyed")) {
>+    nsCOMPtr<nsISupportsPRUint64> wrapper = do_QueryInterface(aSubject);
>+    NS_ENSURE_TRUE(wrapper, NS_ERROR_FAILURE);
>+
>+    uint64_t innerID;
>+    nsresult rv = wrapper->GetData(&innerID);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (innerID == GetOwner()->WindowID()) {
>+      Close();
>+    }
>+  }
>+
>+  return NS_OK;
>+}
So as far as I see using DisconnectFromOwner would let you remove this.
(but you may need to observe other stuff.)


How does this all handle bfcache/session history?
EventSource uses DOM_WINDOW_FROZEN_TOPIC
sr+ for the .webidl, but other small things to fix, so r-.
Attachment #8454290 - Flags: superreview?(bugs)
Attachment #8454290 - Flags: superreview+
Attachment #8454290 - Flags: review-
(In reply to Olli Pettay [:smaug] from comment #129)
> Observing inner-window-destroyed feels odd.
> You could just override DOMEventTargetHelper's DisconnectFromOwner, 
> similar to EventSource for example.

Yeah, I felt that way too.  I let it slide because that's what TCPSocket does, but you're right, there are better options for C++.

> How does this all handle bfcache/session history?
> EventSource uses DOM_WINDOW_FROZEN_TOPIC
> sr+ for the .webidl, but other small things to fix, so r-.

Well there's no bfcache on b2g, afaik.
I don't see anything which would disable bfcache there,
but sure, if the implementation ends up adding request to the document's loadgroup,
that ends up disabling bfcache for that document (if the request stays in the group when
session history would be used).
I don't think there's any loadgroups involved in Necko's UDP stuff.  If there were, I don't think we would have needed the manual handling of inner-window-destroyed.
b2g seems to limit sHistoryMaxTotalViewers to 1
Attached patch Part 4 - UDPSocket webidl, v7 (obsolete) — Splinter Review
Update according to review comments, carry sr+. UDP socket doesn't have anything to do with loadgroups/session so I think we don't need to manually handle it in UDPSocket.

Since we already have two DOM peers (@khuey and @smaug) reviewing this patch, I remove @jdm from the reviewer list to avoid too much redundant effort. @jdm, feel free to add yourself back if necessary.
Attachment #8454290 - Attachment is obsolete: true
Attachment #8454290 - Flags: review?(josh)
Attachment #8455141 - Flags: superreview+
Attachment #8455141 - Flags: review?(bugs)
Flags: needinfo?(josh)
Comment on attachment 8455141 [details] [diff] [review]
Part 4 - UDPSocket webidl, v7

So this doesn't handle bfcache yet.
As far as I see, we may put a document into bfcache even in b2g.

You may want to add something to nsDocument::CanSavePresentation, if you just
want to disable bfcache for the document/window using UDPSocket.
Attachment #8455141 - Flags: review?(bugs) → review-
FWIW smaug, I doubt that TCPSocket handles bfcaching either.  You might want to investigate and file a bug there.

SC, the easiest way to deal with bfcaching is just to grab the document from the window and call DisallowBFCaching in your constructor.  I think that's completely acceptable here.
(I don't see how DisallowBFCaching can work reliably in case of reusing inner windows.)
But feel free to use DisallowBFCaching. That method needs to be fixed to work always anyway.
I want to put some test cases for the bfcaching scenario, do we have any existing test case for reference?
There's dom/indexedDB/test/test_bfcache.html.  I guess you would want to test that the events did not continue to fire after bfcaching the subframe.
Attached patch Part 4 - UDPSocket webidl, v8 (obsolete) — Splinter Review
disable bfcaching for page that use UDPSocket and create a test case for it.

However, an assertion is found while running the test case (happened after loading about:blank in testBFCache()) and it only happens while turning on subframe cache. In addition, onmessage event will not be fired with or without disabling BFCaching because CheckInnerWindowCorrectness() will handle it. I'm a bit confused about in what scenario bfcaching will cause trouble.

===Assert stack===
0:37.31 [65197] ###!!! ASSERTION: User did not call nsIContentViewer::Destroy: '!mPresShell && !mPresContext', file /Users/hellfire/workspace/hg/mozilla-central/layout/base/nsDocumentViewer.cpp, line 539
 0:37.31 nsDocumentViewer::~nsDocumentViewer()+0x0000000E [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01FF4AFE]
 0:37.31 nsDocumentViewer::Release()+0x00000068 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01FF46D8]
 0:37.31 nsDocumentViewer::Show()+0x000004A7 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01FFBD27]
 0:37.32 nsPresContext::EnsureVisible()+0x00000101 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x020D6841]
 0:37.32 PresShell::UnsuppressAndInvalidate()+0x0000002E [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0201357E]
 0:37.32 nsDocumentViewer::LoadComplete(tag_nsresult)+0x0000095C [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01FF7B0C]
 0:37.32 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, tag_nsresult)+0x000002AD [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0246677D]
 0:37.32 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult)+0x00000CD5 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02465145]
 0:37.32 non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult)+0x00000010 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x024
65260]
 0:37.32 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, tag_nsresult)+0x00000304 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008BC254]
 0:37.33 nsDocLoader::doStopDocumentLoad(nsIRequest*, tag_nsresult)+0x000001C9 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008BBB39]
 0:37.33 nsDocLoader::DocLoaderIsEmpty(bool)+0x00000464 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008BA9C4]
 0:37.33 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult)+0x000002F1 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008BB401]
 0:37.33 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult)+0x0000000D [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008BB8ED]
 0:37.33 nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult)+0x0000055C [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00162EEC]
 0:37.33 nsDocument::DoUnblockOnload()+0x000000E9 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x018D96B9]
 0:37.33 nsDocument::UnblockOnload(bool)+0x0000019D [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x018D958D]
 0:37.34 nsDocument::DispatchContentLoadedEvents()+0x000006A3 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x018CA203]
 0:37.34 nsRunnableMethodImpl<void (nsDocument::*)(), void, true>::Run()+0x00000027 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x018ED4B7]
 0:37.34 nsThread::ProcessNextEvent(bool, bool*)+0x000004FB [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x000BB6AB]
 0:37.34 NS_ProcessPendingEvents(nsIThread*, unsigned int)+0x0000004D [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00020E0D]
 0:37.34 nsBaseAppShell::NativeEventCallback()+0x00000077 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01329CD7]
 0:37.34 nsAppShell::ProcessGeckoEvents(void*)+0x000000BE [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x012E257E]
 0:37.34 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__+0x00000011 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x0007F5B1]
 0:37.34 __CFRunLoopDoSources0+0x000000F2 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x00070C62]
 0:37.34 __CFRunLoopRun+0x0000033F [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x000703EF]
 0:37.34 CFRunLoopRunSpecific+0x00000135 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x0006FE75]
 0:37.34 RunCurrentEventLoopInMode+0x000000E2 [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x0002EA0D]
 0:37.34 ReceiveNextEventCommon+0x000001DF [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x0002E7B7]
 0:37.34 _BlockUntilNextEventMatchingListInModeWithFilter+0x00000041 [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x0002E5BC]
 0:37.34 _DPSNextEvent+0x0000059A [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x0002424E]
 0:37.34 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]+0x0000007A [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x0002389B]
 0:37.34 -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]+0x00000056 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x012E1A36]
 0:37.35 -[NSApplication run]+0x00000229 [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x0001799C]
 0:37.35 nsAppShell::Run()+0x00000212 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x012E2C22]
 0:37.35 nsAppStartup::Run()+0x00000082 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x026A6132]
 0:37.35 XREMain::XRE_mainRun()+0x0000160C [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02650ABC]
 0:37.35 XREMain::XRE_main(int, char**, nsXREAppData const*)+0x00000117 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02651187]
 0:37.35 XRE_main+0x000000DF [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0265152F]
 0:37.35 main+0x000007B3 [/Users/hellfire/workspace/hg/mozilla-central/obj-firefox.noindex/dist/NightlyDebug.app/Contents/MacOS/firefox +0x000022B3]
Attachment #8456099 - Flags: feedback?(khuey)
Attachment #8456099 - Flags: feedback?(bugs)
Subframe bfcache isn't really supported. Please test using toplevel docshells.
Attachment #8456099 - Flags: feedback?(bugs)
Blocks: Stream3_2.1
Attached patch Part 4 - UDPSocket webidl, v8.1 (obsolete) — Splinter Review
use mozbrowser iframe to test bfcache.

From my observation, the window object is still not release UDPSocket while navigating to other URL. Event handler is not invoked in this situation because we use |CheckInnerWindowCorrectness| dispatching events, so disabling bfcaching doesn't make difference here.
Attachment #8455141 - Attachment is obsolete: true
Attachment #8456099 - Attachment is obsolete: true
Attachment #8456099 - Flags: feedback?(khuey)
Attachment #8457807 - Flags: review?(bugs)
Attachment #8457807 - Flags: review?(bugs) → review+
Whiteboard: [FT:Stream3] → [FT:Stream3][dependency: marketplace-partners]
Target Milestone: --- → 2.1 S1 (1aug)
Flags: needinfo?(josh)
@ekr, may I know the progress of the review? Or maybe you can help reassign the reviewer if you don't have time to review this recently. Thanks. :)
Flags: needinfo?(ekr)
Comment on attachment 8446480 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v3

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

I'm only qualified to review the nr_socket pieces but from that perspective
I'm concerned about making socket opening and binding synchronous. That seems
like a step backward. Why is that a good idea? ICE has to open a lot of
sockets so this seems like a recipe for performance problems

Clearing my r? and adding byron

::: media/mtransport/nr_socket_prsock.cpp
@@ +963,5 @@
>  
>    socket_child_->SetFilterName(nsCString("stun"));
>  
> +  if (NS_FAILED(socket_child_->Bind(this, host, port,
> +                                    /* reuse = */ true,

Do we actually want reuse when port is 0? I know Byron found problems here.
Attachment #8446480 - Flags: review?(ekr) → review?(docfaraday)
Flags: needinfo?(ekr)
So, I'm not thrilled about bind blocking main, since setting up a single webrtc call involves binding at least 4 sockets, and pushing 40 in conference call scenarios. Coupled with the fact that main is already going to be quite busy, this is likely to be a pretty big problem for us. Additionally, we've footgunned ourselves over and over again with deadlocks and reentrancy problems when trying to block main for stuff; it is likely to be dangerous as well as non-performant. Is this sort of behavior the norm for e10s?
Also, ekr is correct in mentioning that using reuseaddr with UDP port 0 is a bad idea, because the linux kernel interprets that as "Give me any port, and I don't care if it is already in use." I am about to land a patch to UDPSocketParent that refuses to set that sockopt when the port is 0, so this will probably not be a big deal in the end.
The reason I make |bind| blocking is because UDPSocket WebAPI needs a blocking semantic. I could try providing both blocking and non-blocking version of |bind| and use the non-blocking version in nr_socket. For the reuseaddr issue, I can also change the parameter we used in nr_socket to false. @bwc, How do you think?
Flags: needinfo?(docfaraday)
Providing the non-blocking variant would serve our needs, so in that sense I would be satisfied. I'm still apprehensive about blocking main here, but if that's what the specification requires, so be it. A lot of my concern here would be alleviated if UDPSocketChild were usable from places other than main, since that would let us use a worker and avoid blocking main, right?
Flags: needinfo?(docfaraday)
You could set reuse_addr to (port != 0).

Regarding the blocking API, we need to change the API.  Blocking the main thread is unacceptable.  There are several easy options:

1. change from a constructor to a static factory method that returns a promise
2. make the localPort attribute nullable and repurpose the "connecting" state (since connecting is meaningless for UDP, see below)
3. as 2, but with a new state
4. something else, of which I am sure there are plenty of options that I haven't thought of just now

Looking at the webidl, I think that we have some other problems that need addressing.  

"connecting" and "halfclosed" states don't make any sense for UDP.  I see that http://www.w3.org/TR/raw-sockets/ has these shared, but the description doesn't really cover UDP at all.

The idea of buffering on a UDP socket is a little strange.  You don't buffer in UDP; if you can't send, you drop the packet.  I notice that mBufferedAmount only seems to increase in the patches here.  I know that this is for maintaining consistency with the WebSockets API (paragon of design as it is), but that's been a massive burden for us in WebRTC too.

:sicking, are you aware of the status of this API?   What options do we have here to change it?
Flags: needinfo?(jonas)
Perhaps someone can describe to me the point at which a call should be deemed asynchronous as opposed to synchronous? Calls like connect/accept/read/write etc I can easily understand. But AFAIK, bind() just updates a local port table entry or errors out. What can happen in a bind() call that would make it complete in more than an eyeblink? If the accepted specification says that this call is synchronous, haven't the considerations been considered?

(I'm a little sensitive to end-runs around specs at the moment, having been hosed by an implementation of the WebSocket constructor that decided to essentially do its stuff in a timeout. Errors are communicated by console.error(), making them impossible to handle. So please forgive.)
(In reply to Jason Proctor from comment #151)
> Perhaps someone can describe to me the point at which a call should be
> deemed asynchronous as opposed to synchronous? 

I have a similar comment based on watching this comment trail go by. I'm all about not blocking the main thread - but bind() doesn't strike me as a threat.

Are we talking about the bind system call (or winsock equivalent) here? That's not going to incur any i/o, so there is not a requirement to make it async. (sure its going to do the kernel dance and probably fight for a lock or two - but that hardly makes it unique).
What blocks here is the dispatch to the parent process.  The bind call itself is benign.
(In reply to Byron Campen [:bwc] from comment #149)
> but if that's what the specification requires, so be it.

Unless the spec has backwards compatibility constraints or other compelling reasons to do bad things this is absolutely the wrong attitude to take.  Most API specs are written without writing any code and invariably run into issues when implemented.
What Kyle said. If we see bad things in a spec that we're implementing we should definitely push back. Sync IO definitely counts as "bad things".
Flags: needinfo?(jonas)
Beg to report that IMHO, bind() isn't I/O. If anything that crosses the child/parent divide (which I don't really understand) has to be async, then is anything safe?

Btw, is FF unique in this regard? Can other environments do bind() etc synchronously without potentially blocking?

Async comes with a cost. Some environments imply synciness - I can't do network I/O in an HTTP request handler in a Node app, for example. I know that Node is *currently* single threaded, but it won't always be this way.
This UDPSocket api is going to be asynchronous according to the latest editor's draft. Although we cannot directly implement the latest version due to the lack of Stream API, we can still borrow the async open/close. The meaningless bufferedAmount/ondrain attribute is also remove from the latest draft. Here is the WebIDL I have in mind:

[Constructor (optional UDPOptions options)]
interface UDPSocket {
    readonly    attribute DOMString       localAddress;
    readonly    attribute unsigned short  localPort;
    readonly    attribute DOMString?      remoteAddress;
    readonly    attribute unsigned short? remotePort;
    readonly    attribute boolean         addressReuse;
    readonly    attribute boolean         loopback;
    readonly    attribute ReadyState      readyState;
    readonly    attribute Promise         opened;
    readonly    attribute Promise         closed;

                attribute EventHandler onmessage;

    Promise close ();
    void    joinMulticast (DOMString multicastGroupAddress);
    void    leaveMulticast (DOMString multicastGroupAddress);
    boolean send ((DOMString or Blob or ArrayBuffer or ArrayBufferView) data,
                  optional DOMString? remoteAddress,
                  optional unsigned short? remotePort);
};

- opened/closed Promise is introduced for asynchronous open/close.
- bufferAmount/ondrain/suspend/resume is removed because there is no concept of buffering in UDP socket.
- onerror is removed because the error callback is combined with closed Promise. The promise will be resolved with an error cause.
That API looks mostly OK.

The return values need to be Promise<void> for our WebIDL rather than Promise.

The values for localAddress and localPort won't be filled until the opened promise is fulfilled.  How do you plan to resolve that?

(BTW, do you have a link to the editor's draft?  The link from the latest w3c spec is broken.)
QA Whiteboard: [2.1-feature-qa+]
QA Whiteboard: [2.1-feature-qa+] → [2.1-feature-qa-]
QA Whiteboard: [2.1-feature-qa-] → [2.1-feature-qa?]
Hi Martin, the latest editor's draft is here: http://www.w3.org/2012/sysapps/tcp-udp-sockets/
About your question, I think the localAddress/localPort should be null until opened if user doesn't give the init value in constructor.
Move to 2.1 Sprint 2.
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
revert the sync bind support.
Attachment #8446480 - Attachment is obsolete: true
Attachment #8446480 - Flags: review?(docfaraday)
Attachment #8467719 - Flags: review?(josh)
Attachment #8467719 - Flags: review?(docfaraday)
Attached patch [WIP] UDPSocket webidl, v9 (obsolete) — Splinter Review
1. support async socket open
2. remove bufferedAmount/ondrain/onerror/suspend/resume
Attachment #8457807 - Attachment is obsolete: true
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #159)
> About your question, I think the localAddress/localPort should be null until
> opened if user doesn't give the init value in constructor.

The attributes need to be nullable then (DOMString?/unsigned short?).
Comment on attachment 8467719 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v4

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

Part 1 needs to be unrotted, and even after it is, this stuff doesn't build:

error: static_assert failed "Reference-counted class PendingSendStream should not have a public destructor. Try to make this class's destructor non-public. If that is really not possible, you can whitelist this class by providing a HasDangerousPublicDestructor specialization for it."

(I need to be able to build this in order to effectively review it)
Attachment #8467719 - Flags: review?(docfaraday)
(In reply to Martin Thomson [:mt] from comment #163)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #159)
> > About your question, I think the localAddress/localPort should be null until
> > opened if user doesn't give the init value in constructor.
> 
> The attributes need to be nullable then (DOMString?/unsigned short?).

Yes and it's already in my WIP for the WebIDL part.
(In reply to Byron Campen [:bwc] from comment #164)
> Comment on attachment 8467719 [details] [diff] [review]
> Part 3 - support socket options/multicast/input stream functionality in
> PUDPSocket.ipdl, v4
> 
> Review of attachment 8467719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Part 1 needs to be unrotted, and even after it is, this stuff doesn't build:
> 
> error: static_assert failed "Reference-counted class PendingSendStream
> should not have a public destructor. Try to make this class's destructor
> non-public. If that is really not possible, you can whitelist this class by
> providing a HasDangerousPublicDestructor specialization for it."
> 
> (I need to be able to build this in order to effectively review it)

I forgot to upload the rebased version of patch part 1 and 2. Sorry about that.
rebased to latest m-c, carry r+.
Attachment #8437551 - Attachment is obsolete: true
Attachment #8468161 - Flags: review+
Attached patch Part 2 - udp-socket permission (obsolete) — Splinter Review
rebase to latest m-c, carry r+.
Attachment #8437552 - Attachment is obsolete: true
Attachment #8468162 - Flags: review+
Attached patch Part 4 - UDPSocket webidl, v9 (obsolete) — Splinter Review
1. support async open via Promise
2. support closed Promise
3. move ReadyState to SocketCommon.webidl for sharing with TCPSocket API
4. support join/leave multicast before openedPromise been resolved (pending until underlying socket binding complete)

NOTE:
I found in my test case the UDPSocket opened in an embeded mozbrowser iframe will be released until application closed. DisconnectFromOwner() is not invoked while navigating to other page. I'm still debugging on it, not sure if it is a error in the implementation or in the test case.
Attachment #8467720 - Attachment is obsolete: true
Attachment #8469209 - Flags: superreview?(bugs)
Attachment #8469209 - Flags: review?(khuey)
Attachment #8469209 - Flags: review?(bugs)
DisconnectFromOwner() should be called at some point after navigating to a new page.
If the old page goes to bfcache, DisconnectFromOwner() is called only when bfache is evicted.
We should probably prevent any page which has an open UDPSocket from going into bfcache. The same way that we prevent any page which has an open network connection from going into bfcache.
Comment on attachment 8469209 [details] [diff] [review]
Part 4 - UDPSocket webidl, v9

>+++ b/dom/webidl/SocketCommon.webidl
>@@ -0,0 +1,16 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/.
>+ *
>+ * The origin of this IDL file is
>+ * http://www.w3.org/2012/sysapps/tcp-udp-sockets/#readystate
>+ */
>+
>+enum ReadyState {
>+    "opening",
>+    "open",
>+    "closing",
>+    "closed",
>+    "halfclosed"
>+};
It is not really fair for SysAppsWG to take commonly used name 'ReadyState'
for an enum in the global scope. I think it should be SocketReadyState.
Could you file a spec bug?
And change the patch to use SocketReadyState


>diff --git a/dom/webidl/UDPMessageEvent.webidl b/dom/webidl/UDPMessageEvent.webidl
>new file mode 100644
>--- /dev/null
>+++ b/dom/webidl/UDPMessageEvent.webidl
>@@ -0,0 +1,23 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/.
>+ *
>+ * The origin of this IDL file is
>+ * http://www.w3.org/TR/raw-sockets/#interface-udpmessageevent
>+ */
>+
>+[Constructor(DOMString type, optional UDPMessageEventInit eventInitDict),
>+ Pref="dom.udpsocket.enabled",
>+ CheckPermissions="udp-socket"]
>+interface UDPMessageEvent : Event {
>+    readonly    attribute DOMString      remoteAddress;
>+    readonly    attribute unsigned short remotePort;
>+    readonly    attribute any            data;
>+};
>+
>+dictionary UDPMessageEventInit : EventInit {
>+  DOMString remoteAddress = "";
>+  unsigned short remotePort = 0;
>+  any data = null;
>+};
>diff --git a/dom/webidl/UDPSocket.webidl b/dom/webidl/UDPSocket.webidl
>new file mode 100644
>--- /dev/null
>+++ b/dom/webidl/UDPSocket.webidl
>@@ -0,0 +1,40 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/.
>+ *
>+ * The origin of this IDL file is
>+ * http://www.w3.org/2012/sysapps/tcp-udp-sockets/#interface-udpsocket
>+ * http://www.w3.org/2012/sysapps/tcp-udp-sockets/#dictionary-udpoptions
>+ */
>+
>+dictionary UDPOptions {
>+    DOMString      localAddress;
>+    unsigned short localPort;
>+    DOMString      remoteAddress;
>+    unsigned short remotePort;
>+    boolean        addressReuse = true;
>+    boolean        loopback = false;
>+};
>+
>+[Constructor (optional UDPOptions options),
>+ Pref="dom.udpsocket.enabled",
>+ CheckPermissions="udp-socket"]
>+interface UDPSocket : EventTarget {
>+    readonly    attribute DOMString?      localAddress;
>+    readonly    attribute unsigned short? localPort;
>+    readonly    attribute DOMString?      remoteAddress;
>+    readonly    attribute unsigned short? remotePort;
>+    readonly    attribute boolean         addressReuse;
>+    readonly    attribute boolean         loopback;
>+    readonly    attribute ReadyState      readyState;
>+    readonly    attribute Promise<void>   opened;
>+    readonly    attribute Promise<void>   closed;
>+//    readonly    attribute ReadableStream  output; //Stream API is not ready
>+//    readonly    attribute WriteableStream input; //Stream API is not ready
The spec terminology is wrong here. it uses input for writing and output for reading.
Attachment #8469209 - Flags: superreview?(bugs) → superreview+
Two spec bugs are filed according to comment #172.
Rename ReadyState to SocketReadyState: https://github.com/sysapps/tcp-udp-sockets/issues/68
Interchange the type of input/output: https://github.com/sysapps/tcp-udp-sockets/issues/69
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #173)
> Two spec bugs are filed according to comment #172.
Thanks

> Interchange the type of input/output:
> https://github.com/sysapps/tcp-udp-sockets/issues/69
And Domenic is just wrong there, and I complained to him on IRC.
Comment on attachment 8469209 [details] [diff] [review]
Part 4 - UDPSocket webidl, v9

>+UDPSocket::Constructor(const GlobalObject& aGlobal,
>+                       const UDPOptions& aOptions,
>+                       ErrorResult& aRv)
>+{
>+  nsCOMPtr<nsPIDOMWindow> ownerWindow = do_QueryInterface(aGlobal.GetAsSupports());
>+  if (!ownerWindow) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  bool addressReuse = aOptions.mAddressReuse;
>+  bool loopback = aOptions.mLoopback;
>+
>+  Nullable<nsString> remoteAddress;
Nullable<nsString>?
Strings can deal with nullability themselves
See 
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#DOMString


>+  // If the options.localAddress argument is absent then bind the
>+  // socket to the IPv4/6 address of the default local interface.
Ditto



khuey, you reviewed this part before, so I was mostly looking at webidl and event handling parts.
I assume you'll review the rest.
Attachment #8469209 - Flags: review?(bugs) → review+
QA Whiteboard: [2.1-feature-qa?]
Whiteboard: [FT:Stream3][dependency: marketplace-partners] → [FT:Stream3][dependency: marketplace-partners][2.1-feature-qa+]
Flags: in-moztrap?(fyen)
QA Contact: fyen
Attached patch Part 4 - UDPSocket webidl, v9.1 (obsolete) — Splinter Review
update according to @smaug's review comments, carry @smaug's r+/sr+.

Changes included:
1. change ReadyState to SocketReadyState
2. reverse the type of UDPSocket.input and UDPSocket.output
3. use nsString instead of Nullable<nsString>
Attachment #8469209 - Attachment is obsolete: true
Attachment #8469209 - Flags: review?(khuey)
Attachment #8470600 - Flags: superreview+
Attachment #8470600 - Flags: review?(khuey)
Comment on attachment 8467719 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v4

Sorry I forgot to r? bwc.
Attachment #8467719 - Flags: review?(docfaraday)
Hi Kyle, Josh, and Byron,

We're trying to see if we can land it by the end of v2.1 sprint 2, which is this Friday(August 15).
Could you please help to review the patches when you're available?
Thank you very much!!
I'm going to be out tomorrow, but I'll review it on Thursday; the latest patch looked ok to me after a preliminary look, so hopefully shouldn't take long.
Comment on attachment 8467719 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v4

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

I have not been hit by a bus, but I sure feel like it. I was way too optimistic about being able to work Thursday. Odds are good that is it going to be next week before I can return to work. My apologies.
Attachment #8467719 - Flags: review?(docfaraday) → review?(ekr)
Comment on attachment 8470600 [details] [diff] [review]
Part 4 - UDPSocket webidl, v9.1

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

I have stolen this review from khuey's queue. I'm happy to review this patch again.

::: dom/network/src/UDPSocket.cpp
@@ +14,5 @@
> +#include "nsComponentManagerUtils.h"
> +#include "nsContentUtils.h"
> +#include "nsIDOMFile.h"
> +#include "nsINetAddr.h"
> +#include "nsIUDPSocketChild.h"

This is not needed.

@@ +71,5 @@
> +  nsString localAddress;
> +  if (aOptions.mLocalAddress.WasPassed()) {
> +    localAddress = aOptions.mLocalAddress.Value();
> +  } else {
> +    SetDOMStringToNull(localAddress);

why this is needed?

@@ +128,5 @@
> +}
> +
> +already_AddRefed<Promise>
> +UDPSocket::Close()
> +{

MOZ_ASSERT(mClosed);

@@ +147,5 @@
> +    return;
> +  }
> +
> +  if (mReadyState == SocketReadyState::Opening && mOpened) {
> +    mOpened->MaybeReject(aReason);

This is strange. In several place you call CloseWithReason(NS_OK) and this code will reject the mOpened promise with NS_OK.
What about if I do:

var udp = new UDPSocket(something);
var p = udp.opened().catch(function(e) {
  console.log(e);
});
udp.close();

This would print NS_OK as error code. Probably you want a different error code.

@@ +183,5 @@
> +    return;
> +  }
> +
> +  if (mReadyState == SocketReadyState::Opening) {
> +    MulticastCommand joinCommand = { true, nsString(aMulticastGroupAddress) };

If you add a constructor to Multicastcommand class, you can do:

MulticastCommand joinCommand(true, aMulticastGoupAddress);

@@ +188,5 @@
> +    mPendingMcastCommands.AppendElement(joinCommand);
> +    return;
> +  }
> +
> +  nsCString address = NS_ConvertUTF16toUTF8(aMulticastGroupAddress);

NS_ConvertUTF16toUTF8 address(aMulticastGroupAddress);

@@ +190,5 @@
> +  }
> +
> +  nsCString address = NS_ConvertUTF16toUTF8(aMulticastGroupAddress);
> +  if (mSocket) {
> +    nsresult rv = mSocket->JoinMulticast(address, EmptyCString());

What about:

aRv = mSocket->JoinMulticast(address, EmptyCString());
if (NS_WARN_IF(aRv.Failed())) {
  return;
}

@@ +191,5 @@
> +
> +  nsCString address = NS_ConvertUTF16toUTF8(aMulticastGroupAddress);
> +  if (mSocket) {
> +    nsresult rv = mSocket->JoinMulticast(address, EmptyCString());
> +    if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv)))

@@ +195,5 @@
> +    if (NS_FAILED(rv)) {
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return;
> +    }
> +  } else if (mSocketChild) {

if you have mSocket you don't have mSocketChild, correct?

so you can do:

MOZ_ASSERT(mSocket || mSocketChild);

if (mSocket) {
  MOZ_ASSERT(!mSocketChild);
  ...
  return;
}

MOZ_ASSERT(mSocketChild);
aRv = mSocketChild = JoinMulticast(address, EmptyCString());
...

@@ +196,5 @@
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return;
> +    }
> +  } else if (mSocketChild) {
> +    mSocketChild->JoinMulticast(address, EmptyCString());

does it return an error code?

@@ +212,5 @@
> +    return;
> +  }
> +
> +  if (mReadyState == SocketReadyState::Opening) {
> +    MulticastCommand leaveCommand = { false, nsString(aMulticastGroupAddress) };

MulticastCommand leaveCommand(false, aMulticastGroupAddress);

@@ +217,5 @@
> +    mPendingMcastCommands.AppendElement(leaveCommand);
> +    return;
> +  }
> +
> +  nsCString address = NS_ConvertUTF16toUTF8(aMulticastGroupAddress);

NS_ConvertUTF16toUTF8 address(aMulticastGroupAddress);

@@ +219,5 @@
> +  }
> +
> +  nsCString address = NS_ConvertUTF16toUTF8(aMulticastGroupAddress);
> +  if (mSocket) {
> +    nsresult rv = mSocket->LeaveMulticast(address, EmptyCString());

aRv = ...
if (NS_WARN_IF(aRv.Failed())) ...

@@ +225,5 @@
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return;
> +    }
> +  } else if (mSocketChild) {
> +    mSocketChild->LeaveMulticast(address, EmptyCString());

error code.

@@ +228,5 @@
> +  } else if (mSocketChild) {
> +    mSocketChild->LeaveMulticast(address, EmptyCString());
> +  } else {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +  }

Same comment about JoinMulticastGroup related to MOZ_ASSERT(mSocket || mSocketChild).

@@ +244,5 @@
> +    if (command.mJoin) {
> +      JoinMulticastGroup(command.mAddress, rv);
> +    } else {
> +      LeaveMulticastGroup(command.mAddress, rv);
> +    }

I think you should check the error code and at least print it with a NS_WARNING in case it fails.

@@ +264,5 @@
> +
> +  // If the type of the data is not compatible with any expected type, throw InvalidAccessError.
> +  if (!aData.IsString() && !aData.IsBlob() &&
> +      !aData.IsArrayBuffer() && !aData.IsArrayBufferView()) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);

can this really happen?

@@ +295,5 @@
> +  nsCOMPtr<nsIInputStream> stream;
> +  if (aData.IsBlob()) {
> +    nsCOMPtr<nsIDOMBlob> blob = aData.GetAsBlob();
> +
> +    rv = blob->GetInternalStream(getter_AddRefs(stream));

aRv = blob->GetInternalStream(getter_AddRefs(stream));
if (NS_WARN_IF(aRv.Failed())) {
  return false;
}

@@ +309,5 @@
> +    }
> +
> +    if (aData.IsString()) {
> +      const nsACString& data = NS_ConvertUTF16toUTF8(aData.GetAsString());
> +      rv = strStream->SetData(data.BeginReading(), data.Length());

aRv =

@@ +313,5 @@
> +      rv = strStream->SetData(data.BeginReading(), data.Length());
> +    } else if (aData.IsArrayBuffer()) {
> +      const ArrayBuffer& data = aData.GetAsArrayBuffer();
> +      data.ComputeLengthAndData();
> +      rv = strStream->SetData(reinterpret_cast<const char*>(data.Data()), data.Length());

aRv =

@@ +317,5 @@
> +      rv = strStream->SetData(reinterpret_cast<const char*>(data.Data()), data.Length());
> +    } else {
> +      const ArrayBufferView& data = aData.GetAsArrayBufferView();
> +      data.ComputeLengthAndData();
> +      rv = strStream->SetData(reinterpret_cast<const char*>(data.Data()), data.Length());

aRv =

@@ +320,5 @@
> +      data.ComputeLengthAndData();
> +      rv = strStream->SetData(reinterpret_cast<const char*>(data.Data()), data.Length());
> +    }
> +
> +    if (NS_FAILED(rv)) {

if (NS_WARN_IF(aRv.Failed())..

@@ +327,5 @@
> +    }
> +
> +    stream = strStream;
> +  }
> +

MOZ_ASSERT(stream);

@@ +329,5 @@
> +    stream = strStream;
> +  }
> +
> +  if (mSocket) {
> +    rv = mSocket->SendBinaryStream(remoteAddress, remotePort, stream);

aRv =

@@ +331,5 @@
> +
> +  if (mSocket) {
> +    rv = mSocket->SendBinaryStream(remoteAddress, remotePort, stream);
> +  } else if (mSocketChild) {
> +    rv = mSocketChild->SendBinaryStream(remoteAddress, remotePort, stream);

aRv =

@@ +333,5 @@
> +    rv = mSocket->SendBinaryStream(remoteAddress, remotePort, stream);
> +  } else if (mSocketChild) {
> +    rv = mSocketChild->SendBinaryStream(remoteAddress, remotePort, stream);
> +  } else {
> +    aRv.Throw(NS_ERROR_FAILURE);

can this really happen?

@@ +369,5 @@
> +    mozilla::net::NetAddr addr;
> +    PRNetAddrToNetAddr(&prAddr, &addr);
> +    rv = sock->InitWithAddress(&addr, mAddressReuse, /* optionalArgc = */ 1);
> +  }
> +  NS_ENSURE_SUCCESS(rv, rv);

if (NS_WARN_IF(NS_FAILED(rv))) ... here and everywhere.

@@ +395,5 @@
> +  rv = mSocket->AsyncListen(this);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mReadyState = SocketReadyState::Open;
> +  DoPendingMcastCommand();

Maybe this should return an error code and reject in case the pending mcast commands fail.

@@ +398,5 @@
> +  mReadyState = SocketReadyState::Open;
> +  DoPendingMcastCommand();
> +  mOpened->MaybeResolve(JS::UndefinedHandleValue);
> +
> +  return rv;

NS_OK

@@ +416,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mSocketChild = sock;
> +
> +  return rv;

NS_OK

@@ +437,5 @@
> +  ErrorResult rv;
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
> +
> +  mOpened = Promise::Create(global, rv);
> +  NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());

We are trying to remove NS_ENSURE_SUCCESS macro. Can you do:

if (NS_WARN_IF(rv.ErrorCode())) {
  return rv.ErrorCode();
}

here and everywhere is needed?

@@ +442,5 @@
> +
> +  mClosed = Promise::Create(global, rv);
> +  NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());
> +
> +  class OpenSocketRunnable MOZ_FINAL : public nsRunnable

Can you move this class in an anonymous namespace?

@@ +451,5 @@
> +    }
> +
> +    NS_IMETHOD Run() MOZ_OVERRIDE
> +    {
> +      if (!mSocket) {

This cannot happen. correct?
MOZ_ASSERT(mSocket);

@@ +467,5 @@
> +      } else {
> +        rv = mSocket->InitLocal(mSocket->mLocalAddress, localPort);
> +      }
> +
> +      if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv)))...

@@ +479,5 @@
> +    nsRefPtr<UDPSocket> mSocket;
> +  };
> +
> +  nsRefPtr<OpenSocketRunnable> runnable = new OpenSocketRunnable(this);
> +  NS_DispatchToMainThread(runnable);

return NS_DispatchToMainThread(runnable);

@@ +499,5 @@
> +    return;
> +  }
> +
> +  if (NS_FAILED(DispatchReceivedData(aRemoteAddress, aRemotePort, aData, aDataLength))) {
> +    CloseWithReason(NS_ERROR_TYPE_ERR);

why this error code? I think you should be add a check.
Plus, reject the promise with the correct error code.

@@ +517,5 @@
> +
> +  JSContext* cx = jsapi.cx();
> +
> +  // Copy packet data to ArrayBuffer
> +  JS::RootedObject arrayBuf(cx, ArrayBuffer::Create(cx, aDataLength, aData));

JS::Rooted<JSObject*>

@@ +518,5 @@
> +  JSContext* cx = jsapi.cx();
> +
> +  // Copy packet data to ArrayBuffer
> +  JS::RootedObject arrayBuf(cx, ArrayBuffer::Create(cx, aDataLength, aData));
> +  NS_ENSURE_TRUE(arrayBuf, NS_ERROR_FAILURE);

if (!arrayBuf) { ...

@@ +520,5 @@
> +  // Copy packet data to ArrayBuffer
> +  JS::RootedObject arrayBuf(cx, ArrayBuffer::Create(cx, aDataLength, aData));
> +  NS_ENSURE_TRUE(arrayBuf, NS_ERROR_FAILURE);
> +
> +  JS::RootedValue jsData(cx, JS::ObjectValue(*arrayBuf));

JS::Rooted<JS::Value>

@@ +544,5 @@
> +
> +NS_IMETHODIMP
> +UDPSocket::OnPacketReceived(nsIUDPSocket* aSocket, nsIUDPMessage* aMessage)
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Not running on main thread");

Why this check only in this method?

@@ +609,5 @@
> +UDPSocket::CallListenerVoid(const nsACString &type) {
> +
> +  if (type.EqualsLiteral("onopen")) {
> +    // Get real local address and local port
> +    nsCString localAddress;

MOZ_ASSERT(mSocketChild)

@@ +618,5 @@
> +    mSocketChild->GetLocalPort(&localPort);
> +    mLocalPort.SetValue(localPort);
> +
> +    mReadyState = SocketReadyState::Open;
> +    DoPendingMcastCommand();

error code and maybe reject mOpened in case something fails

::: dom/network/src/UDPSocket.h
@@ +173,5 @@
> +  nsCOMPtr<nsIUDPSocketChild> mSocketChild;
> +
> +  struct MulticastCommand {
> +    bool mJoin;
> +    nsString mAddress;

Add a constructor:

 MulticastCommand(bool aJoin, const nsAString& aAddress)
   : mJoin(mJoin), mAddress(aAddress)
 { }

::: dom/webidl/SocketCommon.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * http://www.w3.org/2012/sysapps/tcp-udp-sockets/#readystate
> + */
> +
> +enum SocketReadyState {

It's called ReadyState in the spec and maybe we can move it into UDBPsocket.webidl

::: dom/webidl/UDPMessageEvent.webidl
@@ +12,5 @@
> + CheckPermissions="udp-socket"]
> +interface UDPMessageEvent : Event {
> +    readonly    attribute DOMString      remoteAddress;
> +    readonly    attribute unsigned short remotePort;
> +    readonly    attribute any            data;

I don't see this in the spec. Maybe it's in the draft, but the spec has a 404 link for the draft version.

::: dom/webidl/UDPSocket.webidl
@@ +20,5 @@
> +[Constructor (optional UDPOptions options),
> + Pref="dom.udpsocket.enabled",
> + CheckPermissions="udp-socket"]
> +interface UDPSocket : EventTarget {
> +    readonly    attribute DOMString?       localAddress;

no '?' in the spec.

@@ +21,5 @@
> + Pref="dom.udpsocket.enabled",
> + CheckPermissions="udp-socket"]
> +interface UDPSocket : EventTarget {
> +    readonly    attribute DOMString?       localAddress;
> +    readonly    attribute unsigned short?  localPort;

no '?' in the spec

@@ +26,5 @@
> +    readonly    attribute DOMString?       remoteAddress;
> +    readonly    attribute unsigned short?  remotePort;
> +    readonly    attribute boolean          addressReuse;
> +    readonly    attribute boolean          loopback;
> +    readonly    attribute SocketReadyState readyState;

ReadyState

@@ +29,5 @@
> +    readonly    attribute boolean          loopback;
> +    readonly    attribute SocketReadyState readyState;
> +    readonly    attribute Promise<void>    opened;
> +    readonly    attribute Promise<void>    closed;
> +//    readonly    attribute ReadableStream   output; //Stream API is not ready

Do we have a bug ID? Can you write it here?
Attachment #8470600 - Flags: review?(khuey) → review-
(In reply to Andrea Marchesini (:baku) from comment #181)
> Comment on attachment 8470600 [details] [diff] [review]
> ::: dom/webidl/SocketCommon.webidl
> @@ +6,5 @@
> > + * The origin of this IDL file is
> > + * http://www.w3.org/2012/sysapps/tcp-udp-sockets/#readystate
> > + */
> > +
> > +enum SocketReadyState {
> 
> It's called ReadyState in the spec and maybe we can move it into
> UDBPsocket.webidl
We've filed a spec bug for renaming this enum, see comment #173. SocketReadyState is used by both TCPSocket and UDPSocket in spec, move it into UDPSocket.webidl will make this enum local to UDPSocket IMO.
> 
> ::: dom/webidl/UDPMessageEvent.webidl
> @@ +12,5 @@
> > + CheckPermissions="udp-socket"]
> > +interface UDPMessageEvent : Event {
> > +    readonly    attribute DOMString      remoteAddress;
> > +    readonly    attribute unsigned short remotePort;
> > +    readonly    attribute any            data;
> 
> I don't see this in the spec. Maybe it's in the draft, but the spec has a
> 404 link for the draft version.
We consider the definition in spec has bug, see comment #114.
> 
> ::: dom/webidl/UDPSocket.webidl
> @@ +20,5 @@
> > +[Constructor (optional UDPOptions options),
> > + Pref="dom.udpsocket.enabled",
> > + CheckPermissions="udp-socket"]
> > +interface UDPSocket : EventTarget {
> > +    readonly    attribute DOMString?       localAddress;
> 
> no '?' in the spec.
> > +    readonly    attribute unsigned short?  localPort;
> 
> no '?' in the spec
> 
Socket open is asynchronous so these attribute will be null at the beginning, see comment #158. I've filed a spec bug for it: https://github.com/sysapps/tcp-udp-sockets/issues/70
(In reply to Andrea Marchesini (:baku) from comment #181)
> Comment on attachment 8470600 [details] [diff] [review]
> @@ +71,5 @@
> > +  nsString localAddress;
> > +  if (aOptions.mLocalAddress.WasPassed()) {
> > +    localAddress = aOptions.mLocalAddress.Value();
> > +  } else {
> > +    SetDOMStringToNull(localAddress);
> 
> why this is needed?
The initial value of nsString is "" instead of null and we want this attribute to be null.
> @@ +147,5 @@
> > +    return;
> > +  }
> > +
> > +  if (mReadyState == SocketReadyState::Opening && mOpened) {
> > +    mOpened->MaybeReject(aReason);
> 
> This is strange. In several place you call CloseWithReason(NS_OK) and this
> code will reject the mOpened promise with NS_OK.
> What about if I do:
> 
> var udp = new UDPSocket(something);
> var p = udp.opened().catch(function(e) {
>   console.log(e);
> });
> udp.close();
> 
> This would print NS_OK as error code. Probably you want a different error
> code.
You're right. For the case of CloseWithReason(NS_OK) I should not resolve the mOpened because it's considered as normal close().
> 
> @@ +196,5 @@
> > +      aRv.Throw(NS_ERROR_FAILURE);
> > +      return;
> > +    }
> > +  } else if (mSocketChild) {
> > +    mSocketChild->JoinMulticast(address, EmptyCString());
> 
> does it return an error code?
Acquiring error code from IPC method will make the IPDL synchronous, it seems overkill in this case.
> 
> @@ +264,5 @@
> > +
> > +  // If the type of the data is not compatible with any expected type, throw InvalidAccessError.
> > +  if (!aData.IsString() && !aData.IsBlob() &&
> > +      !aData.IsArrayBuffer() && !aData.IsArrayBufferView()) {
> > +    aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> 
> can this really happen?
> 
If WebIDL already have this guarantee on type checking, I'm happy to remove such error check.
> 
> @@ +333,5 @@
> > +    rv = mSocket->SendBinaryStream(remoteAddress, remotePort, stream);
> > +  } else if (mSocketChild) {
> > +    rv = mSocketChild->SendBinaryStream(remoteAddress, remotePort, stream);
> > +  } else {
> > +    aRv.Throw(NS_ERROR_FAILURE);
> 
> can this really happen?
> 
Not really, I'll also use MOZ_ASSERT in this case.
> @@ +442,5 @@
> > +
> > +  mClosed = Promise::Create(global, rv);
> > +  NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());
> > +
> > +  class OpenSocketRunnable MOZ_FINAL : public nsRunnable
> 
> Can you move this class in an anonymous namespace?
> 
Inline class can access class private members and methods. Move this class to anonymous namespace means we need either add a friend class in UDPSocket or move the related function to public. Do we have a preference for this case?
I chose inline class because it's not going to be used out side of Init().
> @@ +499,5 @@
> > +    return;
> > +  }
> > +
> > +  if (NS_FAILED(DispatchReceivedData(aRemoteAddress, aRemotePort, aData, aDataLength))) {
> > +    CloseWithReason(NS_ERROR_TYPE_ERR);
> 
> why this error code? I think you should be add a check.
> Plus, reject the promise with the correct error code.
In the latest draft it has following statement:

When a new UDP datagram has been received and upon detction that it is not possible to convert the received UDP data to ArrayBuffer, [TYPED-ARRAYS], the following steps MUST run:
    Call error() with TypeError.
    Reject closedPromise with TypeError. 

I interpreted it as "reject as TypeError if anything wrong while creating the ArrayBuffer".
> @@ +544,5 @@
> > +
> > +NS_IMETHODIMP
> > +UDPSocket::OnPacketReceived(nsIUDPSocket* aSocket, nsIUDPMessage* aMessage)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread(), "Not running on main thread");
> 
> Why this check only in this method?
> 
OnPacketReceived and OnStopListening are the two functions not having strong guarantee of the running thread, others are either triggered by WebIDL or IPC. Since OnStopListening does nothing, I only put the assertion here in case anyone breaks the nsUDPSocket. Want me to remove this assertion?
> Acquiring error code from IPC method will make the IPDL synchronous, it
> seems overkill in this case.

totally agree.

> > Can you move this class in an anonymous namespace?
> > 
> Inline class can access class private members and methods. Move this class
> to anonymous namespace means we need either add a friend class in UDPSocket
> or move the related function to public. Do we have a preference for this
> case?
> I chose inline class because it's not going to be used out side of Init().

ok.

> I interpreted it as "reject as TypeError if anything wrong while creating
> the ArrayBuffer".

ok

> OnPacketReceived and OnStopListening are the two functions not having strong
> guarantee of the running thread, others are either triggered by WebIDL or
> IPC. Since OnStopListening does nothing, I only put the assertion here in
> case anyone breaks the nsUDPSocket. Want me to remove this assertion?

Keep the assertion and add a comment.
Thanks for your answers.
Comment on attachment 8467719 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v4

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

I'm only giving this a once-over sanity check for :bwc since he is indisposed today.

::: dom/network/src/UDPSocketChild.cpp
@@ +153,5 @@
> +  SerializeInputStream(aStream, stream, fds);
> +
> +  MOZ_ASSERT(fds.IsEmpty());
> +
> +  SendData(UDPData(stream), UDPSocketAddr(UDPAddressInfo(nsCString(aHost), aPort)));

I haven't looked.  How do we deal with over-sized packets?  I can't see anywhere that might be handled here.

::: dom/network/src/UDPSocketParent.cpp
@@ +70,5 @@
> +  // We don't have browser actors in xpcshell, and hence can't run automated
> +  // tests without this loophole.
> +  if (net::UsingNeckoIPCSecurity() && !mFilter &&
> +      !AssertAppProcessPermission(Manager()->Manager(), "udp-socket")) {
> +    FireInternalError(this, NS_LITERAL_CSTRING("opening"), __LINE__);

Please use a constant for aReadyState throughout.  Typos will cause problems, and I doubt that your test coverage can be thorough enough to catch all the error branches.

@@ +143,5 @@
> +}
> +
> +bool
> +UDPSocketParent::RecvData(const UDPData& aData,
> +                          const UDPSocketAddr& aAddr)

This needs commentary, or a better name.  This receives data from the child for sending.  That's confusing.

@@ +170,5 @@
> +    // Sending unallowed data, kill content.
> +    NS_ENSURE_SUCCESS(rv, false);
> +
> +    if (!allowed) {
> +      rv = NS_ERROR_FAILURE;

Unused assignment.

@@ +184,5 @@
> +      Send(aData.get_InputStreamParams(), aAddr);
> +      break;
> +    default:
> +      MOZ_ASSERT(false, "Invalid data type!");
> +      return true;

true?

@@ +254,5 @@
> +UDPSocketParent::RecvJoinMulticast(const nsCString& aMulticastAddress,
> +                                   const nsCString& aInterface)
> +{
> +  nsresult rv = mSocket->JoinMulticast(aMulticastAddress, aInterface);
> +  NS_ENSURE_SUCCESS(rv, true);

returning true on failure?

@@ +266,3 @@
>  {
> +  nsresult rv = mSocket->LeaveMulticast(aMulticastAddress, aInterface);
> +  NS_ENSURE_SUCCESS(rv, true);

returning true on failure?

::: netwerk/ipc/NeckoParent.cpp
@@ +443,5 @@
>    return true;
>  }
>  
>  PUDPSocketParent*
> +NeckoParent::AllocPUDPSocketParent(const nsCString& aFilter)

unused argument
Attachment #8467719 - Flags: review+
(In reply to Martin Thomson [:mt] from comment #185)
> Comment on attachment 8467719 [details] [diff] [review]
> Part 3 - support socket options/multicast/input stream functionality in
> PUDPSocket.ipdl, v4
> 
> Review of attachment 8467719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm only giving this a once-over sanity check for :bwc since he is
> indisposed today.
> 
> ::: dom/network/src/UDPSocketChild.cpp
> @@ +153,5 @@
> > +  SerializeInputStream(aStream, stream, fds);
> > +
> > +  MOZ_ASSERT(fds.IsEmpty());
> > +
> > +  SendData(UDPData(stream), UDPSocketAddr(UDPAddressInfo(nsCString(aHost), aPort)));
> 
> I haven't looked.  How do we deal with over-sized packets?  I can't see
> anywhere that might be handled here.
over-sized packets is handled by PR_SendTo in nsUDPSocket::SendWithAddress. I didn't handle it in IPC code because only the platform knows the limitation.
> 
> ::: dom/network/src/UDPSocketParent.cpp
> @@ +70,5 @@
> > +  // We don't have browser actors in xpcshell, and hence can't run automated
> > +  // tests without this loophole.
> > +  if (net::UsingNeckoIPCSecurity() && !mFilter &&
> > +      !AssertAppProcessPermission(Manager()->Manager(), "udp-socket")) {
> > +    FireInternalError(this, NS_LITERAL_CSTRING("opening"), __LINE__);
> 
> Please use a constant for aReadyState throughout.  Typos will cause
> problems, and I doubt that your test coverage can be thorough enough to
> catch all the error branches.
Good suggestion. I'll replace it with constant.
> 
> @@ +143,5 @@
> > +}
> > +
> > +bool
> > +UDPSocketParent::RecvData(const UDPData& aData,
> > +                          const UDPSocketAddr& aAddr)
> 
> This needs commentary, or a better name.  This receives data from the child
> for sending.  That's confusing.
The "Recv" prefix is determined by IPDL codegen. Maybe I can change the function name from "Data" to "OutgoingData" in IPDL. That will generate "RecvOutgoingData". Will this make more sense while reading the code?
> 
> @@ +170,5 @@
> > +    // Sending unallowed data, kill content.
> > +    NS_ENSURE_SUCCESS(rv, false);
> > +
> > +    if (!allowed) {
> > +      rv = NS_ERROR_FAILURE;
> 
> Unused assignment.
Removed.
> 
> @@ +184,5 @@
> > +      Send(aData.get_InputStreamParams(), aAddr);
> > +      break;
> > +    default:
> > +      MOZ_ASSERT(false, "Invalid data type!");
> > +      return true;
> 
> true?
Returning false in IPC method will destroy the IPC actor and that's not the behavior we want here. Same reason to the following questions.
> 
> @@ +254,5 @@
> > +UDPSocketParent::RecvJoinMulticast(const nsCString& aMulticastAddress,
> > +                                   const nsCString& aInterface)
> > +{
> > +  nsresult rv = mSocket->JoinMulticast(aMulticastAddress, aInterface);
> > +  NS_ENSURE_SUCCESS(rv, true);
> 
> returning true on failure?
> 
> @@ +266,3 @@
> >  {
> > +  nsresult rv = mSocket->LeaveMulticast(aMulticastAddress, aInterface);
> > +  NS_ENSURE_SUCCESS(rv, true);
> 
> returning true on failure?
> 
> ::: netwerk/ipc/NeckoParent.cpp
> @@ +443,5 @@
> >    return true;
> >  }
> >  
> >  PUDPSocketParent*
> > +NeckoParent::AllocPUDPSocketParent(const nsCString& aFilter)
> 
> unused argument
This is also an IPDL codegen function which I cannot modify the function signature (in pair with RecvPUDPSocketConstructor), but I can add a comment like /* unused */ to make it more clear.
update according to comment #185.

summary of changes:
1. divide |Callback| in PUDPSocket.ipdl into 4 separate functions: |CallbackOpened|, |CallbackClosed|, |CallbackReceivedData|, |CallbackError|. In this case we can get rid of all the readyState/type strings. nsIUDPSocketInternal also provide one-to-one mapping of the callback functions.

2. clean up the NS_ENSURE_SUCCESS in UDPSocketChild.cpp and UDPSocketParent.cpp.

3. remove columnNumber in error call back because this value is always 0 in our implementation.
Attachment #8467719 - Attachment is obsolete: true
Attachment #8467719 - Flags: review?(josh)
Attachment #8467719 - Flags: review?(ekr)
Attachment #8473594 - Flags: review?(martin.thomson)
Attachment #8473594 - Flags: review?(josh)
minor update for fixing unused-return-value warning in UDPSocketChild.cpp and UDPSocketParent.cpp.
Attachment #8473594 - Attachment is obsolete: true
Attachment #8473594 - Flags: review?(martin.thomson)
Attachment #8473594 - Flags: review?(josh)
Attachment #8473653 - Flags: review?(martin.thomson)
Attachment #8473653 - Flags: review?(josh)
Attached patch Part 4 - UDPSocket webidl, v10 (obsolete) — Splinter Review
update according to review comment.
Attachment #8470600 - Attachment is obsolete: true
Attachment #8473655 - Flags: superreview+
Attachment #8473655 - Flags: review?(amarchesini)
Attachment #8473653 - Flags: review?(martin.thomson) → review+
Comment on attachment 8473655 [details] [diff] [review]
Part 4 - UDPSocket webidl, v10

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

::: dom/network/src/UDPSocket.cpp
@@ +471,5 @@
> +  if (NS_WARN_IF(rv.Failed())) {
> +    return rv.ErrorCode();
> +  }
> +
> +  class OpenSocketRunnable MOZ_FINAL : public nsRunnable

For the try result, looks like B2G ICS doesn't allow inline class. Cancel this review until I fix it.
Attachment #8473655 - Flags: review?(amarchesini)
Move to v2.1 sprint 3.
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Attached patch Part 4 - UDPSocket webidl, v10.1 (obsolete) — Splinter Review
fix ICS build error and test failure on TBPL.

summary of change:
1. use nsCOMPtr<nsIRunnable> instead of nsRefPtr<OpenSocketRunnable>. Compiler on ICS cannot use inner class as type parameter.
2. CallListenerOpened need to check mReadyState before doing anything because socket might be closed before receiving IPC callback.
Attachment #8473655 - Attachment is obsolete: true
Attachment #8474453 - Flags: superreview+
Attachment #8474453 - Flags: review?(amarchesini)
Comment on attachment 8473653 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v5.1

@jduell, could you help review the UDPSocket IPC code because @jdm is not around until 8/24?
Attachment #8473653 - Flags: review?(josh) → review?(jduell.mcbugs)
Sandip, how can we nominate this as a b2g 2.2 feature?
Flags: needinfo?(skamat)
Comment on attachment 8474453 [details] [diff] [review]
Part 4 - UDPSocket webidl, v10.1

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

It's good. r- because of the constructor seems not in sync with what the spec wants.

::: dom/network/src/UDPSocket.cpp
@@ +59,5 @@
> +  if (aOptions.mRemoteAddress.WasPassed()) {
> +    remoteAddress = aOptions.mRemoteAddress.Value();
> +  } else {
> +    SetDOMStringToNull(remoteAddress);
> +  }

I don't see this:

"If the options argument's remoteAddress member is present and it is a valid IPv4/6 address then set the mySocket.remoteAddress attribute (default remote address) to the requested address. Else, if the remoteAddress member is present but it is not a valid IPv4/6 address then throw DOMException InvalidAccessError and abort the remaining steps."

"... Else, if the remotePort member is present but it is not a valid port number then throw DOMException InvalidAccessError and abort the remaining steps. "

Can you double check what the constructor should do? In particular I think the points number 2, 3 4 are not fully covered.

@@ +79,5 @@
> +    localPort.SetValue(aOptions.mLocalPort.Value());
> +  }
> +
> +  nsRefPtr<UDPSocket> socket = new UDPSocket(ownerWindow, remoteAddress, remotePort);
> +  nsresult rv = socket->Init(localAddress, localPort, addressReuse, loopback);

aRv = socket->Init(localAddress, localPort, addressReuse, loopback);
if (NS_WARN_IF(aRv.Failed()) {
  return nullptr;
}

@@ +82,5 @@
> +  nsRefPtr<UDPSocket> socket = new UDPSocket(ownerWindow, remoteAddress, remotePort);
> +  nsresult rv = socket->Init(localAddress, localPort, addressReuse, loopback);
> +
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);

This error should be thrown only when the spec says that. If the creation of the promise fails, I think it's correct to return a 'correct' error code.

@@ +97,5 @@
> +  , mRemoteAddress(aRemoteAddress)
> +  , mRemotePort(aRemotePort)
> +  , mReadyState(SocketReadyState::Opening)
> +{
> +  SetIsDOMBinding();

This is not needed. SetIsDOMBinding is already called by DOMEventTargetHelper.

@@ +148,5 @@
> +    return;
> +  }
> +
> +  if (mReadyState == SocketReadyState::Opening && NS_FAILED(aReason) && mOpened) {
> +    mOpened->MaybeReject(aReason);

I didn't find in the spec that when Close() is called, mOpened must be rejected with some error code.
It seems a bug in the spec. To me it should be something like:

if (mReadyState == SocketReadyState::Opening) {
  MOZ_ASSERT(mOpened);
  mOpened->MaybeReject(SomeErrorCode);
}

@@ +316,5 @@
> +      return false;
> +    }
> +
> +    if (aData.IsString()) {
> +      const nsACString& data = NS_ConvertUTF16toUTF8(aData.GetAsString());

NS_ConvertUTF16ToUTF8 data(aData.GetAsString());

@@ +451,5 @@
> +                const Nullable<uint16_t>& aLocalPort,
> +                const bool& aAddressReuse,
> +                const bool& aLoopback)
> +{
> +  MOZ_ASSERT(!(mSocket || mSocketChild));

MOZ_ASSERT(!mSocket && !mSocketChild)

@@ +519,5 @@
> +                              const uint16_t& aRemotePort,
> +                              const uint8_t* aData,
> +                              const uint32_t& aDataLength)
> +{
> +  if (mReadyState != SocketReadyState::Open) {

Can this really happen?

@@ +642,5 @@
> +
> +NS_IMETHODIMP
> +UDPSocket::CallListenerOpened()
> +{
> +  if (mReadyState != SocketReadyState::Opening) {

Also this should not happen. MOZ_ASSERT?

::: dom/network/src/UDPSocket.h
@@ +155,5 @@
> +
> +  void
> +  CloseWithReason(nsresult aReason);
> +
> +  nsresult 

extra space

@@ +176,5 @@
> +    MulticastCommand(bool aJoin, const nsAString& aAddress)
> +      : mJoin(aJoin), mAddress(aAddress)
> +    { }
> +
> +    bool mJoin;

What about this:

enum {
  CommandJoin,
  ComamndLeave
} mCommand;

::: dom/webidl/UDPSocket.webidl
@@ +29,5 @@
> +    readonly    attribute boolean          loopback;
> +    readonly    attribute SocketReadyState readyState;
> +    readonly    attribute Promise<void>    opened;
> +    readonly    attribute Promise<void>    closed;
> +//    readonly    attribute ReadableStream   output; //Bug 891286: Stream API is not ready

File a bug for implementing this UDPSocket.output and UDPSocket.input
Attachment #8474453 - Flags: review?(amarchesini) → review-
Comment on attachment 8474453 [details] [diff] [review]
Part 4 - UDPSocket webidl, v10.1

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

::: dom/network/src/UDPSocket.cpp
@@ +59,5 @@
> +  if (aOptions.mRemoteAddress.WasPassed()) {
> +    remoteAddress = aOptions.mRemoteAddress.Value();
> +  } else {
> +    SetDOMStringToNull(remoteAddress);
> +  }

For localPort/remotePort, I should check if the initial value is greater than 0. The type uint16_t already guarantee the value is less than 65536.

For localAddress, I should check if it is a valid IPv4/6 address.

For remoteAddress, I think we should support domain name as well so I've filed a spec bug for it. https://github.com/sysapps/tcp-udp-sockets/issues/73

For the remoteAddress reach-ability check in clause #4, I think it's a spec bug because routing table cannot be used to determine if a remote address is reachable. https://github.com/sysapps/tcp-udp-sockets/issues/74

@@ +148,5 @@
> +    return;
> +  }
> +
> +  if (mReadyState == SocketReadyState::Opening && NS_FAILED(aReason) && mOpened) {
> +    mOpened->MaybeReject(aReason);

|CloseWithReason| is also the clean up function, therefore error during socket open (clause #11) and garbage collection will enter this function as well. |mOpened| will become null if garbage collector decide to kill it during socket open, so we should not use MOZ_ASSERT(mOpened) here.

For the scenario of close during socket open, I found the behavior is defined in socket.output.cancel() and openedPromise should be rejected with AbortError.

@@ +519,5 @@
> +                              const uint16_t& aRemotePort,
> +                              const uint8_t* aData,
> +                              const uint32_t& aDataLength)
> +{
> +  if (mReadyState != SocketReadyState::Open) {

It can happen when closing a socket while receiving packet.

@@ +642,5 @@
> +
> +NS_IMETHODIMP
> +UDPSocket::CallListenerOpened()
> +{
> +  if (mReadyState != SocketReadyState::Opening) {

This can happen when closing a socket before openedPromise is resolve.

::: dom/network/src/UDPSocket.h
@@ +176,5 @@
> +    MulticastCommand(bool aJoin, const nsAString& aAddress)
> +      : mJoin(aJoin), mAddress(aAddress)
> +    { }
> +
> +    bool mJoin;

If we prefer using enum here, I'll suggest to stripe the "Command" prefix for the enum value because it is redundant to the struct name "MulticastCommand".

Here is what I'll do:

struct MulticastCommand {
  enum CommandType { Join, Leave };
  MulticastCommand(CommandType aCommand, const nsAString& aAddress)
    : mCommand(aCommand), mAddress(aAddress)
  { }

  CommandType mCommand;
  nsString mAddress;
};

Creating this struct will be like this:

MulticastCommand command(MulticastCommand::Join, someString);

::: dom/webidl/UDPSocket.webidl
@@ +29,5 @@
> +    readonly    attribute boolean          loopback;
> +    readonly    attribute SocketReadyState readyState;
> +    readonly    attribute Promise<void>    opened;
> +    readonly    attribute Promise<void>    closed;
> +//    readonly    attribute ReadableStream   output; //Bug 891286: Stream API is not ready

Bug 1056444 is filed for implementing UDPSocket.input and UDPSocket.output.
I'm totally fine with all your comments. If you can submit a patch, I review it for today.
Attached patch Part 4 - UDPSocket webidl, v11 (obsolete) — Splinter Review
update according to review comments.
Attachment #8474453 - Attachment is obsolete: true
Attachment #8476514 - Flags: superreview+
Attachment #8476514 - Flags: review?(amarchesini)
Comment on attachment 8476514 [details] [diff] [review]
Part 4 - UDPSocket webidl, v11

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

lgtm.

::: dom/network/src/UDPSocket.cpp
@@ +309,5 @@
> +  nsCString remoteAddress;
> +  if (aRemoteAddress.WasPassed()) {
> +    remoteAddress = NS_ConvertUTF16toUTF8(aRemoteAddress.Value());
> +  } else if (!DOMStringIsNull(mRemoteAddress)) {
> +    remoteAddress = NS_ConvertUTF16toUTF8(mRemoteAddress);

if mRemoteAddress is nsCString, you don't have to convert it at every send(). Do this in the constructor.

@@ +392,5 @@
> +  } else {
> +    PRNetAddr prAddr;
> +    PR_InitializeNetAddr(PR_IpAddrAny, aLocalPort, &prAddr);
> +    PRStatus status = PR_StringToNetAddr(NS_ConvertUTF16toUTF8(aLocalAddress).BeginReading(),
> +                                         &prAddr);

This should not happen. You already did this check in the constructor.
Attachment #8476514 - Flags: review?(amarchesini) → review+
Attached patch Part 4 - UDPSocket webidl, v11.1 (obsolete) — Splinter Review
@baku, thanks a lot for the prompt review response. Patch is updated according to review comments, carry r+ and sr+.

latest try result: https://tbpl.mozilla.org/?tree=Try&rev=71cd04d9c6a3
Attachment #8476514 - Attachment is obsolete: true
Attachment #8477248 - Flags: superreview+
Attachment #8477248 - Flags: review+
(In reply to Bill Walker [:bwalker] [@wfwalker] from comment #196)
> Sandip, how can we nominate this as a b2g 2.2 feature?

Hi Bill,

This bug was nominated as FxOS V2.1 feature already.
By the way,
may I know why do you need this as the feature in next release?
(In reply to Marco Chen [:mchen] from comment #203)
> (In reply to Bill Walker [:bwalker] [@wfwalker] from comment #196)
> > Sandip, how can we nominate this as a b2g 2.2 feature?
> 
> Hi Bill,
> 
> This bug was nominated as FxOS V2.1 feature already.
> By the way,
> may I know why do you need this as the feature in next release?

Hi Marco,

We have some very high profile partners who need this UDP API to support printing from b2g. Write me offline for more details, they aren't public at this time.

-Bill
Comment on attachment 8473653 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v5.1

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

This looks fine.

One thing you should add (feel free to make it a followup if things are a crazy rush, but it's reasonably serious and an easy fix):  all 'SendFoo' methods called from the parent (i.e. IPDL messages sent to a child from the parent) must be guarded by checking first to see if the IPDL connection has gone down (else you will crash the entire phone if the child has died!).  Just keep a bool 'mIPCClosed' variable, and use ActorDestroy to mark it if the IPDL connection goes away).  See the uses of 'mIPCClosed' in the FTPChannelParent for a simple example:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/FTPChannelParent.cpp#48

Note how it's used to guard Send calls:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/FTPChannelParent.cpp#325

(except for some SendFailedAsyncOpen calls, which IIRC are called synchronously from RecvAsyncOpen, so since we're in the middle of receving IPDL we can assume it's not gone away yet).

If you can add these simply you do not need another review from me--just mark next patch +r.

::: dom/network/src/UDPSocketParent.cpp
@@ +280,4 @@
>    nsresult rv = mSocket->Close();
>    mSocket = nullptr;
> +
> +  mozilla::unused << NS_WARN_IF(NS_FAILED(rv));

Do you need the 'unused' here?
Attachment #8473653 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8473653 [details] [diff] [review]
Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v5.1

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

We already have a "mIPCOpen" flag in UDPSocketParent, which has been used as a guard in OnPacketReceived and OnStopListening. I'll add this check to FireInternalError because other functions are called synchronously in the middle of receiving IPDL.

::: dom/network/src/UDPSocketParent.cpp
@@ +280,4 @@
>    nsresult rv = mSocket->Close();
>    mSocket = nullptr;
> +
> +  mozilla::unused << NS_WARN_IF(NS_FAILED(rv));

We need this to solve the "unused return value" compile error in debug build.
rebase to latest m-c tip. carry r+.
Attachment #8468161 - Attachment is obsolete: true
Attachment #8477800 - Flags: review+
Attached patch Part 2 - udp-socket permission (obsolete) — Splinter Review
rebase to latest m-c tip, carry r+.
Attachment #8468162 - Attachment is obsolete: true
Attachment #8477802 - Flags: review+
rebase to latest m-c tip and update according to review comment, carry r+.
Attachment #8473653 - Attachment is obsolete: true
Attachment #8477803 - Flags: review+
Attached patch Part 4 - UDPSocket webidl, v11.1 (obsolete) — Splinter Review
rebase to latest m-c, carry r+ and sr+.

latest try result: https://tbpl.mozilla.org/?tree=Try&rev=b547c0728206
Attachment #8477248 - Attachment is obsolete: true
Attachment #8477806 - Flags: superreview+
Attachment #8477806 - Flags: review+
try looks good, ready to land.
Keywords: checkin-needed
Sorry, I had to back this out because it hit merge conflicts with bug 786419 that I wasn't able to resolve. Please rebase :(

https://hg.mozilla.org/integration/b2g-inbound/rev/5258e5e96848
rebase to latest m-c tip.
Attachment #8477802 - Attachment is obsolete: true
Attachment #8478760 - Flags: review+
rebase to latest m-c tip.
Attachment #8477806 - Attachment is obsolete: true
Attachment #8478762 - Flags: superreview+
Attachment #8478762 - Flags: review+
Confirmed with EM/EPM, and this can be landed before FL.
I had to back these out again in https://hg.mozilla.org/mozilla-central/rev/607421044238 for conflicting with the backout of bug 786419. 

The original, unrebased patches still applied cleanly, so I relanded those in:
remote:   https://hg.mozilla.org/mozilla-central/rev/b693a1350656
remote:   https://hg.mozilla.org/mozilla-central/rev/69e68a879b44
remote:   https://hg.mozilla.org/mozilla-central/rev/d0ab836a6517
remote:   https://hg.mozilla.org/mozilla-central/rev/4f6affdf52b6

Sorry for the churn here. Hopefully it sticks for real this time.
Flags: needinfo?(schien)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Realized that the later part of comment indicated this was relanded, so I shouldn't have reopened this. Sorry for the churn.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Non-clobber builds were failing, so I revved the UUIDs again since they did technically change:
https://hg.mozilla.org/mozilla-central/rev/0753f7b93ab7
Flags: in-moztrap?(fyen)
happily using this to do Bonjour in FxOS - thanks so much!
Flags: needinfo?(schien)
i notice that in my custom b2g system application, UDPSocket is not defined even though i have udp-socket permission in its manifest. do i need to do something else to make it available? 

i've yet to encounter an issue using UDPSocket from a regular app -- great job!
Flags: needinfo?(skamat)
Flags: in-moztrap-
(In reply to Jason Proctor from comment #225)
> i notice that in my custom b2g system application, UDPSocket is not defined
> even though i have udp-socket permission in its manifest. do i need to do
> something else to make it available? 
> 
> i've yet to encounter an issue using UDPSocket from a regular app -- great
> job!

Have you |make reset-gaia| after changing the permission of system application? You'll need to do that after changing manifest of core apps.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #226)
> (In reply to Jason Proctor from comment #225)
> > i notice that in my custom b2g system application, UDPSocket is not defined
> > even though i have udp-socket permission in its manifest. do i need to do
> > something else to make it available? 
> > 
> > i've yet to encounter an issue using UDPSocket from a regular app -- great
> > job!
> 
> Have you |make reset-gaia| after changing the permission of system
> application? You'll need to do that after changing manifest of core apps.

does |make reset-gaia| have side effects other than updating the apps on the phone?

the system app is updated OK, but we don't have access to UDPSocket, still. wondering if there's another step.

thanks for any help.
No end-user QA verification needed, so marking verified.
Status: RESOLVED → VERIFIED
(In reply to Jason Proctor from comment #227)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #226)
> > (In reply to Jason Proctor from comment #225)
> > > i notice that in my custom b2g system application, UDPSocket is not defined
> > > even though i have udp-socket permission in its manifest. do i need to do
> > > something else to make it available? 
> > > 
> > > i've yet to encounter an issue using UDPSocket from a regular app -- great
> > > job!
> > 
> > Have you |make reset-gaia| after changing the permission of system
> > application? You'll need to do that after changing manifest of core apps.
> 
> does |make reset-gaia| have side effects other than updating the apps on the
> phone?
> 
> the system app is updated OK, but we don't have access to UDPSocket, still.
> wondering if there's another step.
> 
> thanks for any help.

Yes, |make reset-gaia| will also wipe out all the user data and settings and execute first-time boot sequence after reboot. Currently the permission of core apps only update in two scenarios: 1. first-time boot sequence; 2. after OTA.
Is there a way to get access to UDPSocket on Desktop Firefox at this point?  I can set the dom.udpsocket.enabled to true in about:config and that gives me some access to UDPSocket in the console (if I say, run the inspector inside a new tab page), but I can't see a way to allow for UDPSocket access from a regular old web page.  The UDPSocket object is null in a loaded html page, even with the flag flipped.

If I set it as a permission in a manifest, I don't see how I can get myself into a certified permissions mode, either.  Trying to load a privileged or certified manifest in the desktop browser throws a "INVALID_SECURITY_LEVEL" error, so... I'm stuck.  

Are there currently any options to test this API in Desktop?

Thanks!
(In reply to misteranderson from comment #230)
> Are there currently any options to test this API in Desktop?
The only way to use UDPSocket in Desktop currently is in chrome code. https://github.com/schien/ssdp.js has a demo addon that using UDPSocket.
Depends on: 1080096
FYI,

I see that this thread refers the W3C SysApps TCP and UDP Socket API that I am editing. To get a guick update on the current status you can check http://lists.w3.org/Archives/Public/public-sysapps/2014Oct/att-0056/TCP_UDP_Socket_API_-_TPAC_2014.pdf. The latest version of the specification is here: http://www.w3.org/2012/sysapps/tcp-udp-sockets/ and this version will shortly be published as a new W3C public draft.
G'day, 

I need UDP for a home automation programming/diagnostic system I'm developing for Firefox (I've already had something working on node.js so decided it would be a good idea to port to Firefox, so no computer is needed). 

The MOX LT home automation uses an inbound and outbound UDP socket to send and receive. UDP is the only option for receiving/sending messages without using some kind of software proxy running on a computer.  

More info on the protocol is available on: https://jayvee.com.au/downloads/docs/mox/mox-lt-protocol-1_00_01.pdf
Also, I'll add, that uses ports 6667 and 6670, so not sure if privileged ports also apply to UDP, but they aren't required
Hi Andrew, the UDPSocket WebAPI is only available on Firefox OS currently, which means you can not use it on Firefox now. The alternative is using nsUDPSocket in Firefox add-on.

BTW, I'll suggest you to put your question on mailing list dev-platform@lists.mozilla.org. It is the appropriate place to raise this kind of how-should-I-do questions and there will be more people can help. :)
G'day, 

Actually I meant FX OS over B2G.. But, I'll post there instead.. Just providing another application use case where UDP is required (its not only required for performance). And I wasn't sure if it was available in B2G yet
Andrew, FWIW we have been able to get UDP up and running for Firefox OS apps (as long as they are given the permissions and are "Certified" in their manifests).  We are using UDP to do MDNS broadcast and discovery in Firefox OS (what Apple calls Bonjour). Rather than explaining the whole process here, have a look at the Firefox OS sample applications and sensible.js in the Sensible preview here: http://sensible.mono.hm
Your app doesn't have to be certified. Privileged apps (the ones you can distribute from the marketplace) have access to UDP sockets (see https://mxr.mozilla.org/mozilla-central/source/dom/apps/PermissionsTable.jsm#70).

sensible looks cool btw!
Thanks, Fabrice, good to know!  I'll make sure to mention that in the Sensible docs as well.
Sadly, Sensible apps *do* have to be certified, as they need to obtain their IP number in order to advertise it via MDNS, and the only way to get your IP number is off mozWifiManager, which is certified only. I keep meaning to log a bug that wifi-manage permission shouldn't be necessary to simply obtain your IP number, but somehow I never get to it.

I hope someone knows better!
I haven't check, but do we set localAddress on udp sockets when it's no specified in the socket constructor?
a quick check reveals that localAddress is set to the bound address --

> var x = new UDPSocket ();
undefined
> x
UDPSocket { localAddress: "0.0.0.0", localPort: 41273, remoteAddress: null, remotePort: null, addressReuse: true, loopback: false, readyState: "open", opened: Promise, closed: Promise, onmessage: null }
UDPSocket will be bound to any interface if no localAddress is specified in constructor.
OK so the question bears repeating -- is there a way of obtaining local IP addresses from a non-certified application?

Humbly beg to suggest that navigator.mozWifiManager should be available to non-certified apps. The actual wi-fi management API, of course, should remain available to certified apps only.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #243)
> UDPSocket will be bound to any interface if no localAddress is specified in
> constructor.

The IPv4 any address.  I assume that "::" works as a localAddress if you want to use v6.
(In reply to Jason Proctor from comment #244)
> OK so the question bears repeating -- is there a way of obtaining local IP
> addresses from a non-certified application?
> 
> Humbly beg to suggest that navigator.mozWifiManager should be available to
> non-certified apps. The actual wi-fi management API, of course, should
> remain available to certified apps only.

Hmm...good question, I don't think we have such API for privilege app now. Mind to open a new bug and discuss over there?
question - is broadcasting supported over UDP sockets? it doesn't seem like it (at least to 255.255.255.255 and a hardwired local 192.168.0.255 i tried). the code doesn't seem to setsockopt(SO_BROADCAST) anywhere, but i thought i would check with the authorities :-)

thanks!
Multicast is supported, see http://mxr.mozilla.org/mozilla-central/source/dom/webidl/UDPSocket.webidl#37

For instance, to do ssdp discovery:
socket = new UDPSocket({ loopback: true, localPort: 1900 });
socket.joinMulticastGroup("239.255.255.250");

and then socket.send(msgData, "239.255.255.250", 1900);
(In reply to Jason Proctor from comment #248)
> question - is broadcasting supported over UDP sockets? it doesn't seem like
> it (at least to 255.255.255.255 and a hardwired local 192.168.0.255 i
> tried). the code doesn't seem to setsockopt(SO_BROADCAST) anywhere, but i
> thought i would check with the authorities :-)
> 
> thanks!

No, it doesn't support broadcast now, only unicast/multicast are supported. Please file a new bug for continuing the discussion. :)
gents, thanks for the answers.

we're happily using multicast, but one device we'd like to talk to currently only supports discovery via broadcast. no worries, the device will evolve in time :-)
Blocks: 1247634
No longer blocks: 1247634
The following almost-empty documentation topic cites this bug. Is the document still needed?
https://developer.mozilla.org/en-US/docs/Web/API/UDP_Socket_API
No longer blocks: 1026362

(In reply to rolfedh from comment #252)

The following almost-empty documentation topic cites this bug. Is the
document still needed?
https://developer.mozilla.org/en-US/docs/Web/API/UDP_Socket_API

Please leave the link, just followed it tonight. Hope to find a way of sending my own packets.

Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.