960397 - Multicast UDP socket options for nsIUDPSocket

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

Multicast UDP makes it possible to implement things like service discovery in Gecko.  The main options that would be great to have access to are:

* PR_SockOpt_AddMember
* PR_SockOpt_DropMember

Beyond those two, these would be nice to have too:

* PR_SockOpt_McastLoopback
* PR_SockOpt_McastInterface

Comment 1

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

Attachment: Part 1: Add multicast options to UDP IDL

I've attached an example of the IDL changes that could be done to make this happen.

Honza, what do you think of this?

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8360896 [details] [diff] [review]
Part 1: Add multicast options to UDP IDL

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

Yep, I think this is good.

Also, all could have getters, but I'm not sure it's that useful, up to you whether you want to do all the work.

Comment 3

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

Attachment: Part 1 (v2): Add multicast options to UDP IDL

* Changed getters to attributes
* Using strings of IPs to make things much easier to set from JS

Comment 4

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

Attachment: Part 2: Multicast option support for UDPSocket

Comment 5

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=08b90571ec89

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to J. Ryan Stinnett [:jryans] from comment #3)
> * Using strings of IPs to make things much easier to set from JS

But harder from C++ when you have the address in hands.  I think this should go in hands with how nsISocketTransport is defined.  The string taking methods should have "scriptable" in name and you should have [noscript] methods working with NetAddr.

Comment 7

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

(In reply to Honza Bambas (:mayhemer) from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] from comment #3)
> > * Using strings of IPs to make things much easier to set from JS
> 
> But harder from C++ when you have the address in hands.  I think this should
> go in hands with how nsISocketTransport is defined.  The string taking
> methods should have "scriptable" in name and you should have [noscript]
> methods working with NetAddr.

Ah, fair enough, didn't realize that was a pattern.  I'll make this update.

Comment 8

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

Attachment: Part 1 (v3): Add multicast options to UDP IDL

Instead of adding "Scriptable" to the JS methods, I added "Addr" to the noscript methods, as that seemed to the pattern used above in this IDL file.

Comment 9

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

Attachment: Part 2 (v2): Multicast option support for UDPSocket

Comment 10

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=7f42908f5d42

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8388005 [details] [diff] [review]
Part 2 (v2): Multicast option support for UDPSocket

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

There is one major problem with this patch - you must access mFD only on the socket thread.  Hence any PR_SetSocketOption must be inside events posted to mSts (mSts->Dispatch).  Please try to have just one nsRunnable implementation for all the options.  Also keep in mind we might reuse it for any future added options.  To have getters (like ::GetMulticastInterface) just cache the values in nsUDPSocket or drop options getters support altogether (up to you).

::: netwerk/base/src/nsUDPSocket.cpp
@@ +803,5 @@
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +
> +  PRNetAddr prAddr;
> +  PR_StringToNetAddr(aAddr.BeginReading(), &prAddr);

You need to check the result (and fail).  Applies to every call to PR_StringToNetAddr!

@@ +839,5 @@
> +  return JoinMulticast(prAddr, prIface);
> +}
> +
> +NS_IMETHODIMP
> +nsUDPSocket::JoinMulticast(const PRNetAddr &aAddr, const PRNetAddr &aIface)

Please rename to JoinMulticastInternal and don't use NS_IMETHOD[IMP] to declare/define.  Just let it have nsresult result type.

::: netwerk/base/src/nsUDPSocket.h
@@ +44,5 @@
>  
> +  NS_IMETHOD JoinMulticast(const PRNetAddr &aAddr, const PRNetAddr &aIface);
> +  NS_IMETHOD LeaveMulticast(const PRNetAddr &aAddr, const PRNetAddr &aIface);
> +  NS_IMETHOD GetMulticastInterface(PRNetAddr &aIface);
> +  NS_IMETHOD SetMulticastInterface(const PRNetAddr &aIface);

Use nsresult result type instead of NS_IMETHOD.  NS_IMETHOD means virtual nsresult and that is not what you want.  Rename these methods to *Internal(..).

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8388004 [details] [diff] [review]
Part 1 (v3): Add multicast options to UDP IDL

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

