mobile: Fix the Cronvoy error mapping logic and add tests (envoyproxy…

…#33876)

EnvoyFinalStreamIntel.getResponseFlags() is set from the StreamInfo's
response flags as a bitmap. It's possible that multiple filters set
their own response flags, so there can end up being more than one
response flag in StreamInfo. The previous logic of checking equality
instead of checking in the bitmap was incorrect, so this commit fixes it
and adds tests for the mapEnvoyMobileErrorToNetError method.

Signed-off-by: Ali Beyad <[email protected]>

mobile/library/java/io/envoyproxy/envoymobile/engine/BUILD

@@ -55,6 +55,7 @@ java_library(
     visibility = ["//visibility:public"],
     deps = [
         "//library/java/io/envoyproxy/envoymobile/engine/types:envoy_c_types_lib",
+        "@maven//:androidx_annotation_annotation",
         "@maven//:com_google_code_findbugs_jsr305",
         "@maven//:com_google_protobuf_protobuf_javalite",
     ],

mobile/library/java/io/envoyproxy/envoymobile/engine/EnvoyFinalStreamIntelImpl.java

@@ -1,8 +1,10 @@
 package io.envoyproxy.envoymobile.engine;
 
 import io.envoyproxy.envoymobile.engine.types.EnvoyFinalStreamIntel;
+import androidx.annotation.VisibleForTesting;
 
-class EnvoyFinalStreamIntelImpl implements EnvoyFinalStreamIntel {
+@VisibleForTesting
+public class EnvoyFinalStreamIntelImpl implements EnvoyFinalStreamIntel {
   private final long streamStartMs;
   private final long dnsStartMs;
   private final long dnsEndMs;
@@ -39,6 +41,11 @@ class EnvoyFinalStreamIntelImpl implements EnvoyFinalStreamIntel {
     upstreamProtocol = values[15];
   }
 
+  @VisibleForTesting
+  public static EnvoyFinalStreamIntelImpl createForTesting(long[] values) {
+    return new EnvoyFinalStreamIntelImpl(values);
+  }
+
   @Override
   public long getStreamStartMs() {
     return streamStartMs;

mobile/library/java/org/chromium/net/impl/Errors.java

@@ -8,7 +8,7 @@
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.Collections;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import org.chromium.net.NetworkException;
 
@@ -75,18 +75,26 @@ public String toString() {
   public static NetError mapEnvoyMobileErrorToNetError(EnvoyFinalStreamIntel finalStreamIntel) {
     // if connection fails to be established, check if user is offline
     long responseFlag = finalStreamIntel.getResponseFlags();
-    if ((responseFlag == EnvoyMobileError.DNS_RESOLUTION_FAILED ||
-         responseFlag == EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE) &&
+    if (((responseFlag & EnvoyMobileError.DNS_RESOLUTION_FAILED) != 0 ||
+         (responseFlag & EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE) != 0) &&
         !AndroidNetworkMonitor.getInstance().isOnline()) {
       return NetError.ERR_INTERNET_DISCONNECTED;
     }
 
     // Check if negotiated_protocol is quic
+    // TODO(abeyad): Assigning all errors that happen when using HTTP/3 to QUIC_PROTOCOL_ERROR
+    // can mask the real error (DNS, upstream connection, etc). Revisit the error conversion logic.
     if (finalStreamIntel.getUpstreamProtocol() == UpstreamHttpProtocol.HTTP3) {
       return NetError.ERR_QUIC_PROTOCOL_ERROR;
     }
 
-    return ENVOYMOBILE_ERROR_TO_NET_ERROR.getOrDefault(responseFlag, NetError.ERR_OTHER);
+    // This will only map the first matched error to a NetError code.
+    for (Map.Entry<Long, NetError> entry : ENVOYMOBILE_ERROR_TO_NET_ERROR.entrySet()) {
+      if ((responseFlag & entry.getKey()) != 0) {
+        return entry.getValue();
+      }
+    }
+    return NetError.ERR_OTHER;
   }
 
   /**
@@ -127,7 +135,10 @@ public static boolean isQuicException(int javaError) {
   }
 
   private static Map<Long, NetError> buildErrorMap() {
-    Map<Long, NetError> errorMap = new HashMap<>();
+    // Mapping potentially multiple response flags to a NetError requires iterating over the map's
+    // entries in a deterministic order, so using a LinkedHashMap here, at the expense of a little
+    // extra memory overhead.
+    Map<Long, NetError> errorMap = new LinkedHashMap<>();
     errorMap.put(EnvoyMobileError.DNS_RESOLUTION_FAILED, NetError.ERR_NAME_NOT_RESOLVED);
     errorMap.put(EnvoyMobileError.DURATION_TIMEOUT, NetError.ERR_TIMED_OUT);
     errorMap.put(EnvoyMobileError.STREAM_IDLE_TIMEOUT, NetError.ERR_TIMED_OUT);

mobile/test/java/org/chromium/net/impl/BUILD

@@ -13,6 +13,7 @@ envoy_mobile_android_test(
         "CronetBidirectionalStateTest.java",
         "CronvoyEngineTest.java",
         "CronvoyLoggerTest.java",
+        "ErrorsTest.java",
         "UrlRequestCallbackTester.java",
     ],
     exec_properties = {

mobile/test/java/org/chromium/net/impl/ErrorsTest.java

@@ -0,0 +1,99 @@
+package org.chromium.net.impl;
+
+import static org.chromium.net.impl.Errors.mapEnvoyMobileErrorToNetError;
+import static org.junit.Assert.assertEquals;
+
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+import io.envoyproxy.envoymobile.engine.EnvoyFinalStreamIntelImpl;
+import io.envoyproxy.envoymobile.engine.UpstreamHttpProtocol;
+import org.chromium.net.impl.Errors.NetError;
+import org.junit.runner.RunWith;
+import org.junit.Test;
+
+@RunWith(AndroidJUnit4.class)
+public class ErrorsTest {
+
+  @Test
+  public void testMapEnvoyMobileErrorToNetErrorHttp3() throws Exception {
+    // 8 corresponds to NoRouteFound in StreamInfo::CoreResponseFlag:
+    // https://github.com/envoyproxy/envoy/blob/410e9a77bd6b74abb3e1545b4fd077e734d0fce3/envoy/stream_info/stream_info.h#L39
+    long responseFlags = 1L << 8;
+    EnvoyFinalStreamIntelImpl intel =
+        constructStreamIntel(responseFlags, UpstreamHttpProtocol.HTTP3);
+    NetError error = mapEnvoyMobileErrorToNetError(intel);
+    // It's an HTTP3 error, so it becomes a QUIC protocol error regardless of the response flag.
+    assertEquals(NetError.ERR_QUIC_PROTOCOL_ERROR, error);
+  }
+
+  @Test
+  public void testMapEnvoyMobileErrorToNetErrorFoundInMap() throws Exception {
+    // 4 corresponds to UpstreamRemoteReset in StreamInfo::CoreResponseFlag:
+    // https://github.com/envoyproxy/envoy/blob/410e9a77bd6b74abb3e1545b4fd077e734d0fce3/envoy/stream_info/stream_info.h#L39
+    long responseFlags = 1L << 4;
+    EnvoyFinalStreamIntelImpl intel =
+        constructStreamIntel(responseFlags, UpstreamHttpProtocol.HTTP2);
+    NetError error = mapEnvoyMobileErrorToNetError(intel);
+    assertEquals(NetError.ERR_CONNECTION_RESET, error);
+  }
+
+  @Test
+  public void testMapEnvoyMobileErrorToNetErrorMultipleResponseFlags() throws Exception {
+    // 4 corresponds to UpstreamRemoteReset and 16 corresponds to StreamIdleTimeout in
+    // StreamInfo::CoreResponseFlag:
+    // https://github.com/envoyproxy/envoy/blob/410e9a77bd6b74abb3e1545b4fd077e734d0fce3/envoy/stream_info/stream_info.h#L39
+    long responseFlags = 1L << 4;
+    responseFlags |= (1L << 16);
+    EnvoyFinalStreamIntelImpl intel =
+        constructStreamIntel(responseFlags, UpstreamHttpProtocol.HTTP2);
+    NetError error = mapEnvoyMobileErrorToNetError(intel);
+    // STREAM_IDLE_TIMEOUT is first in the map's entries, so ERR_TIMED_OUT should be chosen over
+    // ERR_CONNECTION_RESET.
+    assertEquals(NetError.ERR_TIMED_OUT, error);
+  }
+
+  @Test
+  public void testMapEnvoyMobileErrorToNetErrorNotFoundInMap() throws Exception {
+    // 1 corresponds to NoHealthyUpstream in StreamInfo::CoreResponseFlag:
+    // https://github.com/envoyproxy/envoy/blob/410e9a77bd6b74abb3e1545b4fd077e734d0fce3/envoy/stream_info/stream_info.h#L39
+    long responseFlags = 1L << 1;
+    EnvoyFinalStreamIntelImpl intel =
+        constructStreamIntel(responseFlags, UpstreamHttpProtocol.HTTP2);
+    NetError error = mapEnvoyMobileErrorToNetError(intel);
+    // There is no NetError mapping from NoHealthyUpstream, so the default is ERR_OTHER.
+    assertEquals(NetError.ERR_OTHER, error);
+  }
+
+  @Test
+  public void testMapEnvoyMobileErrorToNetErrorEmptyResponseFlags() throws Exception {
+    // 0 means no response flags are set on the bitmap.
+    long responseFlags = 0;
+    EnvoyFinalStreamIntelImpl intel =
+        constructStreamIntel(responseFlags, UpstreamHttpProtocol.HTTP2);
+    NetError error = mapEnvoyMobileErrorToNetError(intel);
+    // The default is ERR_OTHER.
+    assertEquals(NetError.ERR_OTHER, error);
+  }
+
+  private EnvoyFinalStreamIntelImpl constructStreamIntel(long responseFlags, long protocol) {
+    long[] values = new long[16];
+    values[0] = 0;
+    values[1] = 0;
+    values[2] = 0;
+    values[3] = 0;
+    values[4] = 0;
+    values[5] = 0;
+    values[6] = 0;
+    values[7] = 0;
+    values[8] = 0;
+    values[9] = 0;
+    values[10] = 0;
+    values[11] = 0;
+    values[12] = 0;
+    values[13] = 0;
+    // We only care about the response flags and upstream protocol values.
+    values[14] = responseFlags;
+    values[15] = protocol;
+
+    return EnvoyFinalStreamIntelImpl.createForTesting(values);
+  }
+}