945533 - Use mozilla/Endian.h for reading/writing data in/to network-endian format

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jeff Walden [:Waldo]]

Necko uses PR_ntohs, PR_ntohl, and PR_ntohll for reading data in host format, and it uses PR_htons, PR_htonl, and PR_htonll for writing data to host format.  This is better and more clearly done using #include "mozilla/Endian.h" functionality these days.

This is basically straightforward changes, just have to be careful about how you compute the address to read/write from/to.  Technically this could be broken down into Necko sub-components, but it doesn't seem worth the bug-filing trouble.  (Patches will be separate, tho.)

I've had a few patches for this lying in my tree for unknown numbers of months now, not sure what else will need doing after they're done -- will keep posting/landing patches until netwerk/ is clean, I guess.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Convert various spdy code

There might be more spdy code to convert (do we keep copying the existing spdy code every time there's a protocol bump or something?), judging by an MXR search skim.  But this is all easy to do in separate bits, might as well clear this bit out while it's done, and not let the overall thing grow too much.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Convert various websocket code

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Convert various serversocket code

Overall try-run for all of these patches, parent rev is a m-i backout so might need repushing, but let's be optimistic:

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

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #0)
> host format.  This is better and more clearly done using #include
> "mozilla/Endian.h" functionality these days.

I don't agree with the assertion. Why do you feel so strongly that way to propose patches on code you don't regularly work on? (that sounds argumentative, but its a sincere question.)

If it were the last vestige of nspr I would eagerly jettison it, but that's hardly the case.. nonetheless weakening our dependence on nspr is the part I like here.

ntoh[ls]/hton[ls] are bog standard network idioms across network programming.. the notion of reading and writing a NetworkEndian class requires a double take... at least from me.

its not a big deal, but I'm just not sure what justifies the churn.

Comment 5

[Jeff Walden [:Waldo]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> Why do you feel so strongly that way to
> propose patches on code you don't regularly work on? (that sounds
> argumentative, but its a sincere question.)

A lot of this is to make it easier for new contributors, or even for Gecko hands new to Necko:

* having only one, well-named way to do a common thing like deal with byte order
* having that way exist in mfbt/ (not NSPR) so that if you want to do something common, in the long run you'll be able to check one place rather than many
* seeing the input or output type in the relevant method names (s/l/ll are very unclear unless you're an old C hand, and really I doubt most people are these days)
* the improved readability and approachability (to new and old contributors alike) from using that one way
* most uses here will byte-swap an already-read value, so something like |aID| has multiple interpretations over time, due to its having different byte orderings over time -- extra mental complexity to consider ("does this value *now* have the byte-ordering I want it to have, or no?") versus all variables having host byte order

You may know what PR_ntohll means on first read.  I think it's doubtful that people who haven't done network coding using C APIs could guess on first read-through.  That's much less true for a call including "readUint32" or similar with clear endianness.

Beyond improvements for the newer reader:

* removing uses of prnetdb.h (this removes a few, an improvement if a small one) aids us in working toward fixing bug 796941 (prnetdb.h includes prtypes.h)
* the change eliminates dubiously-safe aliasing performed under the hood in the NSPR functions
* general line count reduction from not needing a memcpy prior to swapping, etc.
* Endian.h byte-swapping methods should be inlined by a good compiler, but the NSPR boundary means you need intelligent link-time codegen (able to unify a call *plus* surrounding code into the low-level assembly) to get the same benefits

I expect froydnj could add to this list, too, as he added this in bug 798172, and blogged about it as well: <https://blog.mozilla.org/nfroyd/2013/04/11/introducing-mozillaendian-h/>.

(Regarding NetworkEndian specifically: when Endian.h was introduced, we naturally included BigEndian and LittleEndian classes.  NetworkEndian was added as a typedef of BigEndian specifically to make networking code, thinking in terms of "network" byte order in specs, more consistent with that thinking.  Speaking for myself, having written networking code from time to time if not much recently, that's what I'd want.  But it's possible we misjudged that.  Comments from others besides me appreciated on this.)

Comment 6

[Jason Duell]

Comment on attachment 8341414 [details] [diff] [review]
Convert various websocket code

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

FWIW I think a patch that uses "NetworkEndian" would be fine (it does read more cleanly). As long as someone contributes a complete version of that patch (as opposed to us writing it) :)

Final say is Patrick's, of course, so I'll hold off on marking anything +r until/unless he agrees.

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8341413 [details] [diff] [review]
Convert various spdy code

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

I really don't care for this - it substitutes well known networking code idioms for newish gecko specific ones.

But in the name of removing nspr dependence (which I am fond of!) and gecko unity I think it ought to go forward.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8341415 [details] [diff] [review]
Convert various serversocket code

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

Looks good. Just add a couple of static_casts for clarity please. r=me

::: netwerk/base/src/nsUDPSocket.cpp
@@ +521,4 @@
>    else
>      return NS_ERROR_NOT_INITIALIZED;
>  
> +  *aResult = NetworkEndian::readUint16(&port);

Can you add a "static_cast<int32_t>" here please, mainly for clarity. Same with the change in nsServerSocket.cpp.

Comment 9

[Jason Duell]

Comment on attachment 8341414 [details] [diff] [review]
Convert various websocket code

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

FWIW I think a patch that uses "NetworkEndian" would be fine (it does read more cleanly). As long as someone contributes a complete version of that patch (as opposed to us writing it) :)

Final say is Patrick's, of course, so I'll hold off on marking anything +r until/unless he agrees.

Comment 10

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd377ba90f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c982e84c0a00
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9a20acba5d

More patches to come for the rest of the uses of these methods, stay tuned.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fd377ba90f5
https://hg.mozilla.org/mozilla-central/rev/c982e84c0a00
https://hg.mozilla.org/mozilla-central/rev/3d9a20acba5d

Comment 12

[Patrick McManus [:mcmanus]]

more patches should go in a new bug at this point