bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Server socket support in Necko

RESOLVED FIXED in mozilla1.6beta

Status

()

Core
Networking
--
enhancement
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: Daniel Wippermann, Assigned: Darin Fisher)

Tracking

Trunk
mozilla1.6beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

17 years ago
I'm currently implementing server socket support in Necko integrated into the 
nsISocketTransport object. A new method called "asyncListen" is introduced, 
which takes a newly created "nsISocketListener" interface as a parameter. The 
socket used to call the method is switched into listening mode and is put in 
the work queue of nsISocketTransportService. Everytime a client application 
connects to this socket, the socket is accepted and the listener 
interface's "onSocketAccepted" method is called with the accepted socket as a 
parameter.

With this support it should be possible to realize at least the following 
things:

1) P2P servent (that's why I need it)
2) PORT command implementation during FTP session with servers that are not 
PASV-capable
3) ident server (see bug #40879)

If anybody has any comments to this idea and/or its implementation I'd like to 
hear from her/him. If this is considered useful, perhaps it gets part of the 
necko core (which I would appreciate because the lack of this feature regularly 
was/is subject in the netlib newsgroup).
(Reporter)

Comment 1

17 years ago
Created attachment 44107 [details]
implementation proposal (for information see above)
ccing some networking folks and adding dependencies.

The attachment looks like a zip file... perhaps it would be better to attach
such things as application/zip  
Blocks: 40879, 74045
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

17 years ago
*** Bug 74045 has been marked as a duplicate of this bug. ***

Comment 4

17 years ago
This would make DCC chat and file transfers possible as well, I'm all for it!
Blocks: 73257
No longer blocks: 74045

Comment 5

17 years ago
Daniel, what format is the attachment in?
(Reporter)

Comment 6

17 years ago
Created attachment 44196 [details]
Resent same attachment as id 44107, but correct MIME type "application/zip" (Thanks Boris)
(Assignee)

Comment 7

17 years ago
at first glance, the patch looks good.  however, it's unlikely to make it in for
0.9.4 (as checkins are going to be tightly controlled and limited in scope).

i'll try to do a more thorough review soon (but no promises ;-)

Comment 8

17 years ago
also blocks bug 465.
Blocks: 465
darin: 0.9.4 is out... :)

(I haven't looked at the patch)
Keywords: patch, review
(Assignee)

Comment 10

17 years ago
please reattach the patch as a diff.. thanks!

Comment 11

17 years ago
Not that you don't already have enough reasons, but this would also allow
2player network games (why I need it).

Gets my vote :)
(Assignee)

Comment 12

