951677 - Support bind in SocketTransport

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

To support bind() in TCPSocket in content process, we need to add bind() function in SocketTransport. (or in SocketTransportService)

Comment 1

[Honza Bambas (:mayhemer)]

I think this can be exposed on nsISocketTransport.  Get inspired by nsIUDPSocket.idl how to pass an address (mozilla::net::NetAddr).  The new nsISocketTransport.bind(in NetAddr aAddr) must only be called called before opening streams on the transport (i.e. before PR_Connect call).  Otherwise it must/should fail.

Please be aware of bug 444328 and patch https://bugzilla.mozilla.org/attachment.cgi?id=8346933&action=edit.  Please base your work on that patch, otherwise Steve may not like you ;)  That patch is already r+'ed and soon should land anyway.

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> Please be aware of bug 444328 and patch
> https://bugzilla.mozilla.org/attachment.cgi?id=8346933&action=edit.  Please
> base your work on that patch, otherwise Steve may not like you ;)  

Yup, please stay in sync with the status of that bug :)

> That patch is already r+'ed and soon should land anyway.

The first patch has landed; two more are in review. The first one is most important: it adds PRFileDescAutoLock which you should use to get access to the PRFileDesc in a function. The second and third patches are also important, but are probably more isolated from your work. Still, keep me updated please and let me know (needinfo me) if you have any problems.

Comment 3

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

Thank for the suggestion!

I have another question: PR_Connect is called automatically after SocketTransportService::CreateTransport(), is there a chance to call Bind() after SocketTransport is created and before its socket is connected? Or we should create SocketTransport with new when we want to bind to an address?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Patrick Wang [:kk1fff] from comment #3)
> Thank for the suggestion!
> 
> I have another question: PR_Connect is called automatically after
> SocketTransportService::CreateTransport(), 

It's definitely not.  It actually cannot be.

> is there a chance to call Bind()
> after SocketTransport is created and before its socket is connected? Or we
> should create SocketTransport with new when we want to bind to an address?

InitiateSocket() where from PR_Connect is called cannot fire sooner then you open output or input stream.  Just bind before a stream is open.  If already done so, throw.  See comment 1.

Comment 5

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

Thanks for clarification, Honza!

Comment 6

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

Attachment: Patch: add Bind function to SocketTransport.

Comment 7

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

Attachment: Patch: add Bind function to SocketTransport.

Rebasing.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8463777 [details] [diff] [review]
Patch: add Bind function to SocketTransport.

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

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +2218,5 @@
> +{
> +    NS_ENSURE_ARG(aLocalAddr);
> +
> +    if (mFDconnected) {
> +        return NS_ERROR_FAILURE;

I think mAttached is a better check.  You should not be able to change this after PR_Connect is called.  However, mFDconnected means we've reached the peer and that could be racy.

Comment 9

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

Attachment: Proposed Patch

Check mAttached before binding to a local address.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8490612 [details] [diff] [review]
Proposed Patch

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

r- for a missing test.  Please write something that at least exercises the code, best to also check at least the port number is as expected.  This way it seems like you've never run it at all.  However, if you find (or argument) the test would be hard to write (since to find the right port number that will not in 100% cases collide), we can omit it.  But that research first needs to be done.

Comment 11

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

Attachment: Test case

Test binding a port. To avoid binding to a port that is already used, it retries with next port when input stream gets error, until test server received connection or the retry limit is reached.

Comment 12

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

(In reply to Honza Bambas (:mayhemer) from comment #10)
> r- for a missing test.  Please write something that at least exercises the
> code, best to also check at least the port number is as expected.  This way
> it seems like you've never run it at all.  However, if you find (or
> argument) the test would be hard to write (since to find the right port
> number that will not in 100% cases collide), we can omit it.  But that
> research first needs to be done.

I used to think that write a test for bind is not easy, since there may need many interfaces to be on the test machine to tell if we bound to an interface successfully. But as you've mentioned, binding to a port can also be a way to implement the test case. I've added a test case for this. Thanks.

Comment 13

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

Attachment: Test case v.2

Fix build failure on try.

Comment 14

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

Attachment: Test case v.3

Remove unnecessary header files.

Comment 15

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

Attachment: Proposed Patch

Using an auto pointer to hold the NetAddr that holds the address we are going to bind. I just realized how large a NetAddr is, as Bind() won't be used by many people, it will be an unnecessary waste. (And leak detector treat the increment of memory size as a leak.)

Comment 16

[Honza Bambas (:mayhemer)]

Can you explain why you need the lock over mBindAddress now?

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8495041 [details] [diff] [review]
Test case v.3

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

Cool!  Have you tested the retry logic carefully?  I'll leave this up to you.

Comment 18

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

(In reply to Honza Bambas (:mayhemer) from comment #16)
> Can you explain why you need the lock over mBindAddress now?

Yes. Since it looks like InitiateSocket() is called on STS, while Bind() is called on main thread.

Comment 19

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

(In reply to Honza Bambas (:mayhemer) from comment #17)
> Have you tested the retry logic carefully?  I'll leave this up to you.

Yes, I've tested this by binding to a port that is already in use. And it will bind to next port.

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8499302 [details] [diff] [review]
Proposed Patch

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

Few nits, I'll check quickly on the next version.  Thanks!

::: netwerk/base/public/nsISocketTransport.idl
@@ +56,5 @@
>       */
>      [noscript] NetAddr getSelfAddr();
>  
>      /**
> +     * Bind to specific local address.

to a specfic

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +1356,5 @@
>      //
>      PRNetAddr prAddr;
> +    {
> +        MutexAutoLock lock(mLock);
> +        if (mBindAddr) {

to optimize this a bit, check mBindAddr being non-null outside the lock, but work with it under the lock.  Like:

if (mBindAddr) {
  lock(mLock);
  ...
  PR_Bind(..);
  mBindAddr = nullptr;
}

@@ +2237,5 @@
> +nsSocketTransport::Bind(NetAddr *aLocalAddr)
> +{
> +    NS_ENSURE_ARG(aLocalAddr);
> +
> +    if (mAttached) {

check this also under the lock.

Comment 21

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

Attachment: Proposed Patch

Revised.

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 8500287 [details] [diff] [review]
Proposed Patch

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

Thanks!  r=honzab.

Comment 23

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

Thank you for review, Honza!

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d52ce400c01
https://hg.mozilla.org/integration/mozilla-inbound/rev/27626f68637d

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d52ce400c01
https://hg.mozilla.org/mozilla-central/rev/27626f68637d

Comment 26

[Dragana Damjanovic [:dragana]]

If you need this for tcp sockets only it is fine, if you want to use it for a "ssl" socket it will not work.
I will fix this as a part of another bug. This is just a remark since bind function is exposed in nsISocketTransport and it can be an "ssl" socket as well.

Comment 27

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

I see. Thank you for finding and fixing the bug. Dragana. Just curious, since this patch is just done recently, is there already a an SSL socket working with bind()?

Comment 28

[Dragana Damjanovic [:dragana]]

I posted a new bug, because the other bug I was talking about probably will take longer to lend
The new bug is bug 1087213.