745283 - Expose a client UDP datagram socket API to web applications

Status: VERIFIED FIXED

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

Description

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 1

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

SecReview Scheduled:
https://mail.mozilla.com/home/[e-mail]/Security%20Review.html?view=month&action=view&invId=110473-110472&pstat=AC&exInvId=110473-179809&useInstance=1&instStartTime=1337803200000&instDuration=3600000

Comment 2

[Rob Gilson]

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.

Comment 3

[Honza Bambas (:mayhemer)]

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.

Comment 4

[Matthew Phillips]

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.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I try to provide ipdl interface for WebRTC e10s in bug 869869. We can implement mozUDP API based on that patch.

Comment 6

[Donovan Preston [:fzzzy]]

I'm not working on this so I'll release it in case someone else wants to take it

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 1 - WebIDL

First WIP interface.. simply copied from http://raw-sockets.sysapps.org/

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 9

[dch]

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/

Comment 10

[Florian Vallee]

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

Comment 11

[Peter Poirier [:peterp70]]

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?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 13

[Peter Poirier [:peterp70]]

Attachment: WIP - Implements basic string send/response

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

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Peter, please see comment 12. This API needs to be implemented in C++ because we want to make it available in workers.

Comment 15

[Donovan Preston [:fzzzy]]

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.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Awesome, makes sense, thanks!

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 18

[Peter Poirier [:peterp70]]

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.

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Yeah sure, I can start the c++ implementation immediately.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 1 - udp-socket-webidl.patch

This is a compilable stub code for UDP socket API. I'd like to have some feedback for WebIDL before diving into the implementation.

Comment 21

[Josh Matthews [:jdm]]

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.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 23

[Josh Matthews [:jdm]]

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?

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use event codegen, v1

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

Comment 25

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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?

Comment 26

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: use event codegen, v2

I fix the compile error by adding the corresponding |MessageEvent(EventTarget*)| constructor, but I'm not sure if this modification really make sense.

Comment 27

[Josh Matthews [:jdm]]

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.

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 1 - provide multicast functionality on nsUDPSocket

support join/leave multicast group.

Comment 29

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 2 - WebIDL and implementation for UDP Socket API

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

Comment 30

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 1 - support socket options on nsUDPSocket init functions

Comment 31

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 2 - support socket options/multicast functionality in PUDPSocket.ipdl

Comment 32

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 3 - UDPSocket webidl

Comment 33

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [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

Comment 34

[dch]

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.

Comment 35

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 3 - UDPSocket webidl

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/

Comment 36

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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

Comment 37

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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/[e-mail]-6103e2f0dcc4/

Comment 38

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v1

support multicast loopback, reuse_addr, send input stream in nsIUDPSocket

Comment 39

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 2 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl

Update: support input stream.

Comment 40

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] Part 3 - UDPSocket webidl

Update: support send blob

Comment 41

[Jason Duell]

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.

Comment 42

[Jason Duell]

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?

Comment 43

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 44

[Honza Bambas (:mayhemer)]

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

Comment 45

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v2

support send huge buffer by multiple packet.

Comment 46

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - testcase.patch

Comment 47

[Josh Matthews [:jdm]]

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.

Comment 48