17 years ago
nevermind my request to see the whole diff as a single patch... i didn't realize
that that would require CVS write access :-(

at any rate, there is definitely work to be done on the patch as outlined in the
newsgroup thread on n.p.m.netlib "Re: Bug 92928 (Server sockets): New DIFF"
(Reporter)

Comment 13

17 years ago
COPY OF MESSAGE FROM n.p.m.netlib:
==================================

Hi,

 > the nsIServerSocket would still need to participate in the same
 > PR_Poll loop, which would mean changes to nsSocketTransportService to
 > work with something other than just nsSocketTransport objects.

one possible (very object oriented) approach to this would be to use a
common base class, that implements just enough to work as a layer
between the nsSocketTransportService and objects that are part of the
work queue. An example is the class nsSocketTransportServiceLayer
located in the nsSocketTransportService.h after applying the patch
attached to this mail.

Although this class inherits nsISupports it should not be available by
calling QueryInterface because the methods are too implementation
dependend (I don't know if this proceeding is acceptable or do we rely
on noboby calling the contained functions???). For this reason the
interface has no IID and the member functions don't follow the calling
conventions for interface methods.

You get a pointer to this layer interface using a static cast on the
super class. The nsISupports is only necessary for the reference
counting during the work queue processing.

Both classes nsSocketTransport and the new nsServerSocket inherit of
this class and implement the pure virtual functions.

This already works fine for the nsSocketTransport class except for the
methods using a static cast to convert a nsITransport pointer into a
nsSocketTransport pointer (this looks a bit critical...)

One of them is nsSocketTransportService::Wakeup().The original
implementation uses the static cast to call
nsSocketTransportService::AddToWorkQ afterwards. The patch's
implementation relies on the fact, that the transport passed to the
Wakeup method has to be in the selection list of
nsSocketTransportService, otherwise a wakeup would not make any sense.
So it traverses all selection list entries to find the
nsSocketTransportServiceLayer that belongs to the i_Transport parameter.
In the worst case that means MAX_OPEN_CONNECTIONS loop passes. But since
this function is rarely used I think this is okay... Other opinions???

The second (and more tricky) method is
nsSocketTransportService::ReuseTransport(). In this case I cannot assume
the requested transport to be in the selection list. The most simple way
(but not my goal, see above) would be to let QueryInterface return the
nsSocketTransportServiceLayer (or a nsISocketTransportServiceLayer
respectively) and call its methods... Suggestions welcome!

If you agree with this basic approach I would start implementing the
server socket support as well.

Thanks,
    Daniel
(Reporter)

Comment 14

17 years ago
Created attachment 57617 [details] [diff] [review]
Patch referring to comment above
(Assignee)

Comment 15

17 years ago
using a common base class for server sockets and socket transports is the right
thing IMO.  Wakeup should be modified to accept a nsSocketTransportServerLayer
(btw, this really could use a shorter name!), and ReuseTransport should QI the
given i_Transport for nsISocketTransport.  if the QI succeeds, then you can just
STATIC_CAST to nsSocketTransport and call CanBeReused.  on the other hand, no
one is currently calling ReuseTransport, and IMO it should simply be eliminated
in favor of a method on nsISocketTransport.
(Reporter)

Comment 16

17 years ago
1) "nsSocketTransportServerLayer (btw, this really could use a shorter name!)":

nsSocketTransportServiceLayer => nsCommonSocketLayer or just nsSocketLayer ???


2) "Wakeup should be modified to accept a nsSocketTransportServerLayer": 

Should Wakeup take a pointer to a nsSocketTransportServiceLayer object as a
parameter (which makes no sense to me) or do you mean "modified" in the way the
patch does?

(Assignee)

Comment 17

17 years ago
Wakeup should not be a method on nsISocketTransportService... it should be a
method on nsSocketTransportService that nsSocketTransport can call.  there is no
need for us to expose it (as well as ReuseTransport) from
nsISocketTransportService.  if we remove these functions from the .idl file,
then Wakeup could be modified to take a nsSocketTransportServiceLayer parameter,
and ReuseTransport could be removed completely.
(Assignee)

Comment 18

17 years ago
maybe nsSTSLayer?  i like nsSocketTransportServiceLayer because it correctly
describes what it is, but it would just be nice if it were shorter... i don't
know about nsCommonSocketLayer or nsSocketLayer... seems too vague... since
"socket" shows up elsewhere in the code as part of class names.
(Reporter)

Comment 19

17 years ago
Created attachment 58539 [details] [diff] [review]
Implementation proposal

Implementation proposal based on attachment 57617 [details] [diff] [review] and this topic's discussions
(incl. thread on n.p.m.netlib).

Opinions and suggestions welcome :-)
   Daniel

Comment 20

17 years ago
Will bug 465 be reopened if this gets fixed? This is currently my top blocker. 
I need port support. I know most broadband is firewalled thus passive mode is 
popular, but... Weirdly my broadband requires port :(

Comment 21

17 years ago
It is hard to say given the problems with FTP PORT. I'm sure we will have a
lengthy discussion and make a good decision.

Comment 22

17 years ago
It would be really great if this bug  were fixed by moz 1.0 - I know of several 
applications for this - one of which I am working on myself.  It seems like 
this could really enhance the range of applications that mozilla could 
potentially be used for - such as IRC server, peer to peer, file transfer in 
AIM, etc. etc.

Comment 23

17 years ago
Why on Earth would you want to run an IRC *server* as part of Mozilla? 

Comment 24

17 years ago
gwalla: Necko can be used in more places than just mozilla the browser no?
Besides DCC and ident for chatzilla needs it.

Comment 25

17 years ago
> Necko can be used in more places than just mozilla the browser no?

True. Just wondering why anyone would want to base an IRC server on necko, 
that's all. DCC is another thing entirely (and why I'm following this bug).
(Assignee)

