New issue
Advanced search Search tips

Issue 876885 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Add ReadIfReady() and CancelReadIfReady() to net::QuicProxyClientSocket

Project Member Reported by xunji...@chromium.org, Aug 22

Issue description

Network service exposes socket functionalities via mojo APIs. 
One such case is network::ProxyResolvingSocketMojo which wraps around //net proxy socket classes.

With mojo sockets, we need a way to cancel pending reads without having buffered data in socket subclasses. This functionality is needed for upgrading a TCP socket to TLS. This requirement means that we need to use net::Socket::ReadIfReady() instead of net::Socket::Read().

I have implemented ReadIfReady() for HttpProxyClientSocket, SpdyProxyClientSocket and SOCKSClientSocket. To be consistent with other proxy socket classes, it will be good if QuicProxyClientSocket can support ReadIfReady as well.

Currently QuicProxyClientSocket is disabled in the mojo socket APIs (https://cs.chromium.org/chromium/src/services/network/proxy_resolving_client_socket.cc?rcl=0d326b48bd894605b4e41e22f6f085af7c12274b&l=269). I didn't find a way to add ReadIfReady() to QuicProxyClientSocket easily, so I am filing a bug for this one.

A side note:
Currently proxy_resolving_socket.mojom doesn't support upgrading to TLS. The other approach (if we want Quic proxies to be used for proxy_resolving_socket.mojom) is to modify services/network/socket_data_pump.cc to use Read() instead for network::ProxyResolvingSocketMojo and ReadIfReady() for network::TCPConnectedSocket, because only TCPConnectedSocket needs to support upgrading to TLS.
 
Cc: mmenke@chromium.org
+mmenke: Matt, QuicProxyClientSocket is a bit tricky to modify. I think I am going to just add a note in the code and link it to this bug. It's not urgent, because ProxyResolvingClientSocket explicitly removes quic proxies (https://cs.chromium.org/chromium/src/services/network/proxy_resolving_client_socket.cc?rcl=0d326b48bd894605b4e41e22f6f085af7c12274b&l=269).
SGTM.  Mind updating the TODO with a link to this bug, too?
Cc: morlovich@chromium.org
+morlovich@ FHI. From conversation with Maks this morning, looks like we will need to support upgrading to TLS for proxied sockets. An existing use case is jingle/glue/xmpp_client_socket_factory.h.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99eb4b03a14c3cf1ae77db3a48efdc7981f1ee66

commit 99eb4b03a14c3cf1ae77db3a48efdc7981f1ee66
Author: Helen Li <xunjieli@chromium.org>
Date: Fri Aug 24 14:51:59 2018

Add a note to network::ProxyResolvingClientSocket on quic proxy

This CL adds a note to ProxyResolvingClientSocket on why QUIC proxy
isn't yet supported and links to crbug.

Bug: 876885
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I5d07064e252e8bdd9b28281488d88613fa102bd4
Reviewed-on: https://chromium-review.googlesource.com/1185746
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585837}
[modify] https://crrev.com/99eb4b03a14c3cf1ae77db3a48efdc7981f1ee66/services/network/proxy_resolving_client_socket.cc

Sign in to add a comment