[Cronet] Properly report SSLCertificateError.

- Use net::EmbeddedTestServer::CERT_EXPIRED SSL Config to trigger errors.

Bug: 910760
Change-Id: I7cae58099003634eb17a6b739b5fd3631ba5bc35
Reviewed-on: https://chromium-review.googlesource.com/c/1359088
Commit-Queue: Misha Efimov <mef@chromium.org>
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#613686}(cherry picked from commit 6af3825)
Reviewed-on: https://chromium-review.googlesource.com/c/1364232
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#93}
Cr-Branched-From: d897fb1-refs/heads/master@{#612437}

components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java

@@ -37,7 +37,9 @@
 import org.chromium.net.TestUrlRequestCallback.ResponseStep;
 import org.chromium.net.impl.CronetUrlRequest;
 import org.chromium.net.impl.UrlResponseInfoImpl;
+import org.chromium.net.test.EmbeddedTestServer;
 import org.chromium.net.test.FailurePhase;
+import org.chromium.net.test.ServerCertificate;
 
 import java.io.IOException;
 import java.net.ConnectException;
@@ -755,6 +757,42 @@ public void testMockSSLCertificateError() throws Exception {
         assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
     }
 
+    /**
+     * Tests that an SSL cert error with upload will be reported via {@link
+     * UrlRequest.Callback#onFailed}.
+     */
+    @Test
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet // Java impl doesn't support MockUrlRequestJobFactory
+    public void testSSLCertificateError() throws Exception {
+        EmbeddedTestServer sslServer = EmbeddedTestServer.createAndStartHTTPSServer(
+                getContext(), ServerCertificate.CERT_EXPIRED);
+
+        TestUrlRequestCallback callback = new TestUrlRequestCallback();
+        UrlRequest.Builder builder = mTestFramework.mCronetEngine.newUrlRequestBuilder(
+                sslServer.getURL("/"), callback, callback.getExecutor());
+
+        TestUploadDataProvider dataProvider = new TestUploadDataProvider(
+                TestUploadDataProvider.SuccessCallbackMode.SYNC, callback.getExecutor());
+        dataProvider.addRead("test".getBytes());
+        builder.setUploadDataProvider(dataProvider, callback.getExecutor());
+        builder.addHeader("Content-Type", "useless/string");
+        builder.build().start();
+        callback.blockForDone();
+        dataProvider.assertClosed();
+
+        assertNull(callback.mResponseInfo);
+        assertNotNull(callback.mError);
+        assertTrue(callback.mOnErrorCalled);
+        assertEquals(-201, ((NetworkException) callback.mError).getCronetInternalErrorCode());
+        assertContains("Exception in CronetUrlRequest: net::ERR_CERT_DATE_INVALID",
+                callback.mError.getMessage());
+        assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
+
+        sslServer.stopAndDestroyServer();
+    }
+
     /**
      * Checks that the buffer is updated correctly, when starting at an offset.
      */

components/cronet/cronet_url_request.cc

@@ -170,6 +170,7 @@ CronetURLRequest::NetworkTasks::NetworkTasks(std::unique_ptr<Callback> callback,
       initial_priority_(priority),
       initial_load_flags_(load_flags),
       received_byte_count_from_redirects_(0l),
+      error_reported_(false),
       enable_metrics_(enable_metrics),
       metrics_reported_(false),
       traffic_stats_tag_set_(traffic_stats_tag_set),
@@ -211,9 +212,9 @@ void CronetURLRequest::NetworkTasks::OnSSLCertificateError(
     const net::SSLInfo& ssl_info,
     bool fatal) {
   DCHECK_CALLED_ON_VALID_THREAD(network_thread_checker_);
-  request->Cancel();
   int net_error = net::MapCertStatusToNetError(ssl_info.cert_status);
   ReportError(request, net_error);
+  request->Cancel();
 }
 
 void CronetURLRequest::NetworkTasks::OnResponseStarted(net::URLRequest* request,
@@ -243,6 +244,7 @@ void CronetURLRequest::NetworkTasks::OnReadCompleted(net::URLRequest* request,
   }
 
   if (bytes_read == 0) {
+    DCHECK(!error_reported_);
     MaybeReportMetrics();
     callback_->OnSucceeded(received_byte_count_from_redirects_ +
                            request->GetTotalReceivedBytes());
@@ -338,6 +340,10 @@ void CronetURLRequest::NetworkTasks::ReportError(net::URLRequest* request,
   DCHECK_NE(net::ERR_IO_PENDING, net_error);
   DCHECK_LT(net_error, 0);
   DCHECK_EQ(request, url_request_.get());
+  // Error may have already been reported.
+  if (error_reported_)
+    return;
+  error_reported_ = true;
   net::NetErrorDetails net_error_details;
   url_request_->PopulateNetErrorDetails(&net_error_details);
   VLOG(1) << "Error " << net::ErrorToString(net_error)

components/cronet/cronet_url_request.h

@@ -256,6 +256,10 @@ class CronetURLRequest {
     // Count of bytes received during redirect is added to received byte count.
     int64_t received_byte_count_from_redirects_;
 
+    // Whether error has been already reported, for example from
+    // OnSSLCertificateError().
+    bool error_reported_;
+
     // Whether detailed metrics should be collected and reported.
     const bool enable_metrics_;
     // Whether metrics have been reported.

components/cronet/native/test/test_util.cc

@@ -23,6 +23,29 @@ void TestExecutor_Execute(Cronet_ExecutorPtr self,
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, cronet::test::RunnableWrapper::CreateOnceClosure(runnable));
 }
+
+// Test Cert Verifier that successfully verifies any cert from test.example.com.
+class TestCertVerifier : public net::MockCertVerifier {
+ public:
+  TestCertVerifier() = default;
+  ~TestCertVerifier() override = default;
+
+  // CertVerifier implementation
+  int Verify(const RequestParams& params,
+             net::CertVerifyResult* verify_result,
+             net::CompletionOnceCallback callback,
+             std::unique_ptr<Request>* out_req,
+             const net::NetLogWithSource& net_log) override {
+    if (params.hostname() == "test.example.com") {
+      verify_result->verified_cert = params.certificate();
+      verify_result->cert_status = MapNetErrorToCertStatus(net::OK);
+      return net::OK;
+    }
+    return net::MockCertVerifier::Verify(params, verify_result,
+                                         std::move(callback), out_req, net_log);
+  };
+};
+
 }  // namespace
 
 namespace cronet {
@@ -54,8 +77,7 @@ Cronet_EnginePtr CreateTestEngine(int quic_server_port) {
   // Create Cronet Engine.
   Cronet_EnginePtr cronet_engine = Cronet_Engine_Create();
   // Set Mock Cert Verifier.
-  auto cert_verifier = std::make_unique<net::MockCertVerifier>();
-  cert_verifier->set_default_result(net::OK);
+  auto cert_verifier = std::make_unique<TestCertVerifier>();
   Cronet_Engine_SetMockCertVerifierForTesting(cronet_engine,
                                               cert_verifier.release());
   // Start Cronet Engine.

components/cronet/native/test/url_request_test.cc

@@ -17,7 +17,7 @@
 #include "components/cronet/native/test/test_util.h"
 #include "components/cronet/test/test_server.h"
 #include "cronet_c.h"
-#include "net/cert/mock_cert_verifier.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -386,6 +386,27 @@ TEST_P(UrlRequestTest, UploadSync) {
   EXPECT_EQ("Test", callback->response_as_string_);
 }
 
+TEST_P(UrlRequestTest, SSLCertificateError) {
+  net::EmbeddedTestServer ssl_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  ssl_server.SetSSLConfig(net::EmbeddedTestServer::CERT_EXPIRED);
+  ASSERT_TRUE(ssl_server.Start());
+
+  const std::string url = ssl_server.GetURL("/").spec();
+  TestUploadDataProvider data_provider(TestUploadDataProvider::SYNC,
+                                       /* executor = */ nullptr);
+  data_provider.AddRead("Test");
+  auto callback = std::make_unique<TestUrlRequestCallback>(GetParam());
+  callback = StartAndWaitForComplete(url, std::move(callback), std::string(),
+                                     &data_provider);
+  data_provider.AssertClosed();
+  EXPECT_EQ(4, data_provider.GetUploadedLength());
+  EXPECT_EQ(0, data_provider.num_read_calls());
+  EXPECT_EQ(0, data_provider.num_rewind_calls());
+  EXPECT_EQ(nullptr, callback->response_info_);
+  EXPECT_EQ("", callback->response_as_string_);
+  EXPECT_EQ("net::ERR_CERT_INVALID", callback->last_error_message_);
+}
+
 TEST_P(UrlRequestTest, UploadMultiplePiecesSync) {
   const std::string url = cronet::TestServer::GetEchoRequestBodyURL();
   auto callback = std::make_unique<TestUrlRequestCallback>(GetParam());