Comment 26

17 years ago
daniel: i've looked over the patch and one problem i see right off the bat is
that the indentation is all wacked.  be sure that indentation does not consist
of tab characters, as that is required by the mozilla coding standard. 
otherwise, this patch looks very good.  i'll want to do a more thorough review
once it is a bit more readable ;-)  thx for working on this btw!

Comment 27

17 years ago
Created attachment 65927 [details] [diff] [review]
readable patch - cleaned up tabs (andreww@netscape.com)

I took the previous patch and cleaned up the tabs in my editor. I did this with
the intent that the reviewer might better be able to see and make comments on
the patch.  And perhaps help move this bug along :)

Note this attachment  is not from the owner of the bug, just from me to help
make it more readable ....

Comment 28

17 years ago
hi. any activity on this bug?  could the interested parties chime in?  
(Assignee)

Comment 29

17 years ago
i'll take ownership of this bug and try to land the patch for 0.9.9, but no
guarantees ;-)
Assignee: daniel → darin
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P5
Target Milestone: --- → mozilla0.9.9
(Reporter)

Comment 30

17 years ago
Hi,

I'm still interested in finishing this bug as well...
(Assignee)

Comment 31

17 years ago
i'm a bit confused by the state machine impl.  the STS expects Process() to
return BASE_STREAM_WOULD_BLOCK if the socket should remain on the select list. 
it appears that BASE_STREAM_WOULD_BLOCK is never returned from Process() which
would imply that Accept is blocking.  is this correct/intended?  have i
overlooked something that still enables non-blocking Accept?
(Reporter)

Comment 32

17 years ago
> the STS expects Process() to return BASE_STREAM_WOULD_BLOCK if the socket
> should remain on the select list.

Correct.

> it appears that BASE_STREAM_WOULD_BLOCK is never returned from Process() which
> would imply that Accept is blocking.

nsServerSocket::Process returns NS_BASE_STREAM_WOULD_BLOCK during listening
state (nsServerSocket::doListening). After a socket is accepted the server
socket is placed back in the work queue and afterwards in the select list. 
(Assignee)

Comment 33

17 years ago
daniel: thx for clearing that up... i misread the patch :-/

Comment 34

17 years ago
ccing mstoltz for possible security issues. 
(Assignee)

Comment 35

17 years ago
sorry, i don't want to risk this one for mozilla 1.0 ...

i hope everyone on this list will understand the need to limit the addition of
new potentially risky features at this time.
Target Milestone: mozilla0.9.9 → mozilla1.1

Comment 36

17 years ago
I think this should definately be targeted for mozilla 1.0.  From the mozilla
1.0 manifesto:

* A set of promises to keep compatibility with various APIs, broadly construed
(XUL 1.0 is an API), until a 2.0 or higher-numbered major release. All milestone
releases and trunk development between 1.0 and 2.0 will preserve frozen
interface compatibility. Mozilla 1.0 is a greenlight to hackers, corporations,
and book authors to get busy building atop this stable base set of APIs.

Offering a stable server socket support API for 1.0 is IMHO very important for
developers to allow them a large base of users to deploy their software.  If
chatzilla can't get DCC support w/o this, many other projects will be stunted by
the lack of such a feature.  We should promise this feature set to developers.
(Assignee)

Comment 37

17 years ago
-> moz 1.0 (i'm not making any promises that server sockets won't crash moz or
otherwise cause problems... so please don't use this API unless you are willing
to deal w/ being on the cutting edge.)
Target Milestone: mozilla1.1 → mozilla1.0
(Assignee)

