Closed
Bug 951677
Opened 10 years ago
Closed 10 years ago
Support bind in SocketTransport
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: kk1fff, Assigned: kk1fff)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 6 obsolete files)
6.58 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
5.86 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
To support bind() in TCPSocket in content process, we need to add bind() function in SocketTransport. (or in SocketTransportService)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(honzab.moz)
Comment 1•10 years ago
|
||
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.
Flags: needinfo?(honzab.moz)
Comment 2•10 years ago
|
||
(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.
Assignee | ||
Comment 3•10 years ago
|
||
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?
Flags: needinfo?(honzab.moz)
Comment 4•10 years ago
|
||
(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.
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 5•10 years ago
|
||
Thanks for clarification, Honza!
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → kk1fff
Assignee | ||
Updated•10 years ago
|
Attachment #8463777 -
Flags: review?(honzab.moz)
Comment 8•10 years ago
|
||
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.
Attachment #8463777 -
Flags: review?(honzab.moz) → feedback+
Assignee | ||
Comment 9•10 years ago
|
||
Check mAttached before binding to a local address.
Attachment #8463777 -
Attachment is obsolete: true
Attachment #8490612 -
Flags: review?(honzab.moz)
Comment 10•10 years ago
|
||
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.
Attachment #8490612 -
Flags: review?(honzab.moz) → review-
Assignee | ||
Comment 11•10 years ago
|
||
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.
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
Fix build failure on try.
Attachment #8494374 -
Attachment is obsolete: true
Attachment #8494936 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•10 years ago
|
Attachment #8490612 -
Flags: review- → review?(honzab.moz)
Assignee | ||
Comment 14•10 years ago
|
||
Remove unnecessary header files.
Attachment #8494936 -
Attachment is obsolete: true
Attachment #8494936 -
Flags: review?(honzab.moz)
Attachment #8495041 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 15•10 years ago
|
||
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.)
Attachment #8490612 -
Attachment is obsolete: true
Attachment #8490612 -
Flags: review?(honzab.moz)
Attachment #8499302 -
Flags: review?(honzab.moz)
Comment 16•10 years ago
|
||
Can you explain why you need the lock over mBindAddress now?
Comment 17•10 years ago
|
||
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.
Attachment #8495041 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
(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•10 years ago
|
||
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.
Attachment #8499302 -
Flags: review?(honzab.moz) → feedback+
Assignee | ||
Comment 21•10 years ago
|
||
Revised.
Attachment #8499302 -
Attachment is obsolete: true
Attachment #8500287 -
Flags: review?(honzab.moz)
Comment 22•10 years ago
|
||
Comment on attachment 8500287 [details] [diff] [review] Proposed Patch Review of attachment 8500287 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! r=honzab.
Attachment #8500287 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 23•10 years ago
|
||
Thank you for review, Honza!
Assignee | ||
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d52ce400c01 https://hg.mozilla.org/integration/mozilla-inbound/rev/27626f68637d
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d52ce400c01 https://hg.mozilla.org/mozilla-central/rev/27626f68637d
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 26•10 years ago
|
||
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.
Flags: needinfo?(kk1fff)
Assignee | ||
Comment 27•10 years ago
|
||
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()?
Flags: needinfo?(kk1fff)
Comment 28•10 years ago
|
||
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.
Updated•9 years ago
|
Keywords: dev-doc-needed
Updated•9 years ago
|
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•