::: netwerk/base/public/nsIUDPSocket.idl
@@ +171,5 @@
> +     *        this is not specified, the OS may join the group on all interfaces
> +     *        or only the primary interface.
> +     */
> +    [noscript] void joinMulticastAddr([const] in NetAddrPtr addr,
> +                                      [const, optional] in NetAddrPtr iface);

Please see nsISocketTransport idl for how NetAddr vs NetAddrPtr is used there.  Do you have a reason not to use NetAddr directly here?

Maybe have just a single comment for bot NetAddr and String version of the api like:

/**
 * ...
 */
void joinMulticast(..);
[noscript] void joinMulticastAddr(..);

Comment 13

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

(In reply to Honza Bambas (:mayhemer) from comment #11)
> Comment on attachment 8388005 [details] [diff] [review]
> Part 2 (v2): Multicast option support for UDPSocket
> 
> Review of attachment 8388005 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is one major problem with this patch - you must access mFD only on the
> socket thread.  Hence any PR_SetSocketOption must be inside events posted to
> mSts (mSts->Dispatch).  Please try to have just one nsRunnable
> implementation for all the options.  Also keep in mind we might reuse it for
> any future added options.  To have getters (like ::GetMulticastInterface)
> just cache the values in nsUDPSocket or drop options getters support
> altogether (up to you).

Thanks for this great feedback, and sorry for missing this issue!  I do think it's nice to have the getters for completeness.  But what if someone calls a getter before ever using the setter?  We wouldn't have a cached value in that case, so don't we need a runnable to get values as well?

Comment 14

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

(In reply to Honza Bambas (:mayhemer) from comment #12)
> Comment on attachment 8388004 [details] [diff] [review]
> Part 1 (v3): Add multicast options to UDP IDL
> 
> Review of attachment 8388004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/public/nsIUDPSocket.idl
> @@ +171,5 @@
> > +     *        this is not specified, the OS may join the group on all interfaces
> > +     *        or only the primary interface.
> > +     */
> > +    [noscript] void joinMulticastAddr([const] in NetAddrPtr addr,
> > +                                      [const, optional] in NetAddrPtr iface);
> 
> Please see nsISocketTransport idl for how NetAddr vs NetAddrPtr is used
> there.  Do you have a reason not to use NetAddr directly here?

You are right that using NetAddr is likely better for the attribute, and the first argument of join/leaveMulicastAddr.  For the second argument, which is optional, a pointer seemed like a sufficient way to easily test if a value is supplied or not, so I can default the value if it's not present.  It doesn't seem like default parameters mix well with XPIDL, so I avoided that.

I haven't written much XPIDL.  Is there a better way to support optional arguments?  The examples I found weren't very enlightening...  No one seems to have used NetAddr as an optional argument yet, and most other things are nsI* types that do get passed as pointers by default.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to J. Ryan Stinnett [:jryans] from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #11)
> > Comment on attachment 8388005 [details] [diff] [review]
> > Part 2 (v2): Multicast option support for UDPSocket
> > 
> > Review of attachment 8388005 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > There is one major problem with this patch - you must access mFD only on the
> > socket thread.  Hence any PR_SetSocketOption must be inside events posted to
> > mSts (mSts->Dispatch).  Please try to have just one nsRunnable
> > implementation for all the options.  Also keep in mind we might reuse it for
> > any future added options.  To have getters (like ::GetMulticastInterface)
> > just cache the values in nsUDPSocket or drop options getters support
> > altogether (up to you).
> 
> Thanks for this great feedback, and sorry for missing this issue!  I do
> think it's nice to have the getters for completeness.  But what if someone
> calls a getter before ever using the setter?  We wouldn't have a cached
> value in that case, so don't we need a runnable to get values as well?

Just throw (maybe NS_ERROR_NOT_INITIALIZED).  When this becomes a problem we will find a solution.

(In reply to J. Ryan Stinnett [:jryans] from comment #14)
> (In reply to Honza Bambas (:mayhemer) from comment #12)
> > Comment on attachment 8388004 [details] [diff] [review]
> > Part 1 (v3): Add multicast options to UDP IDL
> > 
> > Review of attachment 8388004 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/base/public/nsIUDPSocket.idl
> > @@ +171,5 @@
> > > +     *        this is not specified, the OS may join the group on all interfaces
> > > +     *        or only the primary interface.
> > > +     */
> > > +    [noscript] void joinMulticastAddr([const] in NetAddrPtr addr,
> > > +                                      [const, optional] in NetAddrPtr iface);
> > 
> > Please see nsISocketTransport idl for how NetAddr vs NetAddrPtr is used
> > there.  Do you have a reason not to use NetAddr directly here?
> 
> You are right that using NetAddr is likely better for the attribute, and the
> first argument of join/leaveMulicastAddr.  For the second argument, which is
> optional, a pointer seemed like a sufficient way to easily test if a value
> is supplied or not, so I can default the value if it's not present.  It
> doesn't seem like default parameters mix well with XPIDL, so I avoided that.
> 
> I haven't written much XPIDL.  Is there a better way to support optional
> arguments?  The examples I found weren't very enlightening...  No one seems
> to have used NetAddr as an optional argument yet, and most other things are
> nsI* types that do get passed as pointers by default.

I'm unfortunately not a deep XPIDL expert either.  What you say sounds like a good argument to have ptrs here.  Maybe ask :bz or :bsmedberg about this, but if there is no easy way, just go with pointers.

Comment 16

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

Attachment: Part 1 (v4): Add multicast options to UDP IDL

I've made the IDL cleanups you suggested.  I asked around, and there's no great way to have an optional C++ argument from XPIDL, so I kept the pointer for the optional ones, and used NetAddr for the other cases.

Comment 17

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

Attachment: Part 2 (v3): Multicast option support for UDPSocket

I'm now using a runnable to set socket options on the STS thread.

I've also made all other changes you suggested.

Also, in the end I decided to drop the getters for now.  Even with caching the values, it gets pretty awkward, since there are different types used depending on whether it was set from C++ vs. JS, etc.  So, we can add that back in if it's needed later.

Comment 18

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=d9744765556d

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8408614 [details] [diff] [review]
Part 2 (v3): Multicast option support for UDPSocket

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

::: netwerk/base/src/nsUDPSocket.cpp
@@ +68,5 @@
> +
> +class SetSocketOptionRunnable : public nsRunnable
> +{
> +public:
> +  SetSocketOptionRunnable(PRFileDesc* aFD, PRSocketOptionData aOpt)

maybe pass aOpt as a const ref?  the way you do it in this patch the structure is copied 2 times (once to the arg and then to the member)

@@ +75,5 @@
> +  {}
> +
> +  NS_IMETHOD Run()
> +  {
> +    PR_SetSocketOption(mFD, &mOpt);

ref the whole nsUDPSocket here, mFD on it may go away in the meantime before this gets invoked on the socket thread!  checking for non-null is then enough

assert you are on the socket thread here (if not too complicated)

log failures with SOCKET_LOG

::: netwerk/test/TestUDPSocket.cpp
@@ +333,5 @@
> +  nsCOMPtr<MulticastTimerCallback> timerCb = new MulticastTimerCallback();
> +
> +  // Send multicast ping
> +  timerCb->mResult = NS_OK;
> +  timer->InitWithCallback(timerCb, 500, nsITimer::TYPE_ONE_SHOT);

longer please (2000ms. or even longer) at all timers, have a #define for it

::: netwerk/test/unit/test_udp_multicast.js
@@ +58,5 @@
> +  do_print("Joining multicast group");
> +  gSocket.joinMulticast(ADDRESS);
> +  sendPing().then(
> +    run_next_test,
> +    () => do_throw("Joined group, but no packet received")

is this correct?

Comment 20

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

(In reply to Honza Bambas (:mayhemer) from comment #19)
> Comment on attachment 8408614 [details] [diff] [review]
> Part 2 (v3): Multicast option support for UDPSocket
> @@ +75,5 @@
> > +  {}
> > +
> > +  NS_IMETHOD Run()
> > +  {
> > +    PR_SetSocketOption(mFD, &mOpt);
> 
> ref the whole nsUDPSocket here, mFD on it may go away in the meantime before
> this gets invoked on the socket thread!  checking for non-null is then enough

Okay, I'll ref the socket, but should I still pass mFD to the runnable and not use the socket at all?  mFD is a private member, so I can't pull it from the socket in the runnable.  Or I could move the call to PR_SetSocketOption inside a new public method of the socket that the runnable calls, and that would then use mFD.  Any preference?

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to J. Ryan Stinnett [:jryans] from comment #20)
> (In reply to Honza Bambas (:mayhemer) from comment #19)
> > Comment on attachment 8408614 [details] [diff] [review]
> > Part 2 (v3): Multicast option support for UDPSocket
> > @@ +75,5 @@
> > > +  {}
> > > +
> > > +  NS_IMETHOD Run()
> > > +  {
> > > +    PR_SetSocketOption(mFD, &mOpt);
> > 
> > ref the whole nsUDPSocket here, mFD on it may go away in the meantime before
> > this gets invoked on the socket thread!  checking for non-null is then enough
> 
> Okay, I'll ref the socket, but should I still pass mFD to the runnable and
> not use the socket at all?  mFD is a private member, so I can't pull it from
> the socket in the runnable.  Or I could move the call to PR_SetSocketOption
> inside a new public method of the socket that the runnable calls, and that
> would then use mFD.  Any preference?

New method on the socket is a good idea.  You put the check inside and things are nicely encapsulated.  Make that method private/friend for your runnable class.

Comment 22

[Honza Bambas (:mayhemer)]

Also, there is NS_NewRunnableMethodWithArg, [1].  I don't say you have to use it, but it may simplify the code here.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#426

Comment 23

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

Attachment: Part 2 (v4): Multicast option support for UDPSocket

(In reply to Honza Bambas (:mayhemer) from comment #19)
> Comment on attachment 8408614 [details] [diff] [review]
> Part 2 (v3): Multicast option support for UDPSocket
> 
> Review of attachment 8408614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/src/nsUDPSocket.cpp
> @@ +68,5 @@
> > +
> > +class SetSocketOptionRunnable : public nsRunnable
> > +{
> > +public:
> > +  SetSocketOptionRunnable(PRFileDesc* aFD, PRSocketOptionData aOpt)
> 
> maybe pass aOpt as a const ref?  the way you do it in this patch the
> structure is copied 2 times (once to the arg and then to the member)

Okay, I am using a const ref now, to avoid the extra copy.

> @@ +75,5 @@
> > +  {}
> > +
> > +  NS_IMETHOD Run()
> > +  {
> > +    PR_SetSocketOption(mFD, &mOpt);
> 
> ref the whole nsUDPSocket here, mFD on it may go away in the meantime before
> this gets invoked on the socket thread!  checking for non-null is then enough

I'm now using a private method SetSocketOption that dispatches a runnable (that calls back to SetSocketOption) if we're not on the right thread.  I tried out NS_NewRunnableMethodWithArg (thanks for the tip!), but it didn't seem to allow for the right copying semantics for the option data.  So, the runnable now uses a nsRefPtr to the socket, and the private method checks mFD.

> assert you are on the socket thread here (if not too complicated)

This is now done in SetSocketOption as part of determining whether to dispatch a runnable, like the SendWithAddress method above it.

> log failures with SOCKET_LOG

Done.

> ::: netwerk/test/TestUDPSocket.cpp
> @@ +333,5 @@
> > +  nsCOMPtr<MulticastTimerCallback> timerCb = new MulticastTimerCallback();
> > +
> > +  // Send multicast ping
> > +  timerCb->mResult = NS_OK;
> > +  timer->InitWithCallback(timerCb, 500, nsITimer::TYPE_ONE_SHOT);
> 
> longer please (2000ms. or even longer) at all timers, have a #define for it

Okay, done for both the C++ and JS tests.

> ::: netwerk/test/unit/test_udp_multicast.js
> @@ +58,5 @@
> > +  do_print("Joining multicast group");
> > +  gSocket.joinMulticast(ADDRESS);
> > +  sendPing().then(
> > +    run_next_test,
> > +    () => do_throw("Joined group, but no packet received")
> 
> is this correct?

Yes, it seems right.  If the ping is received correctly, we move to the next test.  If not, we throw and the test fails.  If I comment out the call to |joinMulticast()| above, the test fails as expected.

Try: https://tbpl.mozilla.org/?tree=Try&rev=f2822a127861

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8409870 [details] [diff] [review]
Part 2 (v4): Multicast option support for UDPSocket

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

r=honzab

please, before landing, push to try and rerun the tests you have introduced at least 10 times on *each* platform to make sure they don't intermittently time out or otherwise fail.

::: netwerk/base/src/nsUDPSocket.cpp
@@ +955,5 @@
> +    // Dispatch to STS thread and re-enter this method there
> +    nsCOMPtr<nsIRunnable> runnable = new SetSocketOptionRunnable(this, aOpt);
> +    nsresult rv = mSts->Dispatch(runnable, NS_DISPATCH_NORMAL);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return NS_ERROR_FAILURE;

return rv here.

@@ +966,5 @@
> +  }
> +
> +  if (PR_SetSocketOption(mFD, &aOpt) != PR_SUCCESS) {
> +    SOCKET_LOG(("nsUDPSocket::SetSocketOption [this=%p] failed for type %d\n",
> +      this, aOpt.option));

Please also log PR_GetError() as %d.

::: netwerk/base/src/nsUDPSocket.h
@@ +50,5 @@
> +  nsresult LeaveMulticastInternal(const PRNetAddr& aAddr,
> +                                  const PRNetAddr& aIface);
> +  nsresult SetMulticastInterfaceInternal(const PRNetAddr& aIface);
> +
> +

only one blank line

Comment 25

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

Attachment: Part 2 (v5): Multicast option support for UDPSocket

Carrying over :mayhemer's r+ from attachment 8409870 [details] [diff] [review].

* All review comments addressed
* Adjusted test cases to account for behavior on Windows

https://tbpl.mozilla.org/?tree=Try&rev=8f6ec7c90eb1

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to J. Ryan Stinnett [:jryans] from comment #25)
> https://tbpl.mozilla.org/?tree=Try&rev=8f6ec7c90eb1

MacOSX Snow Leopard 10.6 try debug

TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_udp_multicast.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_udp_multicast.js | Loopback disabled, but still got a packet - See following stack:

:(

Comment 27

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

Made some test fixes, we're looking good now!

https://tbpl.mozilla.org/?tree=Try&rev=69fffa5a17d9

Comment 28

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

Attachment: Part 2 (v6): Multicast option support for UDPSocket

Carrying over :mayhemer's r+ from attachment 8409870 [details] [diff] [review].

* Using one socket per test phase in xpcshell
* Reduced shared test state that collided when multiple sockets were used

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72ea2bcb65f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6888e03eb627

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72ea2bcb65f3
https://hg.mozilla.org/mozilla-central/rev/6888e03eb627

Comment 31

[Masatoshi Kimura [:emk]]

Please do not add any new GetVersionEx() calls. Use helper functions from WindowsVersion.h instead.

Comment 32

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

(In reply to Masatoshi Kimura [:emk] from comment #31)
> Please do not add any new GetVersionEx() calls. Use helper functions from
> WindowsVersion.h instead.

Ah sorry, I didn't realize...  Just searched for other uses of the APIs.  Will remember for next time!

Comment 33

[Eric Shepherd [:sheppy]]

Actually, this entire interface needs documentation. For now, the fact that this has happened has been added to Firefox 32 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/32#XPCOM

I have filed a new bug (bug 1245989) for documenting the entire interface, but keep in mind that XPCOM related docs are basically the lowest priority right now for various reasons. For the purposes of this bug, removing doc-needed.