Comment 38

17 years ago
Comment on attachment 65927 [details] [diff] [review]
readable patch - cleaned up tabs (andreww@netscape.com)

r=darin
Attachment #65927 - Flags: review+

Comment 39

17 years ago
Here are some questions and comments i've got about the patch:

1. Should nsServerSocket::mService be an owning reference?  right now, it's just
weak.

2. Currently there is not locking around access to the member data in
nsServerSocket...  Do these methods require the same kind of synchronization
that nsSocketTransport requires??  Especially for hte nsIRequest and Process()
methods...
3. Does this work on the Mac?  It's makefiles have not been updated...

4. the #ifdef XP_WIN16 in nsServerSocket is unnecessary because we no longer
support that platform :-)

5. Some comments in nsIServerSocket.idl explaining what the interface and its
methods do would be nice :-)

6. Since nsSTSLayer is a proper base-class (not an interface) it might be
clearer to have *it* implement nsISupports and have nsServerSocket and
nsSocketTransport use the NS_DECL_ISUPPORTS_INHERITIED and
NS_IMPL_ISUPPORTS_INHERITED macros instead...

I'll sr= the patch once i'm sure that 1 & 3 are not issues... All the others are
just suggestions/questions :-)

-- rick
(Assignee)

Comment 40

17 years ago
Comment on attachment 65927 [details] [diff] [review]
readable patch - cleaned up tabs (andreww@netscape.com)

rick: you are right... this patch doesn't at all consider thread safety issues.
 mListener isn't even proxied off the socket transport thread.

mState, for example, needs to be protected by a lock.  it is modified both in
nsServerSocket::Cancel and nsServerSocket::Process.  Process runs on the socket
transport thread and Cancel typically comes in from the UI thread.
Attachment #65927 - Flags: review+ → needs-work+
(Assignee)

Comment 41

17 years ago
punt -> 1.1alpha
Target Milestone: mozilla1.0 → mozilla1.1alpha

Comment 42

17 years ago
Any chance someone could cut a branch with this in so we might start testing 
it out and (in general) helping out?
(Assignee)

Comment 43

17 years ago
the patch isn't ready.  there's not much point to putting it on a branch until
someone has made an effort to make the patch threadsafe.  as is, it is
guaranteed to crash.
(Assignee)

Updated

16 years ago
Target Milestone: mozilla1.1alpha → ---
(Assignee)

Comment 44

16 years ago
mass futuring of untargeted bugs
Target Milestone: --- → Future

Comment 45

16 years ago
Sorry for the spam,

I just wanted to know if this bug was left out in the cold by the recent api 
freezes


Don
(Assignee)

Comment 46

16 years ago
yes, intentionally.  we're not ready to freeze such low-level functionality yet.

Comment 47

16 years ago
Created attachment 109154 [details] [diff] [review]
Updated/cleaned patch for server socket support

I've cleaned up Daniel's original patch, addressing some of the previous
concerns over threadsafety.  This patch is against the current HEAD sources,
compiles cleanly, and (most importantly) it actually works.

Comment 48

16 years ago
I dont want to get my hopes up, but given the new trends in web services, file
sharing, blah blah, it would be very advantageous to begin to enable some of
this stuff - even if it was only some special debug pref for testing, or some
branch that folks could pull and try out.  

Comment 49

16 years ago
Some additional comments on my patch...

1)  I did not remove any nsI* interfaces/methods.  (Daniel's patch had removed
nsISocketTransportService.reuseTransport() and .wakeup(), whereas I kept these.)
 Only public interface changes were adding createServerSocket() and
createServerSocketOfType() to nsISocketTransportService, the nsIServerSocket
interface, and the nsIServerSocketListener interface.

2)  This patch could not realistically be a debug pref -- it adds a layer of
code between the existing client sockets and the socket transport service that
must be present to support server sockets.  It could be a branch, though.

