Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
336113002:
Avoid error messages from UDP sockets in Chromoting client. (Closed)
|
Created:
6 years, 6 months ago by Sergey Ulanov Modified:
6 years, 6 months ago Reviewers:
Wez CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAvoid error messages from UDP sockets in Chromoting client.
1. Fix SendTo() result handler for UDP sockets to properly handle errors
we may get. It wasn't possible in the past because pepper sockets
were not returning correct error codes. Now they do.
2. Don't try binding to link-local IPv6 addresses because it always
fails due to crbug.com/384854 .
BUG=382697
R=wez@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277848
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... File remoting/client/plugin/pepper_network_manager.cc (right): https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... remoting/client/plugin/pepper_network_manager.cc:83: // anyway. If Bind() fails in some well-defined way for this case, that can be handled at that point, then perhaps it's better to put this handling there, so that things will Just Work if/when that bug gets resolved? https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... File remoting/client/plugin/pepper_packet_socket_factory.cc (right): https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:30: bool IsTransientError(int error) { Are all of these errors really transient per-packet? Are none of them per-socket transient, or even permanent (e.g. PP_ERROR_NOACCESS)? https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:33: error == PP_ERROR_CONNECTION_RESET || error == PP_ERROR_NOMEMORY; I think it'd be good to clarify why this particular errors can/should/must be ignored the first time we see them.
I'll follow up with a separate CL for the host to use the same logic for error handling https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... File remoting/client/plugin/pepper_network_manager.cc (right): https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... remoting/client/plugin/pepper_network_manager.cc:83: // anyway. On 2014/06/16 22:36:36, Wez wrote: > If Bind() fails in some well-defined way for this case, that can be handled at > that point, then perhaps it's better to put this handling there, so that things > will Just Work if/when that bug gets resolved? They won't just work in that case. Once scopeId support is added in PPB_NetAddress we will need to make changes in our code to pass correct scopedId when calling Bind() and Send(). https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... File remoting/client/plugin/pepper_packet_socket_factory.cc (right): https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:30: bool IsTransientError(int error) { On 2014/06/16 22:36:36, Wez wrote: > Are all of these errors really transient per-packet? Are none of them per-socket > transient, or even permanent (e.g. PP_ERROR_NOACCESS)? This list is based on what chrome does for sockets in WebRTC (see content/browser/renderer_host/p2p/socket_host_udp.cc). PP_ERROR_NOACCESS can returned when the packet was blocked by the local firewall (on OSX), see https://code.google.com/p/webrtc/issues/detail?id=1207. https://codereview.chromium.org/336113002/diff/20001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:33: error == PP_ERROR_CONNECTION_RESET || error == PP_ERROR_NOMEMORY; On 2014/06/16 22:36:36, Wez wrote: > I think it'd be good to clarify why this particular errors can/should/must be > ignored the first time we see them. Done. Also added |retry| flag. It makes sense to retry sending only for some errors, but not the others.
https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... File remoting/client/plugin/pepper_network_manager.cc (right): https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... remoting/client/plugin/pepper_network_manager.cc:81: // Pepper socket API currently doesn't work with link-local IPv6 nit: Suggest "Link-local IPv6 addresses can't be bound via the current PPAPI Bind() interface as designed (see crbug.com384854); trying to do so would fail." https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... File remoting/client/plugin/pepper_packet_socket_factory.cc (right): https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:31: bool IsTransientError(int error, bool* retry) { This would be cleaner as a GetErrorAction(int error) that returns a KErrorActionFail, kErrorActionIgnore, kErrorActionRetry enum https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:31: bool IsTransientError(int error, bool* retry) { Adding a "retry" member to a function called IsTransientError ends up being confusing because it makes the caller wonder why would you ever _not_ retry if the error is transient - surely it might go away? The point here is that some errors should be treated as packet drops, and others are likely one-offs that should trigger a retry - the rest are real errors. https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:33: // sent to an unreachable address. What's the point of retrying if we tried to send to an unreachable address? What's the likelihood that it will suddenly become reachable within the retry interval?
https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... File remoting/client/plugin/pepper_network_manager.cc (right): https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... remoting/client/plugin/pepper_network_manager.cc:81: // Pepper socket API currently doesn't work with link-local IPv6 On 2014/06/17 01:13:26, Wez wrote: > nit: Suggest "Link-local IPv6 addresses can't be bound via the current PPAPI > Bind() interface as designed (see crbug.com384854); trying to do so would fail." Done. https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... File remoting/client/plugin/pepper_packet_socket_factory.cc (right): https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:31: bool IsTransientError(int error, bool* retry) { On 2014/06/17 01:13:26, Wez wrote: > This would be cleaner as a GetErrorAction(int error) that returns a > KErrorActionFail, kErrorActionIgnore, kErrorActionRetry enum Done. https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:31: bool IsTransientError(int error, bool* retry) { On 2014/06/17 01:13:26, Wez wrote: > Adding a "retry" member to a function called IsTransientError ends up being > confusing because it makes the caller wonder why would you ever _not_ retry if > the error is transient - surely it might go away? > > The point here is that some errors should be treated as packet drops, and others > are likely one-offs that should trigger a retry - the rest are real errors. Done. https://codereview.chromium.org/336113002/diff/60001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:33: // sent to an unreachable address. On 2014/06/17 01:13:26, Wez wrote: > What's the point of retrying if we tried to send to an unreachable address? > What's the likelihood that it will suddenly become reachable within the retry > interval? PP_ERROR_ADDRESS_UNREACHABLE and PP_ERROR_CONNECTION_RESET are returned in response to an ICMP packets from a remote host that failed to deliver a message we sent before. They indicate that at least one packet that was sent from this socket in the past was sent to an unreachable address. That doesn't mean that the current packet couldn't be sent. Actually, now that I looked at this list again it doesn't make sense to try resending after PP_ERROR_ADDRESS_INVALID, fixed now.
https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... File remoting/client/plugin/pepper_packet_socket_factory.cc (right): https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:36: // Returns ErrorAction that must be taken after |error| returned from sendto(). nit: is returned (which triggers line-wrap, so I'd suggest "Returns ErrorAction to perform if sendto() fails with |error|." https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:40: // an unreachable address. nit: Be explicit that the error could correspond to an earlier send, e.g. "UDP is connectionless, so we may receive ICMP unreachable or reset errors for previous sends to different addresses." This makes me wonder whether the sendto() we just performed actually gets performed even though the stack returns the error? https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:47: return ERROR_ACTION_IGNORE; Again, it would be good to clarify why we want to ignore this case; surely it indicates that we've been sent a completely bogus address, which is a real error? https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:51: // transient because the firewall may block only some target addresses. nit: Suggest for the second sentence "The firewall may still allow us to send to other addresses, so ignore the error for this particular send." https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:56: // still usable. nit: Suggest extending second sentence ".... is full, so drop this packet and assume the socket is still usable."
https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... File remoting/client/plugin/pepper_packet_socket_factory.cc (right): https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:36: // Returns ErrorAction that must be taken after |error| returned from sendto(). On 2014/06/17 02:34:10, Wez wrote: > nit: is returned (which triggers line-wrap, so I'd suggest "Returns ErrorAction > to perform if sendto() fails with |error|." Done. https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:40: // an unreachable address. On 2014/06/17 02:34:10, Wez wrote: > nit: Be explicit that the error could correspond to an earlier send, e.g. "UDP > is connectionless, so we may receive ICMP unreachable or reset errors for > previous sends to different addresses." Done, though I think the previous comment is explicit enough about it: "...one of the previous datagrams was sent..." > This makes me wonder whether the sendto() we just performed actually gets > performed even though the stack returns the error? No, it doesn't. https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:47: return ERROR_ACTION_IGNORE; On 2014/06/17 02:34:10, Wez wrote: > Again, it would be good to clarify why we want to ignore this case; surely it > indicates that we've been sent a completely bogus address, which is a real > error? Done. https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:51: // transient because the firewall may block only some target addresses. On 2014/06/17 02:34:10, Wez wrote: > nit: Suggest for the second sentence "The firewall may still allow us to send to > other addresses, so ignore the error for this particular send." Done. https://codereview.chromium.org/336113002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_packet_socket_factory.cc:56: // still usable. On 2014/06/17 02:34:10, Wez wrote: > nit: Suggest extending second sentence ".... is full, so drop this packet and > assume the socket is still usable." Done.
lgtm
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/336113002/100001
Message was sent while issue was closed.
Committed patchset #5 manually as r277848 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
