Closed Bug 869869 Opened 11 years ago Closed 11 years ago

e10s for UDP socket

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox28 --- fixed

People

(Reporter: schien, Assigned: schien)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, Whiteboard: [ft:webrtc])

Attachments

(5 files, 34 obsolete files)

architecture
44.51 KB, image/png
Details
Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v6
50.16 KB, patch
schien
: review+
Details | Diff | Splinter Review
Part 2 - ipdl for nsUDPSocket, v5
41.24 KB, patch
schien
: review+
Details | Diff | Splinter Review
Part 3 - Bridge nr_socket to UDP e10s API, v10.1
27.66 KB, patch
schien
: review+
Details | Diff | Splinter Review
Part 4, test cases for NrSocketIpc
12.56 KB, patch
schien
: review+
Details | Diff | Splinter Review 
Provide IPC interface for UDP socket.
1. rename nsUDPServerSocket to nsUDPSocket
2. add |send| interface
Attachment #746860 - Flags: feedback?(honzab.moz)
provide ipdl interface of nsUDPSocket
Attachment #746864 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 746860 [details] [diff] [review]
WIP - Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface

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

Rename it self seems to be OK.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +107,5 @@
> +    * @return number of bytes written. (may be less than dataLength)
> +    */
> +    unsigned long send(in DOMString host, in unsigned short port,
> +                       [array, size_is(dataLength)]in uint8_t data,
> +                       in unsigned long dataLength);

Do you plan to send large amounts of data using this API?  If so, then it's not the best API.

I'd rather use nsIUDPMessage and its output stream for sending data out.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +623,5 @@
> +  PRNetAddr addr;
> +  PR_InitializeNetAddr(PR_IpAddrAny, aPort, &addr);
> +  PRStatus status = PR_StringToNetAddr(NS_ConvertUTF16toUTF8(aHost).get(), &addr);
> +
> +  // return fail if the host is not a valid ipv4 or ipv6 address

No DNS ?

@@ +638,5 @@
> +    int32_t count = PR_SendTo(mFD, aData, sizeof(uint8_t) * aDataLength,
> +                               0, &addr, PR_INTERVAL_NO_WAIT);
> +    if (count < 0) {
> +      PRErrorCode code = PR_GetError();
> +      return ErrorAccordingToNSPR(code);

This can easily be WOULD_BLOCK.  How the consumer can handle it?

::: netwerk/build/nsNetCID.h
@@ +336,5 @@
>  }
>  
> +// component implementing nsIUDPSocket
> +#define NS_UDPSOCKET_CONTRACTID \
> +    "@mozilla.org/network/udp-socket;1"

Bad this is under "network" namespace... I had to speak up during the review... it should be under "dom".
Attachment #746860 - Flags: feedback?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #3)
 
> ::: netwerk/base/public/nsIUDPSocket.idl
> @@ +107,5 @@
> > +    * @return number of bytes written. (may be less than dataLength)
> > +    */
> > +    unsigned long send(in DOMString host, in unsigned short port,
> > +                       [array, size_is(dataLength)]in uint8_t data,
> > +                       in unsigned long dataLength);
> 
> Do you plan to send large amounts of data using this API?  If so, then it's
> not the best API.
> 
> I'd rather use nsIUDPMessage and its output stream for sending data out.

For WebRTC, we're talking on the order of a megabit or two outbound, perhaps more inbound, and in some cases (with a good connection!) a couple of times that.

