778680 - Make nsISocketTransport constants compatible with nsresult

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Aryeh Gregor from comment #33)
> CCing bz as a networking peer, and Christian Biesinger because he's listed
> as owner and I don't know any of the other peers.  nsISocketTransport.idl
> currently defines
> 
>     const unsigned long STATUS_RESOLVING      = 0x804b0003;
>     const unsigned long STATUS_RESOLVED       = 0x804b000b;
>     const unsigned long STATUS_CONNECTING_TO  = 0x804b0007;
>     const unsigned long STATUS_CONNECTED_TO   = 0x804b0004;
>     const unsigned long STATUS_SENDING_TO     = 0x804b0005;
>     const unsigned long STATUS_WAITING_FOR    = 0x804b000a;
>     const unsigned long STATUS_RECEIVING_FROM = 0x804b0006;
> 
> and then various things use them instead of nsresult.  This breaks when the
> final patch in this series is applied (making nsresult an enum).  Making
> everything that uses these PRUint32 instead more or less works, but it's
> both less informative and less type-safe.  Plus sometimes it leaks out into
> the wider codebase unless I cast a lot of things.
> 
> So what would you suggest?  Is this API used by JS anywhere, or only C++? 
> Could I move the definitions of these class constants to C++, and define
> them as being equal to nsresults defined centrally in the enum?  Or should I
> leave them as-is, but make the C++ equivalents (NS_NET_STATUS_*) into
> nsresults and port all C++ code to use them instead?

(In reply to Boris Zbarsky (:bz) from comment #54)
> > Is this API used by JS anywhere,
> 
> Yes.  The HUD service uses it, and lots of use in extensions (e.g. any
> extension that needs raw socket access, like ChatZilla).
> 
> Can you just file a separate bug on this?  Make sure to cc the active
> network maintainers (so jduell, mayhemer, mcmanus).  Having a discussion in
> this bug is pretty impossible.

Comment 1

[Jason Duell]

> should I leave them as-is, but make the C++ equivalents (NS_NET_STATUS_*) into
> nsresults and port all C++ code to use them instead?

Sounds like the best plan?

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 - Make netwerk status codes actually nsresult

This is a bit of an abuse of nsresult, but better than using PRUint, once nsresult is a class or enum.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 - Fix incorrect nsresult usage in netwerk/

Also, while I'm here.  The only thing here that looks a little dodgy is the OnMsgCancelTransaction changes.  It has to take a PRInt32 to work with PostEvent, but then it treats it as an nsresult, so I need to cast my way out of it, which is unfortunate.  Tell me if you want me to do this some other way.

Comment 4

[Jason Duell]

Attachment: email and chat with biesi

Aryeh:  here's some communications I've had with biesi on this.  Based on that I'm going to +r patch 1.  Have a look over it to make sure it fits with your understanding too.

Comment 5

[Jason Duell]

Comment on attachment 647528 [details] [diff] [review]
Patch part 1 - Make netwerk status codes actually nsresult

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

::: netwerk/base/public/nsISocketTransport.idl
@@ +179,1 @@
>  %}

Seems like we should put a note in the Transport.idl's that C++ must use these #defines instead of the Transport::STATUS_FOO.   I suppose people can find out when they get compiler errors, but it's nice to document things.

Comment 6

[Jason Duell]

Comment on attachment 647530 [details] [diff] [review]
Patch part 2 - Fix incorrect nsresult usage in netwerk/

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1900,5 @@
>                  nsHttpTransaction *temp = trans;
>                  NS_RELEASE(temp); // b/c NS_RELEASE nulls its argument!
>              }
>          }
> +        trans->Close(static_cast<nsresult>(reason));

I have a slight preference to do the cast once, in a

  nsresult closeCode = static_cast<nsresult>(reason)

at the top of the function, then use closeCode.

Otherwise looks fine--thanks!

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Jason Duell (:jduell) from comment #4)
> Created attachment 648026 [details]
> email and chat with biesi
> 
> Aryeh:  here's some communications I've had with biesi on this.  Based on
> that I'm going to +r patch 1.  Have a look over it to make sure it fits with
> your understanding too.

Just to be clear -- in my final patch, the NS_NET_STATUS_* constants will be moved to the actual enum, along with all other nsresults.  They use netwerk error codes 3-11.  This overlaps with NS_ERROR_ALREADY_CONNECTED (= 11) which has a comment saying it's unused and could be removed, and dxr confirms it is indeed unused.  Also with NS_ERROR_MALFORMED_URI (= 10), and NS_BINDING_REDIRECTED (= 3), and NS_BINDING_RETARGETED (= 4), all of which appear to be used.  But multiple names for the same value in the same enum are fine, they'll just be confusing if you do thinks like "if (rv == NS_ERROR_MALFORMED_URI)", so maybe renumbering some of them would make sense.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31d125be7ac5
https://hg.mozilla.org/integration/mozilla-inbound/rev/11d2ba3ec3d1

Comment 9

[Jason Duell]

> maybe renumbering some of them would make sense.

I'd be good with removing NS_ERROR_ALREADY_CONNECTED.  The other symbols all are likely to be used by enough JS that I'd be wary of changing any of their values.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31d125be7ac5
https://hg.mozilla.org/mozilla-central/rev/11d2ba3ec3d1