Closed Bug 937528 Opened 6 years ago Closed 5 years ago

Accepted client tcp socket (mozTcpSocket) has uninitialized host and port

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S5 (4july)

People

(Reporter: hchang, Assigned: hchang)

Details

(Whiteboard: [p=3][ft:ril])

Attachments

(1 file, 2 obsolete files)

When we call mozTcpSocket.listen and then get a accepted client tcp socket, the client socket has uninitialized host and ip property. Those information might be helpful in certain cases.
Assignee: nobody → hchang
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #831344 - Flags: review?(mcmanus)
Comment on attachment 831344 [details] [diff] [review]
Proposed patch

I'm going to ask jason if he has a chance to look at this
Attachment #831344 - Flags: review?(mcmanus) → review?(jduell.mcbugs)
Comment on attachment 831344 [details] [diff] [review]
Proposed patch

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

::: dom/network/src/PTCPSocket.ipdl
@@ +42,5 @@
>    Resume();
>    Close();
>  
> +  sync GetHost() returns (nsString host);
> +  sync GetPort() returns (uint16_t port);

sync IPDL calls should be avoided whenever possible.  Since the host/port should never change once the socket is opened, the right way to do this AFACIT is to send the {host,port} to the child when the socket is opened on the parent.  Ideally this could piggyback as part of the first "Callback" message that is sent to the child.  I'm not clear on why we have only a single "Callback" message to the child (with a union datatype for data/error) versus just having separate "Data" and "Error" messages.  We could 1) change that, so we have a new

  FirstMessage(string host, int port, nsString type, CallbackData data, nsString readyState)

that gets called only for the first callback, or

2) change the CallbackData to have a new type in the union, which contains both data and the host/port (but maybe we want to have host/port even if we send an error message? Seems possible)

3) worst case: on socket open, send a new 

   HostPort(string host, int port)

message to the child, before any "Callback" message are sent.  This sends a separate IPDL message, which is not ideal for performance, but it's not a big deal, and if that's the simplest architecture that's fine.

I hope I have explained the basic idea here: I want the child to always it's own mHost/mPort variables, which we set before we fire any data/error for the socket, and GetHost/Port will just check those local variables instead of doing a sync IPDL call.
Attachment #831344 - Flags: review?(jduell.mcbugs) → feedback+
s/I want the child to always it's own/I want the child to have its own/
Thanks for the review :) Actually, GetHost and GetPort will be called only once at createAcceptedChild(). After that, these two values will store in the returned object. But I definitely agree we should try to avoid sync IPDL call as possible. I'll take a look at your suggestion and try to get rid of them. Thanks.
Attached patch Patch v2 (got rid of sync ipdl) (obsolete) — Splinter Review
Attachment #831344 - Attachment is obsolete: true
Comment on attachment 8349873 [details] [diff] [review]
Patch v2 (got rid of sync ipdl)

The idea of this patch is to construct PTCPSocket on the child side with host and port. So PTCPSocket child side will have mHost/mPort as its attributes, having no need of doing a ipdl call for getting the host/port from TCPSocketParent.
Attachment #8349873 - Flags: review?(jduell.mcbugs)
Comment on attachment 8349873 [details] [diff] [review]
Patch v2 (got rid of sync ipdl)

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

Henry--sorry to take so long to review this.  Looks good to me.

::: dom/network/tests/unit/test_tcpserversocket.js
@@ +175,5 @@
>    server = TCPSocket.listen(PORT, options, BACKLOG);
>    server.onconnect = function(socket) {
> +    // Bug 937528 - Accepted client tcp socket (mozTcpSocket) has
> +    //              uninitialized host and port.
> +    if (socket.host !== '127.0.0.1') {

if there's any way to test the port too, that would be nice.  But OK to skip it if it's not easy to do.

::: netwerk/ipc/PNecko.ipdl
@@ +71,5 @@
>  
>  both:
> +  // Actually we need PTCPSocket() for parent. But ipdl disallows us having different
> +  // signatures on parent and child. So when constructing the parent side object, we just 
> +  // leave host/port unused.

You can 'fix' this if you want by using an IPDL 'union' type. See NeckoChannelParams.ipdlh and the way the union types are used in PNecko.  But I'm also fine with your comment and leaving it the way it is.  That should be fine.  I'm just letting you know for future reference.
Attachment #8349873 - Flags: review?(jduell.mcbugs) → review+
Whiteboard: [p=3][ft:ril]
Thanks for your review! I've rebased it and get try run successful.
Flagged checkin-needed. Thanks!

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

(In reply to Jason Duell (:jduell) from comment #9)
> Comment on attachment 8349873 [details] [diff] [review]
> Patch v2 (got rid of sync ipdl)
> 
> Review of attachment 8349873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Henry--sorry to take so long to review this.  Looks good to me.
> 
> ::: dom/network/tests/unit/test_tcpserversocket.js
> @@ +175,5 @@
> >    server = TCPSocket.listen(PORT, options, BACKLOG);
> >    server.onconnect = function(socket) {
> > +    // Bug 937528 - Accepted client tcp socket (mozTcpSocket) has
> > +    //              uninitialized host and port.
> > +    if (socket.host !== '127.0.0.1') {
> 
> if there's any way to test the port too, that would be nice.  But OK to skip
> it if it's not easy to do.
> 
> ::: netwerk/ipc/PNecko.ipdl
> @@ +71,5 @@
> >  
> >  both:
> > +  // Actually we need PTCPSocket() for parent. But ipdl disallows us having different
> > +  // signatures on parent and child. So when constructing the parent side object, we just 
> > +  // leave host/port unused.
> 
> You can 'fix' this if you want by using an IPDL 'union' type. See
> NeckoChannelParams.ipdlh and the way the union types are used in PNecko. 
> But I'm also fine with your comment and leaving it the way it is.  That
> should be fine.  I'm just letting you know for future reference.
Attached patch Patch v3 (r+'d)Splinter Review
Attachment #8349873 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5d4056c9923a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S5 (4july)
You need to log in before you can comment on or make changes to this bug.