Mostly in relatively-full packets; maybe 50 pps Audio (smaller), 30*3 to 30*9ish pps for video (large), plus a few to a few dozen RTCP packets, ICE, etc (smaller).  DataChannels could be almost nothing in most cases, to full-bandwidth xfers for seconds to minutes in others.
(In reply to Randell Jesup [:jesup] from comment #4)
> (In reply to Honza Bambas (:mayhemer) from comment #3)
>  
> > ::: netwerk/base/public/nsIUDPSocket.idl
> > @@ +107,5 @@
> > > +    * @return number of bytes written. (may be less than dataLength)
> > > +    */
> > > +    unsigned long send(in DOMString host, in unsigned short port,
> > > +                       [array, size_is(dataLength)]in uint8_t data,
> > > +                       in unsigned long dataLength);
> > 
> > Do you plan to send large amounts of data using this API?  If so, then it's
> > not the best API.
> > 
> > I'd rather use nsIUDPMessage and its output stream for sending data out.
> 
> For WebRTC, we're talking on the order of a megabit or two outbound, perhaps
> more inbound, and in some cases (with a good connection!) a couple of times
> that.

WebRTC is going to use this code?

> 
> Mostly in relatively-full packets; maybe 50 pps Audio (smaller), 30*3 to
> 30*9ish pps for video (large), plus a few to a few dozen RTCP packets, ICE,
> etc (smaller).  DataChannels could be almost nothing in most cases, to
> full-bandwidth xfers for seconds to minutes in others.

So, you may easily fill the system output buffers.  Hmm.. I think you will hit WOULD_BLOCK or sent<dataLength case sooner or later.

The UDPMessage impl is using large buffer under the stream to avoid this.  The buffer is a price to pay for simple sync API (maybe semi-sync since it's able to tell you "stop! buffer grew to much, wait for it to free up using ondrain event".)

Maybe we should have something similar for UDP socket as well?
(In reply to Honza Bambas (:mayhemer) from comment #5)
> (In reply to Randell Jesup [:jesup] from comment #4)
> > (In reply to Honza Bambas (:mayhemer) from comment #3)

> > > Do you plan to send large amounts of data using this API?  If so, then it's
> > > not the best API.
> > > 
> > > I'd rather use nsIUDPMessage and its output stream for sending data out.
> > 
> > For WebRTC, we're talking on the order of a megabit or two outbound, perhaps
> > more inbound, and in some cases (with a good connection!) a couple of times
> > that.
> 
> WebRTC is going to use this code?

I'm assuming so (which is why I was cc'd on it).

> > Mostly in relatively-full packets; maybe 50 pps Audio (smaller), 30*3 to
> > 30*9ish pps for video (large), plus a few to a few dozen RTCP packets, ICE,
> > etc (smaller).  DataChannels could be almost nothing in most cases, to
> > full-bandwidth xfers for seconds to minutes in others.
> 
> So, you may easily fill the system output buffers.  Hmm.. I think you will
> hit WOULD_BLOCK or sent<dataLength case sooner or later.

Note this is all UDP, but yes.

> The UDPMessage impl is using large buffer under the stream to avoid this. 
> The buffer is a price to pay for simple sync API (maybe semi-sync since it's
> able to tell you "stop! buffer grew to much, wait for it to free up using
> ondrain event".)

Also note we have a full-on network stack running (SCTP for DataChannels) which depends on getting reasonable feedback from the network interface and are running congestion control algorithms on the RTP data which requires accurate arrival timestamps wherever possible (and we'd like to have an option to fill in an RTP header extension with "data was *really* sent at this time" - that might not be possible since the header data would need to run through the SRTP encryption code. 

We really *don't* want any "extra" buffering to occur - we need low-delay input and output.
 
> Maybe we should have something similar for UDP socket as well?

SCTP might be possible to hook to buffering stuff, but it normally runs it's own buffers and provides that sort of service to the higher levels.  The RTP data - not really.
(In reply to Randell Jesup [:jesup] from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > (In reply to Randell Jesup [:jesup] from comment #4)
> > > (In reply to Honza Bambas (:mayhemer) from comment #3)
> 
> > > > Do you plan to send large amounts of data using this API?  If so, then it's
> > > > not the best API.
> > > > 
> > > > I'd rather use nsIUDPMessage and its output stream for sending data out.
> > > 
> > > For WebRTC, we're talking on the order of a megabit or two outbound, perhaps
> > > more inbound, and in some cases (with a good connection!) a couple of times
> > > that.
> > 
> > WebRTC is going to use this code?
> 
> I'm assuming so (which is why I was cc'd on it).
> 

Yes. That is the motivation for this task


> > > Mostly in relatively-full packets; maybe 50 pps Audio (smaller), 30*3 to
> > > 30*9ish pps for video (large), plus a few to a few dozen RTCP packets, ICE,
> > > etc (smaller).  DataChannels could be almost nothing in most cases, to
> > > full-bandwidth xfers for seconds to minutes in others.
> > 
> > So, you may easily fill the system output buffers.  Hmm.. I think you will
> > hit WOULD_BLOCK or sent<dataLength case sooner or later.
> 
> Note this is all UDP, but yes.
> 
> > The UDPMessage impl is using large buffer under the stream to avoid this. 
> > The buffer is a price to pay for simple sync API (maybe semi-sync since it's
> > able to tell you "stop! buffer grew to much, wait for it to free up using
> > ondrain event".)
> 
> Also note we have a full-on network stack running (SCTP for DataChannels)
> which depends on getting reasonable feedback from the network interface and
> are running congestion control algorithms on the RTP data which requires
> accurate arrival timestamps wherever possible (and we'd like to have an
> option to fill in an RTP header extension with "data was *really* sent at
> this time" - that might not be possible since the header data would need to
> run through the SRTP encryption code. 
> 
> We really *don't* want any "extra" buffering to occur - we need low-delay
> input and output.
>  
> > Maybe we should have something similar for UDP socket as well?
> 
> SCTP might be possible to hook to buffering stuff, but it normally runs it's
> own buffers and provides that sort of service to the higher levels.  The RTP
> data - not really.
(In reply to Eric Rescorla (:ekr) from comment #7)
> > > WebRTC is going to use this code?
> > 
> > I'm assuming so (which is why I was cc'd on it).
> > 
> 
> Yes. That is the motivation for this task
> 

OK, then I think we can go with just a simple send(databuffer) method as introduced here.  Any async buffering can be created on demand later.

Maybe, we may also need/want to add the same send() method to nsIUDPMessage then to make answering simpler.
(In reply to Honza Bambas (:mayhemer) from comment #3)
> Comment on attachment 746860 [details] [diff] [review]
> WIP - Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send|
> interface
> 
> Review of attachment 746860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Rename it self seems to be OK.
> 
> ::: netwerk/base/public/nsIUDPSocket.idl
> @@ +107,5 @@
> > +    * @return number of bytes written. (may be less than dataLength)
> > +    */
> > +    unsigned long send(in DOMString host, in unsigned short port,
> > +                       [array, size_is(dataLength)]in uint8_t data,
> > +                       in unsigned long dataLength);
> 
> Do you plan to send large amounts of data using this API?  If so, then it's
> not the best API.
> 
> I'd rather use nsIUDPMessage and its output stream for sending data out.
How to use nsIUDPMessage for sending out first packet from a client socket? Do you mean to provide |nsIUDPMessage createMessage(remoteAddr)| or |void sendMessage(nsIUDPMessage)|?
> 
> ::: netwerk/base/src/nsUDPSocket.cpp
> @@ +623,5 @@
> > +  PRNetAddr addr;
> > +  PR_InitializeNetAddr(PR_IpAddrAny, aPort, &addr);
> > +  PRStatus status = PR_StringToNetAddr(NS_ConvertUTF16toUTF8(aHost).get(), &addr);
> > +
> > +  // return fail if the host is not a valid ipv4 or ipv6 address
> 
> No DNS ?
I'll add DNS provisioning in next version of patch. WebRTC currently uses ip address only.
> 
> @@ +638,5 @@
> > +    int32_t count = PR_SendTo(mFD, aData, sizeof(uint8_t) * aDataLength,
> > +                               0, &addr, PR_INTERVAL_NO_WAIT);
> > +    if (count < 0) {
> > +      PRErrorCode code = PR_GetError();
> > +      return ErrorAccordingToNSPR(code);
> 
> This can easily be WOULD_BLOCK.  How the consumer can handle it?
> 
> ::: netwerk/build/nsNetCID.h
> @@ +336,5 @@
> >  }
> >  
> > +// component implementing nsIUDPSocket
> > +#define NS_UDPSOCKET_CONTRACTID \
> > +    "@mozilla.org/network/udp-socket;1"
> 
> Bad this is under "network" namespace... I had to speak up during the
> review... it should be under "dom".
The nsIUDPSocket is still one of the interfaces provided by Necko, are you suggesting exposing this interface to DOM API directly? What I have in mind is providing a nsIDOMUDPSocket that have open/send/recv/close/event API, and the implementation of nsIDOMUDPSocket will choose between nsUDPSocket and UDPSocketChild based on the process type (chrome or content), pretty much like what we did in TCPSocket (bug 733573 and bug 770778).
Depends on: 870660
No longer depends on: 870660
Hi Shih-Chiang,

Pardon my intrusion but I've played a bit with these patches, and they look like an interesting step forward in bringing a more complete UDP support within Necko.

I noticed there has not been much activity on this bug for a month, is this still being considered for inclusion ?
(In reply to Florian Vallee from comment #10)
> Hi Shih-Chiang,
> 
> Pardon my intrusion but I've played a bit with these patches, and they look
> like an interesting step forward in bringing a more complete UDP support
> within Necko.
> 
> I noticed there has not been much activity on this bug for a month, is this
> still being considered for inclusion ?

I'm wrapping up further modification and doing performance measurement on it. The latest patch will be uploaded in these two days.
1. NrSocketIpc operation can only be invoked on main thread since ipdl IPC can only run on main thread.
2. nr_socket_ipc API will be invoked on STS thread and submit the NrSocketIpc operation to main thread.
3. content process will get received UDP message on main thread from ipdl IPC, therefore, we need to submit a job on STS thread to invoke the real nr_socket callbacks.
Attachment #760298 - Flags: review?(ekr)
Update patch according to review comments.
Attachment #760298 - Attachment is obsolete: true
Attachment #760298 - Flags: review?(ekr)
Attachment #764074 - Flags: review?(ekr)
Add DNS support.
Attachment #746860 - Attachment is obsolete: true
Attachment #764611 - Flags: review?(honzab.moz)
Attachment #764611 - Attachment description: WIP - Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v2 → Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v2
Attached patch Part 2 - ipdl for nsUDPSocket (obsolete) — — Splinter Review 
Honza, would you like to review the UDP socket e10s implementation?
Attachment #746864 - Attachment is obsolete: true
Attachment #746864 - Flags: feedback?(jduell.mcbugs)
Attachment #764614 - Flags: review?(honzab.moz)
Comment on attachment 764074 [details] [diff] [review]
Part 3, Bridge nr_socket to UDP e10s API, v2

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

I reviewed this on rietveld.

http://firefox-codereview.appspot.com/20001/



http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
File media/mtransport/nr_socket_prsock.cpp (right):

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:139: addPollFlags(flag);
Why not just directly frob mPollFlags

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:445: NrSocketIpc::NrSocketIpc() :
mPollFlags(0), udp_buffer(nullptr), mErr(false),
initializers go on the next line with a four-stop indent. See the Google style
guide.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:451: MOZ_ASSERT(false, "Failed to create
UDP socket in content process.");
Since this stuff can fail, let's move it to the create function so it can return
error.

Then abck these asserts up with an error return.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:465: uint32_t lineNumber,
convention for arguments is underscore, no camel case. Here and below.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:467: r_log_e(LOG_GENERIC,LOG_ERR,
NS_ConvertUTF16toUTF8(message).get());
Please do not call r_log_e() with random strings. Do:

r_log_e(..., "%s", ...)

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:472: NS_IMETHODIMP
NrSocketIpc::CallListenerArrayBuffer(const nsAString &type,
What thread is this called on?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:476: if (mState != NR_CONNECTED) {
How can this happen? Does it need an assert?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:481: moz_malloc(sizeof(nr_udp_message)));
Seems like new would be preferred in C++ code, no?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:483:
PR_StringToNetAddr(NS_ConvertUTF16toUTF8(host).get(), &msg->from);
This can fail. Check return value

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:484: PR_SetNetAddr(PR_IpAddrNull,
msg->from.raw.family, port, &msg->from);
And here.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:486: FallibleTArray<uint8_t>
fallibleArray;
Please use either RefPtr<DataBuffer> or nsAutoPtr<DataBuffer>

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:494:
mSTSThread->Dispatch(mozilla::WrapRunnable(this, &NrSocketIpc::recv_callback,
msg),
Let's use a reference counted object here to avoid the need for delete.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:500: ReentrantMonitorAutoEnter
mon(mMonitor);
Would a condition variable be simpler here?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:505:
mSocketChild->GetLocalAddress(address);
Can any of these fail?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:508:
PR_StringToNetAddr(NS_ConvertUTF16toUTF8(address).get(), &praddr);
Check for failure please on both these.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:530: }
Please check for other values of readyState than the expected ones. A
MOZ_ASSERT() is probably OK. What happens if we go to closed here?

Would it make more sense to invert the control discipline and look at the
readyState as the outer loop and then the mState inside that?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:538: break;
indent.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:551: default:
Maybe MOZ_CRASH()?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:561: // sock operation will perform on
main thread.
Please assert the thread.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:565: mMonitor.Wait();
Please check the state after this, since it might fail.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:576:
NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::sendto_int,
Is this going to work right in unit test?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:583:
NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::close_int));
Samehere.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:590: if (mState != NR_CONNECTED) {
MOZ_ASSERT?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:590: if (mState != NR_CONNECTED) {
Is there a race condition here between the state being updated.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:595: ABORT(R_IO_ERROR); //XXX other error
code?
How could this hapen?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:595: ABORT(R_IO_ERROR); //XXX other error
code?
Is this a "can't happen"

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:597: if
((r=nr_praddr_to_transport_addr(&udp_buffer->from,from,0))) {
space after ,

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:601: memcpy(buf,
udp_buffer->data->Elements(), udp_buffer->data->Length());
You need to check maxlen here.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:606: udp_buffer = nullptr;
If you use DataBuffer and smart pointers, this will be simpler.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:622: int r, _status, port;
Please put r, _status on their own lines.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:624: char addr_string[64];
Please move these closer to the use site?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:627: ABORT(R_INTERNAL);
Is this a can't happen? If so, please add a MOZ_ASSERT

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:630: if
((r=nr_transport_addr_get_addrstring(addr, addr_string, sizeof(addr_string)))) {
can't you just use .as_string?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:646:
nr_transport_addr_copy(&my_addr_,addr);
space after ,

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:671: if ((r=nr_transport_addr_get_port(to,
&port))) {
Can you refactor the addr handling so it's not duplicated?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:679: (uint16_t)port, (uint8_t*)msg, len)))
{
C++-style casts, please.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:694: mSocketChild->Close();
I note that this is a smart pointer. Does the destructor automatically close?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:699: void
NrSocketIpc::recv_callback(nr_udp_message *msg) {
Have this take a smart pointer

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:705: } else {
This seems to lose packets if you're not waiting for read. Isn't the right
answer to buffer up the data, not discard it.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:742: // create IPC bridge for content
process
Should we make this conditional on being Gonk?

I guess we're going to add E10S for desktop, but maybe until then? Thoughts?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.cpp:745: } else  {
Extra space between else {

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
File media/mtransport/nr_socket_prsock.h (right):

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.h:71: class NrSocketBase {
Why not have this inherit from nsASocketHandler?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.h:97: NS_IMETHOD_(nsrefcnt) Release(void) = 0;
Not needed if you inherit from nsASocketHandler, right?

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.h:134: virtual void addPollFlags(uint32_t
flags) {
I tend to just directly manipulate these, but regardless they should be in the
base class.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.h:193: void close_int();
Our style has changed here. Do _m (for main) instead of _int

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.h:195: void recv_callback(nr_udp_message
*msg);
recv_callback_m

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.h:196: private:
Whitespace.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.h:200: uint32_t mPollFlags;
Please use style that matches the existing code. Members are foo_, not mFoo.

http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc...
media/mtransport/nr_socket_prsock.h:200: uint32_t mPollFlags;
This shadows stuff that's in the base class.
Attachment #764074 - Flags: review?(ekr) → review-
How priority is this?  I am afraid I may not have time for weeks to do these reviews.
(In reply to Honza Bambas (:mayhemer) from comment #17)
> How priority is this?  I am afraid I may not have time for weeks to do these
> reviews.

This patch should land before Firefox 26 and I expect the review process for this patch will be long. Is it possible to get your first review comment within 2 weeks?
minor fix for using PUDPSocketParent after destroy.
Attachment #764614 - Attachment is obsolete: true
Attachment #764614 - Flags: review?(honzab.moz)
Attachment #769478 - Flags: review?(honzab.moz)
1. All review comments are addressed.
2. Fix for using memory buffer across threads.
Attachment #764074 - Attachment is obsolete: true
Attachment #769479 - Flags: review?(ekr)
Reviewed on Rietveld:

https://firefox-codereview.appspot.com/20001/#msg5


https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so...
File media/mtransport/nr_socket_prsock.cpp (right):

https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:630: if
((r=nr_transport_addr_get_addrstring(addr, addr_string, sizeof(addr_string)))) {
On 2013/06/21 16:40:13, ekr-webrtc wrote:
> On 2013/06/21 13:49:55, schien wrote:
> > On 2013/06/20 14:38:33, ekr-webrtc wrote:
> > > can't you just use .as_string?
> > 
> > .as_string contains more than ip address, e.g. "IP4:127.0.0.1:1234". You'll
> > still need to parse the string to get ip address.
> 
> OK. add a comment to this effect please.

Still don't see a comment.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.cpp (right):

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:202: mPollFlags = poll_flags();
Should you be setting these if r != 0, here and below.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:321: nsAString &host, int32_t &port) {
Intent to line up with (

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:321: nsAString &host, int32_t &port) {
Out parameters in this code should use: T *foo, not T & foo.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:480: NrSocketIpc::NrSocketIpc() :
err_(false), state_(NR_INIT),
Please fix style.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:489: close();
This has a lifetime problem: close() dispatches asynchronously to the main
thread, which means that the NrSocketIpc can have been destroyed when close_m
runs.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:499: MOZ_ASSERT(NS_IsMainThread());
The mtransport convention is to do

ASSERT_ON_THREAD(main_thread_) or some other variable that has it.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:520: MOZ_ASSERT(status == PR_SUCCESS);
Is it really not possible that these can fail in ordinary operation. If it is
possible, you should be handling the error. 

Generally, MOZ_ASSERTS should be backed up by real error handling and should
return status.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:528: nsIEventTarget::DISPATCH_NORMAL);
I don't think you need nsIEventTarget:: here.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:537: r_log_e(LOG_GENERIC,LOG_ERR, "Failed
to get local port");
Don't you need to return after this error here and below?

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:548: MOZ_ASSERT(status == PR_SUCCESS);
Above comments about MOZ_ASSERT

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:553: ReentrantMonitorAutoEnter
mon(monitor_);
Why do you need to lock this monitor before frobbing my_addr_?

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:565: if
(NS_FAILED(static_cast<nsresult>(value))) {
Do you need to grab the monitor before modifying err_. I see you are grabbing it
elsewhere.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:575: state_ = NR_CLOSED;
Is there a transition from NR_CONNECTING to NR_CLOSED?

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:618:
NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::create_m,
This is async, so I think you have a lifetime issue. If you pass an RefPtr<this>
then the problem should go away. Also check the other _m dispatches.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:636: nr_transport_addr *to) {
Indent one space to line up.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:684: nr_udp_message &msg =
received_msgs_.front();
Space before the conditional.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:709: }
In general, partial UDP reads should just drop the remaining data. If you don't
read the whole packet' it's not generally possible to do anything with the rest.

That will also make this code simpler.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:798: } else  {
Fix indent.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.h (right):

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:164: uint32_t begin;
ctors before member variables.

https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:167: nr_udp_message(const nr_udp_message
&other) {
Hmm... I have a couple of concerns here:
Because you are using nsAutoPtr here, when you copy-construct this, you are
destroying the source, which is disturbing when you are putting these in a
queue. Suggest that we replace this with RefPtr<>.

Alternately, have a std::queue<nr_udp_message *> and ditch the copy ctor.
Attachment #769479 - Flags: review?(ekr) → review-
Comment on attachment 764611 [details] [diff] [review]
Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v2

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

So honza can keep focused on the cache I'm going to steal this review and do the same (or delegate) the other patches.

r- just for the sync use of DNS

::: netwerk/base/public/nsIUDPSocket.idl
@@ +105,5 @@
> +    * @param data The buffer containing the data to be written.
> +    * @param dataLength The maximum number of bytes to be written.
> +    * @return number of bytes written. (may be less than dataLength)
> +    */
> +    unsigned long send(in DOMString host, in unsigned short port,

probly AUTF8String to match the dns service

::: netwerk/base/src/nsUDPSocket.cpp
@@ +53,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIDNSRecord> record;
> +  rv = dns->Resolve(NS_ConvertUTF16toUTF8(host), 0, getter_AddRefs(record));

you have to use the AsyncResolve

@@ +59,5 @@
> +    return rv;
> +  }
> +
> +  nsAutoCString address;
> +  rv = record->GetNextAddrAsString(address);

use GetNextAddr() to avoid string conversions

::: netwerk/test/Makefile.in
@@ +48,5 @@
>  
>  CPP_UNIT_TESTS = \
>  		 TestSTSParser.cpp \
>  		 TestUDPServerSocket.cpp \
> +                 TestUDPSocket.cpp \

did you mean to delete TestUDPServerSocket?
Attachment #764611 - Flags: review?(honzab.moz) → review-
Comment on attachment 769478 [details] [diff] [review]
Part 2 - ipdl for nsUDPSocket, v1.1

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

The basic outlines of this work the way I expect them to, but I don't have enough first hand experience with e10s to review it in detail. I'll steal it and look for a reviewer.

::: dom/network/src/UDPSocketChild.cpp
@@ +64,5 @@
> +                     uint16_t aPort)
> +{
> +  mSocket = aSocket;
> +  AddIPDLReference();
> +  //TODO(schien@mozilla.com) get PBrowser for permission check

I'm not clear on why pb is relevant here
Attachment #769478 - Flags: review?(honzab.moz) → review?(mcmanus)
jdm - jduell is on pto for 10 days or so.. would you be able to look at patch 2 here?
Flags: needinfo?(josh)
Attachment #769478 - Flags: review?(mcmanus) → review?(josh)
Yep.
Flags: needinfo?(josh)
Comment on attachment 764611 [details] [diff] [review]
Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v2

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

::: netwerk/base/public/nsIUDPSocket.idl
@@ +105,5 @@
> +    * @param data The buffer containing the data to be written.
> +    * @param dataLength The maximum number of bytes to be written.
> +    * @return number of bytes written. (may be less than dataLength)
> +    */
> +    unsigned long send(in DOMString host, in unsigned short port,

you should definitely also have a version that takes a nsNetAddr (or nsINetAddr to make more scriptable) so you can avoid doing a DNS service lookup on every send() when you're doing multiple sends over time to the same host
I think this is supposed to be an address, not a DNS name. If not, it's definitely wrong.

If so, it may just be unaesthetic :)
I'm adding a send API without DNS lookup because it'll introduce more delay for multiple sends. In WebRTC use cases, we can use this send-with-address interface because the address information is already available.
Update according to review comments
Attachment #769479 - Attachment is obsolete: true
Attachment #773207 - Flags: review?(ekr)
update according to review comment
Attachment #773207 - Attachment is obsolete: true
Attachment #773207 - Flags: review?(ekr)
Attachment #773209 - Flags: review?(ekr)
update according to review comments, including:
1. async DNS query
2. add send interface with nsINetAddr/NetAddr for skipping DNS query.
3. add nsINetAddr.getNetAddr() for converting nsINetAddr back to NetAddr.
Attachment #764611 - Attachment is obsolete: true
Attachment #775148 - Flags: review?(mcmanus)
Comment on attachment 775148 [details] [diff] [review]
WIP - Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v3

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

r=mcmanus if you feel ok about making the changes in this review.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +104,5 @@
> +     * @param host The remote host name.
> +     * @param port The remote port.
> +     * @param data The buffer containing the data to be written.
> +     * @param dataLength The maximum number of bytes to be written.
> +     * @return number of bytes written. (may be less than dataLength)

it will be 0 or datalength, right? That's kind of the point of a messaging interface vs a stream one.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +635,5 @@
> +
> +private:
> +  nsRefPtr<nsUDPSocket> mSocket;
> +  uint16_t mPort;
> +  InfallibleTArray<uint8_t> mData;

I think mData has to be fallible and error checked. Arbitrary inputs from JS are just potentially too big to allow to crash the browser on OOM when we aren't really low memory.

@@ +645,5 @@
> +PendingSend::OnLookupComplete(nsICancelable *request,
> +                              nsIDNSRecord  *rec,
> +                              nsresult       status)
> +{
> +  if (NS_SUCCEEDED(status)) {

an nspr log on failure would be useful

@@ +647,5 @@
> +                              nsresult       status)
> +{
> +  if (NS_SUCCEEDED(status)) {
> +    NetAddr addr;
> +    if (NS_SUCCEEDED(rec->GetNextAddr(mPort, &addr))) {

if you've got multiple A records on your hostname, the way this is written you will round robin between them.. I suspect that's not really what you want. You want two different sends to go to the same host unless there is an error or whatnot, right? If so you probably need to use rewind().

@@ +714,5 @@
> +  NS_ENSURE_ARG_POINTER(_retval);
> +
> +  PRNetAddr prAddr;
> +  NetAddrToPRNetAddr(aAddr, &prAddr);
> +

it would be nice to init *_retval to 0.. I know it only matters when you return NS_OK, but the practice has dodged bugs in the past.
Attachment #775148 - Flags: review?(mcmanus) → review+
I'm going to review this today; I'm sorry for forgetting about it for so long.
Comment on attachment 769478 [details] [diff] [review]
Part 2 - ipdl for nsUDPSocket, v1.1

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

There's a file missing right? No code in this patch actually implements nsIUDPSocketInternal, so I can't see the implementation of methods like callListenerArrayBuffer.

::: dom/network/interfaces/nsIUDPSocketChild.idl
@@ +16,5 @@
> +
> +  // Tell the chrome process to perform equivalent operations to all following methods
> +  void send(in DOMString host, in unsigned short port,
> +           [array, size_is(byteLength)] in uint8_t bytes,
> +           in unsigned long byteLength);

Nit: indent these one past the ( on the first line.

@@ +28,5 @@
> +interface nsIUDPSocketInternal : nsISupports
> +{
> +  void callListenerError(in DOMString type, in DOMString message, in DOMString filename,
> +                         in uint32_t lineNumber, in uint32_t columnNumber);
> +  void callListenerArrayBuffer(in DOMString type, in DOMString host, in unsigned short port, 

Nit: trailing whitespace.

::: dom/network/src/PUDPSocket.ipdl
@@ +27,5 @@
> +  uint16_t port;
> +};
> +
> +struct UDPReturnValue {
> +  uint32_t value;

This and the struct name could be more informative.

::: dom/network/src/UDPSocketChild.cpp
@@ +47,5 @@
> +  return refcnt;
> +}
> +
> +UDPSocketChild::UDPSocketChild()
> +:mLocalPort(0),mLocalAddress()

mLocalAddress doesn't look like it needs an initializer.

@@ +64,5 @@
> +                     uint16_t aPort)
> +{
> +  mSocket = aSocket;
> +  AddIPDLReference();
> +  //TODO(schien@mozilla.com) get PBrowser for permission check

You can probably follow what bug 890570 is doing instead and avoid having to obtain a PBrowser.

::: dom/network/src/UDPSocketChild.h
@@ +5,5 @@
> +#include "mozilla/net/PUDPSocketChild.h"
> +#include "nsIUDPSocketChild.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsCOMPtr.h"
> +

This needs a header guard.

::: dom/network/src/UDPSocketParent.h
@@ +4,5 @@
> +
> +#include "mozilla/net/PUDPSocketParent.h"
> +#include "nsCOMPtr.h"
> +#include "nsIUDPSocket.h"
> +

This needs a header guard.

::: netwerk/ipc/NeckoParent.cpp
@@ +355,5 @@
> +    printf_stderr("NeckoParent::AllocPUDPSocket: FATAL error: app doesn't permit udp-socket connections \
> +                   KILLING CHILD PROCESS\n");
> +    return nullptr;
> +  }
> +*/

Definitely follow bug 890570 here.
Attachment #769478 - Flags: review?(josh)
Update patch according to the interface modification in part 1.
Attachment #769478 - Attachment is obsolete: true
Attachment #776111 - Flags: review?(josh)
(In reply to Josh Matthews [:jdm] from comment #34)
> Comment on attachment 769478 [details] [diff] [review]
> Part 2 - ipdl for nsUDPSocket, v1.1
> 
> Review of attachment 769478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's a file missing right? No code in this patch actually implements
> nsIUDPSocketInternal, so I can't see the implementation of methods like
> callListenerArrayBuffer.
> 
The implementation of nsIUDPSocketInternal is in pr_nr_socket_prsock.cpp in part 3, NrSocketIpc implement this callback interface.
Comment on attachment 776111 [details] [diff] [review]
Part 2 - ipdl for nsUDPSocket, v2

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

I don't understand. Does UDPSocketInternalImpl only exist in a test? Where is the implementation that exists for regular b2g?
(In reply to Josh Matthews [:jdm] from comment #37)
> Comment on attachment 776111 [details] [diff] [review]
> Part 2 - ipdl for nsUDPSocket, v2
> 
> Review of attachment 776111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand. Does UDPSocketInternalImpl only exist in a test? Where
> is the implementation that exists for regular b2g
NrSocketIpc in the patch part 3 implements nsUDPSocketInternal, which is only used in WebRTC. Currently I don't plan to provide a general implementation of nsUDPsocketInternal for using in other XPCOM and WebAPI. Are you suggesting to include it in this patch?
Comment on attachment 773209 [details] [diff] [review]
Part 3, Bridge nr_socket to UDP e10s API, v4

I reviewed this on Rietveld...

This is looking pretty good. I'm still a little concerned about how you're
handling errors. Shouldn't a lot of these have some negative side effect....

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.cpp (right):

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:269: static int
nr_transport_addr_to_netaddr(nr_transport_addr *addr,
Use PRNetAddrToNetAddr here instead of duplicating the code.

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:539:
NrSocketIpc::NrSocketIpc(nsCOMPtr<nsIEventTarget> main_thread)
const nsCOMPtr<nsIEventTarget>&

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:574: return NS_OK;
Is NS_OK what we really want here? The assert suggests that this is a can't
happen. Does that mean we want to return some error? Maybe set an error flag?

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:766: ReentrantMonitorAutoEnter
mon(monitor_);
Do this at the top.

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:882: sock = new
NrSocketIpc(main_thread.forget());
Why are you doing forget here?

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.h (right):

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:162: struct nr_udp_message : public
RefCounted<nr_udp_message> {
I typically use the NS_INLINE_DECL_ macros

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:164: nsAutoPtr<DataBuffer> data;
Use private:DISALLOW_COPY_ASSIGN so this can't be assigned or copy-constructed.

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:174: NrSocketIpc(nsCOMPtr<nsIEventTarget>
main_thread);
const nsCOMPtr<nsIEventTarget>&

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:175: virtual ~NrSocketIpc() { };
{}

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:188: void create_m(nsACString &host,
uint16_t port);
Should this be const?

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:189: void sendto_m(net::NetAddr &addr,
nsAutoPtr<DataBuffer> buf);
Const?

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:193: private:
Space before private:

https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:199: std::queue< RefPtr<nr_udp_message> >
received_msgs_;
<Ref not < Ref
Attachment #773209 - Flags: review?(ekr) → review-
Attached patch review_update_nr_socket.patch (obsolete) — — Splinter Review 
Here is the additional modification on top of patch part 3 that address the latest review comments in comment 39. But I encounter a link error on firefox build in nr_sock_prsock.cpp complaining about the symbol of PRNetAddrToNetAddr is undefined.
Attachment #778220 - Flags: feedback?(ekr)
Update according to review comment.

1. The return value of send function depends on the behavior of POSIX send. It doesn't have the guarantee success on sending entire packet. http://linux.die.net/man/2/send
2. The nsIDNSRecord doesn't need to rewind because the iterator of each nsIDNSRecord is independent and new instance of nsIDNSRecord is created for each OnLookupComplete invocation. http://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp?from=nsDNSRecord#l288
Attachment #775148 - Attachment is obsolete: true
Attachment #779632 - Flags: review?(mcmanus)
Attached image architecture — 
The architecture of UDP socket e10s
update according to previous review comment.

The nsIUDPSocketInternal is implemented by NrSocketIpc currently, the general UDP Socket for content process will be done later in bug 745283. (see https://bugzilla.mozilla.org/attachment.cgi?id=779708 for the design and plan)
Attachment #776111 - Attachment is obsolete: true
Attachment #776111 - Flags: review?(josh)
Attachment #779709 - Flags: review?(josh)
update according to review comment.
Using PRNetAddrToNetAddr in nr_sock_prsock.cpp will cause a link error in firefox build and I cannot figure out how to resolve it. So, that review comment is left out in this patch.
Attachment #773209 - Attachment is obsolete: true
Attachment #779710 - Flags: review?(ekr)
Comment on attachment 779709 [details] [diff] [review]
Part 2 - ipdl for nsUDPSocket, v3

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

::: dom/network/interfaces/nsIUDPSocketChild.idl
@@ +44,5 @@
> +  void callListenerError(in AUTF8String type, in AUTF8String message, in AUTF8String filename,
> +                         in uint32_t lineNumber, in uint32_t columnNumber);
> +  void callListenerArrayBuffer(in AUTF8String type, in AUTF8String host, in unsigned short port,
> +                               [array, size_is(dataLength)] in uint8_t data,
> +                               in unsigned long dataLength);

This should probably be named something else, since ArrayBuffer is a specific type that's not being used here.

::: dom/network/src/UDPSocketChild.cpp
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +NS_IMPL_THREADSAFE_ISUPPORTS1(UDPSocketChildBase, nsIUDPSocketChild)

This doesn't need to be threadsafe, does it?

::: dom/network/src/UDPSocketParent.cpp
@@ +222,5 @@
> +
> +NS_IMETHODIMP
> +UDPSocketParent::OnStopListening(nsIUDPSocket* aSocket, nsresult aStatus)
> +{
> +  // underlying socket is dead, send error to child process

We're not sending an error, are we?

::: dom/network/tests/unit_ipc/udpsocket_child.js
@@ +162,5 @@
> +function run_test() {
> +  run_next_test();
> +  do_timeout(1000, function() {
> +    do_throw("UDP Test case timeout");
> +  });

Let's get rid of this timeout.
Attachment #779709 - Flags: review?(josh) → review+
Comment on attachment 779632 [details] [diff] [review]
Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v4

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

sorry about the rewind() red herring in my last review - you're right.

r=mcmanus if we agree on the return definition change.

::: netwerk/base/public/nsIUDPSocket.idl
@@ +104,5 @@
> +     * @param host The remote host name.
> +     * @param port The remote port.
> +     * @param data The buffer containing the data to be written.
> +     * @param dataLength The maximum number of bytes to be written.
> +     * @return number of bytes written. (may be less than dataLength)

for a messaging API this really needs to be 0 or datalength. That's the definition of a messaging api vs a streaming one, and UDP is definitiely a messaging API. I don't know how I would even use such an API if it really could send part of my message. (e.g. if I was sending a DNS query and it returned the first half of the message was sent - what would I do then as a user of the API? Send the other half in a new UDP message? That's not DNS compliant.)

send() does indeed provide those semantics when using SOCK_DGRAM sockets. (It does something different with SOCK_STREAM sockets, which is why the overall send() manpage describes the looser behavior).
Attachment #779632 - Flags: review?(mcmanus) → review+
(In reply to Patrick McManus [:mcmanus] [afk until Aug 09] from comment #46) 
> send() does indeed provide those semantics when using SOCK_DGRAM sockets.
> (It does something different with SOCK_STREAM sockets, which is why the
> overall send() manpage describes the looser behavior).
Got it. I'm happy to modify the comment based on this statement.
update according to review comment, carry r+
Attachment #779632 - Attachment is obsolete: true
Attachment #783001 - Flags: review+
update according to review comment
Attachment #779709 - Attachment is obsolete: true
Attachment #783002 - Flags: review?(josh)
update according to review comment
Attachment #779710 - Attachment is obsolete: true
Attachment #779710 - Flags: review?(ekr)
Attachment #783003 - Flags: review?(ekr)
Attachment #778220 - Attachment is obsolete: true
Attachment #778220 - Flags: feedback?(ekr)
Comment on attachment 783001 [details] [diff] [review]
Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v5

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

::: netwerk/test/TestUDPSocket.cpp
@@ +240,5 @@
> +  // Read response from server
> +  NS_ENSURE_SUCCESS(clientListener->mResult, -1);
> +
> +  mozilla::net::NetAddr clientAddr;
> +  rv = client->GetAddress(&clientAddr);

The clientAddr.inet.ip will be 0 after returning from GetAddress, which will cause following SendWithAddress fail on Windows platform.
update TestUDPSocket since cppunittest fail on try server, listen to loopback address will give you the correct address of 127.0.0.1 from nsUDPSocket::GetAddress
Attachment #783001 - Attachment is obsolete: true
Attachment #784760 - Flags: review+
Attachment #783002 - Flags: review?(josh) → review+
minor update for creating instance of UDPSocketChild in main thread since UDPSocketChild is non-thread-safe and can only be used in main thread.
Attachment #783003 - Attachment is obsolete: true
Attachment #783003 - Flags: review?(ekr)
Attachment #785703 - Flags: review?(ekr)
Comment on attachment 783002 [details] [diff] [review]
Part 2 - ipdl for nsUDPSocket, v4

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

::: netwerk/ipc/NeckoParent.cpp
@@ +348,5 @@
> +NeckoParent::AllocPUDPSocketParent(const nsCString& aHost,
> +                                   const uint16_t& aPort)
> +{
> +  if (UsingNeckoIPCSecurity() &&
> +      !AssertAppProcessPermission(Manager(), "udp-socket")) {

I discoverd that using AssertAppProcessPermission will only allow packaged/certified app to grant the permission. Web pages loaded from browser app cannot asking for the "udp-socket" permission. Do we have any way for enabling WebRTC from web page under this permission check? Or maybe I can change this to |AssertAppHasPermission| and grant "udp-socket" permission in browser app?
Comment on attachment 785703 [details] [diff] [review]
Part 3, Bridge nr_socket to UDP e10s API, v6.1

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

I reviewed this on Rietveld:

I went through the whole bug and found a few more issues.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.cpp (right):

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:283: }
This needs to be able to fail if the family is something else.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:363: nsACString *host, int32_t *port) {
Does this compile properly in the unit tests? I remember having string problems.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:542: r_log_e(LOG_GENERIC,LOG_ERR, "UDP
socket error:%s", message.BeginReading());
Please change all the r_log_es in this file to r_log. We shouldn't be trying to
read errno.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:569: if (PR_SUCCESS !=
PR_SetNetAddr(PR_IpAddrNull, addr.raw.family, port, &addr)) {
I think a comment here about what Null does would be appropriate. I had to look
it up.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:586: NS_IMETHODIMP
NrSocketIpc::CallListenerVoid(const nsACString &type) {
Maybe this could be renamed to better match what it does?

CallListenerOnOpen?

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:762: mozilla::RefPtr<nr_udp_message> &msg
= received_msgs_.front();
What happens if this is empty? I bet a crash.

you should be checking for empty first.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:769: ABORT(R_IO_ERROR); //XXX other error
code?
Probably R_WOOULDBLOCK

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:788: ASSERT_ON_THREAD(sts_thread_);
Check the state to make sure we have gotten the response.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:803:
NS_ENSURE_SUCCESS_VOID(socket_child_->Bind(this, host, port));
How do we handle errors in these? Shouldn't we be setting err_ here and below?

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:867: static bool IsChromeProcess() {
Is there a reason for this extra fxn since you only use it once?

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.h (right):

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:116: memset(&my_addr_, 0, sizeof(my_addr_));
Can we put my_addr_ into the base? It seems to be common.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:162: };
Let's put this in NrSocketIpc

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:165: nr_udp_message() {}
Either make this initialize sensibly or remove it.

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:197: void create_m(const nsACString &host,
const uint16_t port);
Can these be private?
Attachment #785703 - Flags: review?(ekr) → review-
update according to review comments
Attachment #785703 - Attachment is obsolete: true
Attachment #788073 - Flags: review?(ekr)
Comment on attachment 788073 [details] [diff] [review]
Part 3, Bridge nr_socket to UDP e10s API, v7

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

Reviewed on Rietveld:
https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.cpp (right):

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:586: NS_IMETHODIMP
NrSocketIpc::CallListenerVoid(const nsACString &type) {
On 2013/08/09 10:13:41, schien wrote:
> On 2013/08/08 22:26:10, ekr-webrtc wrote:
> > Maybe this could be renamed to better match what it does?
> > 
> > CallListenerOnOpen?
> 
> This is actually used as a callback from parent process without any parameter.
> "onopen" and "onclose" events are using this callback. I'd like to keep the
name
> Void for identifying callback an event without attribute.

I don't think the void is a problem, but i'd like to label it some way that
explains what the purpose is

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.h (right):

https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:116: memset(&my_addr_, 0, sizeof(my_addr_));
On 2013/08/09 10:13:41, schien wrote:
> On 2013/08/08 22:26:10, ekr-webrtc wrote:
> > Can we put my_addr_ into the base? It seems to be common.
> 
> In google coding style data member should be private. Do you suggest to move
> my_addr_ to NrSocketBase as a protected member?

That was what I had in mind, though I see it's banned by Google style. I think
this is OK to deviate from.

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.cpp (right):

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:308: praddr_to_netaddr(&praddr, naddr);
Check return value here....

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:581: // Use PR_IpAddrNull to avoid address
been reset to 0.
s/been/being/

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:589: nsAutoPtr<DataBuffer> buf(new
DataBuffer(data, data_length));
Let's move the construction of the udp_message here so that that way we don't
need to do a copy of the addr.

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:601: if (type.EqualsLiteral("onopen")) {
Can anything else besides onopen happen here? If so, what does it mean?

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:682: MOZ_ASSERT(false, "Failed to create
STS thread");
s/create/get/

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:691: if
((r=nr_transport_addr_copy(&my_addr_,addr))) {
space here after comma.

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:703: mon.Wait();
Add a comment here to the effect that this is actually waiting for open to
succed.

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:763: std::swap( received_msgs_, empty);
no space after (

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:783: {
What is the block doing here?

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:786: if
((r=nr_praddr_to_transport_addr(&msg->from, from, 0))) {
Should set err_ here?

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:795: received_msgs_.pop();
Pop right after .front() so that address errors don't leave us in a borked
state.

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.cpp:839: buf->data(),
Check for error and maybe set err_

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
File media/mtransport/nr_socket_prsock.h (right):

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:113: , public nsASocketHandler {
Convention here is to have the comma at the end of the previous line.

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:157: nr_udp_message(const PRNetAddr &from,
nsAutoPtr<DataBuffer> &data) {
please us an initializer here.

https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s...
media/mtransport/nr_socket_prsock.h:172: , public nsIUDPSocketInternal {
,s at end please.
Attachment #788073 - Flags: review?(ekr) → review-
update according to review comment.
Attachment #788073 - Attachment is obsolete: true
Attachment #788857 - Flags: review?(ekr)
minor update: use thread-safe refcount in nr_udp_message since the reference is passed between main thread and STS thread.
Attachment #788857 - Attachment is obsolete: true
Attachment #788857 - Flags: review?(ekr)
Attachment #788902 - Flags: review?(ekr)
Comment on attachment 788902 [details] [diff] [review]
Part 3, Bridge nr_socket to UDP e10s API, v8.1

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

Reviewed on Rietveld.

This is looking good.

I don't see any tests, however. Can you tell me what you did to test this?

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
File media/mtransport/nr_socket_prsock.cpp (right):

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:376: int
nr_transport_addr_get_addrstring_and_port(nr_transport_addr *addr,
Please add a comment about the form this comes out in.

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:550: NS_IMETHODIMP
NrSocketIpc::CallListenerError(const nsACString &type,
Please add comments indicating what each of these fxns does.

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:555: ASSERT_ON_THREAD(main_thread_);
Please mark the unused values as unused.

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:565: NS_IMETHODIMP
NrSocketIpc::CallListenerReceivedData(const nsACString &type,
What is type here and below. Please either check or mark unused.

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:634: if
(nr_praddr_to_transport_addr(&praddr, &my_addr_, 1)) {
Check that the non-port part of my_addr_  is as expected.

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:666: state_ = NR_CLOSED;
Please also have checks/asserts for the other states.

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:710: // Waiting until socket creation
complete
s/Waiting/Wait/ and . at end

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.cpp:805: consumed_len = std::min(maxlen,
msg->data->len());
Please log if we discarded data.

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
File media/mtransport/nr_socket_prsock.h (right):

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.h:96: NS_IMETHOD_(nsrefcnt) AddRef(void) = 0;
Please add a comment here.

http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so...
media/mtransport/nr_socket_prsock.h:159: data(data) {
These can go on one line.
Attachment #788902 - Flags: review?(ekr)
(In reply to Eric Rescorla (:ekr) from comment #61)
> 
> I don't see any tests, however. Can you tell me what you did to test this?
> 
I do integration test via http://people.mozilla.org/~schien/data_test.html, which is a Data channel test page. With this test page, I can create/close NrSocketIpc via PeerConnection and also break the network connection by turning off the Wi-Fi.
update according to review comment, test case will be put in patch part 4.
Attachment #788902 - Attachment is obsolete: true
Attachment #790718 - Flags: review?(ekr)
minor update according to review comment
Attachment #790718 - Attachment is obsolete: true
Attachment #790718 - Flags: review?(ekr)
Attachment #791157 - Flags: review?(ekr)
Comment on attachment 791157 [details] [diff] [review]
Part 3, Bridge nr_socket to UDP e10s API, v9.1

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

Holding review of this till we have a test set. Please re-ask for review once the tests are done and pass.
Attachment #791157 - Flags: review?(ekr)
This test case will execute all PeerConnection mochitest cases (under dom/media/tests/mochitest/) in a remote browser iframe. Therefore, it'll use NrSocketIpc to do the socket operations.
Attachment #795233 - Flags: review?(ekr)
Potential deadlock is found in NrSocketIpc by mochitest, I'm trying to resolve it.
The potential deadlock pattern is invoking sendto in the same call stack of invoking socket receive callback. By preventing holding lock while invoking receive callback, the deadlock warning is disappeared.
Attachment #791157 - Attachment is obsolete: true
Attachment #795387 - Flags: review?(ekr)
Comment on attachment 795387 [details] [diff] [review]
Part 3, Bridge nr_socket to UDP e10s API, v10

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

Please add a comment indicating why you are releasing the lock.
Attachment #795387 - Flags: review?(ekr) → review+
Attachment #795233 - Flags: review?(ted)
Attachment #795233 - Flags: review?(jsmith)
Comment on attachment 795233 [details] [diff] [review]
Part 4, test cases for NrSocketIpc

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

I am unqualified to review this.

::: dom/media/tests/ipc/test_ipc.html
@@ +14,5 @@
> +
> +    // This isn't a single test, really... It runs the entirety of the IndexedDB
> +    // tests. Each of those has a normal timeout handler, so there's no point in
> +    // having a timeout here. I'm setting this really high just to avoid getting
> +    // killed.

Are you still running the IndexedDB tests?
Attachment #795233 - Flags: review?(ekr)
Comment on attachment 795233 [details] [diff] [review]
Part 4, test cases for NrSocketIpc

I don't know much about OOP mochitests, so I'm not a good reviewer for this.

Let's start with Ted to see what he thinks.
Attachment #795233 - Flags: review?(jsmith)
(In reply to Eric Rescorla (:ekr) from comment #70)
> Comment on attachment 795233 [details] [diff] [review]
> Part 4, test cases for NrSocketIpc
> 
> Review of attachment 795233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am unqualified to review this.
> 
> ::: dom/media/tests/ipc/test_ipc.html
> @@ +14,5 @@
> > +
> > +    // This isn't a single test, really... It runs the entirety of the IndexedDB
> > +    // tests. Each of those has a normal timeout handler, so there's no point in
> > +    // having a timeout here. I'm setting this really high just to avoid getting
> > +    // killed.
> 
> Are you still running the IndexedDB tests?
This is a stupid copy & paste error, I'll replace the wording in next revision.
Attachment #784760 - Attachment description: udp-socket-rename.patch → Part 1, rename nsUDPServerSocket to nsUDPSocket
minor update: add initializer for NrSocketBase::poll_flags_ in the ctor, which fix several intermittent test case fail in PeerConnection.
Attachment #795387 - Attachment is obsolete: true
Attachment #796530 - Flags: review?(ekr)

        Attachment #795233 -
        Attachment is obsolete: true

        Attachment #795233 -
        Flags: review?(ted)

        Attachment #796532 -
        Flags: review?(ted)

    
  

        Attachment #796530 -
        Flags: review?(ekr) → review+

    
    

    
  
Found crash at mozilla::layers::BasicTiledLayerTile::OpenDescriptor() on Android while running PeerConnection IPC mochitest. File bug 910661 for followup.

    
    

    
  
Comment on attachment 796532 [details] [diff] [review]
Part 4, test cases for NrSocketIpc

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

The build parts of this are fine. I don't feel qualified to review the test, I don't understand the guts of our e10s well enough right now to do so.

::: dom/media/tests/ipc/Makefile.in
@@ +15,5 @@
> +  $(NULL)
> +
> +
> +include $(topsrcdir)/config/rules.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk

Pretty sure you don't need this line.

::: dom/media/tests/ipc/test_ipc.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for OOP PeerConncection</title>

nit: typo

        Attachment #796532 -
        Flags: review?(ted)

    
  

        Attachment #796532 -
        Flags: review?(bent.mozilla)

    
    

    
  
Comment on attachment 796532 [details] [diff] [review]
Part 4, test cases for NrSocketIpc

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

Is this somehow different from dom/indexedDB/ipc/test_ipc.html ? How exactly?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test_ipc diffs (obsolete)
        — 
          — Splinter Review
      

    


  
bent: here are the diffs against test_ipc.html

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test-ipc.patch (obsolete)
        — 
          — Splinter Review
      

    


  
The test_ipc.html is borrow from dom/devicestorage/ipc/test_ipc.html with following modifications:

1. change the wording from DeviceStorage to PeerConnection
2. extend the IPC test timeout like what we did in dom/indexDB/ipc/test_ipc.html
3. change the iframe.src to /mochitest because the mochitest for PeerConnection is under dom/media/tests/mochitest/
4. remove the prefs for device storage testing.

        Attachment #800482 -
        Attachment is obsolete: true

        Attachment #800821 -
        Flags: feedback?(bent.mozilla)

    
    

    
  
Comment on attachment 800821 [details] [diff] [review]
test-ipc.patch

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

This looks fine.

        Attachment #800821 -
        Flags: feedback?(bent.mozilla) → feedback+

        Attachment #796532 -
        Flags: review?(bent.mozilla) → review+

    
    

    
  
We should implement a more generic ipc testing framework at some point so we don't have to keep copying this file everywhere.

    
    

    
  


  
Merge the modification for nsUDPSocket::SendWithAddress() from bug 901805 for reducing bug dependency. carry r+.

        Attachment #784760 -
        Attachment is obsolete: true

        Attachment #805243 -
        Flags: review+

    
    

    
  


  
According to bug 901810 comment 9, we don't need to grant permission for using UDP socket in WebRTC.
@jdm, do you think it is ok for simply removing "udp-socket" permission check from NeckoParent.cpp?

        Attachment #783002 -
        Attachment is obsolete: true

        Attachment #805245 -
        Flags: review?(josh)
No longer blocks: 901805

    
    

    
  
Bug 745283 looks fairly active, so I don't see any reason to remove code that we're just going to be re-adding when we get a content-accessible UDPSocket API.

    
    

    
  
Yes, the code will be the same after bug 745283 is landed. What I'm thinking is to move the permission check to the patch of bug 745283, so that we can understand that "udp-socket" permission is specific for mozUDPSocket from the check-in history.

    
    

    
  
Comment on attachment 805245 [details] [diff] [review]
Part 2 - ipdl for nsUDPSocket, v5

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

That sounds ok to me.

        Attachment #805245 -
        Flags: review?(josh) → review+

    
    

    
  
Note that we'll want to have per-app traffic monitoring of UDP traffic as part of this bug (or at least as part of a bug that lands at the same time). See bug 784575 and bug 855951 for what I'm talking about.

    
    

    
  
I'm ready to go ahead with bug 745283 when this lands.
Blocks: 923363

    
    

    
  
(In reply to Jason Duell (:jduell) from comment #88)
> Note that we'll want to have per-app traffic monitoring of UDP traffic as
> part of this bug (or at least as part of a bug that lands at the same time).
> See bug 784575 and bug 855951 for what I'm talking about.

Jason,

SC is kind of backed up and I'd like to get this landed soon so we can
get some experience with performance) in parallel with adding traffic
monitoring. Is there a way we can land this now and backfill the stats?

    
  
Blocks: 870660

    
    

    
  
(In reply to Eric Rescorla (:ekr) from comment #90)
> (In reply to Jason Duell (:jduell) from comment #88)
> > Note that we'll want to have per-app traffic monitoring of UDP traffic as
> > part of this bug (or at least as part of a bug that lands at the same time).
> > See bug 784575 and bug 855951 for what I'm talking about.
> 
> Jason,
> 
> SC is kind of backed up and I'd like to get this landed soon so we can
> get some experience with performance) in parallel with adding traffic
> monitoring. Is there a way we can land this now and backfill the stats?

need-info to @jduell
Flags: needinfo?(jduell.mcbugs)

    
  
blocking-b2g: --- → 1.3+
Whiteboard: [ft:webrtc]

    
    

    
  
I'm totally fine with landing this w/o network stats for testing, etc, on phones that aren't shipped to real users.  I suspect we wouldn't want to ship w/o it to real users, but that wouldn't be my call to make.
Flags: needinfo?(jduell.mcbugs)

    
    

    
  
WebRTC on B2G will be shipped to real users until Firefox OS v1.3 (gecko28). There will be no problem if the network stats is ready before gecko28 is released. I can make this into my v1.3 working queue.

    
    

    
  


  
rebase to m-c tip with following change, carry r+
1. pref-on to use UDP socket in content process, will remove the pref after packet filter is ready

        Attachment #805245 -
        Attachment is obsolete: true

    
    

    
  


  
rebase to m-c tip

        Attachment #796530 -
        Attachment is obsolete: true

        Attachment #828435 -
        Flags: review+

    
    

    
  


  
rebase to m-c with following modification, carry r+
1. disable dom/media ipc testcase on Android because bug 910661
2. disable dom/media ipc testcase on B2G desktop and emulator because nested content process is not available.

        Attachment #796532 -
        Attachment is obsolete: true

        Attachment #828437 -
        Flags: review+

        Attachment #800821 -
        Attachment is obsolete: true

    
    

    
  
Bug 935838 is the follow-up bug for per-app UDP traffic metering.

    
    

    
  
Set pref "media.peerconnection.ipc.enabled" to true in prefs.js can turn on UDP e10s for testing.

    
    

    
  
Hi people!

Standard8 thinks Part 1 caused Thunderbird Bug 936801. Who knows about winsock stuff?

http://logbot.glob.com.au/?c=mozilla%23maildev&s=8+Nov+2013&e=8+Nov+2013#c111226

11:33	RattyAway	https://tbpl.mozilla.org/php/getP...p;tree=Thunderbird-Trunk#error0
11:34	RattyAway	Standard8: sorry to bother you but a clobber didn't fix this error https://tbpl.mozilla.org/php/getP...p;tree=Thunderbird-Trunk#error0
11:35	Standard8	*ouch
11:36	Standard8	RattyAway: I guess some windows includes have changed recently
11:36	Standard8	RattyAway: got a regression range?
11:37	RattyAway	Standard8: sorry having dinner plus need to do things tonight
11:41	* Standard8 suspects http://hg.mozilla.org/mozilla-central/rev/4021b7b381f1#l3.12
11:45	Standard8	looks like nsLDAPConnection.cpp has both winsock.h and winsock2.h includes, that are probably conflicting
11:45	Standard8	I don't know how to fix that without experimenting, and I don't really have a windows build atm

    
    

    
  
I saw DNS.h only include winsock2.h while XP_WIN is defined. Is there any unsynchronized MACRO usage between Firefox and Thunderbird?
http://dxr.mozilla.org/mozilla-central/source/netwerk/dns/DNS.h#l17

    
    

    
  
Just add a patch that uses forward declaration instead of include DNS.h directly in nsINetAddr.idl. However I need someone help me verifying it.
https://bug936801.bugzilla.mozilla.org/attachment.cgi?id=829816

    
    

    
  
Comment on attachment 828429 [details] [diff] [review]
Part 1 - rename nsUDPServerSocket to nsUDPSocket and add |send| interface, v6

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

Found some problems while updating an extension.

::: dom/push/src/PushService.jsm
@@ +1433,5 @@
>        debug("UDP support disabled");
>        return;
>      }
>  
> +    this._udpServer = Cc["@mozilla.org/network/socket-udp;1"]

This should be Cc["@mozilla.org/network/udp-socket;1"]

::: netwerk/test/moz.build
@@ -52,5 @@
>  ]
>  
>  CPP_UNIT_TESTS += [
>      'TestSTSParser.cpp',
> -    'TestUDPServerSocket.cpp',

This file is still in the tree

    
    

    
  
(In reply to Hector Zhao [:hectorz] from comment #106)
@hectorz, thanks for pointing these out. I just filed bug 938492 for fixing these up.

    
  
Depends on: 936801
Depends on: 941985

    
    

    
  
Does this only affect B2G? If not can someone please advise QA on a testing strategy? This seems like something we might want tested.
Flags: needinfo?(schien)

    
    

    
  
I would say B2G is the only platform that end user can directly access this feature but this feature are not bound to B2G.
The UDP socket e10s can only be used with WebRTC right now because of the packet filter policy we imposed in bug 870660. So, the only way to test it without modifying is by testing WebRTC on a remote iframe, which I already done in the patch part 4 of this bug. Or, you can add a test preference for by passing the packet filter. You can then re-enable the xpcshell ipc test case I add in the patch part 2, i.e. test_udpsocket_ipc.js, and extend it to cover more scenarios.
Flags: needinfo?(schien)

    
    

    
  
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #108)
> Does this only affect B2G? If not can someone please advise QA on a testing
> strategy? This seems like something we might want tested.

I'd suggest sending this bug over to Nils for QA - this falls under his expertise in WebRTC networking.

As comment 109 states, end developer & user impact on production builds would hit FxOS right now, not other platforms.

    
    

    
  
Nils, is this something you can tackle?
Flags: needinfo?(drno)

    
    

    
  
I'm not sure I have enough context yet to understand the ticket and what I need to do for this ticket.
If someone can explain the missing context to me then I'll give it a shot. Otherwise we might want to find someone who can solve this faster then me.

    
    

    
  
(In reply to Nils Ohlmeier [:drno] from comment #112)
> I'm not sure I have enough context yet to understand the ticket and what I
> need to do for this ticket.
> If someone can explain the missing context to me then I'll give it a shot.
> Otherwise we might want to find someone who can solve this faster then me.

What we should probably do here is get a contextual understanding from Shih-Chiang on how this feature is triggered when doing P2P calls in Firefox OS.

Shih-Chiang - In the context of hitting the P2P API, how would this feature be triggered in Firefox OS? Is this just a fundamental piece always triggered when doing P2P calls on Firefox OS apps & browser tabs, as they run out of process? I'm looking to understand this more from a end developer/user workflow on Firefox OS. That will help us understand what to watch out for when testing P2P on Firefox OS.
Flags: needinfo?(schien)

    
    

    
  
Yes, UDP e10s will be triggered once we use the PeerConnection API out in OOP mode. Browsing the data channel demo page on Firefox OS is one of the simplest examples for testing it.
http://mozilla.github.io/webrtc-landing/data_test.html
Flags: needinfo?(schien)
Blocks: b2g-webrtc

    
    

    
  
I think I understand now enough of the context. But I don't have a Firefox OS device yet. And so far I failed to get the emulator running on my Mac OSX Mavericks.
But if the emulator problems get solved the regular Mochitests should exercise this functionality. 

As I'll be back in the office on the 13th I can't guarantee to get to this early enough.
Flags: needinfo?(drno)

    
    

    
  
(In reply to Nils Ohlmeier [:drno] from comment #115)
> I think I understand now enough of the context. But I don't have a Firefox
> OS device yet. And so far I failed to get the emulator running on my Mac OSX
> Mavericks.
> But if the emulator problems get solved the regular Mochitests should
> exercise this functionality. 

Note - this will only be true if mochitests run out of process, not in process. I don't think we have out of process support for emulators yet, although that's being worked on right now.

> 
> As I'll be back in the office on the 13th I can't guarantee to get to this
> early enough.

I'll help out as needed for this. When you get back, I'll work with you to get you a device to be able to test this.

    
    

    
  
(In reply to Jason Smith [:jsmith] from comment #116)
> Note - this will only be true if mochitests run out of process, not in
> process. I don't think we have out of process support for emulators yet,
> although that's being worked on right now.
Mochitest on emulator runs in OOP mode. Marionette is the one that not in OOP mode now and somebody is working on it.

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] from comment #117)
> (In reply to Jason Smith [:jsmith] from comment #116)
> > Note - this will only be true if mochitests run out of process, not in
> > process. I don't think we have out of process support for emulators yet,
> > although that's being worked on right now.
> Mochitest on emulator runs in OOP mode. Marionette is the one that not in
> OOP mode now and somebody is working on it.

Are you sure about that? I had no knowledge of our mochitests running in try in emulators running OOP.

    
    

    
  
(In reply to Jason Smith [:jsmith] from comment #118)
> Are you sure about that? I had no knowledge of our mochitests running in try
> in emulators running OOP.
Yes, I'm pretty sure about that. The mochitest case are ran inside a remote iframe. https://github.com/mozilla-b2g/gaia/blob/master/test_apps/test-container/index.html#L16

    
    

    
  
What I don't understand is why `nsIUDPMessage::data` has the type `ACString` and not a Uint8Array compatible type like the newly introduced (and awesome!) `nsIUDPSocket::send` and `nsIUDPSocket::sendWithAddr` methods support.

This asymmetric API will be the cause of confusion in the near future and might hurt adoption of this functionality; you send binary data and the receiving end always gets a string, which may yield surprising results!

For all intents and purposes, can't we introduce something like `nsIUDPMessage::rawData`, if backward compatibility is at stake, or any other way of exposing raw, binary, socket data? Text decoding/ encoding is something that should be deferred to API consumers, IMHO, which is already implied as a necessary step in `nsIUDPSocket::send` and `nsIUDPSocket::sendWithAddr`.
Flags: needinfo?(schien)

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] from comment #119)
> (In reply to Jason Smith [:jsmith] from comment #118)
> > Are you sure about that? I had no knowledge of our mochitests running in try
> > in emulators running OOP.
> Yes, I'm pretty sure about that. The mochitest case are ran inside a remote
> iframe.
> https://github.com/mozilla-b2g/gaia/blob/master/test_apps/test-container/
> index.html#L16

Well that's always existed. To my understanding, there was something else had to be done to allow the current mochitests to run OOP in try runs. It wasn't just the implementation of that app - that's been around since 1.01.

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #120)
> For all intents and purposes, can't we introduce something like
> `nsIUDPMessage::rawData`, if backward compatibility is at stake, or any
> other way of exposing raw, binary, socket data? Text decoding/ encoding is
> something that should be deferred to API consumers, IMHO, which is already
> implied as a necessary step in `nsIUDPSocket::send` and
> `nsIUDPSocket::sendWithAddr`.
Providing a nsIUDPMessage::rawData sounds good to me. Would you file a bug for it?
Flags: needinfo?(schien)

    
    

    
  
(In reply to Jason Smith [:jsmith] from comment #121)
> (In reply to Shih-Chiang Chien [:schien] from comment #119)
> > (In reply to Jason Smith [:jsmith] from comment #118)
> > > Are you sure about that? I had no knowledge of our mochitests running in try
> > > in emulators running OOP.
> > Yes, I'm pretty sure about that. The mochitest case are ran inside a remote
> > iframe.
> > https://github.com/mozilla-b2g/gaia/blob/master/test_apps/test-container/
> > index.html#L16
> 
> Well that's always existed. To my understanding, there was something else
> had to be done to allow the current mochitests to run OOP in try runs. It
> wasn't just the implementation of that app - that's been around since 1.01.
I think you're talking about mochitest on b2g-desktop, which is not OOP yet.

From the log I saw on try server, "TEST-START"/"TEST-END" log are all from a pid that different from b2g process. BTW, the python script we initiate emulator mochitest also says we turn on OOP test in default.
http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py#173
Blocks: 952927

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] from comment #122)
> Providing a nsIUDPMessage::rawData sounds good to me. Would you file a bug
> for it?

Filed as bug 952927. I cc'ed you, since I don't know exactly who else is interested... could you? ;-)

Thanks and kudos with the great work you've done here!

    
    

    
  
FWIW, a developer just emailed me directly asking why their Firefox extension which was using the "@mozilla.org/network/server-socket-udp;1" component no longer works in Firefox 28.  It seems like this is the bug that renamed that component.  Adding the addon-compat keyword and CCing Jorge.
Keywords: addon-compat

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: