Build the Cronet library, tests and unittests under Windows.

- Fix various call-sites that perform string<->FilePath conversions, to assume UTF8 encoded input,
  since Windows' native strings are UTF16.
- Fix the export header to export symbols under Cronet.
- Fix Cronet_Buffer_InitWithAlloc() to handle >32-bit sized allocations correctly under 32-bit.
- Fix the BufferTest.TestInitWithHugeAllocFails to request an allocation size that cannot be
  satisfied even under systems with over-commit.

Bug: 812268
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: If18a2c223e3ee18619543484a55d268ec12b5b78
Reviewed-on: https://chromium-review.googlesource.com/972674Reviewed-by: Misha Efimov <mef@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545993}

BUILD.gn

@@ -178,8 +178,7 @@ group("gn_all") {
     ]
   }
 
-  # TODO(https://crbug.com/812268): Fix Windows build, and enable it.
-  if (!is_ios && !is_android && !is_win) {
+  if (!is_ios && !is_android) {
     deps += [
       "//components/cronet:cronet_tests",
       "//components/cronet:cronet_unittests",

components/cronet/BUILD.gn

@@ -69,8 +69,7 @@ source_set("cronet_common_unittests") {
 # For platforms on which the native Cronet library is used, build the library,
 # a cronet_tests binary that exercises it, and a unit-tests binary.
 # Android and iOS have their own platform-specific rules to build Cronet.
-# TODO(https://crbug.com/812268): Fix Windows build, and enable it.
-if (!is_ios && !is_android && !is_win) {
+if (!is_ios && !is_android) {
   shared_library("cronet") {
     deps = [
       "//base",

components/cronet/cronet_prefs_manager.cc

@@ -30,9 +30,11 @@ namespace {
 // Name of the pref used for HTTP server properties persistence.
 const char kHttpServerPropertiesPref[] = "net.http_server_properties";
 // Name of preference directory.
-const char kPrefsDirectoryName[] = "prefs";
+const base::FilePath::CharType kPrefsDirectoryName[] =
+    FILE_PATH_LITERAL("prefs");
 // Name of preference file.
-const char kPrefsFileName[] = "local_prefs.json";
+const base::FilePath::CharType kPrefsFileName[] =
+    FILE_PATH_LITERAL("local_prefs.json");
 // Current version of disk storage.
 const int32_t kStorageVersion = 1;
 // Version number used when the version of disk storage is unknown.
@@ -60,7 +62,7 @@ bool IsCurrentVersion(const base::FilePath& version_filepath) {
 // TODO(xunjieli): Handle failures.
 void InitializeStorageDirectory(const base::FilePath& dir) {
   // Checks version file and clear old storage.
-  base::FilePath version_filepath = dir.Append("version");
+  base::FilePath version_filepath(dir.AppendASCII("version"));
   if (IsCurrentVersion(version_filepath)) {
     // The version is up to date, so there is nothing to do.
     return;
@@ -88,7 +90,7 @@ void InitializeStorageDirectory(const base::FilePath& dir) {
     DLOG(WARNING) << "Cannot write to version file.";
     return;
   }
-  base::FilePath prefs_dir = dir.Append(FILE_PATH_LITERAL(kPrefsDirectoryName));
+  base::FilePath prefs_dir = dir.Append(kPrefsDirectoryName);
   if (!base::CreateDirectory(prefs_dir)) {
     DLOG(WARNING) << "Cannot create prefs directory";
     return;
@@ -214,7 +216,12 @@ CronetPrefsManager::CronetPrefsManager(
   DCHECK(network_task_runner->BelongsToCurrentThread());
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
+#if defined(OS_WIN)
+  base::FilePath storage_file_path(
+      base::FilePath::FromUTF8Unsafe(storage_path));
+#else
   base::FilePath storage_file_path(storage_path);
+#endif
 
   // Make sure storage directory has correct version.
   {
@@ -223,8 +230,7 @@ CronetPrefsManager::CronetPrefsManager(
   }
 
   base::FilePath filepath =
-      storage_file_path.Append(FILE_PATH_LITERAL(kPrefsDirectoryName))
-          .Append(FILE_PATH_LITERAL(kPrefsFileName));
+      storage_file_path.Append(kPrefsDirectoryName).Append(kPrefsFileName);
 
   json_pref_store_ = new JsonPrefStore(filepath, file_task_runner,
                                        std::unique_ptr<PrefFilter>());

components/cronet/cronet_url_request_context.cc

@@ -350,7 +350,12 @@ void CronetURLRequestContext::NetworkTasks::Initialize(
 
   // Set up pref file if storage path is specified.
   if (!config->storage_path.empty()) {
+#if defined(OS_WIN)
+    base::FilePath storage_path(
+        base::FilePath::FromUTF8Unsafe(config->storage_path));
+#else
     base::FilePath storage_path(config->storage_path);
+#endif
     // Set up the HttpServerPropertiesManager.
     cronet_prefs_manager_ = std::make_unique<CronetPrefsManager>(
         config->storage_path, network_task_runner_, file_task_runner,
@@ -505,7 +510,11 @@ CronetURLRequestContext::GetNetworkTaskRunner() const {
 
 bool CronetURLRequestContext::StartNetLogToFile(const std::string& file_name,
                                                 bool log_all) {
+#if defined(OS_WIN)
+  base::FilePath file_path(base::FilePath::FromUTF8Unsafe(file_name));
+#else
   base::FilePath file_path(file_name);
+#endif
   base::ScopedFILE file(base::OpenFile(file_path, "w"));
   if (!file) {
     LOG(ERROR) << "Failed to open NetLog file for writing.";
@@ -640,8 +649,13 @@ void CronetURLRequestContext::NetworkTasks::StartNetLogToBoundedFile(
 
   // TODO(eroman): The cronet API passes a directory here. But it should now
   // just pass a file path.
-  base::FilePath file_path =
-      base::FilePath(dir_path).AppendASCII("netlog.json");
+#if defined(OS_WIN)
+  base::FilePath file_path(base::FilePath::FromUTF8Unsafe(dir_path));
+#else
+  base::FilePath file_path(dir_path);
+#endif
+  file_path = file_path.AppendASCII("netlog.json");
+
   {
     base::ScopedAllowBlocking allow_blocking;
     if (!base::PathIsWritable(file_path)) {

components/cronet/host_cache_persistence_manager_unittest.cc

@@ -45,7 +45,7 @@ class HostCachePersistenceManagerTest : public testing::Test {
   // not the full contents, since the tests in this file are only intended
   // to test that writes happen when they're supposed to, not serialization
   // correctness.
-  void CheckPref(uint size) {
+  void CheckPref(size_t size) {
     const base::Value* value = pref_service_->GetUserPref(kPrefName);
     base::ListValue list;
     if (value)

components/cronet/native/buffer.cc

@@ -7,6 +7,7 @@
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/numerics/safe_conversions.h"
 
 namespace {
 
@@ -62,6 +63,8 @@ void Cronet_BufferImpl::InitWithDataAndCallback(
 }
 
 void Cronet_BufferImpl::InitWithAlloc(uint64_t size) {
+  if (!base::IsValueInRangeForNumericType<size_t, uint64_t>(size))
+    return;
   data_ = malloc(size);
   if (!data_)
     return;

components/cronet/native/engine.cc

@@ -114,9 +114,15 @@ Cronet_RESULT Cronet_EngineImpl::StartWithParams(
     case Cronet_EngineParams_HTTP_CACHE_MODE_IN_MEMORY:
       context_config_builder.http_cache = URLRequestContextConfig::MEMORY;
       break;
-    case Cronet_EngineParams_HTTP_CACHE_MODE_DISK:
+    case Cronet_EngineParams_HTTP_CACHE_MODE_DISK: {
       context_config_builder.http_cache = URLRequestContextConfig::DISK;
-      if (!base::DirectoryExists(base::FilePath(params->storage_path))) {
+#if defined(OS_WIN)
+      const base::FilePath storage_path(
+          base::FilePath::FromUTF8Unsafe(params->storage_path));
+#else
+      const base::FilePath storage_path(params->storage_path);
+#endif
+      if (!base::DirectoryExists(storage_path)) {
         return CheckResult(
             Cronet_RESULT_ILLEGAL_ARGUMENT_STORAGE_PATH_MUST_EXIST);
       }
@@ -128,6 +134,7 @@ Cronet_RESULT Cronet_EngineImpl::StartWithParams(
       }
       in_use_storage_path_ = params->storage_path;
       break;
+    }
     default:
       context_config_builder.http_cache = URLRequestContextConfig::DISABLED;
   }

components/cronet/native/include/cronet_export.h

@@ -6,7 +6,7 @@
 #define COMPONENTS_CRONET_NATIVE_INCLUDE_CRONET_EXPORT_H_
 
 #if defined(WIN32)
-#define CRONET_EXPORT
+#define CRONET_EXPORT __declspec(dllexport)
 #else
 #define CRONET_EXPORT __attribute__((visibility("default")))
 #endif

components/cronet/native/test/buffer_test.cc

@@ -4,6 +4,8 @@
 
 #include "cronet_c.h"
 
+#include <limits>
+
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
@@ -64,30 +66,36 @@ TEST_F(BufferTest, TestInitWithAlloc) {
   // Create Cronet buffer and allocate buffer data.
   Cronet_BufferPtr buffer = Cronet_Buffer_Create();
   Cronet_Buffer_InitWithAlloc(buffer, kTestBufferSize);
-  ASSERT_TRUE(Cronet_Buffer_GetData(buffer));
-  ASSERT_EQ(Cronet_Buffer_GetSize(buffer), kTestBufferSize);
+  EXPECT_TRUE(Cronet_Buffer_GetData(buffer));
+  EXPECT_EQ(Cronet_Buffer_GetSize(buffer), kTestBufferSize);
   Cronet_Buffer_Destroy(buffer);
   ASSERT_FALSE(on_destroy_called());
 }
 
-#if defined(ADDRESS_SANITIZER)
+#if defined(ADDRESS_SANITIZER) || defined(OS_FUCHSIA)
 // ASAN malloc by default triggers crash instead of returning null on failure.
+// Fuchsia malloc() also crashes on allocation failure in some kernel builds.
 #define MAYBE_TestInitWithHugeAllocFails DISABLED_TestInitWithHugeAllocFails
 #else
 #define MAYBE_TestInitWithHugeAllocFails TestInitWithHugeAllocFails
 #endif
-// Example of allocating buffer with hugereasonable size.
+// Verify behaviour when an unsatisfiably huge buffer allocation is requested.
+// On 32-bit platforms, we want to ensure that a 64-bit range allocation size
+// is rejected, rather than resulting in a 32-bit truncated allocation.
+// Some platforms over-commit allocations, so we request an allocation of the
+// whole 64-bit address-space, which cannot possibly be satisfied in a 32- or
+// 64-bit process.
 TEST_F(BufferTest, MAYBE_TestInitWithHugeAllocFails) {
-  // Create Cronet buffer and allocate buffer data.
   Cronet_BufferPtr buffer = Cronet_Buffer_Create();
-  Cronet_Buffer_InitWithAlloc(buffer, 1000 * 1000 * 1000 * 1000);
-  ASSERT_FALSE(Cronet_Buffer_GetData(buffer));
-  ASSERT_EQ(Cronet_Buffer_GetSize(buffer), 0ull);
+  const uint64_t kHugeTestBufferSize = std::numeric_limits<uint64_t>::max();
+  Cronet_Buffer_InitWithAlloc(buffer, kHugeTestBufferSize);
+  EXPECT_FALSE(Cronet_Buffer_GetData(buffer));
+  EXPECT_EQ(Cronet_Buffer_GetSize(buffer), 0ull);
   Cronet_Buffer_Destroy(buffer);
   ASSERT_FALSE(on_destroy_called());
 }
 
-// Example of intializing buffer with app-allocated data.
+// Example of initializing buffer with app-allocated data.
 TEST_F(BufferTest, TestInitWithDataAndCallback) {
   Cronet_BufferCallbackPtr buffer_callback =
       Cronet_BufferCallback_CreateWith(BufferCallback_OnDestroy);
@@ -96,8 +104,8 @@ TEST_F(BufferTest, TestInitWithDataAndCallback) {
   Cronet_BufferPtr buffer = Cronet_Buffer_Create();
   Cronet_Buffer_InitWithDataAndCallback(buffer, malloc(kTestBufferSize),
                                         kTestBufferSize, buffer_callback);
-  ASSERT_TRUE(Cronet_Buffer_GetData(buffer));
-  ASSERT_EQ(Cronet_Buffer_GetSize(buffer), kTestBufferSize);
+  EXPECT_TRUE(Cronet_Buffer_GetData(buffer));
+  EXPECT_EQ(Cronet_Buffer_GetSize(buffer), kTestBufferSize);
   Cronet_Buffer_Destroy(buffer);
   ASSERT_TRUE(on_destroy_called());
 }

components/cronet/native/test/engine_test.cc

@@ -75,7 +75,7 @@ TEST_F(EngineTest, StartResults) {
   EXPECT_TRUE(temp_dir.CreateUniqueTempDir());
   base::FilePath temp_path = base::MakeAbsoluteFilePath(temp_dir.GetPath());
   Cronet_EngineParams_storage_path_set(engine_params,
-                                       temp_path.value().c_str());
+                                       temp_path.AsUTF8Unsafe().c_str());
   // Now the engine should start successfully.
   EXPECT_EQ(Cronet_RESULT_SUCCESS,
             Cronet_Engine_StartWithParams(engine, engine_params));
@@ -170,7 +170,7 @@ TEST_F(EngineTest, StartNetLogToFile) {
       "{ \"QUIC\" : {\"max_server_configs_stored_in_properties\" : 8} }");
   // Test that net log cannot start/stop before engine start.
   EXPECT_FALSE(Cronet_Engine_StartNetLogToFile(
-      engine, net_log_file.value().c_str(), true));
+      engine, net_log_file.AsUTF8Unsafe().c_str(), true));
   Cronet_Engine_StopNetLog(engine);
 
   // Start the engine.
@@ -179,15 +179,15 @@ TEST_F(EngineTest, StartNetLogToFile) {
 
   // Test that normal start/stop net log works.
   EXPECT_TRUE(Cronet_Engine_StartNetLogToFile(
-      engine, net_log_file.value().c_str(), true));
+      engine, net_log_file.AsUTF8Unsafe().c_str(), true));
   Cronet_Engine_StopNetLog(engine);
 
   // Test that double start/stop net log works.
   EXPECT_TRUE(Cronet_Engine_StartNetLogToFile(
-      engine, net_log_file.value().c_str(), true));
+      engine, net_log_file.AsUTF8Unsafe().c_str(), true));
   // Test that second start fails.
   EXPECT_FALSE(Cronet_Engine_StartNetLogToFile(
-      engine, net_log_file.value().c_str(), true));
+      engine, net_log_file.AsUTF8Unsafe().c_str(), true));
   // Test that multiple stops work.
   Cronet_Engine_StopNetLog(engine);
   Cronet_Engine_StopNetLog(engine);
@@ -207,7 +207,7 @@ TEST_F(EngineTest, StartNetLogToFile) {
   Cronet_Engine_Shutdown(engine);
   // Test that net log cannot start/stop after engine shutdown.
   EXPECT_FALSE(Cronet_Engine_StartNetLogToFile(
-      engine, net_log_file.value().c_str(), true));
+      engine, net_log_file.AsUTF8Unsafe().c_str(), true));
   Cronet_Engine_StopNetLog(engine);
   Cronet_Engine_Destroy(engine);
 }

components/cronet/stale_host_resolver.h

@@ -15,9 +15,9 @@ namespace cronet {
 // A HostResolver that wraps a HostResolverImpl and uses it to make requests,
 // but "impatiently" returns stale data (if available and usable) after a delay,
 // to reduce DNS latency at the expense of accuracy.
-class NET_EXPORT StaleHostResolver : public net::HostResolver {
+class StaleHostResolver : public net::HostResolver {
  public:
-  struct NET_EXPORT StaleOptions {
+  struct StaleOptions {
     StaleOptions();
 
     // How long to wait before returning stale data, if available.

components/cronet/url_request_context_config.cc

@@ -42,7 +42,8 @@ namespace cronet {
 namespace {
 
 // Name of disk cache directory.
-const char kDiskCacheDirectoryName[] = "disk_cache";
+const base::FilePath::CharType kDiskCacheDirectoryName[] =
+    FILE_PATH_LITERAL("disk_cache");
 // TODO(xunjieli): Refactor constants in io_thread.cc.
 const char kQuicFieldTrialName[] = "QUIC";
 const char kQuicConnectionOptions[] = "connection_options";
@@ -461,7 +462,8 @@ void URLRequestContextConfig::ParseAndSetExperimentalOptions(
     } else if (it.key() == kSSLKeyLogFile) {
       std::string ssl_key_log_file_string;
       if (it.value().GetAsString(&ssl_key_log_file_string)) {
-        base::FilePath ssl_key_log_file(ssl_key_log_file_string);
+        base::FilePath ssl_key_log_file(
+            base::FilePath::FromUTF8Unsafe(ssl_key_log_file_string));
         if (!ssl_key_log_file.empty()) {
           // SetSSLKeyLogFile is only safe to call before any SSLClientSockets
           // are created. This should not be used if there are multiple
@@ -551,9 +553,8 @@ void URLRequestContextConfig::ConfigureURLRequestContextBuilder(
     net::URLRequestContextBuilder::HttpCacheParams cache_params;
     if (http_cache == DISK && !storage_path.empty()) {
       cache_params.type = net::URLRequestContextBuilder::HttpCacheParams::DISK;
-      cache_params.path =
-          base::FilePath(storage_path)
-              .Append(FILE_PATH_LITERAL(kDiskCacheDirectoryName));
+      cache_params.path = base::FilePath::FromUTF8Unsafe(storage_path)
+                              .Append(kDiskCacheDirectoryName);
     } else {
       cache_params.type =
           net::URLRequestContextBuilder::HttpCacheParams::IN_MEMORY;