3)  In Daniel's original patch, nsIServerSocket inherited from nsIRequest, and
nsIServerSocketListener inherited from nsIRequestObserver.  It made much more
sense (to me) for both these classes to simply inherit from nsISupports.  Is
this the correct approach?

4)  Any assistance in getting the Mac buildfiles updated would be appreciated.

Finally...  What is the chance of this patch showing up in 1.4?  (1.3 probably
is not realistic, as we're already out of 1.3alpha...)

Comment 50

16 years ago
I need this to implement file transfer for Jabberzilla. The sooner, the better :-)
(Assignee)

Comment 51

16 years ago
Andrew: Thank you for working on this patch; however you should know that we are
in the process of completely rewriting the socket transport.  This patch will
surely require significant revision once that lands. 

Comment 52

16 years ago
Darin:  Thank you for letting me know that.  Might I suggest you include server
socket support in your re-write?  It's really not that complex, and especially
if it's designed in from the start, there won't be (IMHO) ugly kludges such as
nsSTSLayer.

In the meantime, I need server sockets for my own uses, hence the updated patch.
 What's the timeframe for the re-write of the socket layer, and would it be
appropriate to adopt this patch in the meantime?
(Assignee)

Comment 53

16 years ago
andrew: yeah, i have in fact been keeping server socket support in the back of
my mind while working on this rewrite... i agree that it should not be difficult
to add support for it.  the rewrite is planned for moz 1.3 beta, so i think it
might be best to hold off on trying to land this patch.  of course, by saying
that i now better deliver on the promise to include server socket support ;-)

if things go wrong with the rewrite, i'll be sure to revisit the work you have
done.  thanks again!

Comment 54

16 years ago
Is anyone working on updating this patch now that the socket transport stuff 
has landed?  If not, I may try to work on it.

Comment 55

16 years ago
Stewart : please do!

Comment 56

15 years ago
/me rattles the cage.
(Assignee)

Updated

15 years ago
Priority: P5 → --
Target Milestone: Future → mozilla1.6beta
(Assignee)

Comment 57

15 years ago
Created attachment 135009 [details] [diff] [review]
v2.0 patch

okay, this is a fresh patch against the current trunk.	it looks very different
than the other attempts because of how much the socket transport code has
changed.

i've changed the interface around a bit too.  instead or allocating a server
socket from the socket transport service, the server socket is just a component
that can be instantiated using the xpcom component manager.  there are two init
methods that allow the server socket to be configured.	i have left out any
option for specifying a socket type because i just don't see it being used.  i
could be wrong; it is a very simple way of making the socket code more
flexible, so perhaps i will add it.  (PSM certainly doesn't expose SSL server
socket support.)

i've also provided a test program in netwerk/test/TestServ.cpp, which shows how
to setup a simple HTTP-like server.
Attachment #57617 - Attachment is obsolete: true
Attachment #65927 - Attachment is obsolete: true
Attachment #109154 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #135009 - Flags: superreview?(bz-vacation)
Attachment #135009 - Flags: review?(dougt)
Comment on attachment 135009 [details] [diff] [review]
v2.0 patch

+    nsCOMPtr<nsISocketTransportService> sts =
+	 do_GetService(kSocketTransportServiceCID);
+    NS_ASSERTION(sts, "no socket transport service");

would it be better to put this into a ::Init method?

+  // make sure the STS sticks around
+  NS_ADDREF(gSocketTransportService);

you could release it in the destructor
(Assignee)

Comment 59

15 years ago
biesi: thx for looking at this patch!

>+    nsCOMPtr<nsISocketTransportService> sts =
>+	 do_GetService(kSocketTransportServiceCID);
>+    NS_ASSERTION(sts, "no socket transport service");
>
>would it be better to put this into a ::Init method?

no, it should never fail.  both of these XPCOM components are in the same
library.  if GetService on the STS fails from within necko, then things are
really wacked!  a debug assertion should be sufficient, and i'd be happy to
have it crash shortly thereafter.