[Josh Matthews [:jdm]]

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 (

Comment 49

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 50

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 51

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v2

summary of changes:
1. support addressReuse parameter
2. expose local address as nsINetAddr
3. support send input stream
4. update xpcshell test case

Comment 52

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl

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

Comment 53

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - UDPSocket webidl

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.

Comment 54

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - UDPSocket webidl

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.

Comment 55

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - test case for UDPSocket

summary of changes:
1. test for dom.udpsocket.enabled
2. test for UDPSocket functionality

Comment 56

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8428553 [details] [diff] [review]
Part 3 - UDPSocket webidl

@khuey, can you take a look at JSAPI usage in this patch?

Comment 57

[Eric Rescorla (:ekr)]

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?

Comment 58

[(no longer active)]

This should not be exposed to Web content, the fact that attachment 8428553 [details] [diff] [review] doesn't prevent that is a bug.

Comment 59

[(no longer active)]

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.

Comment 60

[Eric Rescorla (:ekr)]

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

Comment 61

[(no longer active)]

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

Comment 62

[Eric Rescorla (:ekr)]

(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

Comment 63

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 64

[Eric Rescorla (:ekr)]

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

Comment 65

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 66

[Eric Rescorla (:ekr)]

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

Comment 67

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 68

[Eric Rescorla (:ekr)]

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

Comment 69

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 70

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 71

[[:fabrice] Fabrice Desré]

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!

Comment 72

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - udp-socket permission

extract the change for "udp-socket" permission into another patch.

Comment 73

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl

move the change for "udp-socket" permission to another patch.

Comment 74

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl

update the permission control for non-oop frame and merge test case into one patch.

Comment 75

[Jason Duell]

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

Comment 76

[(no longer active)]

Shih-Chiang, what parts of the patch would you like me to provide feedback on?  Thanks!

Comment 77

[(no longer active)]

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.

Comment 78

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 79

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 80

[Honza Bambas (:mayhemer)]

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.

Comment 81

[(no longer active)]

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

Comment 82

[Josh Matthews [:jdm]]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 84

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 85

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 86

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 87

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 88

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

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

Comment 89

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 90

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 91

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

If there is no such interface anymore, no need to file a spec bug :)

Comment 92

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 93

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v2.1

update according to review comment and rebase to latest m-c, carry r+.

Comment 94

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - udp-socket permission

rebase to latest m-c, carry r+.

Comment 95

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part3, support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v2

Comment 96

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v2

update according to review comment.

Comment 97

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v2

Ask @khuey to review JSAPI usage first. Will add r? to jdm if he think I can go with [NoInterfaceObject].

Comment 98

[Josh Matthews [:jdm]]

Since there's no UDPMessageEvent, why are we implementing it in place of the UDPMessage dictionary?

Comment 99

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 100

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 101

[Josh Matthews [:jdm]]

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.

Comment 102

[Josh Matthews [:jdm]]

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

Comment 103

[Donovan Preston [:fzzzy]]

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

Comment 104

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 105

[Jason Duell]

Honza, can you answer comment 100?

Comment 106

[Honza Bambas (:mayhemer)]

(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

Comment 107

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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

Comment 108

[Patrick McManus [:mcmanus]]

(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

Comment 109

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8439064 [details] [diff] [review]
Part 4 - UDPSocket webidl, v2

start the WebAPI review.

Comment 110

[Peter Poirier [:peterp70]]

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.

Comment 111

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 112

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v3

Update according to review comment.

@ekr, please review nrsocket and packet filter related modification. Thanks.

Comment 113

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v3

Update according to latest AutoJSAPI change in Bug 1025476.

Comment 114

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

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?

Comment 115

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 116

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

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

Comment 117

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v4

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.

Comment 118

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 119

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v5

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.

Comment 120

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

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.

Comment 121

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v6

Update according to @khuey's review comment.

Comment 122

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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] ([e-mail]) 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
>

Comment 123

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

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

Comment 124

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

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

Comment 125

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

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.

Comment 126

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

And I wouldn't expect the working group to be interested in improving an obsolete draft.

Comment 127

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

And we need the API before we'll have Streams?

Comment 128

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

Yes :(

Comment 129

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

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

Comment 130

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

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

Comment 131

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

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

Comment 132

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

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.

Comment 133

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

b2g seems to limit sHistoryMaxTotalViewers to 1

Comment 134

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v7

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.

Comment 135

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

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.

Comment 136

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

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.

Comment 137

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

(I don't see how DisallowBFCaching can work reliably in case of reusing inner windows.)

Comment 138

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

But feel free to use DisallowBFCaching. That method needs to be fixed to work always anyway.

Comment 139

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I want to put some test cases for the bfcaching scenario, do we have any existing test case for reference?

Comment 140

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

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.

Comment 141

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v8

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]

Comment 142

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

Subframe bfcache isn't really supported. Please test using toplevel docshells.

Comment 143

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v8.1

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.

Comment 144

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 145

[Eric Rescorla (: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.

Comment 146

[Byron Campen [:bwc]]

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?

Comment 147

[Byron Campen [:bwc]]

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.

Comment 148

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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?

Comment 149

[Byron Campen [:bwc]]

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?

Comment 150

[Martin Thomson [:mt:]]

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?

Comment 151

[Jason Proctor]

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

Comment 152

[Patrick McManus [:mcmanus]]

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

Comment 153

[Martin Thomson [:mt:]]

What blocks here is the dispatch to the parent process.  The bind call itself is benign.

Comment 154

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

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

Comment 155

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 156

[Jason Proctor]

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.

Comment 157

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 158

[Martin Thomson [:mt:]]

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

Comment 159

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 160

[Jenny Liu[:jennyliu]]

Move to 2.1 Sprint 2.

Comment 161

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v4

revert the sync bind support.

Comment 162

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [WIP] UDPSocket webidl, v9

1. support async socket open
2. remove bufferedAmount/ondrain/onerror/suspend/resume

Comment 163

[Martin Thomson [:mt:]]

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

[Byron Campen [:bwc]]

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)

Comment 165

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 166

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 167

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v2.1

rebased to latest m-c, carry r+.

Comment 168

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - udp-socket permission

rebase to latest m-c, carry r+.

Comment 169

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v9

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.

Comment 170

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

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.

Comment 171

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 172

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

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.

Comment 173

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 174

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

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

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

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.

Comment 176

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v9.1

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>

Comment 177

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 178

[Jenny Liu[:jennyliu]]

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!!

Comment 179

[Byron Campen [:bwc]]

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 180

[Byron Campen [:bwc]]

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.

Comment 181

[Andrea Marchesini [:baku]]

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?

Comment 182

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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

Comment 183

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 184

[Andrea Marchesini [:baku]]

> 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 185

[Martin Thomson [:mt:]]

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

Comment 186

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 187

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v5

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.

Comment 188

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v5.1

minor update for fixing unused-return-value warning in UDPSocketChild.cpp and UDPSocketParent.cpp.

Comment 189

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v10

update according to review comment.

Comment 190

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

try result: https://tbpl.mozilla.org/?tree=Try&rev=7bbec93ad199

Comment 191

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 192

[Jenny Liu[:jennyliu]]

Move to v2.1 sprint 3.

Comment 193

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v10.1

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.

Comment 194

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

latest try result (with the patch for Bug 1054908): https://tbpl.mozilla.org/?tree=Try&rev=88d8364226a3

Comment 195

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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?

Comment 196

[Bill Walker [:bwalker] [@wfwalker]]

Sandip, how can we nominate this as a b2g 2.2 feature?

Comment 197

[Andrea Marchesini [:baku]]

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

Comment 198

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 199

[Andrea Marchesini [:baku]]

I'm totally fine with all your comments. If you can submit a patch, I review it for today.

Comment 200

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v11

update according to review comments.

Comment 201

[Andrea Marchesini [:baku]]

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.

Comment 202

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v11.1

@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

Comment 203

[Marco Chen [:mchen]]

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

Comment 204

[Bill Walker [:bwalker] [@wfwalker]]

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

[Jason Duell]

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?

Comment 206

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 207

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v2.1

rebase to latest m-c tip. carry r+.

Comment 208

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - udp-socket permission

rebase to latest m-c tip, carry r+.

Comment 209

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v5.2

rebase to latest m-c tip and update according to review comment, carry r+.

Comment 210

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v11.1

rebase to latest m-c, carry r+ and sr+.

latest try result: https://tbpl.mozilla.org/?tree=Try&rev=b547c0728206

Comment 211

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

try looks good, ready to land.

Comment 212

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c6ad8ab94461
https://hg.mozilla.org/integration/b2g-inbound/rev/ce19c464f5d8
https://hg.mozilla.org/integration/b2g-inbound/rev/fc08327b3d3e
https://hg.mozilla.org/integration/b2g-inbound/rev/70bad98676c8

Comment 213

[Ryan VanderMeulen [:RyanVM]]

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

Comment 214

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - support socket options on nsUDPSocket init functions, support send input stream, v2.1

rebase to latest m-c tip.

Comment 215

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - udp-socket permission

rebase to latest m-c tip.

Comment 216

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - support socket options/multicast/input stream functionality in PUDPSocket.ipdl, v5.2

rebase to latest m-c tip.

Comment 217

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - UDPSocket webidl, v11.1

rebase to latest m-c tip.

Comment 218

[Kai-Chih Hu [:khu]]

Confirmed with EM/EPM, and this can be landed before FL.

Comment 219

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0ab47e8ac7
https://hg.mozilla.org/integration/mozilla-inbound/rev/37c10c9f2a4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc6f4275bbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1af198d91bd

Comment 220

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bc0ab47e8ac7
https://hg.mozilla.org/mozilla-central/rev/37c10c9f2a4d
https://hg.mozilla.org/mozilla-central/rev/9fc6f4275bbb
https://hg.mozilla.org/mozilla-central/rev/a1af198d91bd

Comment 221

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 222

[Jason Smith [:jsmith]]

Realized that the later part of comment indicated this was relanded, so I shouldn't have reopened this. Sorry for the churn.

Comment 223

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Non-clobber builds were failing, so I revved the UUIDs again since they did technically change:
https://hg.mozilla.org/mozilla-central/rev/0753f7b93ab7

Comment 224

[Jason Proctor]

happily using this to do Bonjour in FxOS - thanks so much!

Comment 225

[Jason Proctor]

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!

Comment 226

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 227

[Jason Proctor]

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

Comment 228

[Jason Smith [:jsmith]]

No end-user QA verification needed, so marking verified.

Comment 229

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 230

[Aubrey Anderson]

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!

Comment 231

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 232

[Claes]

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.

Comment 233

[Andrew Luecke]

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

Comment 234

[Andrew Luecke]

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

Comment 235

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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 [e-mail]. It is the appropriate place to raise this kind of how-should-I-do questions and there will be more people can help. :)

Comment 236

[Andrew Luecke]

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

Comment 237

[Aubrey Anderson]

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

Comment 238

[[:fabrice] Fabrice Desré]

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!

Comment 239

[Aubrey Anderson]

Thanks, Fabrice, good to know!  I'll make sure to mention that in the Sensible docs as well.

Comment 240

[Jason Proctor]

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!

Comment 241

[[:fabrice] Fabrice Desré]

I haven't check, but do we set localAddress on udp sockets when it's no specified in the socket constructor?

Comment 242

[Jason Proctor]

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 }

Comment 243

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

UDPSocket will be bound to any interface if no localAddress is specified in constructor.

Comment 244

[Jason Proctor]

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.

Comment 245

[Martin Thomson [:mt:]]

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

Comment 246

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 247

[Jason Proctor]

done.

https://bugzilla.mozilla.org/show_bug.cgi?id=1107744

Comment 248

[Jason Proctor]

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!

Comment 249

[[:fabrice] Fabrice Desré]

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

Comment 250

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 251

[Jason Proctor]

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

Comment 252

[rolfedh]

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

Comment 254

[Jesse.Bower]

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