Closed Bug 1203503 Opened 9 years ago Closed 6 years ago

ICE/TURN/TCP via an HTTP-Proxy does not support Authentication

Categories

(Core :: WebRTC: Networking, defect, P3)

Product:
Core
Component:
WebRTC: Networking
Version:
40 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: tsohr, Assigned: bwc)

References

Details

Attachments

(2 files, 19 obsolete files)

part 2. replace proxy tunnel with new ipc-based tunnel
158.48 KB, patch
bwc
: review+
Details | Diff | Splinter Review
part 1. change necko to allow CONNECT-only requests
34.32 KB, patch
bwc
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36

Steps to reproduce:

Configure ICE to use a turn server with tcp-transport. Block all udp ports in your firewall.


Actual results:

A response like
HTTP/1.1 407 Proxy Authentication Required
Proxy-Authenticate: NEGOTIATE
Proxy-Authenticate: NTLM
Cache-Control: no-cache
Pragma: no-cache
Content-Type: text/html; charset=utf-8
Proxy-Connection: close
Connection: close
Content-Length: 813

is not handled correctly bye the turn/tcp/proxy stack (nr_proxy_tunnel.c).


Expected results:

/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c should add Proxy-Authorization headers to the request.
Component: Untriaged → Networking
Product: Firefox → Core
Whiteboard: [http-conn]
Whiteboard: [http-conn] → [http-conn][necko-backlog]
Status: UNCONFIRMED → NEW
Component: Networking → WebRTC: Networking
Ever confirmed: true
Whiteboard: [http-conn][necko-backlog]
backlog: --- → webrtc/webaudio+
Rank: 27
Priority: -- → P2
Byron, Nils -- I recently received a request to bump this bug up in priority.  Can you give me a rough estimate of the work needed to implement this?
Flags: needinfo?(drno)
Flags: needinfo?(docfaraday)
This bug actually does not specify which auth mechanisms we should support.
I think adding support for Basic and Digest should be a matter of a few days.
But adding support for NTLM, Negotiate or even Kerberos is probably quite a bit more challenging.
Flags: needinfo?(drno)
The description seems to involve the harder stuff that Nils mentions. I suspect that if we want to get support for the more advanced stuff, we're going to want to use the proxy stuff in Gecko instead of the hand-rolled support down in nICEr.
Flags: needinfo?(docfaraday)
Thanks team for giving this one some more thought. TokBox is seeing an increase in WebRTC adoption from enterprises with endpoints operating in highly restrictive networks. Proxy servers are frequently a part of the network topology and authenticated proxies are not uncommon. NTLM support would not add much value because of security concerns. I hope you will consider supporting something more secure. TokBox is hoping to have support for this functionality in the OpenTok IE plug-in, Chrome, and FireFox in the not too distant future. Let me know if/how I can help.
(In reply to Rob Hainer from comment #5)
> TokBox is seeing an
> increase in WebRTC adoption from enterprises with endpoints operating in
> highly restrictive networks. Proxy servers are frequently a part of the
> network topology and authenticated proxies are not uncommon.

Rob, do you have any insight into what kind authentication scheme these proxies use?

> NTLM support
> would not add much value because of security concerns.

Could you please clarify your concerns here?
I would think that most users, especially in enterprise networks, don't have control over which authentication method the corporate proxy supports/uses. So if the IT departments only configure NTLM support on their proxies I guess we would need NTLM auth support.

> I hope you will
> consider supporting something more secure.

I'm not an expert on the topic, but NTLM being a challenge response based auth mechanism appears to make it at least more secure then HTTP basic auth. And my only guess would be that Kerberos might be more secure. But again I'm not expert on this.

According to our this Telemetry data:
https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-05-25&keys=__none__!__none__!__none__&max_channel_version=nightly%252F49&measure=HTTP_AUTH_TYPE_STATS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-05-04&table=0&trim=1&use_submission_date=0

it looks like we should add support for basic and digest first as that is ~80% of the auth schemes. But it looks like that Telemetry probe is only looking for 401 responses and not for the 407 Proxy authentications.
We are typically interfacing with IT departments to solve connectivity issues they have when trying to make WebRTC applications work for end points in their environment. We do usually have some insight into the their requirements for working with the proxy they have in place. I will do a bit of research into what we have come across and get back to you.

I'm far from an expert on this stuff too but my understanding is there is something about the way NTLM hash generation works that is not considered secure. I'm guessing you have people at Mozilla that understand this way better than I. 

Starting with Basic auth might be a good first step that would open some doors but I need to do a bit of research.
Nils - I did a bit of research with our partners and seems like the functionality provided by the Blue Coat web gate product is representative of what is commonly used in corporate enterprise networks that have authenticated proxy servers. For integrated windows authentication (IWA) they support Basic, NTLM, and Kerberos credentials. I don't know if Basic is viable. My understanding is that basic auth requires a prompt for username and password. We do have one partner that explicitly does not allow NTLM due to weak encryption security concerns. Targeting Kerberos would definitely be best but I understand that may also be the most difficult.
Any updates on this item? How can we push this forward?
Could you please provide an update on this?
Work on a new HTTP proxy client has been started in bug 1287225.
See Also: → 1287225
Hi,

Can you give an estimate date of proxy authen working with TURN server (not STURN) ?

this is a typical scenario in company where you are obliged to use a HTTP(S) proxy with (basic most of the time) authentication to get out on the NET.

Firefox works far better than chrome in decoding RTP packet in webrtc (less delay, no stuttering) so I'm mad not being able to use FF instead of chrome
Any updates? more than a year now...
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
I see Education First has kindly posted a generous bounty on this issue, through bountysource.

To EF: Is it possible to get a clarification on the scope you have in mind. All common authentication methods? Just some subset?
Hi Book Moons,

Support for NTLM & Kerberos authentication with Firefox is what we are looking for. Corporate proxies sometimes use these types of authentication and this is a non-starter for accessing WebRTC content.
So, I did look into it and decided that it's too big a time investment for me to implement properly.

Still, here's some things I found to be of interest to fellow hackers:

- The code in question is mostly contained to /media/mtransport folder. The WebRTC transport layer is mostly a wrapper for an obsolete fork of nICEr, a library from reSIProcate project. The upstream doesn't support proxy auth as of yet, though, so it's of no help.

- There are generic NTLM and Kerberos auth implementations, those can be found in /extensions/auth folder. They are seemingly incomplete (I've found neither auth by certificate or server verification features), but those are a good starting point.

- The Firefox's transport layer, Necko (or netwerk as it's called internally) isn't used at all, though there have been some plans to merge those two years ago. Therefore most of the HTTP code used is kludgy and only serves to negotiate a protocol switch.

My problem is, I can't figure out how to even approach isolating and testing this code, so I can't in good conscience offer untested code for review. Otherwise the task doesn't seem like it'll take two or three weeks and I'll be very surprised if this won't be done in a month or two. I apologize if I got anyone's hopes up, though.
(In reply to ggbright from comment #18)
> So, I did look into it and decided that it's too big a time investment for
> me to implement properly.
> 
> Still, here's some things I found to be of interest to fellow hackers:
> 
> - The code in question is mostly contained to /media/mtransport folder. The
> WebRTC transport layer is mostly a wrapper for an obsolete fork of nICEr, a
> library from reSIProcate project. The upstream doesn't support proxy auth as
> of yet, though, so it's of no help.

You have this backwards. Mozilla are the primary people maintaining nICEr.
Since the bounty for this has been raised it might be helpful to clarify the current state and what is needed to get this fixed:

Currently code in nr_proxy_tunnel.c sends out a simple HTTP Connect, and if a 200 Ok comes back it will use the Proxy connection to talk to the TURN service over TCP. If any other HTTP response comes back the current code simply gives up.
Now trying to teach that code in nr_proxy_tunnel.c to handle authentication first of all means re-inventing the wheel, since we have already HTTP authentication code in Necko, which is used for regular HTTP requests. Even if one would put authentication code there the problem would still be where to get the username and password from.
The nICEr code has no means to show a modal dialog asking for credentials. Nor can it ask the Necko code for the potentially already entered proxy credentials.

What I think needs to happen to get this fixed:

1) The Necko proxy code as I have been told is written so that it assumes that everything what gets send to and received from a proxy are HTTP requests and responses. Since that it not how WebRTC uses a HTTP proxy, that Necko code first needs to get changed to support two modes: the existing HTTP only mode and a new "get me a raw pipe through the proxy" mode, similar to the upgrade of a WebSocket connection.

2) Once the Necko code supports the raw pipe through proxies with authentication mode a little bit of interface code needs to get written to allow WebRTC to simply request such a raw pipe. WebRTC should either get such a pipe, or a simple error back.

3) Since the WebRTC networking code runs in the content process and Necko in the parent process now new IPC code needs to get written to allow the content process to requests such a raw pipe from Necko.

The alternative is to wait until WebRTCs mtransport code moves in with the Necko code into it's own process. If that was the case already you could simply skip step 3. That is also the reason why this hasn't made much progress, as step 3 is kind of thro away effort once the mtransport code moves.

And lastly the existing proxy code in nr_proxy_tunnel.c should get removed. Also all of the proxy detection code needs to get removed.