>+  // make sure the STS sticks around
>+  NS_ADDREF(gSocketTransportService);
>
>you could release it in the destructor

it is released in the destructor.
>it is released in the destructor.

oh, indeed

another comment:
shouldn't base/src/nsServerSocket.h have a license header?
(Assignee)

Comment 61

15 years ago
Created attachment 135093 [details] [diff] [review]
v2.1 patch

biesi: yup, thanks for noticing the missing copyright header.
Attachment #135009 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #135009 - Flags: superreview?(bz-vacation)
Attachment #135009 - Flags: review?(dougt)
(Assignee)

Updated

15 years ago
Attachment #135093 - Flags: superreview?(bz-vacation)
Attachment #135093 - Flags: review?(dougt)
I'm not likely to get to this until next week....

Comment 63

15 years ago
Comment on attachment 135093 [details] [diff] [review]
v2.1 patch

nice work... get it in so we test it out!
Attachment #135093 - Flags: review?(dougt) → review+
Comment on attachment 135093 [details] [diff] [review]
v2.1 patch

>Index: base/public/nsIServerSocket.idl

Fix up the comments as I mentioned?  In particular, make it clear that when you
get a callback the client data lives on the socket transport.

Also, clarify whether notifications after asyncListen and close() are sync or
async (the start listening and stop listening notifications).

>Index: base/src/nsServerSocket.cpp

>+typedef void (nsServerSocket:: *nsServerSocketFunc)(void);

Will this work even though the funcs we pass are private members and the
nsServerSocketEvent struct is not a friend or anything?  I don't see why this
even compiles, given the following:

>+    nsServerSocket *s = (nsServerSocket *) ev->owner;
>+    nsServerSocketEvent *event = (nsServerSocketEvent *) ev;
>+    nsServerSocketFunc func = event->func;
>+    (s->*func)();


>+  EventCleanup(PLEvent *ev)
>+  {
>+    nsServerSocket *s = (nsServerSocket *) ev->owner;
>+    NS_RELEASE(s);
>+    delete ev;

Probably better to |delete (nsServerSocketEvent*) ev;|, no?

>+nsServerSocket::nsServerSocket()
>+  if (!gSocketTransportService)
>+  {
>+    nsCOMPtr<nsISocketTransportService> sts =
>+        do_GetService(kSocketTransportServiceCID);
>+    NS_ASSERTION(sts, "no socket transport service");

This is evil.. comment that the socket transport service constructor sets
gSocketTransportService?

>+nsServerSocket::OnMsgListen()

>+  if (!gSocketTransportService->CanAttachSocket())
>+  {
...
>+  }
>+  //
>+  // ok, we can now attach our socket to the STS for polling
>+  //
>+  nsresult rv = gSocketTransportService->AttachSocket(mFD, this);

Um.... Isn't this a race? If CanAttachSocket() succeeds and then we get
interrupted, we could call AttachSocket when the socket transport service is
not accepting sockets...

Or are we guaranteed that CanAttachSocket is always called off the UI thread
(and that we are on the UI thread)?

>+nsServerSocket::Close()

Document how this this method ends up calling OnStopListening() on the
listener? 

Please document why this works.

>Index: base/src/nsServerSocket.h

>+  PRLock                           *mLock;

Document what accesses should be protected by mLock and why?

sr=bzbarsky with those issues addressed.
Attachment #135093 - Flags: superreview?(bz-vacation) → superreview+
(Assignee)

Comment 65

15 years ago
fixed-on-trunk

thanks for the review comments bz!
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 66

15 years ago
I can get an example which shows how to use a server side socket in JavaScript.
(Assignee)

Comment 67

15 years ago
Biju: yes, i just checked in a JavaScript testcase.  here's the LXR link:

http://lxr.mozilla.org/mozilla/source/netwerk/test/TestServ.js

NOTE: it may take an hour or so before LXR shows the file.  it is there now in
CVS if you want to access it that way.
You need to log in before you can comment on or make changes to this bug.