From 7bf274cf073491601d14d38e07ac948b91b2f872 Mon Sep 17 00:00:00 2001 From: Chidera Olibie Date: Mon, 24 Oct 2022 16:35:23 +0100 Subject: [PATCH] add quic exception and delete tests The mockurlrequestmock tests are not useful because the different FailurPhases being tested are not used anywhere in the code. Also deleted FailurePhase as it's also unneeded Signed-off-by: Chidera Olibie --- .../io/envoyproxy/envoymobile/engine/BUILD | 1 + .../engine/UpstreamHttpProtocol.java | 20 ++++++ .../net/impl/CronetBidirectionalStream.java | 4 +- .../chromium/net/impl/CronetUrlRequest.java | 4 +- .../java/org/chromium/net/impl/Errors.java | 15 ++-- .../filters/http/test_read/filter.cc | 25 +++++-- .../io/envoyproxy/envoymobile/engine/BUILD | 2 +- .../chromium/net/CronetUrlRequestTest.java | 72 +++++-------------- test/java/org/chromium/net/testing/BUILD | 1 - .../chromium/net/testing/FailurePhase.java | 12 ---- .../net/testing/MockUrlRequestJobFactory.java | 16 ++--- 11 files changed, 76 insertions(+), 96 deletions(-) create mode 100644 library/java/io/envoyproxy/envoymobile/engine/UpstreamHttpProtocol.java delete mode 100644 test/java/org/chromium/net/testing/FailurePhase.java diff --git a/library/java/io/envoyproxy/envoymobile/engine/BUILD b/library/java/io/envoyproxy/envoymobile/engine/BUILD index 2c037a7262..857d242a69 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/BUILD +++ b/library/java/io/envoyproxy/envoymobile/engine/BUILD @@ -10,6 +10,7 @@ android_library( "AndroidJniLibrary.java", "AndroidNetworkMonitor.java", "AndroidProxyMonitor.java", + "UpstreamHttpProtocol.java", ], custom_package = "io.envoyproxy.envoymobile.engine", manifest = "AndroidEngineManifest.xml", diff --git a/library/java/io/envoyproxy/envoymobile/engine/UpstreamHttpProtocol.java b/library/java/io/envoyproxy/envoymobile/engine/UpstreamHttpProtocol.java new file mode 100644 index 0000000000..1153d5b1df --- /dev/null +++ b/library/java/io/envoyproxy/envoymobile/engine/UpstreamHttpProtocol.java @@ -0,0 +1,20 @@ +package io.envoyproxy.envoymobile.engine; + +import androidx.annotation.LongDef; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; + +/** + * The upstream protocol, if an upstream connection was established. Field + * entries are based off of Envoy's Http::Protocol + * https://github.com/envoyproxy/envoy/blob/main/envoy/http/protocol.h + */ +@LongDef({UpstreamHttpProtocol.HTTP10, UpstreamHttpProtocol.HTTP11, UpstreamHttpProtocol.HTTP2, + UpstreamHttpProtocol.HTTP3}) +@Retention(RetentionPolicy.SOURCE) +public @interface UpstreamHttpProtocol { + long HTTP10 = 0; + long HTTP11 = 1; + long HTTP2 = 2; + long HTTP3 = 3; +} diff --git a/library/java/org/chromium/net/impl/CronetBidirectionalStream.java b/library/java/org/chromium/net/impl/CronetBidirectionalStream.java index b72ed3042c..cb089e6959 100644 --- a/library/java/org/chromium/net/impl/CronetBidirectionalStream.java +++ b/library/java/org/chromium/net/impl/CronetBidirectionalStream.java @@ -653,13 +653,13 @@ private void onErrorReceived(int errorCode, EnvoyFinalStreamIntel finalStreamInt mResponseInfo.setReceivedByteCount(finalStreamIntel.getReceivedByteCount()); } - NetError netError = mapEnvoyMobileErrorToNetError(finalStreamIntel.getResponseFlags()); + NetError netError = mapEnvoyMobileErrorToNetError(finalStreamIntel); int javaError = mapNetErrorToCronetApiErrorCode(netError); if (isQuicException(javaError)) { mException.set(new QuicExceptionImpl("Exception in BidirectionalStream: " + netError, javaError, netError.getErrorCode(), - /*nativeQuicError*/ 0)); + /*nativeQuicError*/ 1)); } else { mException.set(new BidirectionalStreamNetworkException( "Exception in BidirectionalStream: " + netError, javaError, netError.getErrorCode())); diff --git a/library/java/org/chromium/net/impl/CronetUrlRequest.java b/library/java/org/chromium/net/impl/CronetUrlRequest.java index e9b27db1dc..92a68338cc 100644 --- a/library/java/org/chromium/net/impl/CronetUrlRequest.java +++ b/library/java/org/chromium/net/impl/CronetUrlRequest.java @@ -934,13 +934,13 @@ public void onError(int errorCode, String message, int attemptCount, return; } - NetError netError = mapEnvoyMobileErrorToNetError(finalStreamIntel.getResponseFlags()); + NetError netError = mapEnvoyMobileErrorToNetError(finalStreamIntel); int javaError = mapNetErrorToCronetApiErrorCode(netError); if (isQuicException(javaError)) { enterErrorState(new QuicExceptionImpl("Exception in CronetUrlRequest: " + netError, javaError, netError.getErrorCode(), - /*nativeQuicError*/ 0)); + /*nativeQuicError*/ 1)); return; } diff --git a/library/java/org/chromium/net/impl/Errors.java b/library/java/org/chromium/net/impl/Errors.java index fb135d1a32..9ef13204d0 100644 --- a/library/java/org/chromium/net/impl/Errors.java +++ b/library/java/org/chromium/net/impl/Errors.java @@ -3,6 +3,8 @@ import android.util.Log; import androidx.annotation.LongDef; import io.envoyproxy.envoymobile.engine.AndroidNetworkMonitor; +import io.envoyproxy.envoymobile.engine.UpstreamHttpProtocol; +import io.envoyproxy.envoymobile.engine.types.EnvoyFinalStreamIntel; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.util.Collections; @@ -68,17 +70,20 @@ public String toString() { * @param responseFlag envoymobile's finalStreamIntel responseFlag * @return the NetError that the EnvoyMobileError maps to */ - public static NetError mapEnvoyMobileErrorToNetError(long responseFlag) { - /* Todo(https://github.com/envoyproxy/envoy-mobile/issues/1594): - * if negotiated_protocol is quic return QUIC_PROTOCOL_FAILED - */ + public static NetError mapEnvoyMobileErrorToNetError(EnvoyFinalStreamIntel finalStreamIntel) { + // Check if negotiated_protocol is quic + if (finalStreamIntel.getUpstreamProtocol() == UpstreamHttpProtocol.HTTP3) { + return NetError.ERR_QUIC_PROTOCOL_ERROR; + } // 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) && + responseFlag == EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE) && !AndroidNetworkMonitor.getInstance().isOnline()) { return NetError.ERR_INTERNET_DISCONNECTED; } + return ENVOYMOBILE_ERROR_TO_NET_ERROR.getOrDefault(responseFlag, NetError.ERR_OTHER); } diff --git a/test/integration/filters/http/test_read/filter.cc b/test/integration/filters/http/test_read/filter.cc index 1803da2dd1..fa28f02a9a 100644 --- a/test/integration/filters/http/test_read/filter.cc +++ b/test/integration/filters/http/test_read/filter.cc @@ -8,16 +8,29 @@ namespace TestRead { Http::FilterHeadersStatus TestReadFilter::decodeHeaders(Http::RequestHeaderMap& request_headers, bool) { - // sample path is /failed?start=0x10000 + // sample path is /failed?error=0x10000 Http::Utility::QueryParams query_parameters = Http::Utility::parseQueryString(request_headers.Path()->value().getStringView()); + auto error = query_parameters.find("error"); + // no error was specified, so move on + if (error == query_parameters.end()) { + return Http::FilterHeadersStatus::Continue; + } + + StreamInfo::StreamInfo& stream_info = decoder_callbacks_->streamInfo(); uint64_t response_flag; - if (absl::SimpleAtoi(query_parameters.at("start"), &response_flag)) { - decoder_callbacks_->streamInfo().setResponseFlag( - TestReadFilter::mapErrorToResponseFlag(response_flag)); - decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, "test_read filter threw: ", nullptr, - absl::nullopt, ""); + // set response error code + if (absl::SimpleAtoi(error->second, &response_flag)) { + stream_info.setResponseFlag(TestReadFilter::mapErrorToResponseFlag(response_flag)); + // check if we want a quic server error + if (query_parameters.find("quic") != query_parameters.end()) { + stream_info.setUpstreamInfo(std::make_shared()); + stream_info.upstreamInfo()->setUpstreamProtocol(Http::Protocol::Http3); + } } + + decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, "test_read filter threw: ", nullptr, + absl::nullopt, ""); return Http::FilterHeadersStatus::StopIteration; } diff --git a/test/java/io/envoyproxy/envoymobile/engine/BUILD b/test/java/io/envoyproxy/envoymobile/engine/BUILD index 3beaa52dee..bd8b92011a 100644 --- a/test/java/io/envoyproxy/envoymobile/engine/BUILD +++ b/test/java/io/envoyproxy/envoymobile/engine/BUILD @@ -46,4 +46,4 @@ envoy_mobile_android_test( "//library/java/io/envoyproxy/envoymobile/engine:envoy_engine_lib", "//library/kotlin/io/envoyproxy/envoymobile:envoy_interfaces_lib", ], -) \ No newline at end of file +) diff --git a/test/java/org/chromium/net/CronetUrlRequestTest.java b/test/java/org/chromium/net/CronetUrlRequestTest.java index 5a609398ea..39d82ccf8d 100644 --- a/test/java/org/chromium/net/CronetUrlRequestTest.java +++ b/test/java/org/chromium/net/CronetUrlRequestTest.java @@ -45,7 +45,6 @@ import org.chromium.net.testing.CronetTestRule.CronetTestFramework; import org.chromium.net.testing.CronetTestRule.OnlyRunNativeCronet; import org.chromium.net.testing.CronetTestRule.RequiresMinApi; -import org.chromium.net.testing.FailurePhase; import org.chromium.net.testing.Feature; import org.chromium.net.testing.MockUrlRequestJobFactory; import org.chromium.net.testing.NativeTestServer; @@ -65,6 +64,9 @@ /** * Test functionality of CronetUrlRequest. + * This test makes use of 2 cronetEngines. + * One with request filters to throw custom errors and the other without. + * TODO(colibie): switch to one engine, with the filter ignoring urls with no custom errors. */ @RunWith(RobolectricTestRunner.class) public class CronetUrlRequestTest { @@ -723,45 +725,6 @@ public void testMockNotFound() throws Exception { assertEquals(callback.mResponseStep, ResponseStep.ON_SUCCEEDED); } - @Test - @SmallTest - @Feature({"Cronet"}) - @OnlyRunNativeCronet // Java impl doesn't support MockUrlRequestJobFactory - @Ignore("https://github.com/envoyproxy/envoy-mobile/issues/1549") - public void testMockStartAsyncError() throws Exception { - final int arbitraryNetError = -3; - TestUrlRequestCallback callback = startAndWaitForComplete( - mMockUrlRequestJobFactory.getCronetEngine(), - MockUrlRequestJobFactory.getMockUrlWithFailure(FailurePhase.START, arbitraryNetError)); - assertNull(callback.mResponseInfo); - assertNotNull(callback.mError); - assertEquals(arbitraryNetError, - ((NetworkException)callback.mError).getCronetInternalErrorCode()); - assertEquals(0, callback.mRedirectCount); - assertTrue(callback.mOnErrorCalled); - assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep); - } - - @Test - @SmallTest - @Feature({"Cronet"}) - @OnlyRunNativeCronet // Java impl doesn't support MockUrlRequestJobFactory - @Ignore("https://github.com/envoyproxy/envoy-mobile/issues/1549") - public void testMockReadDataSyncError() throws Exception { - final int arbitraryNetError = -4; - TestUrlRequestCallback callback = startAndWaitForComplete( - mMockUrlRequestJobFactory.getCronetEngine(), - MockUrlRequestJobFactory.getMockUrlWithFailure(FailurePhase.READ_SYNC, arbitraryNetError)); - assertEquals(200, callback.mResponseInfo.getHttpStatusCode()); - assertEquals(15, callback.mResponseInfo.getReceivedByteCount()); - assertNotNull(callback.mError); - assertEquals(arbitraryNetError, - ((NetworkException)callback.mError).getCronetInternalErrorCode()); - assertEquals(0, callback.mRedirectCount); - assertTrue(callback.mOnErrorCalled); - assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep); - } - /** * Tests that request continues when client certificate is requested. */ @@ -2139,12 +2102,11 @@ public void testCookiesArentSavedOrSent() throws Exception { @SmallTest @Feature({"Cronet"}) @OnlyRunNativeCronet - @Ignore("https://github.com/envoyproxy/envoy-mobile/issues/1549") public void testQuicErrorCode() throws Exception { + long envoyMobileError = 0x2000; TestUrlRequestCallback callback = startAndWaitForComplete( mMockUrlRequestJobFactory.getCronetEngine(), - MockUrlRequestJobFactory.getMockUrlWithFailure( - FailurePhase.START, NetError.ERR_QUIC_PROTOCOL_ERROR.getErrorCode())); + MockUrlRequestJobFactory.getMockQuicUrlWithFailure(envoyMobileError)); assertNull(callback.mResponseInfo); assertNotNull(callback.mError); assertEquals(NetworkException.ERROR_QUIC_PROTOCOL_FAILED, @@ -2159,12 +2121,12 @@ public void testQuicErrorCode() throws Exception { @SmallTest @Feature({"Cronet"}) @OnlyRunNativeCronet - @Ignore("https://github.com/envoyproxy/envoy-mobile/issues/1549") + @Ignore("https://github.com/envoyproxy/envoy-mobile/issues/1594: error removed in envoymobile") public void testQuicErrorCodeForNetworkChanged() throws Exception { - TestUrlRequestCallback callback = startAndWaitForComplete( - mMockUrlRequestJobFactory.getCronetEngine(), - MockUrlRequestJobFactory.getMockUrlWithFailure( - FailurePhase.START, NetError.ERR_NETWORK_CHANGED.getErrorCode())); + TestUrlRequestCallback callback = + startAndWaitForComplete(mMockUrlRequestJobFactory.getCronetEngine(), + MockUrlRequestJobFactory.getMockUrlWithFailure( + NetError.ERR_NETWORK_CHANGED.getErrorCode())); assertNull(callback.mResponseInfo); assertNotNull(callback.mError); assertEquals(NetworkException.ERROR_NETWORK_CHANGED, @@ -2185,9 +2147,9 @@ public void testQuicErrorCodeForNetworkChanged() throws Exception { @SmallTest @Feature({"Cronet"}) @OnlyRunNativeCronet - @Ignore("https://github.com/envoyproxy/envoy-mobile/issues/1549") public void testLegacyOnFailedCallback() throws Exception { - final int netError = -123; + final long envoyMobileError = 0x2000; + final int netError = NetError.ERR_OTHER.getErrorCode(); final AtomicBoolean failedExpectation = new AtomicBoolean(); final ConditionVariable done = new ConditionVariable(); UrlRequest.Callback callback = new UrlRequest.Callback() { @@ -2231,8 +2193,8 @@ public void onCanceled(UrlRequest request, UrlResponseInfo info) { } }; - UrlRequest.Builder builder = mTestFramework.mCronetEngine.newUrlRequestBuilder( - MockUrlRequestJobFactory.getMockUrlWithFailure(FailurePhase.START, netError), callback, + UrlRequest.Builder builder = mMockUrlRequestJobFactory.getCronetEngine().newUrlRequestBuilder( + MockUrlRequestJobFactory.getMockUrlWithFailure(envoyMobileError), callback, Executors.newSingleThreadExecutor()); final UrlRequest urlRequest = builder.build(); urlRequest.start(); @@ -2244,9 +2206,9 @@ public void onCanceled(UrlRequest request, UrlResponseInfo info) { private void checkSpecificErrorCode(@EnvoyMobileError long envoyMobileError, NetError netError, int errorCode, String name, boolean immediatelyRetryable) throws Exception { - TestUrlRequestCallback callback = startAndWaitForComplete( - mMockUrlRequestJobFactory.getCronetEngine(), - MockUrlRequestJobFactory.getMockUrlWithFailure(FailurePhase.START, envoyMobileError)); + TestUrlRequestCallback callback = + startAndWaitForComplete(mMockUrlRequestJobFactory.getCronetEngine(), + MockUrlRequestJobFactory.getMockUrlWithFailure(envoyMobileError)); assertNull(callback.mResponseInfo); assertNotNull(callback.mError); assertEquals(netError.getErrorCode(), diff --git a/test/java/org/chromium/net/testing/BUILD b/test/java/org/chromium/net/testing/BUILD index f04fa458e6..888867911a 100644 --- a/test/java/org/chromium/net/testing/BUILD +++ b/test/java/org/chromium/net/testing/BUILD @@ -14,7 +14,6 @@ android_library( "ContextUtils.java", "CronetTestRule.java", "CronetTestUtil.java", - "FailurePhase.java", "Feature.java", "FileUtils.java", "Http2TestHandler.java", diff --git a/test/java/org/chromium/net/testing/FailurePhase.java b/test/java/org/chromium/net/testing/FailurePhase.java deleted file mode 100644 index ccc68de266..0000000000 --- a/test/java/org/chromium/net/testing/FailurePhase.java +++ /dev/null @@ -1,12 +0,0 @@ -package org.chromium.net.testing; - -public enum FailurePhase { - START, - READ_SYNC, - READ_ASYNC; - - @Override - public String toString() { - return name().toLowerCase(); - } -} diff --git a/test/java/org/chromium/net/testing/MockUrlRequestJobFactory.java b/test/java/org/chromium/net/testing/MockUrlRequestJobFactory.java index 6c6393e587..e9e7c5f56b 100644 --- a/test/java/org/chromium/net/testing/MockUrlRequestJobFactory.java +++ b/test/java/org/chromium/net/testing/MockUrlRequestJobFactory.java @@ -50,20 +50,12 @@ public void shutdown() { * org.chromium.net.test.FailurePhase. * @param @param envoyMobileError reported by the engine. */ - public static String getMockUrlWithFailure(FailurePhase phase, long envoyMobileError) { - switch (phase) { - case START: - case READ_SYNC: - case READ_ASYNC: - break; - default: - throw new IllegalArgumentException("phase not in org.chromium.net.test.FailurePhase"); - } - return TEST_URL + "/failed?" + phase + "=" + envoyMobileError; + public static String getMockUrlWithFailure(long envoyMobileError) { + return TEST_URL + "/failed?error=" + envoyMobileError; } - public static String getMockUrlWithFailure(FailurePhase phase, int netError) { - throw new UnsupportedOperationException("To be implemented"); + public static String getMockQuicUrlWithFailure(long envoyMobileError) { + return TEST_URL + "/failed?quic=1&error=" + envoyMobileError; } /**