Normalize path values and add additional error logging (fixes issue #2405)

This change also improves the settings documentation and enforces the
requirement that path values be absolute (see issue #2916).

include/internal/cef_types.h

@@ -170,23 +170,25 @@ typedef struct _cef_settings_t {
   // will be used. If this value is empty on macOS then a helper executable must
   // exist at "Contents/Frameworks/<app> Helper.app/Contents/MacOS/<app> Helper"
   // in the top-level app bundle. See the comments on CefExecuteProcess() for
-  // details. Also configurable using the "browser-subprocess-path" command-line
+  // details. If this value is non-empty then it must be an absolute path. Also
-  // switch.
+  // configurable using the "browser-subprocess-path" command-line switch.
   ///
   cef_string_t browser_subprocess_path;
 
   ///
   // The path to the CEF framework directory on macOS. If this value is empty
   // then the framework must exist at "Contents/Frameworks/Chromium Embedded
-  // Framework.framework" in the top-level app bundle. Also configurable using
+  // Framework.framework" in the top-level app bundle. If this value is
-  // the "framework-dir-path" command-line switch.
+  // non-empty then it must be an absolute path. Also configurable using the
+  // "framework-dir-path" command-line switch.
   ///
   cef_string_t framework_dir_path;
 
   ///
   // The path to the main bundle on macOS. If this value is empty then it
-  // defaults to the top-level app bundle. Also configurable using
+  // defaults to the top-level app bundle. If this value is non-empty then it
-  // the "main-bundle-path" command-line switch.
+  // must be an absolute path. Also configurable using the "main-bundle-path"
+  // command-line switch.
   ///
   cef_string_t main_bundle_path;
 
@@ -227,33 +229,35 @@ typedef struct _cef_settings_t {
 
   ///
   // The location where data for the global browser cache will be stored on
-  // disk. If non-empty this must be either equal to or a child directory of
+  // disk. If this value is non-empty then it must be an absolute path that is
-  // CefSettings.root_cache_path. If empty then browsers will be created in
+  // either equal to or a child directory of CefSettings.root_cache_path. If
-  // "incognito mode" where in-memory caches are used for storage and no data is
+  // this value is empty then browsers will be created in "incognito mode" where
-  // persisted to disk. HTML5 databases such as localStorage will only persist
+  // in-memory caches are used for storage and no data is persisted to disk.
-  // across sessions if a cache path is specified. Can be overridden for
+  // HTML5 databases such as localStorage will only persist across sessions if a
-  // individual CefRequestContext instances via the
+  // cache path is specified. Can be overridden for individual CefRequestContext
-  // CefRequestContextSettings.cache_path value.
+  // instances via the CefRequestContextSettings.cache_path value.
   ///
   cef_string_t cache_path;
 
   ///
   // The root directory that all CefSettings.cache_path and
   // CefRequestContextSettings.cache_path values must have in common. If this
-  // value is empty and CefSettings.cache_path is non-empty then this value will
+  // value is empty and CefSettings.cache_path is non-empty then it will
-  // default to the CefSettings.cache_path value. Failure to set this value
+  // default to the CefSettings.cache_path value. If this value is non-empty
-  // correctly may result in the sandbox blocking read/write access to the
+  // then it must be an absolute path. Failure to set this value correctly may
-  // cache_path directory.
+  // result in the sandbox blocking read/write access to the cache_path
+  // directory.
   ///
   cef_string_t root_cache_path;
 
   ///
   // The location where user data such as spell checking dictionary files will
-  // be stored on disk. If empty then the default platform-specific user data
+  // be stored on disk. If this value is empty then the default
-  // directory will be used ("~/.cef_user_data" directory on Linux,
+  // platform-specific user data directory will be used ("~/.cef_user_data"
-  // "~/Library/Application Support/CEF/User Data" directory on Mac OS X,
+  // directory on Linux, "~/Library/Application Support/CEF/User Data" directory
-  // "Local Settings\Application Data\CEF\User Data" directory under the user
+  // on Mac OS X, "Local Settings\Application Data\CEF\User Data" directory
-  // profile directory on Windows).
+  // under the user profile directory on Windows). If this value is non-empty
+  // then it must be an absolute path.
   ///
   cef_string_t user_data_path;
 
@@ -333,16 +337,17 @@ typedef struct _cef_settings_t {
   // The fully qualified path for the resources directory. If this value is
   // empty the cef.pak and/or devtools_resources.pak files must be located in
   // the module directory on Windows/Linux or the app bundle Resources directory
-  // on Mac OS X. Also configurable using the "resources-dir-path" command-line
+  // on Mac OS X. If this value is non-empty then it must be an absolute path.
-  // switch.
+  // Also configurable using the "resources-dir-path" command-line switch.
   ///
   cef_string_t resources_dir_path;
 
   ///
   // The fully qualified path for the locales directory. If this value is empty
-  // the locales directory must be located in the module directory. This value
+  // the locales directory must be located in the module directory. If this
-  // is ignored on Mac OS X where pack files are always loaded from the app
+  // value is non-empty then it must be an absolute path. This value is ignored
-  // bundle Resources directory. Also configurable using the "locales-dir-path"
+  // on Mac OS X where pack files are always loaded from the app bundle
+  // Resources directory. Also configurable using the "locales-dir-path"
   // command-line switch.
   ///
   cef_string_t locales_dir_path;
@@ -428,13 +433,13 @@ typedef struct _cef_request_context_settings_t {
 
   ///
   // The location where cache data for this request context will be stored on
-  // disk. If non-empty this must be either equal to or a child directory of
+  // disk. If this value is non-empty then it must be an absolute path that is
-  // CefSettings.root_cache_path. If empty then browsers will be created in
+  // either equal to or a child directory of CefSettings.root_cache_path. If
-  // "incognito mode" where in-memory caches are used for storage and no data is
+  // this value is empty then browsers will be created in "incognito mode" where
-  // persisted to disk. HTML5 databases such as localStorage will only persist
+  // in-memory caches are used for storage and no data is persisted to disk.
-  // across sessions if a cache path is specified. To share the global browser
+  // HTML5 databases such as localStorage will only persist across sessions if a
-  // cache and related configuration set this value to match the
+  // cache path is specified. To share the global browser cache and related
-  // CefSettings.cache_path value.
+  // configuration set this value to match the CefSettings.cache_path value.
   ///
   cef_string_t cache_path;
 

libcef/browser/browser_context.cc

@@ -8,7 +8,6 @@
 #include <utility>
 
 #include "libcef/browser/content_browser_client.h"
-#include "libcef/browser/context.h"
 #include "libcef/browser/download_manager_delegate.h"
 #include "libcef/browser/extensions/extension_system.h"
 #include "libcef/browser/media_router/media_router_manager.h"
@@ -292,36 +291,23 @@ CefBrowserContext::~CefBrowserContext() {
 }
 
 void CefBrowserContext::Initialize() {
-  CefContext* context = CefContext::Get();
-
   cache_path_ = base::FilePath(CefString(&settings_.cache_path));
-  if (!context->ValidateCachePath(cache_path_)) {
-    // Reset to in-memory storage.
-    CefString(&settings_.cache_path).clear();
-    cache_path_ = base::FilePath();
-  }
 
   if (!cache_path_.empty())
     g_manager.Get().SetImplPath(this, cache_path_);
 
-  if (settings_.accept_language_list.length == 0) {
-    // Use the global language list setting.
-    CefString(&settings_.accept_language_list) =
-        CefString(&context->settings().accept_language_list);
-  }
-
   if (!!settings_.persist_session_cookies) {
     set_should_persist_session_cookies(true);
   }
 
-  key_ = std::make_unique<ProfileKey>(GetPath());
+  key_ = std::make_unique<ProfileKey>(cache_path_);
   SimpleKeyMap::GetInstance()->Associate(this, key_.get());
 
   // Initialize the PrefService object.
   pref_service_ = browser_prefs::CreatePrefService(
       this, cache_path_, !!settings_.persist_user_preferences);
 
-  content::BrowserContext::Initialize(this, GetPath());
+  content::BrowserContext::Initialize(this, cache_path_);
 
   resource_context_.reset(new CefResourceContext(IsOffTheRecord()));
 

libcef/browser/browser_context.h

@@ -296,7 +296,7 @@ class CefBrowserContext : public ChromeProfileStub,
   ~CefBrowserContext() override;
 
   // Members initialized during construction are safe to access from any thread.
-  CefRequestContextSettings settings_;
+  const CefRequestContextSettings settings_;
   base::FilePath cache_path_;
 
   // CefRequestContextImpl objects referencing this object.

libcef/browser/browser_main.cc

@@ -191,7 +191,7 @@ void CefBrowserMainParts::PreMainMessageLoopRun() {
        base::TaskShutdownBehavior::BLOCK_SHUTDOWN, base::MayBlock()});
 
   CefRequestContextSettings settings;
-  CefContext::Get()->PopulateRequestContextSettings(&settings);
+  CefContext::Get()->PopulateGlobalRequestContextSettings(&settings);
 
   // Create the global RequestContext.
   global_request_context_ =

libcef/browser/context.cc

@@ -160,6 +160,85 @@ bool GetColor(const cef_color_t cef_in, bool is_windowless, SkColor* sk_out) {
   return true;
 }
 
+// Convert |path_str| to a normalized FilePath.
+base::FilePath NormalizePath(const cef_string_t& path_str,
+                             const char* name,
+                             bool* has_error = nullptr) {
+  if (has_error)
+    *has_error = false;
+
+  base::FilePath path = base::FilePath(CefString(&path_str));
+  if (path.EndsWithSeparator()) {
+    // Remove the trailing separator because it will interfere with future
+    // equality checks.
+    path = path.StripTrailingSeparators();
+  }
+
+  if (!path.empty() && !path.IsAbsolute()) {
+    LOG(ERROR) << "The " << name << " directory (" << path.value()
+               << ") is not an absolute path. Defaulting to empty.";
+    if (has_error)
+      *has_error = true;
+    path = base::FilePath();
+  }
+
+  return path;
+}
+
+void SetPath(cef_string_t& path_str, const base::FilePath& path) {
+#if defined(OS_WIN)
+  CefString(&path_str).FromWString(path.value());
+#else
+  CefString(&path_str).FromString(path.value());
+#endif
+}
+
+// Convert |path_str| to a normalized FilePath and update the |path_str| value.
+base::FilePath NormalizePathAndSet(cef_string_t& path_str, const char* name) {
+  const base::FilePath& path = NormalizePath(path_str, name);
+  SetPath(path_str, path);
+  return path;
+}
+
+// Verify that |cache_path| is valid and create it if necessary.
+bool ValidateCachePath(const base::FilePath& cache_path,
+                       const base::FilePath& root_cache_path) {
+  if (cache_path.empty())
+    return true;
+
+  if (!root_cache_path.empty() && root_cache_path != cache_path &&
+      !root_cache_path.IsParent(cache_path)) {
+    LOG(ERROR) << "The cache_path directory (" << cache_path.value()
+               << ") is not a child of the root_cache_path directory ("
+               << root_cache_path.value() << ")";
+    return false;
+  }
+
+  base::ThreadRestrictions::ScopedAllowIO allow_io;
+  if (!base::DirectoryExists(cache_path) &&
+      !base::CreateDirectory(cache_path)) {
+    LOG(ERROR) << "The cache_path directory (" << cache_path.value()
+               << ") could not be created.";
+    return false;
+  }
+
+  return true;
+}
+
+// Like NormalizePathAndSet but with additional checks specific to the
+// cache_path value.
+base::FilePath NormalizeCachePathAndSet(cef_string_t& path_str,
+                                        const base::FilePath& root_cache_path) {
+  bool has_error = false;
+  base::FilePath path = NormalizePath(path_str, "cache_path", &has_error);
+  if (has_error || !ValidateCachePath(path, root_cache_path)) {
+    LOG(ERROR) << "The cache_path is invalid. Defaulting to in-memory storage.";
+    path = base::FilePath();
+  }
+  SetPath(path_str, path);
+  return path;
+}
+
 }  // namespace
 
 int CefExecuteProcess(const CefMainArgs& args,
@@ -370,18 +449,23 @@ bool CefContext::Initialize(const CefMainArgs& args,
   SignalChromeElf();
 #endif
 
-  base::FilePath cache_path = base::FilePath(CefString(&settings_.cache_path));
-  if (!ValidateCachePath(cache_path)) {
-    // Reset to in-memory storage.
-    CefString(&settings_.cache_path).clear();
-    cache_path = base::FilePath();
-  }
   const base::FilePath& root_cache_path =
-      base::FilePath(CefString(&settings_.root_cache_path));
+      NormalizePathAndSet(settings_.root_cache_path, "root_cache_path");
+  const base::FilePath& cache_path =
+      NormalizeCachePathAndSet(settings_.cache_path, root_cache_path);
   if (root_cache_path.empty() && !cache_path.empty()) {
-    CefString(&settings_.root_cache_path) = CefString(&settings_.cache_path);
+    CefString(&settings_.root_cache_path) = cache_path.value();
   }
 
+  // All other paths that need to be normalized.
+  NormalizePathAndSet(settings_.browser_subprocess_path,
+                      "browser_subprocess_path");
+  NormalizePathAndSet(settings_.framework_dir_path, "framework_dir_path");
+  NormalizePathAndSet(settings_.main_bundle_path, "main_bundle_path");
+  NormalizePathAndSet(settings_.user_data_path, "user_data_path");
+  NormalizePathAndSet(settings_.resources_dir_path, "resources_dir_path");
+  NormalizePathAndSet(settings_.locales_dir_path, "locales_dir_path");
+
   main_delegate_.reset(new CefMainDelegate(application));
   browser_info_manager_.reset(new CefBrowserInfoManager);
 
@@ -518,11 +602,14 @@ CefTraceSubscriber* CefContext::GetTraceSubscriber() {
   return trace_subscriber_.get();
 }
 
-void CefContext::PopulateRequestContextSettings(
+void CefContext::PopulateGlobalRequestContextSettings(
     CefRequestContextSettings* settings) {
   CefRefPtr<CefCommandLine> command_line =
       CefCommandLine::GetGlobalCommandLine();
+
+  // This value was already normalized in Initialize.
   CefString(&settings->cache_path) = CefString(&settings_.cache_path);
+
   settings->persist_session_cookies =
       settings_.persist_session_cookies ||
       command_line->HasSwitch(switches::kPersistSessionCookies);
@@ -536,29 +623,17 @@ void CefContext::PopulateRequestContextSettings(
       CefString(&settings_.accept_language_list);
 }
 
-bool CefContext::ValidateCachePath(const base::FilePath& cache_path) {
+void CefContext::NormalizeRequestContextSettings(
-  if (cache_path.empty())
+    CefRequestContextSettings* settings) {
-    return true;
+  // The |root_cache_path| value was already normalized in Initialize.
+  const base::FilePath& root_cache_path = CefString(&settings_.root_cache_path);
+  NormalizeCachePathAndSet(settings->cache_path, root_cache_path);
 
-  const base::FilePath& root_cache_path =
+  if (settings->accept_language_list.length == 0) {
-      base::FilePath(CefString(&settings_.root_cache_path));
+    // Use the global language list setting.
-  if (!root_cache_path.empty() && root_cache_path != cache_path &&
+    CefString(&settings->accept_language_list) =
-      !root_cache_path.IsParent(cache_path)) {
+        CefString(&settings_.accept_language_list);
-    LOG(ERROR) << "The cache_path directory (" << cache_path.value()
-               << ") is not a child of the root_cache_path directory ("
-               << root_cache_path.value() << ")";
-    return false;
   }
-
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
-  if (!base::DirectoryExists(cache_path) &&
-      !base::CreateDirectory(cache_path)) {
-    LOG(ERROR) << "The cache_path directory (" << cache_path.value()
-               << ") could not be created.";
-    return false;
-  }
-
-  return true;
 }
 
 void CefContext::AddObserver(Observer* observer) {

libcef/browser/context.h

@@ -87,10 +87,11 @@ class CefContext {
 
   // Populate request context settings for the global system context based on
   // CefSettings and command-line flags.
-  void PopulateRequestContextSettings(CefRequestContextSettings* settings);
+  void PopulateGlobalRequestContextSettings(
+      CefRequestContextSettings* settings);
 
-  // Verify that |cache_path| is valid and create it if necessary.
+  // Normalize and validate request context settings for user-created contexts.
-  bool ValidateCachePath(const base::FilePath& cache_path);
+  void NormalizeRequestContextSettings(CefRequestContextSettings* settings);
 
   // Manage observer objects. The observer must either outlive this object or
   // remove itself before destruction. These methods can only be called on the

libcef/browser/request_context_impl.cc

@@ -645,9 +645,15 @@ void CefRequestContextImpl::Initialize() {
     // Share storage with |config_.other|.
     browser_context_ =
         CefBrowserContext::GetForContext(config_.other->GetBrowserContext());
+    DCHECK(browser_context_);
   }
 
   if (!browser_context_) {
+    if (!config_.is_global) {
+      // User-specified settings need to be normalized.
+      CefContext::Get()->NormalizeRequestContextSettings(&config_.settings);
+    }
+
     const base::FilePath& cache_path =
         base::FilePath(CefString(&config_.settings.cache_path));
     if (!cache_path.empty()) {
@@ -661,17 +667,17 @@ void CefRequestContextImpl::Initialize() {
     // Create a new CefBrowserContext instance. If the cache path is non-
     // empty then this new instance will become the globally registered
     // CefBrowserContext for that path. Otherwise, this new instance will
-    // be a completely isolated "incongento mode" context.
+    // be a completely isolated "incognito mode" context.
     browser_context_ = new CefBrowserContext(config_.settings);
     browser_context_->Initialize();
+  } else {
+    // Share the same settings as the existing context.
+    config_.settings = browser_context_->GetSettings();
   }
 
   // We'll disassociate from |browser_context_| on destruction.
   browser_context_->AddCefRequestContext(this);
 
-  // Force our settings to match |browser_context_|.
-  config_.settings = browser_context_->GetSettings();
-
   if (config_.other) {
     // Clear the reference to |config_.other| after setting
     // |request_context_getter_|. This is the reverse order of checks in