And then the new raw proxy pipe code needs to get wired into the nICEr stack.
(In reply to Nils Ohlmeier [:drno] from comment #20)
> The nICEr code has no means to show a modal dialog asking for credentials.

What about non-interactive authentication? NTLM and Kerberos probably can do that for current logged-in user.
(In reply to EF from comment #17)
> Hi Book Moons,
> 
> Support for NTLM & Kerberos authentication with Firefox is what we are
> looking for. Corporate proxies sometimes use these types of authentication
> and this is a non-starter for accessing WebRTC content.

Hi EF,

I think it is possible to implement _non-interactive_ authentication flow for NTLM & Kerberos, does it cover your use cases?
Just so that I'm understanding this correctly:
1) There is some proxy service (presumably something that comes with Windows Server)
2) Clients can not use WebRTC based software (e.g. Google Hangouts) while connected to the proxy service
If this is not correct, please provide an example.
(In reply to tair.sabirgaliev from comment #21)
> (In reply to Nils Ohlmeier [:drno] from comment #20)
> > The nICEr code has no means to show a modal dialog asking for credentials.
> 
> What about non-interactive authentication? NTLM and Kerberos probably can do
> that for current logged-in user.

I'm not an expert on how NTLM or Kerberos authentication exactly work. But since you get a 401/407 response I assume that you need to retry the CONNECT request with some additional header in it. And since the Necko code has no interface to query for the current Kerberos token for example I don't see how you would generate such a header.

Besides as I mentioned before reinventing the wheel by adding authentication code to nr_proxy_tunnel.c is not a desirable solution here.
(In reply to saif.tareq from comment #23)
> Just so that I'm understanding this correctly:
> 1) There is some proxy service (presumably something that comes with Windows
> Server)

No this has nothing to do with Windows Server. There is a HTTP Proxy somewhere in the network and Firefox is configured to use that HTTP proxy for connecting to web servers.

> 2) Clients can not use WebRTC based software (e.g. Google Hangouts) while
> connected to the proxy service

Firefox can in fact use WebRTC service if such a HTTP proxy is configured. But if that HTTP proxy requires authentication then Firefox can't use the proxy.
The existing codebase includes implementations of the 4 common auth protocols and supporting services. They are written such that they can be reused. Implementing...
Lots of companies are using authenticated proxy and cannot use webrtc on Firefox.
There's an ETA?
Thanks for the clarifications. I may be able to implement this feature as well but I need a way to recreate the problem.

I researched different HTTP proxies but couldn't find a de facto standard.

What's the easiest way to create an environment that reproduces the problem? (Names of softwares will suffice. I can probably handle configuration myself)
I wasn't able to successfully create an environment for lack of time but I spent few minutes to gather information and squid or dante seems quite a good start to setup a proxy
Necko does not allow for a CONNECT only request to happen.  This adds a flag
to signal an http channel should only CONNECT if proxied.  This flag can only be
set if an HTTPUpgrade handler has been assigned.

In order to support xpcshell-test for this change, nsHttpConnectionMgr does a
little check to see if HTTPUpgrade callback is in JavaScript.  If it is then
the callback is invoked on the main thread.
This only replaces the tunnel using a new nr_socket implementation.  The actual
proxy detection code has not been touched.  The proxy url is retrieved but
address resolution is done by necko code in the parent process.  The new socket
wraps a WebrtcProxyChannel which uses necko to handle proxy negotiation.  This
has a happy side effect of enabling all authentication modes that necko already
supports for http proxies.
Just landed a couple of large commits. :drno, would you be able to review these?
Attachment #8983504 - Flags: review?(docfaraday)
Comment on attachment 8983505 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

I can take these reviews.
Attachment #8983505 - Flags: review?(docfaraday)
Comment on attachment 8983504 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

This is strictly necko stuff, sending it their way.
Attachment #8983504 - Flags: review?(docfaraday) → review?(mcmanus)
Comment on attachment 8983504 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

I haven't read the code, but the comment seems reasonable :)
Attachment #8983504 - Flags: review?(mcmanus) → review?(honzab.moz)
Comment on attachment 8983505 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

Needs necko review also.
Attachment #8983505 - Flags: review?(honzab.moz)
Comment on attachment 8983505 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

Let's change the approach here. Instead of messing around with this nr_socket_wrapper stuff, let's just go straight to the point, and override the ice_ctx's socket factory directly to manufacture what we need.

First, let's modify NrIceCtx::CreateSocket (and modify/rename nr_socket_local_create here: https://searchfox.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#2197) to take an additional param that contains the necessary proxy config, if set.

Using this modified/renamed version of nr_socket_local_create, we make a new socket factory using nr_socket_factory_create_int (here's an example of some code that does this):

https://searchfox.org/mozilla-central/source/media/mtransport/test_nr_socket.cpp#194

Now that we have the new factory, use nr_ice_ctx_set_socket_factory to override the default factory right after we create the ice_ctx (eg; here: https://searchfox.org/mozilla-central/source/media/mtransport/nricectx.cpp#615)

Now, modifying NrIceCtx::CreateSocket is going to break TestNat in a couple of places, but as long as NrIceCtx::SetNat passes the necessary config along to TestNat, that will be easy to fix.

::: media/mtransport/nricectx.cpp
@@ +44,5 @@
>  #include <string>
>  #include <vector>
>  
> +// at top due to defines
> +#include "mozilla/dom/TabChild.h"

What do we need this include for?

@@ +69,5 @@
>  #include "nsIPrefService.h"
>  #include "nsIPrefBranch.h"
> +#include "nsITabChild.h"
> +#include "nsIDocShell.h"
> +#include "nsGlobalWindowInner.h"

I don't see where we use these.

::: media/mtransport/nricectx.h
@@ +53,4 @@
>  #include <string>
>  #include <vector>
>  
> +#include "nsIScriptGlobalObject.h"

Why do we need this if we're forward declaring everything new in this file?

@@ -324,4 @@
>  
>    // Provide the proxy address. Must be called before
>    // StartGathering.
> -  nsresult SetProxyServer(const NrIceProxyServer& proxy_server);

We can remove the NrIceProxyServer class now, right?

::: media/mtransport/proxy_tunnel.h
@@ +70,5 @@
> +{
> +
> +class ProxyTunnelData;
> +
> +class ProxyTunnel : public NrSocketBase,

Let's call this NrSocketProxy, and name the files nr_socket_proxy.h/cpp

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +297,5 @@
> +    AsInner())->
> +    GetDocShell()->
> +    GetDocument();
> +  TabChild* tabChild = static_cast<TabChild*>(
> +    document->GetDocShell()->GetTabChild().take());

Can we use TabChild::GetFrom(GetPC()->GetWindow()) here instead?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +26,4 @@
>  struct RTCInboundRTPStreamStats;
>  struct RTCOutboundRTPStreamStats;
>  class MediaStreamTrack;
> +class PBrowserOrId;

We use this below, so I'm guessing we must be picking up the actual definition by including WebrtcProxyChannelChild?
Attachment #8983505 - Flags: review?(docfaraday)
(In reply to Byron Campen [:bwc] from comment #37)
> Comment on attachment 8983505 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8983505 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thank you for the review and finding reviewers for me!

> Let's change the approach here. Instead of messing around with this
> nr_socket_wrapper stuff, let's just go straight to the point, and override
> the ice_ctx's socket factory directly to manufacture what we need.
> 
> First, let's modify NrIceCtx::CreateSocket (and modify/rename
> nr_socket_local_create here:
> https://searchfox.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.cpp#2197) to take an additional param that contains the
> necessary proxy config, if set.
> 
> Using this modified/renamed version of nr_socket_local_create, we make a new
> socket factory using nr_socket_factory_create_int (here's an example of some
> code that does this):
> 
> https://searchfox.org/mozilla-central/source/media/mtransport/test_nr_socket.
> cpp#194
> 
> Now that we have the new factory, use nr_ice_ctx_set_socket_factory to
> override the default factory right after we create the ice_ctx (eg; here:
> https://searchfox.org/mozilla-central/source/media/mtransport/nricectx.
> cpp#615)
> 
> Now, modifying NrIceCtx::CreateSocket is going to break TestNat in a couple
> of places, but as long as NrIceCtx::SetNat passes the necessary config along
> to TestNat, that will be easy to fix.
> 

I think I'll also have to modify nICEr code to support the new nr_socket_local_create signature.

Additional steps:

Change nr_ice_ctx_'s turn_tcp_socket_wrapper to a new struct/class to containing the config params (PBrowserOrId and nsIPrincipal).  This will be a flag for NrIceCtx::CreateSocket to create an NrSocketProxy when asked for a TCP socket; An unset flag with create a standard TCP socket even if a proxy is set. (related: https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#595)

Modify everything using nr_socket_local_create's signature (see: https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_ctx.c#76).  Update every call of nr_socket_factory_create_socket to use the new signature.

> ::: media/mtransport/nricectx.cpp
> @@ +44,5 @@
> >  #include <string>
> >  #include <vector>
> >  
> > +// at top due to defines
> > +#include "mozilla/dom/TabChild.h"
> 
> What do we need this include for?
> 
> @@ +69,5 @@
> >  #include "nsIPrefService.h"
> >  #include "nsIPrefBranch.h"
> > +#include "nsITabChild.h"
> > +#include "nsIDocShell.h"
> > +#include "nsGlobalWindowInner.h"
> 
> I don't see where we use these.
> 

Don't need, will remove.

> ::: media/mtransport/nricectx.h
> @@ +53,4 @@
> >  #include <string>
> >  #include <vector>
> >  
> > +#include "nsIScriptGlobalObject.h"
> 
> Why do we need this if we're forward declaring everything new in this file?
> 

Don't need, will remove.

> @@ -324,4 @@
> >  
> >    // Provide the proxy address. Must be called before
> >    // StartGathering.
> > -  nsresult SetProxyServer(const NrIceProxyServer& proxy_server);
> 
> We can remove the NrIceProxyServer class now, right?
> 

Yes.  NrIceProxyServer, PeerConnectionMedia::ProtocolProxyQueryHandler::SetProxyOnPcm, and mProxyServer can be updated.  I'll put that in another commit, if that's ok?

> ::: media/mtransport/proxy_tunnel.h
> @@ +70,5 @@
> > +{
> > +
> > +class ProxyTunnelData;
> > +
> > +class ProxyTunnel : public NrSocketBase,
> 
> Let's call this NrSocketProxy, and name the files nr_socket_proxy.h/cpp
> 

OK.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +297,5 @@
> > +    AsInner())->
> > +    GetDocShell()->
> > +    GetDocument();
> > +  TabChild* tabChild = static_cast<TabChild*>(
> > +    document->GetDocShell()->GetTabChild().take());
> 
> Can we use TabChild::GetFrom(GetPC()->GetWindow()) here instead?
> 

Yes.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
> @@ +26,4 @@
> >  struct RTCInboundRTPStreamStats;
> >  struct RTCOutboundRTPStreamStats;
> >  class MediaStreamTrack;
> > +class PBrowserOrId;
> 
> We use this below, so I'm guessing we must be picking up the actual
> definition by including WebrtcProxyChannelChild?

That's pulled in from WebrtcProxyChannelChild.h and can be removed.
(In reply to Paul Vitale from comment #38)
> (In reply to Byron Campen [:bwc] from comment #37)
> > Comment on attachment 8983505 [details] [diff] [review]
> > part 2. replace proxy tunnel with new ipc-based tunnel
> > 
> > Review of attachment 8983505 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> 
> Thank you for the review and finding reviewers for me!
> 
> > Let's change the approach here. Instead of messing around with this
> > nr_socket_wrapper stuff, let's just go straight to the point, and override
> > the ice_ctx's socket factory directly to manufacture what we need.
> > 
> > First, let's modify NrIceCtx::CreateSocket (and modify/rename
> > nr_socket_local_create here:
> > https://searchfox.org/mozilla-central/source/media/mtransport/
> > nr_socket_prsock.cpp#2197) to take an additional param that contains the
> > necessary proxy config, if set.
> > 
> > Using this modified/renamed version of nr_socket_local_create, we make a new
> > socket factory using nr_socket_factory_create_int (here's an example of some
> > code that does this):
> > 
> > https://searchfox.org/mozilla-central/source/media/mtransport/test_nr_socket.
> > cpp#194
> > 
> > Now that we have the new factory, use nr_ice_ctx_set_socket_factory to
> > override the default factory right after we create the ice_ctx (eg; here:
> > https://searchfox.org/mozilla-central/source/media/mtransport/nricectx.
> > cpp#615)
> > 
> > Now, modifying NrIceCtx::CreateSocket is going to break TestNat in a couple
> > of places, but as long as NrIceCtx::SetNat passes the necessary config along
> > to TestNat, that will be easy to fix.
> > 
> 
> I think I'll also have to modify nICEr code to support the new
> nr_socket_local_create signature.


   Nah, that's why we're renaming our existing implementation of nr_socket_local_create, so it does not collide. We'll probably still need to have some implementation of nr_socket_local_create, and maybe it can simply call our new function with nullptr as the extra param (so it ends up doing what it does now maybe). The aim here is to do everything without modifying any nICEr code (except maybe to delete some of the wrapper stuff, since we won't be using that approach any longer). Look at TestNat for an example of a nICEr socket factory that overrides what is used by default; that's the model we're trying to use.


> Additional steps:
> 
> Change nr_ice_ctx_'s turn_tcp_socket_wrapper to a new struct/class to
> containing the config params (PBrowserOrId and nsIPrincipal).  This will be
> a flag for NrIceCtx::CreateSocket to create an NrSocketProxy when asked for
> a TCP socket; An unset flag with create a standard TCP socket even if a
> proxy is set. (related:
> https://searchfox.org/mozilla-central/source/media/mtransport/third_party/
> nICEr/src/ice/ice_component.c#595)
> 

Let's not mess with that. PBrowserOrId and nsIPrincipal can live in a c++ class that only our factory create method knows about (and NrIceCtx::CreateSocket might know about this class too depending on how you pass the params).

> Modify everything using nr_socket_local_create's signature (see:
> https://searchfox.org/mozilla-central/source/media/mtransport/third_party/
> nICEr/src/ice/ice_ctx.c#76).  Update every call of
> nr_socket_factory_create_socket to use the new signature.

   Again, we're aiming to avoid changing nICEr; we should be able to override the factory from c++ entirely.

> > @@ -324,4 @@
> > >  
> > >    // Provide the proxy address. Must be called before
> > >    // StartGathering.
> > > -  nsresult SetProxyServer(const NrIceProxyServer& proxy_server);
> > 
> > We can remove the NrIceProxyServer class now, right?
> > 
> 
> Yes.  NrIceProxyServer,
> PeerConnectionMedia::ProtocolProxyQueryHandler::SetProxyOnPcm, and
> mProxyServer can be updated.  I'll put that in another commit, if that's ok?

I guess that's alright.
Yes, this definitely needs a necko peer review, thanks.  I only overlooked the patches quickly.  I think I get the idea but there will probably need to be few tweaks done.  E.g. adding the new flag on nsHttpConnectionInfo is definitely wrong.  And I think this can be done with fewer lines of code as well.  But I have to go through the patches and the existing code more in detail.

Could you please resubmit the patches with 8 lines context in the meantime?  It's a standard for patch submission.  Thanks.

This is not the best time for me now to review this and suggest the right course, getting quite busy lately.  So, please give me few weeks.
Necko does not allow for a CONNECT only request to happen.  This adds a flag
to signal an http channel should only CONNECT if proxied.  This flag can only be
set if an HTTPUpgrade handler has been assigned.

In order to support xpcshell-test for this change, nsHttpConnectionMgr does a
little check to see if HTTPUpgrade callback is in JavaScript.  If it is then
the callback is invoked on the main thread.
Attachment #8983504 - Attachment is obsolete: true
Attachment #8983504 - Flags: review?(honzab.moz)
Attachment #8984335 - Flags: review?(honzab.moz)
(In reply to Byron Campen [:bwc] from comment #39)
> (In reply to Paul Vitale from comment #38)
> > (In reply to Byron Campen [:bwc] from comment #37)
> > > Comment on attachment 8983505 [details] [diff] [review]
> > > part 2. replace proxy tunnel with new ipc-based tunnel
> > > 
> > > Review of attachment 8983505 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > 
> > Thank you for the review and finding reviewers for me!
> > 
> > > Let's change the approach here. Instead of messing around with this
> > > nr_socket_wrapper stuff, let's just go straight to the point, and override
> > > the ice_ctx's socket factory directly to manufacture what we need.
> > > 
> > > First, let's modify NrIceCtx::CreateSocket (and modify/rename
> > > nr_socket_local_create here:
> > > https://searchfox.org/mozilla-central/source/media/mtransport/
> > > nr_socket_prsock.cpp#2197) to take an additional param that contains the
> > > necessary proxy config, if set.
> > > 
> > > Using this modified/renamed version of nr_socket_local_create, we make a new
> > > socket factory using nr_socket_factory_create_int (here's an example of some
> > > code that does this):
> > > 
> > > https://searchfox.org/mozilla-central/source/media/mtransport/test_nr_socket.
> > > cpp#194
> > > 
> > > Now that we have the new factory, use nr_ice_ctx_set_socket_factory to
> > > override the default factory right after we create the ice_ctx (eg; here:
> > > https://searchfox.org/mozilla-central/source/media/mtransport/nricectx.
> > > cpp#615)
> > > 
> > > Now, modifying NrIceCtx::CreateSocket is going to break TestNat in a couple
> > > of places, but as long as NrIceCtx::SetNat passes the necessary config along
> > > to TestNat, that will be easy to fix.
> > > 
> > 
> > I think I'll also have to modify nICEr code to support the new
> > nr_socket_local_create signature.
> 
> 
>    Nah, that's why we're renaming our existing implementation of
> nr_socket_local_create, so it does not collide. We'll probably still need to
> have some implementation of nr_socket_local_create, and maybe it can simply
> call our new function with nullptr as the extra param (so it ends up doing
> what it does now maybe). The aim here is to do everything without modifying
> any nICEr code (except maybe to delete some of the wrapper stuff, since we
> won't be using that approach any longer). Look at TestNat for an example of
> a nICEr socket factory that overrides what is used by default; that's the
> model we're trying to use.
> 

What about passive tcp sockets?  Wouldn't this make nICEr only get a NrSocketProxy when a proxy is used?  

However, it looks like NrTcpSocketIpc doesn't support listen at all.

https://searchfox.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#1954
(In reply to Paul Vitale from comment #42)
> What about passive tcp sockets?  Wouldn't this make nICEr only get a
> NrSocketProxy when a proxy is used?  
> 
> However, it looks like NrTcpSocketIpc doesn't support listen at all.
> 
> https://searchfox.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.cpp#1954

You are right that TCP in e10s mode, which is the NrTcpSocketIpc doesn't support listening. The feature of being able to listen for incoming connections is not high on our priority list. And for proxies we definitely skip it.
> What about passive tcp sockets?  Wouldn't this make nICEr only get a
> NrSocketProxy when a proxy is used?  

Yeah, that's the notion. Passive TCP can't work when using a proxy anyhow.

> However, it looks like NrTcpSocketIpc doesn't support listen at all.
> 
> https://searchfox.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.cpp#1954
Comment on attachment 8984335 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

Please provide an overview description of what the patch is doing, and also add comments to the code which you are changing.  I want you to comment WHY you do things the way you do them.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1254,1 @@
>              }

these changes need good comments why you do what you do!

@@ -1785,5 @@
>          // or else we'd end up closing the socket prematurely.
>          NS_ERROR("bad ReadSegments implementation");
>          return NS_ERROR_FAILURE; // stop iterating
>      }
> -

leave it

@@ +1856,5 @@
>              LOG(("  No Transaction In OnSocketWritable\n"));
> +        } else if (NS_SUCCEEDED(rv) && mOnlyConnectToProxy) {
> +            rv = ResumeRecv();
> +
> +            break;

again, add comments!

@@ -1971,5 @@
>  nsresult
>  nsHttpConnection::OnSocketReadable()
>  {
>      LOG(("nsHttpConnection::OnSocketReadable [this=%p]\n", this));
> -

as well, please no such blank line changes

::: netwerk/protocol/http/nsHttpConnectionInfo.h
@@ +41,5 @@
>                           const nsACString &username,
>                           nsProxyInfo *proxyInfo,
>                           const OriginAttributes &originAttributes,
> +                         bool endToEndSSL = false,
> +                         bool onlyConnectToProxy = false);

my main concern is that this flag MUST NOT be added on connection info.  it doesn't belong here.

instead add a new connection flag to the list at https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/netwerk/protocol/http/nsHttp.h#60-108

and use it in nsHttpTransaction::SetConnection (which is something I need to double check on too..., since you somehow need to tell your transaction to use a new connection, I forgot how to do it... but there should be a way)

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1685,5 @@
>              }
>              break;
>          }
>  
> +        mNoContent = mNoContent || mConnInfo->OnlyConnectToProxy();

this needs a good explanatory comment!
Attachment #8984335 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #45)
> Comment on attachment 8984335 [details] [diff] [review]
> part 1. change necko to allow CONNECT-only requests
> 

Thanks for the review!

> Review of attachment 8984335 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please provide an overview description of what the patch is doing, and also
> add comments to the code which you are changing.  I want you to comment WHY
> you do things the way you do them.
> 

Definitely needs some comments.  I'll update the patch description too.

> @@ -1971,5 @@
> >  nsresult
> >  nsHttpConnection::OnSocketReadable()
> >  {
> >      LOG(("nsHttpConnection::OnSocketReadable [this=%p]\n", this));
> > -
> 
> as well, please no such blank line changes
> 

Got it.  Didn't notice these when I was reviewing the diff.

> ::: netwerk/protocol/http/nsHttpConnectionInfo.h
> @@ +41,5 @@
> my main concern is that this flag MUST NOT be added on connection info.  it
> doesn't belong here.
> 
> instead add a new connection flag to the list at
> https://searchfox.org/mozilla-central/rev/
> c621276fbdd9591f52009042d959b9e19b66d49f/netwerk/protocol/http/nsHttp.h#60-
> 108
> 

OK.  I needed to propagate the flag to the transaction and connection and decided to tack it on connection info.  Looks like I can accomplish the same thing using mCaps and mTransactionCaps in transaction and connection, respectively.

> and use it in nsHttpTransaction::SetConnection (which is something I need to
> double check on too..., since you somehow need to tell your transaction to
> use a new connection, I forgot how to do it... but there should be a way)
> 

I don't understand this part.  Do I need to tell my transaction to use a new connection?

After I switch over to using the capability flag, the connection manager should be activating the connection with the transaction's caps.  Before that the channel should have initialized the transaction with its own caps.  After the connect is successful the connection is upgraded by the connection manager.

I don't know exactly what happens in the error cases. I also don't know what error would merit a new connection being set in a new place.
Necko does not allow for a CONNECT only request to happen.  This adds a flag
to signal an http channel should only CONNECT if proxied.  This flag can only be
set if an HTTPUpgrade handler has been assigned.  As proposed by rfc7639, an
alpn header will be included in the CONNECT request.  Its value is determined by
the upgrade protocol passed to HTTPUpgrade.

The flag is added as part of nsIHttpChannelInternal because it is dependent on
HTTPUpgrade.  It exists as a capability flag since the channel's transaction
needs to know to complete after a successful CONNECT.  Also the transaction's
connection needs to know to stop writing transaction data after it CONNECTs or
if there's no proxy, and to not tell the transaction to reset the connection.

If an nsHttpChannel has this flag set then the upgrade handler will receive the
socket after the CONNECT succeeds without doing tls if https.

In order to support xpcshell-test for this change, nsHttpConnectionMgr does a
little check to see if HTTPUpgrade callback is in JavaScript.  If it is then
the callback is invoked on the main thread.
Attachment #8986254 - Flags: review?(honzab.moz)
Attachment #8984335 - Attachment is obsolete: true
This replaces the tunnel using a new nr_socket implementation.  Proxy detection
code is still done in the peer connction.  However, the result is only used to
detect a proxy.  The result is unused.  Address resolution is done by necko code
in the parent process.  The new socket wraps a WebrtcProxyChannel which uses
necko to handle proxy negotiation.  This has a happy side effect of enabling all
authentication modes that necko already supports for http proxies.

This adds a protocol for Necko to manage, WebrtcProxyChannel.  This new protocol
serves as a pipe for a CONNECT tunnel.  It is only used in WebRtc and not built
in no WebRtc builds.
Attachment #8986259 - Flags: review?(honzab.moz)
Attachment #8986259 - Flags: review?(docfaraday)
Attachment #8983505 - Attachment is obsolete: true
Attachment #8983505 - Flags: review?(honzab.moz)
I'll get to the reviews here no sooner than the next week, sorry.  The new 'connect-only' patch looks better.  I have to walk through it more in detail, tho, and I don't have resources for it right now.
Comment on attachment 8986259 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

This is just a partial review, I have not looked at everything yet.

::: media/mtransport/WebrtcProxyChannelWrapper.h
@@ +22,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class WebrtcProxyChannelWrapper : public WebrtcProxyChannelCallback

I can't tell what you are attempting to accomplish here, could you please explain?

::: media/mtransport/ipc/WebrtcProxyChannelCallback.h
@@ +15,5 @@
> +class WebrtcProxyChannelCallback
> +{
> +  public:
> +    virtual void OnClose(nsresult aReason) = 0;
> +    virtual void OnComplete() = 0;

What does "OnComplete" mean? Is this the writeable callback? If so, let's name it something like OnWriteable, or OnReadyToWrite.

::: media/mtransport/ipc/WebrtcProxyChannelChild.h
@@ +36,5 @@
> +
> +  void Close();
> +
> +  void SetCallbacks(WebrtcProxyChannelCallback* aProxyCallbacks)
> +    { mProxyCallbacks = aProxyCallbacks; }

I'm not seeing anything that uses this.

@@ +39,5 @@
> +  void SetCallbacks(WebrtcProxyChannelCallback* aProxyCallbacks)
> +    { mProxyCallbacks = aProxyCallbacks; }
> +
> +  NS_IMETHOD_(MozExternalRefCountType) AddRef();
> +  NS_IMETHOD_(MozExternalRefCountType) Release();

Let's use the NS_INLINE_DECL_THREADSAFE_REFCOUNTING macro for the refcount stuff (example here: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.h?q=NrSocketBase&redirect_type=direct#240)

@@ +48,5 @@
> +protected:
> +  virtual ~WebrtcProxyChannelChild();
> +
> +  ThreadSafeAutoRefCnt mRefCnt;
> +  NS_DECL_OWNINGTHREAD

I don't see any owning-thread checks in the implementation for this file, so we could probably do without this.

@@ +50,5 @@
> +
> +  ThreadSafeAutoRefCnt mRefCnt;
> +  NS_DECL_OWNINGTHREAD
> +
> +  WebrtcProxyChannelCallback* mProxyCallbacks;

If we're going to use a bare pointer for this, we definitely need some documentation that Close() must be called (on main) before the WebrtcProxyChannelCallback may be destroyed. I would prefer to avoid using a bare pointer, though.

::: media/mtransport/nr_socket_proxy.cpp
@@ +324,5 @@
> +        this,
> +        aReadData.Length());
> +
> +  MOZ_ALWAYS_SUCCEEDS(mSocketThread->Dispatch(
> +    NewRunnableMethod<nsTArray<uint8_t>&&>("NrSocketProxy::OnRead",

So _that's_ how you get Runnables to play nice with move semantics. Good to know!

@@ +417,5 @@
> +{
> +  uint64_t lastCount = 0xBADBADBADBAD;
> +  uint64_t currentCount = 0;
> +  while ((poll_flags() & PR_POLL_READ) != 0 &&
> +         (mClosed || (currentCount = ReadBehindByCount()) > 0) &&

Should this be "!mClosed && "?

@@ +452,5 @@
> +                                      nsCString& aAddress)
> +{
> +  nsCString rawAddrString(aAddr->as_string);
> +
> +  // expected as_string value like 'IP4:127.0.0.1:12345/TCP' -> 127.0.0.1:12345

Hmm. I think it would be better to pass the addr and port separately over IPC. It would make all of the intermediate/ipdl stuff more readable, and you would not need this special-case, vaguely-named function.

::: media/mtransport/nr_socket_proxy.h
@@ +46,5 @@
> +
> +#include "nsTArray.h"
> +#include "nsDeque.h"
> +#include "nsString.h"
> +#include "nsIPrincipal.h"

Why are we including nsIPrincipal.h?

@@ +59,5 @@
> +#include "WebrtcProxyChannelWrapper.h"
> +
> +namespace mozilla {
> +class NrSocketProxyConfig;
> +} // namespace mozilla

Let's merge this into the namespace mozilla block below.

@@ +71,5 @@
> +class NrSocketProxy : public NrSocketBase,
> +                      public WebrtcProxyChannelCallback
> +{
> +public:
> +  NrSocketProxy(NrSocketProxyConfig* aConfig);

Let's make this an explicit constructor, and pass a const reference.

@@ +103,5 @@
> +  void OpenSafe(const nsCString& aAddress);
> +  void DestroySafe();
> +  void OnCloseSafe(nsresult aReason);
> +  void OnCompleteSafe();
> +  void OnReadSafe(nsTArray<uint8_t>&& aReadData);

Does the "Safe" suffix here mean that these functions are only called on the correct thread? If so, the convention we use is a suffix of "_s" for STS (eg; OnRead_s) and "_m" for main (eg; Open_m). If that is not what it means, what does "Safe" mean here?

Also, I don't see anything that uses |DestroySafe|, nor do I see a definition for it.

Lastly, these look like they ought to be private.

@@ +106,5 @@
> +  void OnCompleteSafe();
> +  void OnReadSafe(nsTArray<uint8_t>&& aReadData);
> +
> +  static int TranslateAddrToAddress(nr_transport_addr* aAddr,
> +                                    nsCString& aAddress);

In the webrtc code, outparams (like |aAddress|) are passed as a pointer.

Also, a heads-up; mtransport has traditionally not followed the Mozilla naming/formatting conventions that you are using here. This might be changing soon, but I do not know.

That said, we probably want to pass the address and port separately over IPC anyhow...

@@ +109,5 @@
> +  static int TranslateAddrToAddress(nr_transport_addr* aAddr,
> +                                    nsCString& aAddress);
> +
> +  NS_IMETHOD_(MozExternalRefCountType) AddRef() override;
> +  NS_IMETHOD_(MozExternalRefCountType) Release() override;

Let's use the NS_INLINE_DECL_THREADSAFE_REFCOUNTING macro for the refcount stuff (example here: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.h?q=NrSocketBase&redirect_type=direct#240)

@@ +111,5 @@
> +
> +  NS_IMETHOD_(MozExternalRefCountType) AddRef() override;
> +  NS_IMETHOD_(MozExternalRefCountType) Release() override;
> +
> +  uint64_t ReadBehindByCount();

Probably should be const. Also, the name is a little confusing, I would suggest something like CountUnreadBytes, or BytesAvailableForRead, or some other name that makes it clear that we're returning the number of bytes that are available for reading.

@@ +114,5 @@
> +
> +  uint64_t ReadBehindByCount();
> +
> +  // used for gtests
> +  void AssignChannel_DoNotUse(WebrtcProxyChannelWrapper* aChannel);

Let's pass a UniquePtr&& here.

@@ +121,5 @@
> +protected:
> +  virtual ~NrSocketProxy();
> +
> +  ThreadSafeAutoRefCnt mRefCnt;
> +  NS_DECL_OWNINGTHREAD

It doesn't look like NrSocketProxy uses owning-thread checks, and even if it did, NrSocketProxy is touched on multiple threads. If we were to do thread checking, I guess we would need to do it based on mMainThread and mSocketThread.

@@ +132,5 @@
> +
> +  bool                  mClosed;
> +
> +  uint32_t              mCurrentRead;
> +  NrSocketProxyData*    mCurrentData;

We seem to own |mCurrentData|, so we should probably put this in a UniquePtr. Alternately, if we switch over to using a more strongly-typed container for the buffers, we could just use the head of the queue directly.

@@ +133,5 @@
> +  bool                  mClosed;
> +
> +  uint32_t              mCurrentRead;
> +  NrSocketProxyData*    mCurrentData;
> +  nsDeque               mReadQueue;

std::list<NrSocketProxyData> (and using emplace_back) is probably a better choice here, since we're dealing with a single class, and probably not very many instances. If that does not work for some reason, we can use some other strongly-typed container.

::: media/mtransport/nr_socket_proxy_config.h
@@ +27,5 @@
> +  const nsCString& GetAlpn() const { return mAlpn; }
> +
> +private:
> +  dom::PBrowserOrId mBrowser;
> +  nsIPrincipal* mLoadingPrincipal;

We need to store this in an nsCOMPtr.

::: media/mtransport/nricectx.cpp
@@ +420,5 @@
> +{
> +  NrIceCtx* ctx = static_cast<NrIceCtx *>(obj);
> +  // don't need to pass obj (a NrIceCtx)
> +  return nr_socket_local_create2(nullptr, addr, sockp, ctx->proxy_config_);
> +}

I think we can simplify this even more. How about we move the pre-existing implementation of nr_socket_local_create into nricectx.cpp, modify it to try to pull the proxy config out of |obj| (by casting to an NrIceCtx), and have it pass the config (if it finds it) to NrSocketBase::CreateSocket.

That way, we don't need to declare any new functions in nricectx.h, and we don't need to have a new nr_socket_local_create2 function either.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +107,5 @@
> +    PBrowserOrId browser = TabChild::GetFrom(pcm_->GetPC()->GetWindow());
> +    nsIPrincipal* loadingPrinicpal = nsContentUtils::GetSystemPrincipal();
> +    pcm_->mProxyConfig.reset(new NrSocketProxyConfig(browser,
> +                                                     loadingPrinicpal,
> +                                                     alpn));

We can just rip out the PeerConnectionMedia::ProtocolProxyQueryHandler stuff now, because we can make one of these NrSocketProxyConfig whenever we want, as long as we do it on main. It probably makes sense to create the NrSocketProxyConfig when we init the PeerConnectionMedia.

@@ +1013,5 @@
>  void
>  PeerConnectionMedia::EnsureIceGathering_s(bool aDefaultRouteOnly,
>                                            bool aProxyOnly) {
> +  if (mProxyConfig) {
> +    mIceCtxHdlr->ctx()->SetProxyServer(mProxyConfig.get());

Let's not pass a bare pointer like this. Let's try to make the proxy config copyable, and pass a copy. If that doesn't work for some reason (eg; nsIPrincipal doesn't have a threadsafe reference count, or PBrowserOrId can't be copied on any thread other than main), there are workarounds we can use.
(In reply to Byron Campen [:bwc] from comment #50)
> Comment on attachment 8986259 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8986259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is just a partial review, I have not looked at everything yet.
> 

Thanks!

> ::: media/mtransport/WebrtcProxyChannelWrapper.h
> @@ +22,5 @@
> > +
> > +namespace mozilla {
> > +namespace net {
> > +
> > +class WebrtcProxyChannelWrapper : public WebrtcProxyChannelCallback
> 
> I can't tell what you are attempting to accomplish here, could you please
> explain?
> 

I finally got my Windows setup working and I found that it didn't compile.  On Windows, generated protocol definitions pull in an INT8 typedef that differs from nrappkit's typedef.  AFAIK, you can't un-typedef something.  At least my Google-fu didn't find anything...  Not that I want to anyway.  The underlying types are different since one is an 8-bit signed integer and the other is an 8-byte signed integer.

PBrowserOrId.h and WebrtcProxyChannelChild.h are the culprits.  I moved over to using stub declarations, pointers, and a wrapper for the actual channel to fix this.

> ::: media/mtransport/ipc/WebrtcProxyChannelCallback.h
> @@ +15,5 @@
> > +    virtual void OnComplete() = 0;
> 
> What does "OnComplete" mean? Is this the writeable callback? If so, let's
> name it something like OnWriteable, or OnReadyToWrite.
> 

Yeah, that's the writable callback.

> ::: media/mtransport/ipc/WebrtcProxyChannelChild.h
> @@ +36,5 @@
> > +  void SetCallbacks(WebrtcProxyChannelCallback* aProxyCallbacks)
> 
> I'm not seeing anything that uses this.
> 

I don't think anything uses it atm.  I think I added it just in case.

> @@ +39,5 @@
> > +  NS_IMETHOD_(MozExternalRefCountType) AddRef();
> > +  NS_IMETHOD_(MozExternalRefCountType) Release();
> 
> Let's use the NS_INLINE_DECL_THREADSAFE_REFCOUNTING macro for the refcount
> stuff (example here:
> https://dxr.mozilla.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.h?q=NrSocketBase&redirect_type=direct#240)
> 
> @@ +48,5 @@
> > +  ThreadSafeAutoRefCnt mRefCnt;
> > +  NS_DECL_OWNINGTHREAD
> 
> I don't see any owning-thread checks in the implementation for this file, so
> we could probably do without this.
> 

OK.  I saw this stuff in StunAddrsListenerChild.h and just copied it.  Never looked it up after I learned dxr existed.

> @@ +50,5 @@
> > +  WebrtcProxyChannelCallback* mProxyCallbacks;
> 
> If we're going to use a bare pointer for this, we definitely need some
> documentation that Close() must be called (on main) before the
> WebrtcProxyChannelCallback may be destroyed. I would prefer to avoid using a
> bare pointer, though.
> 

My assumption right now is that everything in WebrtcProxyChannelChild MUST be called on the main thread. Otherwise, you crash your tab.  It' a bare pointer right now because I only want a weak reference to avoid cyclical references.  I should be able to convert that to a nsWeakPtr though.

> @@ +417,5 @@
> > +  while ((poll_flags() & PR_POLL_READ) != 0 &&
> > +         (mClosed || (currentCount = ReadBehindByCount()) > 0) &&
> 
> Should this be "!mClosed && "?
> 

No, I wanted to make sure that whatever is reading knows we're closed.

> ::: media/mtransport/nr_socket_proxy.h
> @@ +46,5 @@
> > +#include "nsIPrincipal.h"
> 
> Why are we including nsIPrincipal.h?
> 

Should've been removed after I added the wrapper code.

> @@ +59,5 @@
> > +namespace mozilla {
> > +class NrSocketProxyConfig;
> > +} // namespace mozilla
> 
> Let's merge this into the namespace mozilla block below.
> 

OK.

> @@ +71,5 @@
> > +  NrSocketProxy(NrSocketProxyConfig* aConfig);
> 
> Let's make this an explicit constructor, and pass a const reference.
> 

Will mark explicit. Non-const reference related to WebrtcProxyChannelWrapper.

> @@ +103,5 @@
> > +  void OpenSafe(const nsCString& aAddress);
> > +  void DestroySafe();
> 
> Does the "Safe" suffix here mean that these functions are only called on the
> correct thread? If so, the convention we use is a suffix of "_s" for STS
> (eg; OnRead_s) and "_m" for main (eg; Open_m). If that is not what it means,
> what does "Safe" mean here?
> 
> Also, I don't see anything that uses |DestroySafe|, nor do I see a
> definition for it.
> 
> Lastly, these look like they ought to be private.
> 

Yes, the Safe suffix means they're only called on the correct thread.

I thought I'd get a compilation error if I didn't define a method but I guess not!  Will remove.

They're public for gtests.  I might be able to make the gtest inherit NrSocketProxy and move them to protected.  Otherwise, I'll add a comment that they're public for gtest.

> @@ +106,5 @@
> > +  static int TranslateAddrToAddress(nr_transport_addr* aAddr,
> > +                                    nsCString& aAddress);
> 
> In the webrtc code, outparams (like |aAddress|) are passed as a pointer.
> 
> Also, a heads-up; mtransport has traditionally not followed the Mozilla
> naming/formatting conventions that you are using here. This might be
> changing soon, but I do not know.
> 
> That said, we probably want to pass the address and port separately over IPC
> anyhow...
> 
-pulled from above-
> Hmm. I think it would be better to pass the addr and port separately over IPC. It would make all of the intermediate/ipdl stuff more readable, and you would not need this special-case, vaguely-named function.

OK.

> @@ +109,5 @@
> > +  NS_IMETHOD_(MozExternalRefCountType) AddRef() override;
> > +  NS_IMETHOD_(MozExternalRefCountType) Release() override;
> 
> Let's use the NS_INLINE_DECL_THREADSAFE_REFCOUNTING macro for the refcount
> stuff (example here:
> https://dxr.mozilla.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.h?q=NrSocketBase&redirect_type=direct#240)
> 

OK.

> @@ +111,5 @@
> > +  uint64_t ReadBehindByCount();
> 
> Probably should be const. Also, the name is a little confusing, I would
> suggest something like CountUnreadBytes, or BytesAvailableForRead, or some
> other name that makes it clear that we're returning the number of bytes that
> are available for reading.
> 

OK, CountUnreadBytes.

> @@ +114,5 @@
> > +
> > +  uint64_t ReadBehindByCount();
> > +
> > +  // used for gtests
> > +  void AssignChannel_DoNotUse(WebrtcProxyChannelWrapper* aChannel);
> 
> Let's pass a UniquePtr&& here.
> 

OK.

> @@ +121,5 @@
> > +protected:
> > +  virtual ~NrSocketProxy();
> > +
> > +  ThreadSafeAutoRefCnt mRefCnt;
> > +  NS_DECL_OWNINGTHREAD
> 
> It doesn't look like NrSocketProxy uses owning-thread checks, and even if it
> did, NrSocketProxy is touched on multiple threads. If we were to do thread
> checking, I guess we would need to do it based on mMainThread and
> mSocketThread.
> 

Will switch over to NS_INLINE_DECL_THREADSAFE_REFCOUNTING. Don't need NS_DECL_OWNINGTHREAD.

> @@ +132,5 @@
> > +  NrSocketProxyData*    mCurrentData;
> 
> We seem to own |mCurrentData|, so we should probably put this in a
> UniquePtr. Alternately, if we switch over to using a more strongly-typed
> container for the buffers, we could just use the head of the queue directly.
> 
> @@ +133,5 @@
> > +  nsDeque               mReadQueue;
> 
> std::list<NrSocketProxyData> (and using emplace_back) is probably a better
> choice here, since we're dealing with a single class, and probably not very
> many instances. If that does not work for some reason, we can use some other
> strongly-typed container.
> 

I was looking for a Mozilla-ish? data structure with O(1) head removal and landed on nsDeque. Will switch over.

> ::: media/mtransport/nr_socket_proxy_config.h
> @@ +27,5 @@
> > +  nsIPrincipal* mLoadingPrincipal;
> 
> We need to store this in an nsCOMPtr.
> 

OK.

> ::: media/mtransport/nricectx.cpp
> @@ +420,5 @@
> > +{
> > +  NrIceCtx* ctx = static_cast<NrIceCtx *>(obj);
> > +  // don't need to pass obj (a NrIceCtx)
> > +  return nr_socket_local_create2(nullptr, addr, sockp, ctx->proxy_config_);
> > +}
> 
> I think we can simplify this even more. How about we move the pre-existing
> implementation of nr_socket_local_create into nricectx.cpp, modify it to try
> to pull the proxy config out of |obj| (by casting to an NrIceCtx), and have
> it pass the config (if it finds it) to NrSocketBase::CreateSocket.
> 
> That way, we don't need to declare any new functions in nricectx.h, and we
> don't need to have a new nr_socket_local_create2 function either.
> 

Yeah.  I never like assuming a void* will be a certain thing but if we move it into nricectx.cpp that's not really a problem.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +107,5 @@
> > +    PBrowserOrId browser = TabChild::GetFrom(pcm_->GetPC()->GetWindow());
> > +    nsIPrincipal* loadingPrinicpal = nsContentUtils::GetSystemPrincipal();
> > +    pcm_->mProxyConfig.reset(new NrSocketProxyConfig(browser,
> > +                                                     loadingPrinicpal,
> > +                                                     alpn));
> 
> We can just rip out the PeerConnectionMedia::ProtocolProxyQueryHandler stuff
> now, because we can make one of these NrSocketProxyConfig whenever we want,
> as long as we do it on main. It probably makes sense to create the
> NrSocketProxyConfig when we init the PeerConnectionMedia.
> 

Doesn't that detect if a proxy is configured or not?  Would it be better to check preferences or something?

Also it's used as a flag in PeerConnectionMedia::EnsureIceGathering_s.

> @@ +1013,5 @@
> >  void
> >  PeerConnectionMedia::EnsureIceGathering_s(bool aDefaultRouteOnly,
> >                                            bool aProxyOnly) {
> > +  if (mProxyConfig) {
> > +    mIceCtxHdlr->ctx()->SetProxyServer(mProxyConfig.get());
> 
> Let's not pass a bare pointer like this. Let's try to make the proxy config
> copyable, and pass a copy. If that doesn't work for some reason (eg;
> nsIPrincipal doesn't have a threadsafe reference count, or PBrowserOrId
> can't be copied on any thread other than main), there are workarounds we can
> use.

This is related to the purpose of the WebrtcProxyChannelWrapper.
(In reply to Paul Vitale [:iidebyo] from comment #51)
> (In reply to Byron Campen [:bwc] from comment #50)
> > ::: media/mtransport/WebrtcProxyChannelWrapper.h
> > @@ +22,5 @@
> > > +
> > > +namespace mozilla {
> > > +namespace net {
> > > +
> > > +class WebrtcProxyChannelWrapper : public WebrtcProxyChannelCallback
> > 
> > I can't tell what you are attempting to accomplish here, could you please
> > explain?
> > 
> 
> I finally got my Windows setup working and I found that it didn't compile. 
> On Windows, generated protocol definitions pull in an INT8 typedef that
> differs from nrappkit's typedef.  AFAIK, you can't un-typedef something.  At
> least my Google-fu didn't find anything...  Not that I want to anyway.  The
> underlying types are different since one is an 8-bit signed integer and the
> other is an 8-byte signed integer.
> 
> PBrowserOrId.h and WebrtcProxyChannelChild.h are the culprits.  I moved over
> to using stub declarations, pointers, and a wrapper for the actual channel
> to fix this.

What/where exactly is the typedef nrappkit is conflicting with? There might be a way to fix this problem at the root.

> > @@ +50,5 @@
> > > +  WebrtcProxyChannelCallback* mProxyCallbacks;
> > 
> > If we're going to use a bare pointer for this, we definitely need some
> > documentation that Close() must be called (on main) before the
> > WebrtcProxyChannelCallback may be destroyed. I would prefer to avoid using a
> > bare pointer, though.
> > 
> 
> My assumption right now is that everything in WebrtcProxyChannelChild MUST
> be called on the main thread. Otherwise, you crash your tab.  It' a bare
> pointer right now because I only want a weak reference to avoid cyclical
> references.  I should be able to convert that to a nsWeakPtr though.

   You could also use Close to break the cycle. It's better than using Close to prevent a UAF, at any rate.

> > @@ +417,5 @@
> > > +  while ((poll_flags() & PR_POLL_READ) != 0 &&
> > > +         (mClosed || (currentCount = ReadBehindByCount()) > 0) &&
> > 
> > Should this be "!mClosed && "?
> > 
> 
> No, I wanted to make sure that whatever is reading knows we're closed.

   I see. Let's add a comment. I guess we want the same behavior for write also?

> > @@ +71,5 @@
> > > +  NrSocketProxy(NrSocketProxyConfig* aConfig);
> > 
> > Let's make this an explicit constructor, and pass a const reference.
> > 
> 
> Will mark explicit. Non-const reference related to WebrtcProxyChannelWrapper.

Ah, so the hitch here is we need the nsIPrincipal to be non-const. Ok.

> > @@ +103,5 @@
> > > +  void OpenSafe(const nsCString& aAddress);
> > > +  void DestroySafe();
> > 
> > Does the "Safe" suffix here mean that these functions are only called on the
> > correct thread? If so, the convention we use is a suffix of "_s" for STS
> > (eg; OnRead_s) and "_m" for main (eg; Open_m). If that is not what it means,
> > what does "Safe" mean here?
> > 
> > Also, I don't see anything that uses |DestroySafe|, nor do I see a
> > definition for it.
> > 
> > Lastly, these look like they ought to be private.
> > 
> 
> Yes, the Safe suffix means they're only called on the correct thread.
> 
> I thought I'd get a compilation error if I didn't define a method but I
> guess not!  Will remove.
> They're public for gtests.  I might be able to make the gtest inherit
> NrSocketProxy and move them to protected.  Otherwise, I'll add a comment
> that they're public for gtest.

Ok, so I see OnReadSafe and OnCompleteSafe being called by the gtest, but not the others. Are you calling those directly just to avoid the hassle of waiting for the dispatches that OnRead and OnComplete do?

> > @@ +132,5 @@
> > > +  NrSocketProxyData*    mCurrentData;
> > 
> > We seem to own |mCurrentData|, so we should probably put this in a
> > UniquePtr. Alternately, if we switch over to using a more strongly-typed
> > container for the buffers, we could just use the head of the queue directly.
> > 
> > @@ +133,5 @@
> > > +  nsDeque               mReadQueue;
> > 
> > std::list<NrSocketProxyData> (and using emplace_back) is probably a better
> > choice here, since we're dealing with a single class, and probably not very
> > many instances. If that does not work for some reason, we can use some other
> > strongly-typed container.
> > 
> 
> I was looking for a Mozilla-ish? data structure with O(1) head removal and
> landed on nsDeque. Will switch over.

Yeah, I (and much of the webrtc team) prefer to use the stl for most things unless there's a compelling reason to do otherwise.

> > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> > @@ +107,5 @@
> > > +    PBrowserOrId browser = TabChild::GetFrom(pcm_->GetPC()->GetWindow());
> > > +    nsIPrincipal* loadingPrinicpal = nsContentUtils::GetSystemPrincipal();
> > > +    pcm_->mProxyConfig.reset(new NrSocketProxyConfig(browser,
> > > +                                                     loadingPrinicpal,
> > > +                                                     alpn));
> > 
> > We can just rip out the PeerConnectionMedia::ProtocolProxyQueryHandler stuff
> > now, because we can make one of these NrSocketProxyConfig whenever we want,
> > as long as we do it on main. It probably makes sense to create the
> > NrSocketProxyConfig when we init the PeerConnectionMedia.
> > 
> 
> Doesn't that detect if a proxy is configured or not?  Would it be better to
> check preferences or something?

Hmm. That's a good question. At the very least let's strip SetProxyOnPcm down. The "shouldn't happen" error handling (grabbing the host/port) isn't going to be necessary any longer.

> > @@ +1013,5 @@
> > >  void
> > >  PeerConnectionMedia::EnsureIceGathering_s(bool aDefaultRouteOnly,
> > >                                            bool aProxyOnly) {
> > > +  if (mProxyConfig) {
> > > +    mIceCtxHdlr->ctx()->SetProxyServer(mProxyConfig.get());
> > 
> > Let's not pass a bare pointer like this. Let's try to make the proxy config
> > copyable, and pass a copy. If that doesn't work for some reason (eg;
> > nsIPrincipal doesn't have a threadsafe reference count, or PBrowserOrId
> > can't be copied on any thread other than main), there are workarounds we can
> > use.
> 
> This is related to the purpose of the WebrtcProxyChannelWrapper.

Ah, so we can't include the header for the config type from NrIceCtx?
(In reply to Byron Campen [:bwc] from comment #52)
> (In reply to Paul Vitale [:iidebyo] from comment #51)
> > (In reply to Byron Campen [:bwc] from comment #50)
> > PBrowserOrId.h and WebrtcProxyChannelChild.h are the culprits.  I moved over
> > to using stub declarations, pointers, and a wrapper for the actual channel
> > to fix this.
> 
> What/where exactly is the typedef nrappkit is conflicting with? There might
> be a way to fix this problem at the root.
> 

nrappkit typedefs: https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/util/libekr/r_types.h#167
Windows: '<local program files>\Windows Kits\10\include\10.0.16299.0\shared\basetsd.h'

I don't have a full header chain for this but I imagine it's pulled in from Windows.h.  r_types.h is needed for transport addr and various other nr socket files.

related: https://searchfox.org/mozilla-central/source/media/mtransport/common.build#91

files that directly include in r_types.h:

https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/util/libekr/r_memory.h#45
https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/util/libekr/r_common.h#89

> > > @@ +50,5 @@
> > My assumption right now is that everything in WebrtcProxyChannelChild MUST
> > be called on the main thread. Otherwise, you crash your tab.  It' a bare
> > pointer right now because I only want a weak reference to avoid cyclical
> > references.  I should be able to convert that to a nsWeakPtr though.
> 
>    You could also use Close to break the cycle. It's better than using Close
> to prevent a UAF, at any rate.
> 

Yeah.

> > > @@ +417,5 @@
> > No, I wanted to make sure that whatever is reading knows we're closed.
> 
>    I see. Let's add a comment. I guess we want the same behavior for write
> also?
> 

Sure.

That breaks ICE.  IIRC, the issue is the first candidate's socket that is writable, whether the write succeeds or not, is used for transport and other candidates are closed.

> > > @@ +71,5 @@
> > > > +  NrSocketProxy(NrSocketProxyConfig* aConfig);
> > > 
> > > Let's make this an explicit constructor, and pass a const reference.
> > > 
> > 
> > Will mark explicit. Non-const reference related to WebrtcProxyChannelWrapper.
> 
> Ah, so the hitch here is we need the nsIPrincipal to be non-const. Ok.
> 

It can be const.  The issue is that I need a pointer to avoid pulling in PBrowserOrId.

> > > @@ +103,5 @@
> 
> Ok, so I see OnReadSafe and OnCompleteSafe being called by the gtest, but
> not the others. Are you calling those directly just to avoid the hassle of
> waiting for the dispatches that OnRead and OnComplete do?
> 

Yeah.

> > > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> 
> Hmm. That's a good question. At the very least let's strip SetProxyOnPcm
> down. The "shouldn't happen" error handling (grabbing the host/port) isn't
> going to be necessary any longer.
> 

Sure.

> > > @@ +1013,5 @@
> > > >  void
> > > >  PeerConnectionMedia::EnsureIceGathering_s(bool aDefaultRouteOnly,
> > > >                                            bool aProxyOnly) {
> > > > +  if (mProxyConfig) {
> > > > +    mIceCtxHdlr->ctx()->SetProxyServer(mProxyConfig.get());
> > > 
> > > Let's not pass a bare pointer like this. Let's try to make the proxy config
> > > copyable, and pass a copy. If that doesn't work for some reason (eg;
> > > nsIPrincipal doesn't have a threadsafe reference count, or PBrowserOrId
> > > can't be copied on any thread other than main), there are workarounds we can
> > > use.
> > 
> > This is related to the purpose of the WebrtcProxyChannelWrapper.
> 
> Ah, so we can't include the header for the config type from NrIceCtx?

Yes. The config type holds the PBrowserOrId. NrIceCtx uses a bunch of nr socket stuff dependent on r_types.h. I could use a pointer for it but that's basically the same thing.
(In reply to Paul Vitale [:iidebyo] from comment #53)
> (In reply to Byron Campen [:bwc] from comment #52)
> > (In reply to Paul Vitale [:iidebyo] from comment #51)
> > > (In reply to Byron Campen [:bwc] from comment #50)
> > > PBrowserOrId.h and WebrtcProxyChannelChild.h are the culprits.  I moved over
> > > to using stub declarations, pointers, and a wrapper for the actual channel
> > > to fix this.
> > 
> > What/where exactly is the typedef nrappkit is conflicting with? There might
> > be a way to fix this problem at the root.
> > 
> 
> nrappkit typedefs:
> https://searchfox.org/mozilla-central/source/media/mtransport/third_party/
> nrappkit/src/util/libekr/r_types.h#167
> Windows: '<local program files>\Windows
> Kits\10\include\10.0.16299.0\shared\basetsd.h'
> 
> I don't have a full header chain for this but I imagine it's pulled in from
> Windows.h.  r_types.h is needed for transport addr and various other nr
> socket files.
> 
> related:
> https://searchfox.org/mozilla-central/source/media/mtransport/common.build#91
> 
> files that directly include in r_types.h:
> 
> https://searchfox.org/mozilla-central/source/media/mtransport/third_party/
> nrappkit/src/util/libekr/r_memory.h#45
> https://searchfox.org/mozilla-central/source/media/mtransport/third_party/
> nrappkit/src/util/libekr/r_common.h#89

   That's pretty awful.
Comment on attachment 8986259 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

Ok, I guess that's about it for this review pass. I'll need to see it again once you get done handling my review feedback for this round, and at that time I'll review the rest.

::: media/mtransport/WebrtcProxyChannelWrapper.cpp
@@ +26,5 @@
> +
> +  if (mWebrtcProxyChannel) {
> +    mWebrtcProxyChannel->Release();
> +    mWebrtcProxyChannel = nullptr;
> +  }

If mWebrtcProxyChannel is a RefPtr, we won't need this, although if it is set at this point it is probably an error.

@@ +38,5 @@
> +  mWebrtcProxyChannel->AsyncOpen(aAddress,
> +                                 aConfig->GetBrowser(),
> +                                 aConfig->GetLoadingPrincinpal(),
> +                                 aConfig->GetAlpn());
> +  mWebrtcProxyChannel->AddRef();

Won't be necessary once this is a RefPtr.

@@ +45,5 @@
> +void WebrtcProxyChannelWrapper::SendWrite(nsTArray<uint8_t>&& aReadData)
> +{
> +  if (mWebrtcProxyChannel) {
> +    mWebrtcProxyChannel->SendWrite(aReadData);
> +  }

If we only null this out after calling Close on it, we should be able to assert that it is set here.

@@ +59,5 @@
> +void WebrtcProxyChannelWrapper::OnClose(nsresult aReason)
> +{
> +  if (mProxyCallbacks) {
> +    mProxyCallbacks->OnClose(aReason);
> +  }

If we only unset this after calling OnClose on it, we should be able to assert that it is set here and in OnComplete/OnRead.

::: media/mtransport/WebrtcProxyChannelWrapper.h
@@ +22,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class WebrtcProxyChannelWrapper : public WebrtcProxyChannelCallback

Ok, how about this. The stated purpose for this class will be to protect mtransport from IPDL, and vice-versa. This includes:

* Ensuring that neither has to include headers from the other (because you get typedef conflicts when that happens)
* Handling dispatches to main for the IPDL API (eg; SendWrite will dispatch to main and call SendWrite on mWebrtcProxyChannel)
* Handling dispatches to STS for the callback API (eg; OnClose will dispatch to STS and call OnClose on mProxyCallbacks)
* Ensuring that its reference to the WebrtcProxyChannelChild is released on main
* Ensuring that its reference to the WebrtcProxyChannelCallback is released on STS

Sound reasonable?

@@ +42,5 @@
> +protected:
> +  // This MUST be a pointer. The definition cannot be pulled in here.
> +  // INT8 has conflicting typedefs on Windows.
> +  WebrtcProxyChannelCallback* mProxyCallbacks;
> +  WebrtcProxyChannelChild* mWebrtcProxyChannel;

mWebrtcProxyChannel needs to be a RefPtr. We should null it out immediately after calling Close on it.
mProxyCallbacks needs to be a RefPtr also. We should null it out immediately after calling OnClose on it.

::: media/mtransport/ipc/WebrtcProxyChannelChild.h
@@ +50,5 @@
> +
> +  ThreadSafeAutoRefCnt mRefCnt;
> +  NS_DECL_OWNINGTHREAD
> +
> +  WebrtcProxyChannelCallback* mProxyCallbacks;

Ok, so let's make this a RefPtr, and null it out right after we call OnClose on it. Nulling it out when Close is called isn't necessary for breaking the cycle as long as whatever called Close lets go of its reference to us.
Attachment #8986259 - Flags: review?(docfaraday) → review-
(In reply to Byron Campen [:bwc] from comment #55)
> Comment on attachment 8986259 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> > +class WebrtcProxyChannelWrapper : public WebrtcProxyChannelCallback
> 
> Ok, how about this. The stated purpose for this class will be to protect
> mtransport from IPDL, and vice-versa. This includes:
> 
> * Ensuring that neither has to include headers from the other (because you
> get typedef conflicts when that happens)
> * Handling dispatches to main for the IPDL API (eg; SendWrite will dispatch
> to main and call SendWrite on mWebrtcProxyChannel)
> * Handling dispatches to STS for the callback API (eg; OnClose will dispatch
> to STS and call OnClose on mProxyCallbacks)
> * Ensuring that its reference to the WebrtcProxyChannelChild is released on
> main
> * Ensuring that its reference to the WebrtcProxyChannelCallback is released
> on STS
> 
> Sound reasonable?
> 

Very much so.
Same patch description. Updated based on :bwc's review.
Attachment #8987546 - Flags: review?(honzab.moz)
Attachment #8987546 - Flags: review?(docfaraday)
Attachment #8986259 - Attachment is obsolete: true
Attachment #8986259 - Flags: review?(honzab.moz)
Comment on attachment 8987546 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

Partial review, mostly minor stuff.

::: media/mtransport/WebrtcProxyChannelWrapper.cpp
@@ +16,5 @@
> +namespace net {
> +
> +using namespace std;
> +
> +bool OnSocketThread();

Let's not use this, since it is probably supposed to be private to STS. Here's the kind of implementation we use elsewhere in the webrtc code:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h#674

I would also be fine with unconditionally dispatching to STS and passing the proxy channel to the runnable, instead of |this|.

@@ +18,5 @@
> +using namespace std;
> +
> +bool OnSocketThread();
> +
> +typedef shared_ptr<NrSocketProxyConfig> NrSocketProxyConfigPtr;

This is more readable without this typedef.

@@ +30,5 @@
> +{
> +  mMainThread = GetMainThreadEventTarget();
> +  mSocketThread = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID);
> +  MOZ_ASSERT(mMainThread, "no main thread");
> +  MOZ_ASSERT(mSocketThread, "no socket thread");

Let's make these two MOZ_RELEASE_ASSERT.

::: media/mtransport/WebrtcProxyChannelWrapper.h
@@ +9,5 @@
> +
> +#include <memory>
> +
> +#include "nsTArray.h"
> +#include "nsIEventTarget.h"

Can we forward declare nsIEventTarget?

@@ +20,5 @@
> +namespace net
> +{
> +class WebrtcProxyChannelChild;
> +} // namespace net
> +} // namespace mozilla

Let's merge these namespace blocks with the ones below.

@@ +25,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class WebrtcProxyChannelWrapper : public WebrtcProxyChannelCallback

Let's add some class doc here.

@@ +48,5 @@
> +
> +  nsCOMPtr<nsIEventTarget>      mMainThread;
> +  nsCOMPtr<nsIEventTarget>      mSocketThread;
> +
> +  virtual ~WebrtcProxyChannelWrapper() = default;

We should implement this, and either have it use NS_ProxyRelease to release the references to mProxyCallbacks and mWebrtcProxyChannel on the right thread. Alternately, if we're sure the code that uses it handles that already by calling Close/OnClose, we should assert that neither is set.

::: media/mtransport/ipc/WebrtcProxyChannelChild.h
@@ +9,5 @@
> +
> +#include "WebrtcProxyChannelCallback.h"
> +
> +#include "mozilla/net/PWebrtcProxyChannelChild.h"
> +#include "mozilla/dom/PBrowserOrId.h"

Let's forward-declare PBrowserOrId instead. Also, I think we should be able to forward declare WebrtcProxyChannelCallback, right?

::: media/mtransport/nr_socket_proxy.cpp
@@ +38,5 @@
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +*/
> +
> +/* Modified version of nr_proxy_tunnel */

Out of date comment.

@@ +48,5 @@
> +namespace mozilla
> +{
> +using namespace net;
> +
> +typedef std::shared_ptr<NrSocketProxyConfig> NrSocketProxyConfigPtr;

Again, I feel this stuff is more readable without this typedef.

@@ +107,5 @@
> +  }
> +
> +  _status = 0;
> +abort:
> +  return(_status);

You don't really need to use this boilerplate in here.

@@ +155,5 @@
> +  r_log(LOG_GENERIC,LOG_DEBUG,
> +        "NrSocketProxy::Write %p count=%zu\n",
> +        this,
> +        aCount);
> +  *aWrote = 0;

We have not null-checked aWrote yet.

@@ +159,5 @@
> +  *aWrote = 0;
> +
> +  if (mClosed || !aWrote) {
> +    return R_FAILED;
> +  }

We probably want to MOZ_ASSERT(!mClosed) and MOZ_ASSERT(mWebrtcProxyChannel) here.

@@ +163,5 @@
> +  }
> +
> +  if (!mWebrtcProxyChannel) {
> +    return R_WOULDBLOCK;
> +  }

This should probably be R_FAILED also.

@@ +171,5 @@
> +  writeData.SetLength(aCount);
> +  memcpy(writeData.Elements(), aBuffer, aCount);
> +  *aWrote = aCount;
> +
> +  MOZ_ASSERT(mWebrtcProxyChannel, "webrtc proxy channel should be non-null");

I guess make this assertion earlier.

@@ +187,5 @@
> +  r_log(LOG_GENERIC,LOG_DEBUG,"NrSocketProxy::Read %p\n", this);
> +
> +  if (mClosed || !aRead) {
> +    return R_FAILED;
> +  }

We probably want to MOZ_ASSERT(!mClosed) and MOZ_ASSERT(mWebrtcProxyChannel) here.

@@ +193,5 @@
> +  *aRead = 0;
> +
> +  if (!mWebrtcProxyChannel) {
> +    return R_WOULDBLOCK;
> +  }

Probably should be R_FAILED also.

@@ +203,5 @@
> +  do {
> +    const NrSocketProxyData& data = mReadQueue.front();
> +
> +    size_t remainingCount = data.GetData().Length() - mCurrentRead;
> +    size_t amountToCopy = aCount <= remainingCount ? aCount : remainingCount;

std::min

@@ +255,5 @@
> +
> +int
> +NrSocketProxy::listen(int aBacklog)
> +{
> +  return R_INTERNAL;

Does nICEr call this for ICE TCP candidates right now?

@@ +307,5 @@
> +void
> +NrSocketProxy::DoCallbacks()
> +{
> +  uint64_t lastCount = 0xBADBADBADBAD;
> +  uint64_t currentCount = 0;

Let's use size_t for these, and set lastCount to the max for size_t, since that's what nsTArray uses.

@@ +328,5 @@
> +{
> +  uint64_t count = 0;
> +
> +  for(const NrSocketProxyData& data : mReadQueue) {
> +    count += data.GetData().Length();

Let's use size_t for this.

::: media/mtransport/nr_socket_proxy.h
@@ +38,5 @@
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +*/
> +
> +/* Modified version of nr_proxy_tunnel */

Out of date comment.

@@ +111,5 @@
> +  void DoCallbacks();
> +
> +  bool mClosed;
> +
> +  uint32_t                      mCurrentRead;

Let's call this mCurrentReadOffset or mCurrentReadPosition.

@@ +117,5 @@
> +
> +  std::shared_ptr<NrSocketProxyConfig> mConfig;
> +
> +  // only use on main thread
> +  RefPtr<WebrtcProxyChannelWrapper> mWebrtcProxyChannel;

This is safe to use wherever now.

::: media/mtransport/nr_socket_proxy_config.cpp
@@ +21,5 @@
> +NrSocketProxyConfig::~NrSocketProxyConfig()
> +{
> +  NS_ProxyRelease("WebrtcProxyChannel::CleanUpAuthProvider",
> +                  GetMainThreadEventTarget(),
> +                  mLoadingPrincipal.forget());

Probably need a different label here, right?

::: media/mtransport/nricectx.cpp
@@ +117,5 @@
> +static int noop(void** obj) {
> +  return 0;
> +}
> +
> +typedef shared_ptr<NrSocketProxyConfig> NrSocketProxyConfigPtr;

Again, more readable without the typedef.

@@ +1201,5 @@
> +  int r, _status;
> +  NrSocketProxyConfigPtr config = nullptr;
> +
> +  if (obj) {
> +    config = ((NrIceCtx*)obj)->GetProxyConfig();

The convention is to avoid c-style casts, so let's use static_cast here.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +353,5 @@
>    // Used to track the state of the request.
>    bool mProxyResolveCompleted;
>  
>    // Used to store the result of the request.
> +  std::shared_ptr<NrSocketProxyConfig> mProxyConfig;

Comment is out-of-date.
Comment on attachment 8987546 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

::: media/mtransport/WebrtcProxyChannelWrapper.h
@@ +9,5 @@
> +
> +#include <memory>
> +
> +#include "nsTArray.h"
> +#include "nsIEventTarget.h"

Yes if I include nsCOMPtr.h.

::: media/mtransport/ipc/WebrtcProxyChannelChild.h
@@ +9,5 @@
> +
> +#include "WebrtcProxyChannelCallback.h"
> +
> +#include "mozilla/net/PWebrtcProxyChannelChild.h"
> +#include "mozilla/dom/PBrowserOrId.h"

Yes.

::: media/mtransport/nr_socket_proxy.cpp
@@ +163,5 @@
> +  }
> +
> +  if (!mWebrtcProxyChannel) {
> +    return R_WOULDBLOCK;
> +  }

I'll just remove this here and in NrSocketProxy::read in favor of asserts.

@@ +255,5 @@
> +
> +int
> +NrSocketProxy::listen(int aBacklog)
> +{
> +  return R_INTERNAL;

Yes. It is not called if relay-only or proxy-only is true.

::: media/mtransport/nr_socket_proxy.h
@@ +111,5 @@
> +  void DoCallbacks();
> +
> +  bool mClosed;
> +
> +  uint32_t                      mCurrentRead;

I'll drop the Current and go with mReadOffset.

::: media/mtransport/nr_socket_proxy_config.cpp
@@ +21,5 @@
> +NrSocketProxyConfig::~NrSocketProxyConfig()
> +{
> +  NS_ProxyRelease("WebrtcProxyChannel::CleanUpAuthProvider",
> +                  GetMainThreadEventTarget(),
> +                  mLoadingPrincipal.forget());

Yep. I read this a few times but never noticed the label.
Comment on attachment 8987546 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

::: media/mtransport/nr_socket_proxy.cpp
@@ +187,5 @@
> +  r_log(LOG_GENERIC,LOG_DEBUG,"NrSocketProxy::Read %p\n", this);
> +
> +  if (mClosed || !aRead) {
> +    return R_FAILED;
> +  }

We don't want to MOZ_ASSERT(!mClosed) here since nICEr doesn't alway close the socket. We can get OnClose events from the parent process.
(In reply to Paul Vitale [:iidebyo] from comment #60)
> Comment on attachment 8987546 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8987546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/mtransport/nr_socket_proxy.cpp
> @@ +187,5 @@
> > +  r_log(LOG_GENERIC,LOG_DEBUG,"NrSocketProxy::Read %p\n", this);
> > +
> > +  if (mClosed || !aRead) {
> > +    return R_FAILED;
> > +  }
> 
> We don't want to MOZ_ASSERT(!mClosed) here since nICEr doesn't alway close
> the socket. We can get OnClose events from the parent process.

Oh right, this is how nICEr learns that the socket is dead in that case.
Comment on attachment 8987546 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

More partial review.

::: media/mtransport/build/moz.build
@@ -5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  include("/ipc/chromium/chromium-config.mozbuild")
>  
>  EXPORTS.mtransport += [

At the end of this file is a comment about why the sources in mtransport cannot be used with unified compilation; we probably want to mention the IPDL/mtransport conflict between WebrtcProxyChannelChild and nrappkit there too.

@@ +9,5 @@
>  EXPORTS.mtransport += [
>      '../dtlsidentity.h',
>      '../m_cpp_utils.h',
>      '../mediapacket.h',
> +    '../nr_socket_proxy_config.h',

nr_socket_proxy.h and WebrtcProxyChannelWrapper.h should be in here too, right?

::: media/mtransport/ipc/WebrtcProxyChannel.cpp
@@ +136,5 @@
> +{
> +  LOG(("WebrtcProxyChannel::AsyncOpen %p\n", this));
> +  nsresult rv;
> +
> +  if (mClosed) {

We aren't on STS here.

@@ +309,5 @@
> +         std::move(aReadData))));
> +    return;
> +  }
> +
> +  MOZ_ASSERT(mProxyCallbacks, "webrtc proxy callback should be non-null");\

Trailing '\'

@@ +332,5 @@
> +  }
> +
> +  if (mTransport) {
> +    mTransport->SetSecurityCallbacks(nullptr);
> +    mTransport->SetEventSink(nullptr, nullptr);

Didn't we already unset these in OnTransportAvailable?

@@ +347,5 @@
> +{
> +  LOG(("WebrtcProxyChannel::OnTransportAvailable %p\n", this));
> +  MOZ_ASSERT(OnSocketThread(), "not on socket thread");
> +
> +  mTransport = aTransport;

If we expect this callback only once, we should probably assert and bail if this is already set.

@@ +353,5 @@
> +  mSocketOut = aSocketOut;
> +
> +  // Cancel any pending callbacks.
> +  mSocketIn->AsyncWait(nullptr, 0, 0, nullptr);
> +  mSocketOut->AsyncWait(nullptr, 0, 0, nullptr);

This is the first time we've seen either of these. Do we really intend to cancel async waits that someone else might have set up? If so, that's kinda surprising, and we should probably highlight that.

@@ +355,5 @@
> +  // Cancel any pending callbacks.
> +  mSocketIn->AsyncWait(nullptr, 0, 0, nullptr);
> +  mSocketOut->AsyncWait(nullptr, 0, 0, nullptr);
> +
> +  if (mClosed) {

If we check this before we set mTransport/mSocketIn/mSocketOut above, we don't need to call CleanUpConnection_s() again.

@@ +361,5 @@
> +    LOG(("WebrtcProxyChannel::OnTransportAvailable %p closed\n", this));
> +    return NS_OK;
> +  }
> +
> +  if (mConnected || !mOpened) {

mOpened is set on main, right? I think we could get by without checking mOpened here. Also, aren't we connected iff mTransport is set? Do we need mConnected?

@@ +376,5 @@
> +    LOG(("WebrtcProxyChannel::OnTransportAvailable %p sink failed\n", this));
> +    CloseWithReason(rv);
> +    return rv;
> +  }
> +  rv = mTransport->SetSecurityCallbacks(nullptr);

We're unsetting stuff here, but we've never seen mTransport before. Do we really intend to de-register handlers that aren't us? If so, this could use a comment.

@@ +399,5 @@
> +  }
> +
> +  mSocketIn->AsyncWait(this, 0, 0, mSocketThread);
> +
> +  InvokeOnWritable();

This is the only place I see this being called, and this is the only thing that calls OnWritable on the callback object. I dunno if I would call this a writable callback, exactly. OnComplete was probably a better name after all, or maybe OnConnected...

@@ +421,5 @@
> +                                  nsresult aStatusCode)
> +{
> +  LOG(("WebrtcProxyChannel::OnStopRequest %p status=%u\n", this, aStatusCode));
> +
> +  if (mClosed) {

Let's assert we're on STS.

@@ +460,5 @@
> +  nsresult rv;
> +
> +  if (mClosed || !mSocketIn) {
> +    return NS_OK;
> +  }

This should never happen, right?

@@ +465,5 @@
> +
> +  if (mSocketIn != in) {
> +    LOG(("WebrtcProxyChannel::OnInputStreamReady wrong stream %p\n", this));
> +    CloseWithReason(NS_ERROR_UNEXPECTED);
> +    return NS_ERROR_UNEXPECTED;

I think an assert would be a good idea here too.

@@ +470,5 @@
> +  }
> +
> +  char      buffer[4096];
> +  const uint32_t readCapacity = sizeof(buffer);
> +  uint32_t  count;

Let's put |count| in the loop; its value should not be preserved across iterations.

@@ +481,5 @@
> +      break;
> +    }
> +
> +    remainingCapacity -= count;
> +    read = readCapacity - remainingCapacity;

If we do read += count;, that would let us eliminate the readCapacity variable and make this more readable.

@@ +485,5 @@
> +    read = readCapacity - remainingCapacity;
> +
> +    if (NS_FAILED(rv)) {
> +      LOG(("WebrtcProxyChannel::OnInputStreamReady %p failed %u\n", this, rv));
> +      CloseWithReason(rv);

Probably makes sense to put this with the check for NS_BASE_STREAM_WOULD_BLOCK.

@@ +496,5 @@
> +           this));
> +      CloseWithReason(NS_ERROR_FAILURE);
> +      return NS_OK;
> +    }
> +  } while(remainingCapacity > 0);

This is more readable as a plain while loop.

::: media/mtransport/ipc/WebrtcProxyChannel.h
@@ +15,5 @@
> +#include "nsIHttpChannelInternal.h"
> +#include "nsIInterfaceRequestor.h"
> +#include "nsILoadInfo.h"
> +#include "nsIStreamListener.h"
> +#include "nsString.h"

We can probably forward declare nsILoadInfo and nsCString, right?

@@ +18,5 @@
> +#include "nsIStreamListener.h"
> +#include "nsString.h"
> +#include "nsTArray.h"
> +
> +#include "WebrtcProxyChannelCallback.h"

We can forward declare for this too, right?

::: media/mtransport/ipc/WebrtcProxyChannelParent.cpp
@@ +67,5 @@
> +
> +  CleanupChannel();
> +
> +  IProtocol* mgr = Manager();
> +  if (mIPCOpen && !Send__delete__(this)) {

RecvClose comes over IPC, right?

::: media/mtransport/ipc/WebrtcProxyChannelParent.h
@@ +52,5 @@
> +
> +private:
> +  void CleanupChannel();
> +
> +  bool mIPCOpen;

This bool seems to be intended to prevent us from trying to communicate with the child over IPC after IPC has closed. We check it when we get some callback from our channel. But we close our channel when IPC closes; shouldn't that be enough?

@@ +55,5 @@
> +
> +  bool mIPCOpen;
> +
> +  nsCOMPtr<nsIEventTarget> mMainThread;
> +  nsCOMPtr<nsIEventTarget> mSocketThread;

I don't see either of these being used.

::: media/mtransport/nr_socket_proxy.cpp
@@ +319,5 @@
> +  }
> +
> +  if (!mClosed && mWebrtcProxyChannel && (poll_flags() & PR_POLL_WRITE) != 0) {
> +    fire_callback(NR_ASYNC_WAIT_WRITE);
> +  }

Let's add a comment that we assume we are always ready to write, as long as we're connected, because the parent buffers for us.

::: media/mtransport/nr_socket_proxy_config.h
@@ +8,5 @@
> +#define nr_socket_proxy_config__
> +
> +#include "mozilla/dom/PBrowserOrId.h"
> +
> +#include "nsIPrincipal.h"

Can we forward declare nsIPrincipal here?

Also, we should probably include nsCOMPtr.h
(In reply to Byron Campen [:bwc] from comment #62)
> Comment on attachment 8987546 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8987546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> More partial review.
> 

Thanks!

> @@ +9,5 @@
> >  EXPORTS.mtransport += [
> >      '../dtlsidentity.h',
> >      '../m_cpp_utils.h',
> >      '../mediapacket.h',
> > +    '../nr_socket_proxy_config.h',
> 
> nr_socket_proxy.h and WebrtcProxyChannelWrapper.h should be in here too,
> right?
> 

nr_socket_proxy_config.h is only here to expose it to PeerConnectionMedia.

> ::: media/mtransport/ipc/WebrtcProxyChannel.cpp
> @@ +136,5 @@
> > +{
> > +  LOG(("WebrtcProxyChannel::AsyncOpen %p\n", this));
> > +  nsresult rv;
> > +
> > +  if (mClosed) {
> 
> We aren't on STS here.
> 

Yes.  LoadInfo isn't thread-safe.

> @@ +347,5 @@
> > +{
> > +  LOG(("WebrtcProxyChannel::OnTransportAvailable %p\n", this));
> > +  MOZ_ASSERT(OnSocketThread(), "not on socket thread");
> > +
> > +  mTransport = aTransport;
> 
> If we expect this callback only once, we should probably assert and bail if
> this is already set.
> 

OK.

> @@ +353,5 @@
> > +  mSocketOut = aSocketOut;
> > +
> > +  // Cancel any pending callbacks.
> > +  mSocketIn->AsyncWait(nullptr, 0, 0, nullptr);
> > +  mSocketOut->AsyncWait(nullptr, 0, 0, nullptr);
> 
> This is the first time we've seen either of these. Do we really intend to
> cancel async waits that someone else might have set up? If so, that's kinda
> surprising, and we should probably highlight that.
> 

nsHttpConnection owns these streams before handing them to us.  nsHttpConnection does not cancel the awaits and an assert fails.

> @@ +361,5 @@
> > +    LOG(("WebrtcProxyChannel::OnTransportAvailable %p closed\n", this));
> > +    return NS_OK;
> > +  }
> > +
> > +  if (mConnected || !mOpened) {
> 
> mOpened is set on main, right? I think we could get by without checking
> mOpened here. Also, aren't we connected iff mTransport is set? Do we need
> mConnected?
> 

Yeah, don't need mOpened. We can use mTransport instead of mConnected.

> @@ +376,5 @@
> > +    LOG(("WebrtcProxyChannel::OnTransportAvailable %p sink failed\n", this));
> > +    CloseWithReason(rv);
> > +    return rv;
> > +  }
> > +  rv = mTransport->SetSecurityCallbacks(nullptr);
> 
> We're unsetting stuff here, but we've never seen mTransport before. Do we
> really intend to de-register handlers that aren't us? If so, this could use
> a comment.
> 

Don't need this since nsHttpConnectionMgr calls TakeTransport() on our nsHttpConnection which unsets these.  I was using WebSocketChannel as a reference which also does this.  I guess redundancies happen.

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnectionMgr.cpp#2905
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnection.cpp#1353

> @@ +399,5 @@
> > +  }
> > +
> > +  mSocketIn->AsyncWait(this, 0, 0, mSocketThread);
> > +
> > +  InvokeOnWritable();
> 
> This is the only place I see this being called, and this is the only thing
> that calls OnWritable on the callback object. I dunno if I would call this a
> writable callback, exactly. OnComplete was probably a better name after all,
> or maybe OnConnected...
> 

Yeah.  I wasn't sure what to call it.  It's kind of a writable callback?  You can start writing to the tunnel and reasonably expect it to reach your endpoint.  I didn't call it OnConnected since that might imply we connected but not CONNECTed.

> @@ +460,5 @@
> > +  nsresult rv;
> > +
> > +  if (mClosed || !mSocketIn) {
> > +    return NS_OK;
> > +  }
> 
> This should never happen, right?
> 
> @@ +465,5 @@
> > +
> > +  if (mSocketIn != in) {
> > +    LOG(("WebrtcProxyChannel::OnInputStreamReady wrong stream %p\n", this));
> > +    CloseWithReason(NS_ERROR_UNEXPECTED);
> > +    return NS_ERROR_UNEXPECTED;
> 
> I think an assert would be a good idea here too.
> 

It shouldn't ever.

> ::: media/mtransport/ipc/WebrtcProxyChannel.h
> @@ +15,5 @@
> > +#include "nsIHttpChannelInternal.h"
> > +#include "nsIInterfaceRequestor.h"
> > +#include "nsILoadInfo.h"
> > +#include "nsIStreamListener.h"
> > +#include "nsString.h"
> 
> We can probably forward declare nsILoadInfo and nsCString, right?
> 

Yes.

> @@ +18,5 @@
> > +#include "nsIStreamListener.h"
> > +#include "nsString.h"
> > +#include "nsTArray.h"
> > +
> > +#include "WebrtcProxyChannelCallback.h"
> 
> We can forward declare for this too, right?
> 

Yep.

> ::: media/mtransport/ipc/WebrtcProxyChannelParent.cpp
> @@ +67,5 @@
> > +
> > +  CleanupChannel();
> > +
> > +  IProtocol* mgr = Manager();
> > +  if (mIPCOpen && !Send__delete__(this)) {
> 
> RecvClose comes over IPC, right?
> 

Yes.

> ::: media/mtransport/ipc/WebrtcProxyChannelParent.h
> @@ +52,5 @@
> > +
> > +private:
> > +  void CleanupChannel();
> > +
> > +  bool mIPCOpen;
> 
> This bool seems to be intended to prevent us from trying to communicate with
> the child over IPC after IPC has closed. We check it when we get some
> callback from our channel. But we close our channel when IPC closes;
> shouldn't that be enough?
> 

Yeah, it's redundant.
Comment on attachment 8987546 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

::: media/mtransport/ipc/WebrtcProxyChannel.cpp
@@ +421,5 @@
> +                                  nsresult aStatusCode)
> +{
> +  LOG(("WebrtcProxyChannel::OnStopRequest %p status=%u\n", this, aStatusCode));
> +
> +  if (mClosed) {

We're always on the main thread here and in OnStartRequest(). mClosed is only written on STS and read on STS/main.  Also for OnStopRequest() the return value is ignored.
(In reply to Paul Vitale [:iidebyo] from comment #64)
> Comment on attachment 8987546 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8987546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/mtransport/ipc/WebrtcProxyChannel.cpp
> @@ +421,5 @@
> > +                                  nsresult aStatusCode)
> > +{
> > +  LOG(("WebrtcProxyChannel::OnStopRequest %p status=%u\n", this, aStatusCode));
> > +
> > +  if (mClosed) {
> 
> We're always on the main thread here and in OnStartRequest(). mClosed is
> only written on STS and read on STS/main.  Also for OnStopRequest() the
> return value is ignored.

   Ok then, this is racy, and things like TSAN will complain about it. Checking mClosed here might save us the overhead of a thread dispatch in super rare cases, which is something that I can live with.

(In reply to Paul Vitale [:iidebyo] from comment #63)
> (In reply to Byron Campen [:bwc] from comment #62)
> > @@ +9,5 @@
> > >  EXPORTS.mtransport += [
> > >      '../dtlsidentity.h',
> > >      '../m_cpp_utils.h',
> > >      '../mediapacket.h',
> > > +    '../nr_socket_proxy_config.h',
> > 
> > nr_socket_proxy.h and WebrtcProxyChannelWrapper.h should be in here too,
> > right?
> > 
> 
> nr_socket_proxy_config.h is only here to expose it to PeerConnectionMedia.

   Ah, ok.

> > ::: media/mtransport/ipc/WebrtcProxyChannel.cpp
> > @@ +136,5 @@
> > > +{
> > > +  LOG(("WebrtcProxyChannel::AsyncOpen %p\n", this));
> > > +  nsresult rv;
> > > +
> > > +  if (mClosed) {
> > 
> > We aren't on STS here.
> > 
> 
> Yes.  LoadInfo isn't thread-safe.

   Ok, so we shouldn't be checking mClosed here. If we move setting mOpened to true to the top of Open, we'll be able to prevent Open from being called again when it fails the first time. After we do that, I guess we won't notice when someone creates a channel, closes it (without calling Open), and then calls Open. Maybe CloseWithReason could check mOpened if it is run on main to detect that kind of problem.

> > @@ +353,5 @@
> > > +  mSocketOut = aSocketOut;
> > > +
> > > +  // Cancel any pending callbacks.
> > > +  mSocketIn->AsyncWait(nullptr, 0, 0, nullptr);
> > > +  mSocketOut->AsyncWait(nullptr, 0, 0, nullptr);
> > 
> > This is the first time we've seen either of these. Do we really intend to
> > cancel async waits that someone else might have set up? If so, that's kinda
> > surprising, and we should probably highlight that.
> > 
> 
> nsHttpConnection owns these streams before handing them to us. 
> nsHttpConnection does not cancel the awaits and an assert fails.

   Yuck. Let's add a comment about that.

> > @@ +399,5 @@
> > > +  }
> > > +
> > > +  mSocketIn->AsyncWait(this, 0, 0, mSocketThread);
> > > +
> > > +  InvokeOnWritable();
> > 
> > This is the only place I see this being called, and this is the only thing
> > that calls OnWritable on the callback object. I dunno if I would call this a
> > writable callback, exactly. OnComplete was probably a better name after all,
> > or maybe OnConnected...
> > 
> 
> Yeah.  I wasn't sure what to call it.  It's kind of a writable callback? 
> You can start writing to the tunnel and reasonably expect it to reach your
> endpoint.  I didn't call it OnConnected since that might imply we connected
> but not CONNECTed.

   I guess it is true that there are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors.

   I hate to go back and forth on this stuff in a review, but I think OnConnected would be ok here. Maybe OnConnectedEndToEnd if you really wanted it to be unambiguous.
Ok, so I'll stop looking at WebrtcProxyChannel for now, since it is likely to change significantly. I'll turn my attention to the test code, and I can give WebrtcProxyChannel another look once you've updated it.
Comment on attachment 8987546 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

More partial review.

::: media/mtransport/test/proxy_tunnel_socket_unittest.cpp
@@ +21,2 @@
>  const std::string kProxyAddr = "192.0.2.134";
>  const uint16_t kProxyPort = 9999;

These are only used to init proxy_addr_, which is itself unused.

@@ +40,5 @@
> +    // WebrtcProxyChannel's threading model is the same as mtransport
> +    // all socket operations are done on the socket thread
> +    // callbacks are invoked on the main thread
> +    mSocketThread = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
> +    ASSERT_TRUE(NS_SUCCEEDED(rv));

I don't see mSocketThread being used by anything.

@@ +47,2 @@
>          (char *)kProxyAddr.c_str(), kProxyPort, IPPROTO_TCP, &proxy_addr_);
>      ASSERT_EQ(0, r);

Nothing uses this either.

@@ +50,1 @@
>      Configure();

This is the only place Configure is used, we should probably just move the code in Configure to SetUp.

@@ +51,5 @@
>    }
>  
> +  void TearDown() override {
> +    mSProxy->AssignChannel_DoNotUse(nullptr);
> +  }

I think the ordinary teardown performed by destructors should be fine, we probably don't need to override TearDown().

@@ +66,4 @@
>      ASSERT_EQ(0, r);
>  
> +    // fake calling AsyncOpen()
> +    mSProxy->AssignChannel_DoNotUse(new WebrtcProxyChannelWrapper(nullptr));

Ok, so we're replacing the wrapper with one that won't call callbacks on the NrSocketProxy. But nothing pokes that wrapper anyway, right? The original won't have any reason to call the callbacks, so it shouldn't make a difference if I've understood all this correctly.

@@ +79,5 @@
> +                   &read,
> +                   0);
> +    array.SetLength(read);
> +    test->mData.AppendElements(array);
> +    test->mReadAmount += 1;

Ok, so first we do a read of size 0, then of size 1, then of size 2, etc. It might be helpful to add a comment that we're trying to test NrSocketProxy's code for feeding bytes from NrSocketProxy::mReadQueue to the caller.

@@ +89,3 @@
>    }
>  
>    nr_socket *socket() { return nr_socket_; }

This doesn't seem to be used anymore.

@@ +96,2 @@
>    nr_transport_addr proxy_addr_;
>    nr_transport_addr remote_addr_;

Neither of these two are used anymore.

@@ +96,4 @@
>    nr_transport_addr proxy_addr_;
>    nr_transport_addr remote_addr_;
> +
> +  nsCOMPtr<nsIEventTarget> mSocketThread;

This doesn't seem to be used either.

@@ +117,5 @@
> +  mSProxy->OnRead(std::move(array));
> +  ASSERT_TRUE(mSProxy->CountUnreadBytes() == kHelloMessage.length());
> +  NR_ASYNC_WAIT(mSProxy,
> +                NR_ASYNC_WAIT_READ,
> +                &NrSocketProxyTest::readable_cb,

We should probably note (and test) that this first NR_ASYNC_WAIT will not result in anything being read, because the first time we get a readable callback we read 0 bytes, which will cause NrSocketProxy to not retry (until it comes time to fire another callback).

@@ +120,5 @@
> +                NR_ASYNC_WAIT_READ,
> +                &NrSocketProxyTest::readable_cb,
> +                this);
> +  // start callbacks again (first read is 0 then 1,2,3,...)
> +  mSProxy->OnWritable();

This is confusing; let's use OnRead to start the callbacks again here, and have a separate test for the OnConnected/writeable callback stuff. We would never see an OnRead followed by an OnConnected in normal operation.

@@ +133,5 @@
>    ASSERT_EQ(R_WOULDBLOCK, r);
>  
> +  nsTArray<uint8_t> array;
> +  array.AppendElements(kHelloMessage.c_str(), kHelloMessage.length());
> +  mSProxy->OnRead(std::move(array));

I would like to see a test that calls OnRead multiple times with different sizes, then does an NR_SYNC_WAIT for readable, and then verifies that the resulting bytes are what we expect.

It might also be worthwhile to have a test that calls OnRead multiple times, then reads _some_ of the data, then calls OnRead some more, and finally reads everything (tests that OnRead works properly when there is a partially drained read buffer).

::: media/mtransport/test/webrtcproxychannel_unittest.cpp
@@ +132,5 @@
> +  mCallbackTarget = aEventTarget;
> +
> +  if (mAllowCallbacks && mData.Length() > 0) {
> +    DoCallback();
> +  }

Should we be checking these in DoCallback instead?

@@ +165,5 @@
> +  if (mMustFail) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  *aRead = aCount <= mData.Length() ? aCount : mData.Length();
> +  *aRead = *aRead <= mMaxReadSize ? *aRead : mMaxReadSize;

std::min, there's even a nifty new syntax for more than two:

std::min({mData.Length(), aCount, mMaxReadSize});

@@ +168,5 @@
> +  *aRead = aCount <= mData.Length() ? aCount : mData.Length();
> +  *aRead = *aRead <= mMaxReadSize ? *aRead : mMaxReadSize;
> +  memcpy(aBuffer, mData.Elements(), *aRead);
> +  mData.RemoveElementsAt(0, *aRead);
> +  return *aRead > 0 ? NS_OK : NS_BASE_STREAM_WOULD_BLOCK;

This would return WOULD_BLOCK if |aCount| is 0, but this is test code so maybe not a big deal.

@@ +198,5 @@
> +      "WebrtcProxyChannelTestInputStream::DoCallback",
> +      [this]() -> void {
> +        nsCOMPtr<nsIInputStreamCallback> cb = mCallback;
> +        mCallback = nullptr;
> +        mCallbackTarget = nullptr;

Un-setting these after the dispatch might expose us to some race conditions; calling DoCallback twice in rapid succession would dispatch twice, and the second runnable would try to use a nullptr. Let's instead std::move mCallback into the runnable, using NewRunnableMethod (NewRunnableMethod should also allow us to avoid |this| pointing to freed memory).

@@ +204,5 @@
> +      }));
> +  } else {
> +    mCallback = nullptr;
> +    mCallbackTarget = nullptr;
> +  }

Is it valid for one of these to be null, but not the other? It doesn't look valid to me, so we might be able to simplify this whole function to "if (mCallback) {/* dispatch */}"

@@ +221,5 @@
> +
> +  void DoCallback();
> +  void Fail() { mMustFail = true; }
> +
> +  std::string DataString();

Probably should be const.

@@ +280,5 @@
> +{
> +  if (mMustFail) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  *aWrote = aCount <= mMaxWriteSize ? aCount : mMaxWriteSize;

std::min

@@ +322,5 @@
> +      "WebrtcProxyChannelTestOutputStream::DoCallback",
> +      [this]() -> void {
> +        nsCOMPtr<nsIOutputStreamCallback> cb = mCallback;
> +        mCallback = nullptr;
> +        mCallbackTarget = nullptr;

Same feedback as above in this function.

@@ +352,5 @@
> +  virtual ~FakeWebrtcProxyChannel() = default;
> +
> +  void InvokeOnClose(nsresult aReason) override;
> +  void InvokeOnWritable() override;
> +  void InvokeOnRead(nsTArray<uint8_t>&& aReadData) override;

The "real" implementations dispatch if we aren't on main, but that's what we want, right?

@@ +354,5 @@
> +  void InvokeOnClose(nsresult aReason) override;
> +  void InvokeOnWritable() override;
> +  void InvokeOnRead(nsTArray<uint8_t>&& aReadData) override;
> +
> +  RefPtr<WebrtcProxyChannelCallback> mCallback;

I'd be fine with making WebrtcProxyChannel::mProxyCallbacks protected so we don't need to shadow it here, if we do end up needing the overrides above.
(In reply to Byron Campen [:bwc] from comment #67)
> Comment on attachment 8987546 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8987546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> More partial review.
> 
> @@ +51,5 @@
> >    }
> >  
> > +  void TearDown() override {
> > +    mSProxy->AssignChannel_DoNotUse(nullptr);
> > +  }
> 
> I think the ordinary teardown performed by destructors should be fine, we
> probably don't need to override TearDown().
> 

This can be replaced with mSProxy->close(); which does the same thing. nr_socket_proxy asserts the channel is null in the destructor.

> @@ +66,4 @@
> >      ASSERT_EQ(0, r);
> >  
> > +    // fake calling AsyncOpen()
> > +    mSProxy->AssignChannel_DoNotUse(new WebrtcProxyChannelWrapper(nullptr));
> 
> Ok, so we're replacing the wrapper with one that won't call callbacks on the
> NrSocketProxy. But nothing pokes that wrapper anyway, right? The original
> won't have any reason to call the callbacks, so it shouldn't make a
> difference if I've understood all this correctly.
> 

Yeah we don't use but we assert that it's non-null when reading and writing (not that we test writing here).

> ::: media/mtransport/test/webrtcproxychannel_unittest.cpp
> @@ +132,5 @@
> > +  mCallbackTarget = aEventTarget;
> > +
> > +  if (mAllowCallbacks && mData.Length() > 0) {
> > +    DoCallback();
> > +  }
> 
> Should we be checking these in DoCallback instead?
> 
> @@ +168,5 @@
> > +  *aRead = aCount <= mData.Length() ? aCount : mData.Length();
> > +  *aRead = *aRead <= mMaxReadSize ? *aRead : mMaxReadSize;
> > +  memcpy(aBuffer, mData.Elements(), *aRead);
> > +  mData.RemoveElementsAt(0, *aRead);
> > +  return *aRead > 0 ? NS_OK : NS_BASE_STREAM_WOULD_BLOCK;
> 
> This would return WOULD_BLOCK if |aCount| is 0, but this is test code so
> maybe not a big deal.
> 

It's a bit of a misnomer but mAllowCallbacks's purpose is to allow callbacks immediately after getting a request for an async read, if needed.

Yeah, I don't think it matters.

> @@ +204,5 @@
> > +      }));
> > +  } else {
> > +    mCallback = nullptr;
> > +    mCallbackTarget = nullptr;
> > +  }
> 
> Is it valid for one of these to be null, but not the other? It doesn't look
> valid to me, so we might be able to simplify this whole function to "if
> (mCallback) {/* dispatch */}"
> 

It's not valid but possible. Asserting that both are null or non-null in AsyncWait() can fix that and simplify DoCallback().

> @@ +352,5 @@
> > +  virtual ~FakeWebrtcProxyChannel() = default;
> > +
> > +  void InvokeOnClose(nsresult aReason) override;
> > +  void InvokeOnWritable() override;
> > +  void InvokeOnRead(nsTArray<uint8_t>&& aReadData) override;
> 
> The "real" implementations dispatch if we aren't on main, but that's what we
> want, right?
> 

Yeah the gtests are run on main so these need to be called on STS.
Assignee: nobody → paul.m.vitale
Comment on attachment 8987546 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

::: media/mtransport/ipc/WebrtcProxyChannel.cpp
@@ +460,5 @@
> +  nsresult rv;
> +
> +  if (mClosed || !mSocketIn) {
> +    return NS_OK;
> +  }

I thought this couldn't happen but it can. If the input stream dispatches this callback just before CloseWithReason, we'll still receive a callback after closure.
(In reply to Paul Vitale [:iidebyo] from comment #69)
> Comment on attachment 8987546 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8987546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/mtransport/ipc/WebrtcProxyChannel.cpp
> @@ +460,5 @@
> > +  nsresult rv;
> > +
> > +  if (mClosed || !mSocketIn) {
> > +    return NS_OK;
> > +  }
> 
> I thought this couldn't happen but it can. If the input stream dispatches
> this callback just before CloseWithReason, we'll still receive a callback
> after closure.

   So it is possible to get a callback after we AsyncWait(nullptr...?

   Oh, check this out: https://searchfox.org/mozilla-central/source/netwerk/base/nsSocketTransport2.cpp#484

   It looks like passing the event target is optional, if you want the callbacks on STS, which we do. I bet making this change would iron out this wrinkle.

   It looks like the same is true for the output side, so let's do the same there: https://searchfox.org/mozilla-central/source/netwerk/base/nsSocketTransport2.cpp#739
(In reply to Byron Campen [:bwc] from comment #70)
> (In reply to Paul Vitale [:iidebyo] from comment #69)
> > Comment on attachment 8987546 [details] [diff] [review]
> > part 2. replace proxy tunnel with new ipc-based tunnel
> > 
> > Review of attachment 8987546 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/mtransport/ipc/WebrtcProxyChannel.cpp
>    It looks like passing the event target is optional, if you want the
> callbacks on STS, which we do. I bet making this change would iron out this
> wrinkle.
> 

Yep.  Not passing the target parameter fixes it.
Comment on attachment 8987546 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

Ok, we're getting close to done. I'll want to give this one more look after all the review feedback is addressed.

::: media/mtransport/test/webrtcproxychannel_unittest.cpp
@@ +400,5 @@
> +
> +  // WebrtcProxyChannelCallback
> +  void OnClose(nsresult aReason);
> +  void OnWritable();
> +  void OnRead(nsTArray<uint8_t>&& aReadData);

Maybe misleading comment; we don't inherit WebrtcProxyChannelCallback here. Let's comment that |mCallback| forwards its callbacks to these functions.

@@ +407,5 @@
> +  void TearDown() override;
> +
> +  void DoTransportAvailable();
> +
> +  std::string ReadDataAsString();

Should be const.

@@ +463,5 @@
> +
> +  // fake open the proxy channel
> +  mChannel->FakeOpen();
> +  // need to leak this here. cannot deconstruct IPC class w/o IPC setup.
> +  mChannel->AddRef();

This isn't an IPC class, right?

@@ +488,5 @@
> +
> +void
> +WebrtcProxyChannelTest::OnClose(nsresult aReason)
> +{
> +  mCloseReason = aReason;

This is the only place I see this used right now.

@@ +558,5 @@
> +  mInputStream->DoCallback();
> +
> +  ASSERT_TRUE_WAIT(mOnReadCalled, kDefaultTestTimeout);
> +  ASSERT_EQ(mReadData.Length(), sizeof(kReadData));
> +  ASSERT_TRUE(ReadDataAsString() == kReadDataString);

A bunch of these ASSERT_TRUEs would be better written as ASSERT_EQ. Also, with ASSERT_EQ the expected value should come first, otherwise the logging is confusing when there's an error.

@@ +644,5 @@
> +      array.AppendElements(kReadData, sizeof(kReadData));
> +      data += kReadDataString;
> +    }
> +    mChannel->Write(std::move(array));
> +  }

Seems like we could break out a std::string GetLargeData() function, use it here and above, and create |array| from that string.
Attachment #8987546 - Flags: review?(docfaraday) → review-
(In reply to Byron Campen [:bwc] from comment #72)
> Comment on attachment 8987546 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8987546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, we're getting close to done. I'll want to give this one more look after
> all the review feedback is addressed.
> @@ +463,5 @@
> > +
> > +  // fake open the proxy channel
> > +  mChannel->FakeOpen();
> > +  // need to leak this here. cannot deconstruct IPC class w/o IPC setup.
> > +  mChannel->AddRef();
> 
> This isn't an IPC class, right?
> 

Yes.  That's outdated.
Same patch description.  Updated based on :bwc's review.
Attachment #8990053 - Flags: review?(honzab.moz)
Attachment #8990053 - Flags: review?(docfaraday)
Attachment #8987546 - Attachment is obsolete: true
Attachment #8987546 - Flags: review?(honzab.moz)
Please bear with me regarding the reviews.  I'm currently brainstorming/architectting Necko Process isolation that really eats my time.  HTTP proxy connect is one of the affected components, so it makes sense to wait a little until we have a better idea how will the HTTP proxy code and some of the WebRTC components be moved around.
Hey, mt, the webrtc alpn draft expired a long time ago, do we need this stuff anymore?
Flags: needinfo?(martin.thomson)
Comment on attachment 8990053 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

Partial review because I noticed a somewhat serious problem.

::: media/mtransport/WebrtcProxyChannelWrapper.cpp
@@ +110,5 @@
> +
> +void
> +WebrtcProxyChannelWrapper::OnClose(nsresult aReason)
> +{
> +  MOZ_ASSERT(mProxyCallbacks, "webrtc proxy callbacks should be non-null");

Let's also assert that we're on main here, and the two functions below.

::: media/mtransport/WebrtcProxyChannelWrapper.h
@@ +31,5 @@
> + * based on mtransport's and IPDL's threading requirements.
> + *
> + * WebrtcProxyChannelCallback calls are dispatched to the STS thread.
> + * IPDL calls are dispatched to the main thread.
> + */

Much better. Let's also document that this class is used only on the child process, and that thread dispatches for stuff on the parent process are not handled here.

::: media/mtransport/ipc/WebrtcProxyChannel.cpp
@@ +130,5 @@
> +  }
> +
> +  mClosed = true;
> +
> +  CleanUpConnection_s();

This is the only place I see CleanUpConnection_s being called. CloseWithReason isn't that long, so we can probably move that code in here.

@@ +155,5 @@
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIIOService> ioService;
> +  ioService = do_GetService(NS_IOSERVICE_CONTRACTID, &rv);
> +  if (NS_FAILED(rv)) {

Let's do this bit after we build the uri (and check for errors on that).

@@ +166,5 @@
> +
> +  nsCOMPtr<nsIURI> uri;
> +  rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID)
> +         .SetSpec(spec)
> +         .SetPort(aPort)

As long as we're using this API, let's use SetScheme("http") and SetHost(aHost) instead of building |spec| with string operations.

@@ +179,5 @@
> +  // -need to always tunnel since we're using a proxy
> +  // -there shouldn't be an opportunity to send cookies, but explicitly disallow
> +  // them anyway.
> +  // -the previous proxy tunnel didn't support redirects e.g. 307. don't need to
> +  // introduce new behavior. can't follow redirects on connect anyway.

Why can't we follow redirects on CONNECT?

@@ +183,5 @@
> +  // introduce new behavior. can't follow redirects on connect anyway.
> +  nsCOMPtr<nsIChannel> localChannel;
> +  rv = ioService->NewChannelFromURIWithProxyFlags2(
> +         uri,
> +         nullptr,

Please ensure that you get a necko review on this, since I'm not familiar with this API.

@@ +212,5 @@
> +
> +  rv = httpChannel->SetLoadFlags(nsIRequest::LOAD_BACKGROUND |
> +                                 nsIRequest::INHIBIT_CACHING |
> +                                 nsIRequest::LOAD_BYPASS_CACHE |
> +                                 nsIChannel::LOAD_BYPASS_SERVICE_WORKER);

This stuff too.

@@ +222,5 @@
> +  // often more than one of these channels will be created all at once by ICE
> +  nsCOMPtr<nsIClassOfService> cos = do_QueryInterface(localChannel);
> +  if (cos) {
> +    cos->AddClassFlags(nsIClassOfService::Unblocked |
> +                       nsIClassOfService::DontThrottle);

Hmm. How aggressive is the throttling you're disabling here?

@@ +230,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  rv = httpChannel->HTTPUpgrade(aAlpn, this);
> +  NS_ENSURE_SUCCESS(rv, rv);

Hmm, now that I look at the spec for this, it seems to be an expired draft. Not sure we need this ALPN stuff any longer. I have asked mt about it.

@@ +250,5 @@
> +WebrtcProxyChannel::EnqueueWrite_s(nsTArray<uint8_t>&& aWriteData)
> +{
> +  LOG(("WebrtcProxyChannel::EnqueueWrite %p\n", this));
> +  MOZ_ASSERT(OnSocketThread(), "not on socket thread");
> +

Let's assert that we're not closed here.

@@ +467,5 @@
> +    read += count;
> +  }
> +
> +  nsTArray<uint8_t> array(read);
> +  array.AppendElements(buffer, read);

Let's move this stuff into the check below.

@@ +493,5 @@
> +
> +  while(!mWriteQueue.empty()) {
> +    const WebrtcProxyData& data = mWriteQueue.front();
> +
> +    char* buffer = (char*)data.GetData().Elements() + mWriteOffset;

Let's use static_cast here.

::: media/mtransport/ipc/WebrtcProxyChannel.h
@@ +76,5 @@
> +  void CleanUpConnection_s();
> +
> +  void CloseWithReason(nsresult aReason);
> +
> +  uint32_t                        mWriteOffset;

Let's use size_t here.

::: media/mtransport/ipc/WebrtcProxyChannelParent.cpp
@@ +121,5 @@
> +{
> +  LOG(("WebrtcProxyChannelParent::OnRead %p %zu\n", this, aReadData.Length()));
> +
> +  if(!mChannel || !SendOnRead(std::move(aReadData))) {
> +    CleanupChannel();

mChannel && ...

@@ +131,5 @@
> +{
> +  LOG(("WebrtcProxyChannelParent::OnConnected %p\n", this));
> +
> +  if(!mChannel || !SendOnConnected()) {
> +    CleanupChannel();

Same here.

::: media/mtransport/nr_socket_proxy.cpp
@@ +185,5 @@
> +  if (mClosed) {
> +    return R_FAILED;
> +  }
> +
> +  MOZ_ASSERT(mWebrtcProxyChannel, "socket proxy is not open for reads");

We don't use this here, and removing this assertion should allow us to do without AssignChannel_DoNotUse, right?

::: media/mtransport/test/proxy_tunnel_socket_unittest.cpp
@@ +21,5 @@
>  
>  using namespace mozilla;
>  
> +// update TestReadMultipleSizes if you change this
> +const std::string kHelloMessage = "HELLO IS IT ME YOU'RE LOOKING FOR?";

Nice.

@@ +57,5 @@
>    }
>  
> +  static void readable_cb(NR_SOCKET s, int how, void *cb_arg) {
> +    NrSocketProxyTest* test = (NrSocketProxyTest*) cb_arg;
> +    size_t capacity = std::min(test->mReadAmount, test->mReadAllowance);

Let's see... how about we s/mReadAmount/mReadChunkSize/

@@ +153,5 @@
> +  // decreasing buffer sizes; ~17kb
> +  for(int i = 0; i < 32; ++i) {
> +    nsTArray<uint8_t> array;
> +    for(int j = 32 - i; j > 0; --j) {
> +      data += kHelloMessage;

I would find this more readable if we built the 17kb buffer in one loop, and then have a separate loop that copies it, calls OnRead with the copy, and then truncates it.

@@ +172,2 @@
>  
> +  ASSERT_EQ(data.length(), mSProxy->CountUnreadBytes());

Oof... this is edge-triggered, and while nICEr seems to keep its readable callback registered at all times, it does not seem to read until it runs out of data. This means that we can stall if nICEr gets a readable callback but does not manage to read everything in one go. nICEr will also misbehave if we fire the readable callback inside async_wait (we will go into an infinite loop AFAICT).

There's a few ways we could fix this, but I think the best option is to stop buffering reads, insist that the readable callback is registered every time OnRead is called, and that the readable callback consumes all of the data. I _think_ this is actually doable because we get to set the read buffer size on the nsISocketTransport, and as long as we set this to be small enough to fit the read buffer we should be good: https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_socket.c#66

This results in a lot less code, and less test-code as well.

Another option is to modify nICEr to read in a loop every time the readable callback fires. This would probably be a small modification, and could result in significant performance gains in high-bandwidth scenarios, but reading indefinitely like this might carry the risk of starving other work if there is a lot of data flowing. I don't think this is our fix for that reason.

A third option is to override NrSocketBase::async_wait to perform an async dispatch to DoCallbacks, but this won't play nicely with this unit-test.
Attachment #8990053 - Flags: review?(docfaraday) → review-
(In reply to Byron Campen [:bwc] (PTO 7/3-7/11) from comment #77)
> Comment on attachment 8990053 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 8990053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Partial review because I noticed a somewhat serious problem.
> 
> @@ +166,5 @@
> > +
> > +  nsCOMPtr<nsIURI> uri;
> > +  rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID)
> > +         .SetSpec(spec)
> > +         .SetPort(aPort)
> 
> As long as we're using this API, let's use SetScheme("http") and
> SetHost(aHost) instead of building |spec| with string operations.
> 

Yeah, I tried this at first but it seems you need to call SetSpec if you have an empty URI.

> @@ +179,5 @@
> > +  // -need to always tunnel since we're using a proxy
> > +  // -there shouldn't be an opportunity to send cookies, but explicitly disallow
> > +  // them anyway.
> > +  // -the previous proxy tunnel didn't support redirects e.g. 307. don't need to
> > +  // introduce new behavior. can't follow redirects on connect anyway.
> 
> Why can't we follow redirects on CONNECT?
> 

Necko code doesn't support it. 307 and ilk -> NS_ERROR_CONNECTION_REFUSED

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1611

> @@ +183,5 @@
> > +  // introduce new behavior. can't follow redirects on connect anyway.
> > +  nsCOMPtr<nsIChannel> localChannel;
> > +  rv = ioService->NewChannelFromURIWithProxyFlags2(
> > +         uri,
> > +         nullptr,
> 
> Please ensure that you get a necko review on this, since I'm not familiar
> with this API.
> 
> @@ +212,5 @@
> > +
> > +  rv = httpChannel->SetLoadFlags(nsIRequest::LOAD_BACKGROUND |
> > +                                 nsIRequest::INHIBIT_CACHING |
> > +                                 nsIRequest::LOAD_BYPASS_CACHE |
> > +                                 nsIChannel::LOAD_BYPASS_SERVICE_WORKER);
> 
> This stuff too.
> 

Will do.

> @@ +222,5 @@
> > +  // often more than one of these channels will be created all at once by ICE
> > +  nsCOMPtr<nsIClassOfService> cos = do_QueryInterface(localChannel);
> > +  if (cos) {
> > +    cos->AddClassFlags(nsIClassOfService::Unblocked |
> > +                       nsIClassOfService::DontThrottle);
> 
> Hmm. How aggressive is the throttling you're disabling here?
> 

Not very aggressive at all.  The first 16k bytes of the http request are never throttled.  This channel is media-related so I thought it was prudent to add this flag.  I don't think the http request will ever be throttled at its size (<1kb).

> ::: media/mtransport/nr_socket_proxy.cpp
> @@ +185,5 @@
> > +  if (mClosed) {
> > +    return R_FAILED;
> > +  }
> > +
> > +  MOZ_ASSERT(mWebrtcProxyChannel, "socket proxy is not open for reads");
> 
> We don't use this here, and removing this assertion should allow us to do
> without AssignChannel_DoNotUse, right?
> 

Yes, but TestConnected would have to be removed too.

> @@ +172,2 @@
> >  
> > +  ASSERT_EQ(data.length(), mSProxy->CountUnreadBytes());
> 
> Oof... this is edge-triggered, and while nICEr seems to keep its readable
> callback registered at all times, it does not seem to read until it runs out
> of data. This means that we can stall if nICEr gets a readable callback but
> does not manage to read everything in one go. nICEr will also misbehave if
> we fire the readable callback inside async_wait (we will go into an infinite
> loop AFAICT).
> 
> There's a few ways we could fix this, but I think the best option is to stop
> buffering reads, insist that the readable callback is registered every time
> OnRead is called, and that the readable callback consumes all of the data. I
> _think_ this is actually doable because we get to set the read buffer size
> on the nsISocketTransport, and as long as we set this to be small enough to
> fit the read buffer we should be good:
> https://searchfox.org/mozilla-central/source/media/mtransport/third_party/
> nICEr/src/ice/ice_socket.c#66
> 
> This results in a lot less code, and less test-code as well.
> 
> Another option is to modify nICEr to read in a loop every time the readable
> callback fires. This would probably be a small modification, and could
> result in significant performance gains in high-bandwidth scenarios, but
> reading indefinitely like this might carry the risk of starving other work
> if there is a lot of data flowing. I don't think this is our fix for that
> reason.
> 
> A third option is to override NrSocketBase::async_wait to perform an async
> dispatch to DoCallbacks, but this won't play nicely with this unit-test.

I don't need this assert because callbacks are never called inside async_wait/NR_ASYNC_WAIT (or I do because it should never be called?).  Also callbacks are called in a loop, as long as some data is consumed/available, in DoCallbacks to prevent stalls.  This differs slightly from NrTcpSocketIpc which dispatches a runnable to invoke its callbacks again. (https://searchfox.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#2117).  I think there can only be one chunk since nICEr will always have a readable callback registered and will read some data each callback.

Reducing nsISocketTransport's read buffer size impacts video performance quite a bit even on LAN.  After 1-2 minutes video started stalling and after another minute it completely stalled.  It's the socket's internal buffer size.

I did a quick test of option 1 as one Read IPC call (only does an AsyncWait) to start an OnRead IPC call.  I was worried about the performance but it seemed fine.  Not much changes since the readable callback needs to be called multiple times per chunk.
(In reply to Paul Vitale [:iidebyo] from comment #78)
> 
> I don't need this assert because callbacks are never called inside
> async_wait/NR_ASYNC_WAIT (or I do because it should never be called?).  Also
> callbacks are called in a loop, as long as some data is consumed/available,
> in DoCallbacks to prevent stalls.  This differs slightly from NrTcpSocketIpc
> which dispatches a runnable to invoke its callbacks again.
> (https://searchfox.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.cpp#2117).  I think there can only be one chunk since nICEr
> will always have a readable callback registered and will read some data each
> callback.

   I _think_ we're ok here then, although we're left with something that is kinda fragile and not tested in continuous integration. We can at least test the multiple-readable-callbacks stuff in the unit-test, but the other end of the bargain (making sure the readable callback is always registered) lives down in nICEr. Then again, implementing option 1 doesn't really help us in that regard; we still rely on nICEr keeping that callback registered, we're just a little more likely to notice in production.
mt says that while the draft for the webrtc ALPN stuff is past its expiry date, that is only because the IETF is bogged down processing the webrtc-related drafts, so we should consider it active.
Flags: needinfo?(martin.thomson)
Yep, sorry for not responding here, bugzilla was down (grr).  It's in the RFC editor's queue, so it's not only active, it's finished and awaiting publication.
After today I have much better understanding of how the network process isolation will relate to WebRTC.  I'll start review of the patches here now.

Please let you know that I'm offline for vacation between 23.7 - 12.8.  I'll try to do my best to move this forward quickly now.
Status: NEW → ASSIGNED
Comment on attachment 8986254 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

This looks pretty well, I have to say!  Thanks!

Sorry for the delay, but this was bouncing in the middle of larger projects planning.

I have few rather small details to fix before landing this.  It is very close to r+!

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2700,5 @@
>      return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +HttpBaseChannel::GetOnlyConnect(bool* aOnlyConnect) {

formatting nit:  keep the open brace on a new line

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1234,5 @@
>  
>          if (responseStatus == 200) {
> +            LOG(("proxy CONNECT succeeded! endtoendssl=%d onlyconnect=%d\n",
> +                 isHttps, onlyConnect));
> +            // If we're only connecting, we don't need to reset the connection.

|*reset = true| tells the *transaction* to reset its state, not the connection.

@@ +1263,1 @@
>              }

structure like this please:

if (isHttps) {
  if (!onlyConnect) {
    ...
  } else {
    ...
  }
}

@@ +1841,5 @@
>  
> +    if (mTransactionCaps & NS_HTTP_CONNECT_ONLY) {
> +        if (!mCompletedProxyConnect && !mProxyConnectStream) {
> +            // There is no CONNECT ever but we only want to CONNECT. We should
> +            // fail here since there's nothing to do but close the transaction.

I honestly don't understand this sentence at all.  can you rephrase it please?

should it be like:

"CONNECT has never been performed for the connection, but that was what we explicitly asked for, fail ..."

?

@@ +1843,5 @@
> +        if (!mCompletedProxyConnect && !mProxyConnectStream) {
> +            // There is no CONNECT ever but we only want to CONNECT. We should
> +            // fail here since there's nothing to do but close the transaction.
> +            return NS_ERROR_FAILURE;
> +        } else if (mCompletedProxyConnect) {

no else after return!

@@ +2017,5 @@
>  
> +    if ((mTransactionCaps & NS_HTTP_CONNECT_ONLY) &&
> +        !mCompletedProxyConnect && !mProxyConnectStream) {
> +        // There is no CONNECT ever but we only want to CONNECT. We should
> +        // fail here since there's nothing to do but close the transaction.

same sentence as above

@@ +2172,5 @@
> +                         nsHttp::Upgrade,
> +                         val))) {
> +        // rfc7639 proposes using the Alpn header to indicate the protocol used
> +        // in CONNECT when not used for TLS. The protocol is stored in Upgrade.
> +        rv = request->SetHeader(nsHttp::ALPN, val);

setting this header should IMO be done in the channel (nsHttpChannel), somewhere at [1].  unless that is too soon from some reason for you (then please explain here why this has to be done here)

you don't need a new atom, you can pass NS_LITERAL_CSTRING("Alpn"), it will do well.

[1] https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/netwerk/protocol/http/nsHttpChannel.cpp#1109

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +564,5 @@
>          : mConn(aConn)
> +        , mUpgradeListener(aListener)
> +        , mSocketTransport(nullptr)
> +        , mSocketIn(nullptr)
> +        , mSocketOut(nullptr)

no need to init nsCOMPtr members

@@ +588,5 @@
>  nsHttpConnectionMgr::CompleteUpgrade(nsAHttpConnection *aConn,
>                                       nsIHttpUpgradeListener *aUpgradeListener)
>  {
> +    // test if aUpgradeListener is a wrapped JsObject
> +    // bit of a HACK

I think we can live with that...

@@ +591,5 @@
> +    // test if aUpgradeListener is a wrapped JsObject
> +    // bit of a HACK
> +    nsCOMPtr<nsIPropertyBag> wrapper = do_QueryInterface(aUpgradeListener);
> +
> +    bool wrapped = false;

bool wrapped = !!wrapper;

@@ +2910,2 @@
>      nsCompleteUpgradeData *data = static_cast<nsCompleteUpgradeData *>(param);
> +    MOZ_ASSERT(OnSocketThread() || data->mJsWrapped, "not on socket thread");

|| (data->mJsWrapped == NS_IsMainThread())

@@ +2918,5 @@
> +    if (!data->mTook) {
> +        rv = data->mConn->TakeTransport(getter_AddRefs(data->mSocketTransport),
> +                                        getter_AddRefs(data->mSocketIn),
> +                                        getter_AddRefs(data->mSocketOut));
> +        data->mTook = true;

can you just check for (!data->mSocketTransport) instead having this bool member?

@@ +2939,2 @@
>              LOG(("nsHttpConnectionMgr::OnMsgCompleteUpgrade "
> +                 "this=%p conn=%p listener=%p wrapped=%d\n", this,

say in this log that you are forwarding to the main thread

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1681,5 @@
> +        // no keep-alive.
> +        if (mCaps & NS_HTTP_CONNECT_ONLY) {
> +            if ((mCaps & NS_HTTP_ALLOW_KEEPALIVE) ||
> +                !(mCaps & NS_HTTP_STICKY_CONNECTION)) {
> +                NS_WARNING("connection must be sticky and no keep-alive");

IMO, the word "must" is misplaced.  "should" is more appropriate?

would MOZ_ASSERT(!(mCaps & NS_HTTP_ALLOW_KEEPALIVE) && (mCaps & NS_HTTP_STICKY_CONNECTION)) be more appropriate?  It will crash in debug builds.

I would also appreciate some comment what doing |mNoContent = true| has as an effect and why (from the code path point of view) is needed.

do I understand that this is trying to discard the content even in case of a failure response from the proxy?

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +177,5 @@
>      [must_use] void HTTPUpgrade(in ACString aProtocolName,
>                                  in nsIHttpUpgradeListener aListener);
>  
>      /**
> +     * Enable/Disable only CONNECT to a proxy. Fails if no HTTUpgrade listener

HTTUpgrade - typo

::: netwerk/test/unit/test_proxyconnect.js
@@ +9,5 @@
> +// 3. Write data to the tunnel (and read server-side)
> +// 4. Read data from the tunnel (and write server-side)
> +// 5. done
> +// test_connectonly_noproxy tests an http channel with only connect set but
> +// no proxy configured.

what is the purpose of that test?  and what is the expected outcome?

@@ +14,5 @@
> +
> +ChromeUtils.import("resource://gre/modules/NetUtil.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +const socketserver_port = 8978;

this has to be (should be) an autobound port (pass -1 to init()), then read it dynamically from the server socket:

https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/netwerk/base/nsIServerSocket.idl#183

@@ +198,5 @@
> +}
> +
> +function makeChan(url) {
> +  if (!url)
> +    url = "https://localhost:" + socketserver_port + "/";

always if () { .. }

@@ +283,5 @@
> +
> +  do_test_pending();
> +}
> +
> +function test() {

maybe call this nextTest() ?

@@ +292,5 @@
> +    return;
> +  }
> +
> +  do_test_finished();
> +  (tests.shift())();

flip these two

@@ +296,5 @@
> +  (tests.shift())();
> +}
> +
> +var tests = [
> +  test_connectonly_noproxy,

any reason to not list both tests here and in run_test do nextText() (or test())?  setup for prefs would move inside the sub-tests.
Attachment #8986254 - Flags: review?(honzab.moz) → feedback+
Comment on attachment 8990053 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

for the necko bits (not that there would be that many..)
Attachment #8990053 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #83)
> Comment on attachment 8986254 [details] [diff] [review]
> part 1. change necko to allow CONNECT-only requests
> 
> Review of attachment 8986254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty well, I have to say!  Thanks!
> 
> Sorry for the delay, but this was bouncing in the middle of larger projects
> planning.
> 
> I have few rather small details to fix before landing this.  It is very
> close to r+!
> 

Thanks for the review!

> @@ +1841,5 @@
> > +            // There is no CONNECT ever but we only want to CONNECT. We should
> > +            // fail here since there's nothing to do but close the transaction.
> 
> I honestly don't understand this sentence at all.  can you rephrase it
> please?
> 
> should it be like:
> 
> "CONNECT has never been performed for the connection, but that was what we
> explicitly asked for, fail ..."
> 
> ?
> 

Yes, that's pretty much the same. 

> @@ +2918,5 @@
> > +        data->mTook = true;
> 
> can you just check for (!data->mSocketTransport) instead having this bool
> member?
> 

Yep.

> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +1681,5 @@
> > +        // no keep-alive.
> > +        if (mCaps & NS_HTTP_CONNECT_ONLY) {
> > +            if ((mCaps & NS_HTTP_ALLOW_KEEPALIVE) ||
> > +                !(mCaps & NS_HTTP_STICKY_CONNECTION)) {
> > +                NS_WARNING("connection must be sticky and no keep-alive");
> 
> do I understand that this is trying to discard the content even in case of a
> failure response from the proxy?
> 

Yes, the content is ignored anyway.  Only stock error pages are displayed in case of failure.

"must" to "should" is much better.

> ::: netwerk/test/unit/test_proxyconnect.js
> @@ +9,5 @@
> > +// 3. Write data to the tunnel (and read server-side)
> > +// 4. Read data from the tunnel (and write server-side)
> > +// 5. done
> > +// test_connectonly_noproxy tests an http channel with only connect set but
> > +// no proxy configured.
> 
> what is the purpose of that test?  and what is the expected outcome?
> 

It's possible to have a connect-only channel without a proxy configured.  This is a test to confirm that OnTransportAvailable is not called and StopRequest is called.  The connect-only flag messes with the http request flow which causes it to misbehave if there's no proxy configured.

> @@ +296,5 @@
> > +  (tests.shift())();
> > +}
> > +
> > +var tests = [
> > +  test_connectonly_noproxy,
> 
> any reason to not list both tests here and in run_test do nextText() (or
> test())?  setup for prefs would move inside the sub-tests.

No reason.  I'll change test() to nextTest() too.
any progress guys on this ?
This bug https://bugzilla.mozilla.org/show_bug.cgi?id=1385030 is connected to current one.
Just curious where we are now with this...
(In reply to albert.manukyan from comment #86)
> any progress guys on this ?
> This bug https://bugzilla.mozilla.org/show_bug.cgi?id=1385030 is connected
> to current one.
> Just curious where we are now with this...

Yes, there is progress being made.
> @@ +183,5 @@
> > +  // introduce new behavior. can't follow redirects on connect anyway.
> > +  nsCOMPtr<nsIChannel> localChannel;
> > +  rv = ioService->NewChannelFromURIWithProxyFlags2(
> > +         uri,
> > +         nullptr,
> 
> @@ +212,5 @@
> > +
> > +  rv = httpChannel->SetLoadFlags(nsIRequest::LOAD_BACKGROUND |
> > +                                 nsIRequest::INHIBIT_CACHING |
> > +                                 nsIRequest::LOAD_BYPASS_CACHE |
> > +                                 nsIChannel::LOAD_BYPASS_SERVICE_WORKER);
> 

I said I would make sure these bits in WebrtcProxyChannel.cpp would get a Necko review. :mayhemer, did you by chance take a look at these?
Flags: needinfo?(honzab.moz)
(In reply to Paul Vitale [:iidebyo] from comment #88)
> > @@ +183,5 @@
> > > +  // introduce new behavior. can't follow redirects on connect anyway.
> > > +  nsCOMPtr<nsIChannel> localChannel;
> > > +  rv = ioService->NewChannelFromURIWithProxyFlags2(
> > > +         uri,
> > > +         nullptr,
> > 
> > @@ +212,5 @@
> > > +
> > > +  rv = httpChannel->SetLoadFlags(nsIRequest::LOAD_BACKGROUND |
> > > +                                 nsIRequest::INHIBIT_CACHING |
> > > +                                 nsIRequest::LOAD_BYPASS_CACHE |
> > > +                                 nsIChannel::LOAD_BYPASS_SERVICE_WORKER);
> > 
> 
> I said I would make sure these bits in WebrtcProxyChannel.cpp would get a
> Necko review. :mayhemer, did you by chance take a look at these?

Tell me what exactly to look at.  Tomorrow is the last day I'm avail for review, I can try to find time to look at this.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #89)
> (In reply to Paul Vitale [:iidebyo] from comment #88)
> > > @@ +183,5 @@
> > > > +  // introduce new behavior. can't follow redirects on connect anyway.
> > > > +  nsCOMPtr<nsIChannel> localChannel;
> > > > +  rv = ioService->NewChannelFromURIWithProxyFlags2(
> > > > +         uri,
> > > > +         nullptr,
> > > 
> > > @@ +212,5 @@
> > > > +
> > > > +  rv = httpChannel->SetLoadFlags(nsIRequest::LOAD_BACKGROUND |
> > > > +                                 nsIRequest::INHIBIT_CACHING |
> > > > +                                 nsIRequest::LOAD_BYPASS_CACHE |
> > > > +                                 nsIChannel::LOAD_BYPASS_SERVICE_WORKER);
> > > 
> > 
> > I said I would make sure these bits in WebrtcProxyChannel.cpp would get a
> > Necko review. :mayhemer, did you by chance take a look at these?
> 
> Tell me what exactly to look at.  Tomorrow is the last day I'm avail for
> review, I can try to find time to look at this.

1. NewChannelFromURIWithProxyFlags2; I believe the second parameter being nullptr. (WebrtcProxyChannel.cpp:187)
2. The Load Flags for the http channel (WebrtcProxyChannel.cpp:213-216)
Necko does not allow for a CONNECT only request to happen.  This adds a flag
to signal an http channel should only CONNECT if proxied.  This flag can only be
set if an HTTPUpgrade handler has been assigned.  As proposed by rfc7639, an
alpn header will be included in the CONNECT request.  Its value is determined by
the upgrade protocol passed to HTTPUpgrade.

The flag is added as part of nsIHttpChannelInternal because it is dependent on
HTTPUpgrade.  It exists as a capability flag since the channel's transaction
needs to know to complete after a successful CONNECT.  Also the transaction's
connection needs to know to stop writing transaction data after it CONNECTs or
if there's no proxy, and to not tell the transaction to reset.

If an nsHttpChannel has this flag set then the upgrade handler will receive the
socket after the CONNECT succeeds without doing tls if https.

In order to support xpcshell-test for this change, nsHttpConnectionMgr does a
little check to see if HTTPUpgrade callback is in JavaScript.  If it is then
the callback is invoked on the main thread.
Attachment #8993656 - Flags: review?(honzab.moz)
Attachment #8986254 - Attachment is obsolete: true
Updated based on :bwc's review.

Rebase changed one necko part to use MOZ_ASSERT_UNREACHABLE not NS_NOTREACHED.

Also the parts described in comment 90 need necko review.
Attachment #8993658 - Flags: review?(honzab.moz)
Attachment #8993658 - Flags: review?(docfaraday)
Attachment #8990053 - Attachment is obsolete: true
Comment on attachment 8993658 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

Ok, I think this looks great! Thanks for bearing with me, and doing all this work.
Attachment #8993658 - Flags: review?(docfaraday) → review+
Comment on attachment 8993656 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

I looked more closely at this patch today.

r-, see comments.

I'm no more available for reviews, please ask Patrick McManus (:mcmanus) or Dragana (dd.moz) for reviews since now.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1854,5 @@
> +    if (mTransactionCaps & NS_HTTP_CONNECT_ONLY) {
> +        if (!mCompletedProxyConnect && !mProxyConnectStream) {
> +            // A CONNECT has been requested for this connection but will never
> +            // be performed. Fail here to let request callbacks happen.
> +            return NS_ERROR_FAILURE;

what I don't like on this is that we are already trying to establish a connection to the end point while we hit this line.  

this leads to immediate closing of the connection and the socket transport.  it's pretty much racy, hence the test (not expecting to accept a socket) is very unreliable and fails for me locally, while it may pass for you.  and it's simply not a good practice to do something like this.

see further remarks in the test.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1682,5 @@
> +        // the upgrade handler. If we're not successful the content body doesn't
> +        // matter. Proxy http errors are treated as network errors. This
> +        // connection won't be reused since it's marked sticky and no
> +        // keep-alive. The transaction will expect the server to close the
> +        // socket if there's no content length instead of doing the upgrade.

... of doing the upgrade, hence mNoContent = true; (or put the related comment part over that line?)

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +180,5 @@
>      /**
> +     * Enable/Disable only CONNECT to a proxy. Fails if no HTTPUpgrade listener
> +     * has been defined. An ALPN header is set using the upgrade protocol.
> +     */
> +    [must_use] attribute boolean onlyConnect;

I think this should only be one way - setConnectOnly() and readonly attr boolean onlyConnect to get the info.

it may make few things simpler

::: netwerk/test/unit/test_proxyconnect.js
@@ +11,5 @@
> +// 5. done
> +// test_connectonly_noproxy tests an http channel with only connect set but
> +// no proxy configured.
> +// 1. CONNECT to localhost:socketserver_port (channel's uri)
> +// 2. OnTransportAvailable callback NOT called (checked in step 3)

not true.  the |listener| class asserts |accepted == true|.

the 'no-proxy' case *locally fails for me* because:
- we are trying to do GET from https://localhost:port/ 
- there is a plain TCP server responding to the 'CONENCT ...' request with '200 Connected' (otherwise, it throws from the OnSocketAccepted callback what just does totally nothing), but this actually doesn't matter that much now
- we start connecting a TCP conn to that server (with a TLS layer over it) <== this is the bit I don't like about this patch
- onsocketwritable is hit in the transaction that immediately closes the socket (added return NS_ERROR_FAILURE when connect-only cap is set but proxy CONNECT is not being performed)
- this may happen before or after the TCP conn is established, since (for now) we do tcp fast open, but that is irrelevant anyway

so, we need to stop the channel somewhat sooner when the connect-only cap is set:
- connecting a non-https URI
- connecting an https uri, but no *http* proxy is set up - this is resolved asynchronously when PCAP (also via WPAD) is configured
- we *don't want* to allow when the request goes through a socks proxy either

(in a nut shell - allow this only when really going to a proxy)

the |return NS_ERROR_FAILURE;| with some added MOZ_ASSERT(false) may stay in nsHttpTransaction as a safety measure.

we should completely disable caching (the same set of load flags as in the patch 2) when the connect-only flag is set, as well as prevent speculative connections

@@ +331,5 @@
> +
> +  registerCleanupFunction(clearPrefs);
> +
> +  do_test_pending();
> +  nextTest();

flip these too, when nextTest from some reason throws, the way you have it will make the test hang.  when you flip, it will fail instantly.
Attachment #8993656 - Flags: review?(honzab.moz) → review-
Comment on attachment 8993658 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

r+ with all comments addressed.

::: media/mtransport/ipc/WebrtcProxyChannel.cpp
@@ +199,5 @@
> +  rv = ioService->NewChannelFromURIWithProxyFlags2(
> +         uri,
> +         nullptr,
> +         nsIProtocolProxyService::RESOLVE_PREFER_HTTPS_PROXY |
> +         nsIProtocolProxyService::RESOLVE_ALWAYS_TUNNEL,

these two are right!

@@ +205,5 @@
> +         aLoadInfo->LoadingPrincipal(),
> +         aLoadInfo->TriggeringPrincipal(),
> +         nsILoadInfo::SEC_DONT_FOLLOW_REDIRECTS |
> +         nsILoadInfo::SEC_COOKIES_OMIT |
> +         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,

not sure about this one, rather remove it, unless someone who understands these better told you to add it.

@@ +223,5 @@
> +    CloseWithReason(NS_ERROR_FAILURE);
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  rv = httpChannel->SetLoadFlags(nsIRequest::LOAD_BACKGROUND |

LOAD_BACKGROUND does not really have any meaning when this is not part of a load group.

although, if this is bound to a window and the connection is expected to go away with the window when it's closed, then we should add it to a load group.  that will simply cancel the channel and the connection when it's still in progress of being established.  it's a nice 100% correct cleanup.  but this is more for webrtc people to decide what the behavior should be.

meaning of the LOAD_BACKGROUND flag is to not let the progress spinner on the tab UI spin when this channel is in progress of creating the connection (when the correct load group is set on the channel.)  depends on what you want.

@@ +226,5 @@
> +
> +  rv = httpChannel->SetLoadFlags(nsIRequest::LOAD_BACKGROUND |
> +                                 nsIRequest::INHIBIT_CACHING |
> +                                 nsIRequest::LOAD_BYPASS_CACHE |
> +                                 nsIChannel::LOAD_BYPASS_SERVICE_WORKER);

maybe add also LOAD_ANONYMOUS, just to save adding cookies and web-auth headers (this won't effect proxy auth!)

but as said in review of the patch 1, this should be set internally on the channel when setConnectOnly() is called.  also, the above proxy resolve flags should be then added.  the more is hidden inside the channel the better.
Attachment #8993658 - Flags: review?(honzab.moz) → review+
Comment on attachment 8993656 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2719,5 @@
> +HttpBaseChannel::SetOnlyConnect(bool aOnlyConnect)
> +{
> +    if (!mUpgradeProtocolCallback) {
> +      return NS_ERROR_FAILURE;
> +    }

one more nit: maybe add NS_ENSURE_CALLED_BEFORE_OPEN (or what is the name of the macro...) to not mess with the flag(s) after asyncOpen
(In reply to Honza Bambas (:mayhemer) from comment #94)
> what I don't like on this is that we are already trying to establish a
> connection to the end point while we hit this line.  

to help a bit here, you could also persuade others to fix my concern with opening a connection while no proxy is configured in a followup bug (as this is going to be used only by webrtc) and for this patch just disable or remove the racy noproxy test.
Comment on attachment 8993656 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1854,5 @@
> +    if (mTransactionCaps & NS_HTTP_CONNECT_ONLY) {
> +        if (!mCompletedProxyConnect && !mProxyConnectStream) {
> +            // A CONNECT has been requested for this connection but will never
> +            // be performed. Fail here to let request callbacks happen.
> +            return NS_ERROR_FAILURE;

and if this line is to stay here, please log that you return a failure here.

@@ +1863,5 @@
> +            // updated after OnSocketWritable completes.
> +            // We've already done primary tls (if needed) and sent our CONNECT.
> +            // If we're doing a CONNECT only request there's no need to write
> +            // the http transaction or do the SSL handshake here.
> +            return NS_OK;

and OK here.
Comment on attachment 8993656 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

Thinking about it more, I don't believe that the concern I have is for total r- and to involve more reviewers.

Please fix other small comments (add logs, checks, change the API to setConnectOnly(), set flag internally in the channel, fix/remove the racy test etc) and file a new bug to prevent the racy tcp connection in the no-proxy configured case.

Then *push to try* to run all the tests.  If that doesn't show any newly failing tests, you are ok to land.
Attachment #8993656 - Flags: review- → review+
Same patch description. Updated based on :mayhemer's review.

Fixed racy no-proxy test.

Created Bug 1477592 to prevent TCP connection for no http proxy.
Same patch description. Updated based on :mayhemer's review.

SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL is needed because we have to allow loads
from other origins.
Attachment #8993656 - Attachment is obsolete: true
Attachment #8993658 - Attachment is obsolete: true
I think the next step is push to try.  I do not have try commit access.  bwc, would you be willing to vouch for me for level 1 access?
Sure! Link me to the bug for level 1 whenever you're ready.
(In reply to Byron Campen [:bwc] from comment #103)
> Sure! Link me to the bug for level 1 whenever you're ready.

Thanks! I needinfo'd you on bug 1477738.
Same patch description.  Remove unused variables in optimized builds.
WebrtcProxyChannelCallback now uses pure virtual ISupports macro and inherting
classes now override if needed.  Fixes static-analysis.

Adds casts from nsresult to uint32_t to fix compile issues on some platforms.
Attachment #8994053 - Attachment is obsolete: true
Attachment #8994055 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27249216f0309f26b4914aa993c1adbf2288be70

Should I run tests on more platforms and/or keep retrying TC/TCw jobs?
Flags: needinfo?(docfaraday)
You'll definitely need to fix the ASAN failures, but the TC stuff looks like it may not be your fault.

You'll also need to make sure you're running the tests on more platforms than just linux. Since you've touched more than just webrtc code, I think it would be reasonable to use "try: -b do -p all -u all -t none" once you've fixed the ASAN crashes.
Flags: needinfo?(docfaraday)
Fixed an intermittent failure on happy path test.  Mock proxy needs to accept
more than one connection if it doesn't respond fast enough on the first
connection.  Connections after the first are ignored.  Also instead of checking
the socket transport for being closed, use the proxy state.

Disabled the no proxy test.  It passes on every platform in try but fails for me
after updating to mac osx10.13.  It is re-enabled in Bug 1477592.
Fixed linux64 asan issue.  Shrinking an nsTArray can cause a UAF when it reaches
0 length.  Also removed some asserts for Length() when the content is checked
just after.
Attachment #8995300 - Attachment is obsolete: true
Attachment #8995299 - Attachment is obsolete: true
:bwc, is checkin next or is there a 'verification' step since there's no TURN server (AFAIK) in try?
There is a TURN server in try, although there is no HTTP/HTTPS proxy testing there. I'm going to guess that Education First will want to give it a spin, so let's make that happen before landing this stuff.
Blocks: 1477592
Rebase.
Assignee: paul.m.vitale → docfaraday
Rebase, and stop passing a system principal around.
Comment on attachment 9022218 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

We're using the system principal when opening the IO channel in WebrtcProxyChannel.cpp:207, does this look reasonable to you?
Attachment #9022218 - Flags: feedback?(ckerschb)
Comment on attachment 9022218 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

::: media/mtransport/ipc/WebrtcProxyChannel.cpp
@@ +205,5 @@
> +         0,
> +         aLoadInfo->LoadingNode(),
> +         aLoadInfo->LoadingPrincipal(),
> +         aLoadInfo->TriggeringPrincipal(),
> +         nsILoadInfo::SEC_DONT_FOLLOW_REDIRECTS |

(In reply to Byron Campen [:bwc] from comment #116)
> We're using the system principal when opening the IO channel in
> WebrtcProxyChannel.cpp:207, does this look reasonable to you?

I am not seeing that actually. You are using loadinfo->TriggeringPrincipal(), do you set that to be a systemPrincipal somewhere in your code? If so, where is that, maybe you could use the actual NodePrincipal where you create the loadinfo.

Anyway, the rule of thumb is, if the URL to be loaded can be influenced by a webpage, then you should not use the systemprincipal as the triggeringPrincipal. If the URL is hardcoded and can only be influenced by us, then SystemPrincipal is fine in that case.
Attachment #9022218 - Flags: feedback?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #117)
> Comment on attachment 9022218 [details] [diff] [review]
> part 2. replace proxy tunnel with new ipc-based tunnel
> 
> Review of attachment 9022218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/mtransport/ipc/WebrtcProxyChannel.cpp
> @@ +205,5 @@
> > +         0,
> > +         aLoadInfo->LoadingNode(),
> > +         aLoadInfo->LoadingPrincipal(),
> > +         aLoadInfo->TriggeringPrincipal(),
> > +         nsILoadInfo::SEC_DONT_FOLLOW_REDIRECTS |
> 
> (In reply to Byron Campen [:bwc] from comment #116)
> > We're using the system principal when opening the IO channel in
> > WebrtcProxyChannel.cpp:207, does this look reasonable to you?
> 
> I am not seeing that actually. You are using
> loadinfo->TriggeringPrincipal(), do you set that to be a systemPrincipal
> somewhere in your code? If so, where is that, maybe you could use the actual
> NodePrincipal where you create the loadinfo.

   We set that stuff to the system principal over in WebrtcProxyChannelWrapper.cpp:71.

> Anyway, the rule of thumb is, if the URL to be loaded can be influenced by a
> webpage, then you should not use the systemprincipal as the
> triggeringPrincipal. If the URL is hardcoded and can only be influenced by
> us, then SystemPrincipal is fine in that case.

   So, we just reuse the proxy URL that is configured for http traffic, if any. However, the URL that we are connecting to (via the proxy) is a TURN server specified by JS. If that's a problem, fixing it with a principal at the proxy channel creation is too late I think?
Flags: needinfo?(ckerschb)
(In reply to Byron Campen [:bwc] from comment #118)
>    So, we just reuse the proxy URL that is configured for http traffic, if
> any. However, the URL that we are connecting to (via the proxy) is a TURN
> server specified by JS. If that's a problem, fixing it with a principal at
> the proxy channel creation is too late I think?

Well if you can fix up the principal later then just use a freshly created NullPrincipal for the proxy channel. I guess I remember we have done that for other cases like that too. In which case if something goes wrong then the NullPrincipal prevents us from the worst.
Flags: needinfo?(ckerschb)
So I've given this a try, and it seems to work fine with a simple squid proxy, but we have a comment over in bug 1505329 that indicates that it still isn't working with a corporate proxy.
Attachment #9022217 - Attachment is obsolete: true
Comment on attachment 9024819 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

Could you double-check the rebase, particularly in netwerk/protocol/http/nsHttpChannel.cpp?
Attachment #9024819 - Flags: review?(honzab.moz)
Attachment #9022218 - Attachment is obsolete: true
Comment on attachment 9024819 [details] [diff] [review]
part 1. change necko to allow CONNECT-only requests

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

I overlooked the differences between this patch and https://bugzilla.mozilla.org/attachment.cgi?id=8996820&action=edit and it looks good to me.
Attachment #9024819 - Flags: review?(honzab.moz) → review+
Comment on attachment 9024820 [details] [diff] [review]
part 2. replace proxy tunnel with new ipc-based tunnel

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

Carry forward my own r+
Attachment #9024820 - Flags: review+
Attachment #8996820 - Attachment is obsolete: true
Attachment #8996821 - Attachment is obsolete: true
Fix build errors on windows
Fix build errors on windows
Attachment #9025366 - Flags: review+
Attachment #9025367 - Flags: review+
Attachment #9024819 - Attachment is obsolete: true
Attachment #9024820 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/457ec4c616e332f5d28d5cce2c1a86f0ecd4cf54
Bug 1203503 - part 1. change necko to allow CONNECT-only requests r+mayhemer

https://hg.mozilla.org/integration/mozilla-inbound/rev/d87037beffaabb3af5cc2fb4780e0cbadab6e979
Bug 1203503 - part 2. replace proxy tunnel with new ipc-based tunnel r+bwc, r+mayhemer
https://hg.mozilla.org/mozilla-central/rev/457ec4c616e3
https://hg.mozilla.org/mozilla-central/rev/d87037beffaa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Blocks: 1509471
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: