[fuchsia] Many improvements and fixes related to data dir handling.

* Install the Context's persistent data directory in /data.
* Use the existence of /data as the canonical signal for enabling
  incognito mode. (Tests will still use the --incognito switch
  override.)
  ** Add a smoke test suite for running in incognito mode.
* Configure disk_cache to use SimpleCacheBackend. The default persistent
  cache backend will break because it relies on mmap(), which isn't
  supported by minfs.
* Add semantic validation for the |data_directory| handle.
  ** Add tests to exercise validation.
* Remove GetPackageRoot(), since there's no reason to not use
  PathService(DIR_SOURCE_ROOT, ...) anymore.
* Remove unused Fuchsia-specific path DIR_FUCHSIA_RESOURCES.

Bug: 840598
Change-Id: I3efdb072cc4ee8f7df21992d6a84a1b548bb7fed
Reviewed-on: https://chromium-review.googlesource.com/1232524Reviewed-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592513}

base/base_paths_fuchsia.cc

@@ -14,10 +14,6 @@
 
 namespace base {
 
-base::FilePath GetPackageRoot() {
-  return base::FilePath("/pkg");
-}
-
 bool PathProviderFuchsia(int key, FilePath* result) {
   switch (key) {
     case FILE_MODULE:
@@ -27,18 +23,12 @@ bool PathProviderFuchsia(int key, FilePath* result) {
       *result = CommandLine::ForCurrentProcess()->GetProgram();
       return true;
     case DIR_APP_DATA:
-      // TODO(https://crbug.com/840598): Switch to /data when minfs supports
-      // mmap().
-      DLOG(WARNING) << "Using /tmp as app data dir, changes will NOT be "
-                       "persisted! (crbug.com/840598)";
-      *result = FilePath("/tmp");
-      return true;
     case DIR_CACHE:
-      *result = FilePath("/data");
+      *result = base::FilePath("/data");
       return true;
     case DIR_ASSETS:
     case DIR_SOURCE_ROOT:
-      *result = GetPackageRoot();
+      *result = base::FilePath("/pkg");
       return true;
   }
   return false;

base/base_paths_fuchsia.h

@@ -15,20 +15,12 @@ namespace base {
 enum {
   PATH_FUCHSIA_START = 1200,
 
-  // Path to the directory which contains application libraries and resources.
-  DIR_FUCHSIA_RESOURCES,
-
   // Path to the directory which contains application user data.
   DIR_APP_DATA,
 
   PATH_FUCHSIA_END,
 };
 
-// If running inside a package, returns a FilePath of the root path
-// of the currently deployed package.
-// Otherwise returns an empty FilePath.
-BASE_EXPORT base::FilePath GetPackageRoot();
-
 }  // namespace base
 
 #endif  // BASE_BASE_PATHS_FUCHSIA_H_

base/fuchsia/file_utils.cc

@@ -4,6 +4,8 @@
 
 #include "base/fuchsia/file_utils.h"
 
+#include <fcntl.h>
+
 #include <lib/fdio/limits.h>
 #include <lib/fdio/util.h>
 #include <zircon/processargs.h>
@@ -42,5 +44,32 @@ zx::handle GetHandleFromFile(File file) {
   return std::move(owned_handles[0]);
 }
 
+base::File GetFileFromHandle(zx::handle handle) {
+  base::ScopedFD fd;
+  zx_handle_t handles[1] = {handle.release()};
+  zx_handle_t types[1] = {PA_FDIO_REMOTE};
+  zx_status_t status = fdio_create_fd(handles, types, 1, fd.receive());
+  if (status != ZX_OK) {
+    ZX_LOG(WARNING, status) << "fdio_create_fd";
+    return base::File();
+  }
+
+  // Verify that the FD is file-like by querying it with a file-specific fcntl.
+  int flags = fcntl(fd.get(), F_GETFL);
+  if (flags == -1) {
+    LOG(WARNING) << "Handle is not a valid file descriptor.";
+
+    // Release the FD using FDIO directly instead of the ScopedFD
+    // destructor. ScopedFD calls close() which isn't supported by this FD.
+    fdio_t* fdio_to_drop;
+    status = fdio_unbind_from_fd(fd.release(), &fdio_to_drop);
+    ZX_CHECK(status == ZX_OK, status) << "fdio_unbind_from_fd";
+
+    return base::File();
+  }
+
+  return base::File(fd.release());
+}
+
 }  // namespace fuchsia
 }  // namespace base

base/fuchsia/file_utils.h

@@ -19,6 +19,11 @@ namespace fuchsia {
 // namespace.
 BASE_EXPORT zx::handle GetHandleFromFile(base::File file);
 
+// Makes a File object from a Zircon handle.
+// Returns an empty File if |handle| is invalid or not a valid PA_FDIO_REMOTE
+// descriptor.
+BASE_EXPORT base::File GetFileFromHandle(zx::handle handle);
+
 }  // namespace fuchsia
 }  // namespace base
 

base/native_library_fuchsia.cc

@@ -40,7 +40,8 @@ NativeLibrary LoadNativeLibraryWithOptions(const FilePath& library_path,
     return nullptr;
   }
 
-  FilePath computed_path = base::GetPackageRoot();
+  FilePath computed_path;
+  base::PathService::Get(DIR_SOURCE_ROOT, &computed_path);
   computed_path = computed_path.AppendASCII("lib").Append(components[0]);
   base::File library(computed_path,
                      base::File::FLAG_OPEN | base::File::FLAG_READ);

content/common/sandbox_policy_fuchsia.cc

@@ -19,6 +19,7 @@
 #include "base/files/file_util.h"
 #include "base/fuchsia/component_context.h"
 #include "base/fuchsia/filtered_service_directory.h"
+#include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/process/process.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -103,7 +104,9 @@ void SandboxPolicyFuchsia::UpdateLaunchOptionsForSandbox(
 
   // Map /pkg (read-only files deployed from the package) into the child's
   // namespace.
-  options->paths_to_clone.push_back(base::GetPackageRoot());
+  base::FilePath package_root;
+  base::PathService::Get(base::DIR_SOURCE_ROOT, &package_root);
+  options->paths_to_clone.push_back(package_root);
 
   // Clear environmental variables to better isolate the child from
   // this process.

net/disk_cache/disk_cache.cc

@@ -9,6 +9,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task/task_scheduler/task_scheduler.h"
+#include "build/build_config.h"
 #include "net/base/cache_type.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/backend_cleanup_tracker.h"
@@ -96,7 +97,7 @@ CacheCreator::CacheCreator(const base::FilePath& path,
 CacheCreator::~CacheCreator() = default;
 
 int CacheCreator::Run() {
-#if defined(OS_ANDROID)
+#if defined(OS_ANDROID) || defined(OS_FUCHSIA)
   static const bool kSimpleBackendIsDefault = true;
 #else
   static const bool kSimpleBackendIsDefault = false;

webrunner/browser/context_impl_browsertest.cc

@@ -5,6 +5,7 @@
 #include <lib/fidl/cpp/binding.h>
 
 #include "base/macros.h"
+#include "base/path_service.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "net/test/embedded_test_server/default_handlers.h"
@@ -15,6 +16,7 @@
 #include "url/url_constants.h"
 #include "webrunner/browser/frame_impl.h"
 #include "webrunner/browser/webrunner_browser_test.h"
+#include "webrunner/service/common.h"
 
 namespace webrunner {
 
@@ -573,4 +575,31 @@ IN_PROC_BROWSER_TEST_F(ContextImplTest, Stop) {
       context_impl()->GetFrameImplForTest(&frame)->web_contents_->IsLoading());
 }
 
+class IncognitoContextImplTest : public ContextImplTest {
+ public:
+  IncognitoContextImplTest() = default;
+  ~IncognitoContextImplTest() override = default;
+
+  void SetUp() override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(kIncognitoSwitch);
+    ContextImplTest::SetUp();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(IncognitoContextImplTest);
+};
+
+// Verify that the browser can be initialized without a persistent data
+// directory.
+IN_PROC_BROWSER_TEST_F(IncognitoContextImplTest, NavigateFrame) {
+  chromium::web::FramePtr frame = CreateFrame();
+
+  chromium::web::NavigationControllerPtr controller;
+  frame->GetNavigationController(controller.NewRequest());
+
+  CheckLoadUrl(url::kAboutBlankURL, url::kAboutBlankURL, controller.get());
+
+  frame.Unbind();
+}
+
 }  // namespace webrunner

webrunner/browser/webrunner_browser_context.cc

@@ -7,8 +7,11 @@
 #include <memory>
 #include <utility>
 
+#include "base/base_paths_fuchsia.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/resource_context.h"
 #include "net/url_request/url_request_context.h"
@@ -56,11 +59,17 @@ std::unique_ptr<WebRunnerNetLog> CreateNetLog() {
   return result;
 }
 
-WebRunnerBrowserContext::WebRunnerBrowserContext(base::FilePath data_dir_path)
-    : data_dir_path_(std::move(data_dir_path)),
-      net_log_(CreateNetLog()),
-      resource_context_(new ResourceContext()) {
-  BrowserContext::Initialize(this, GetPath());
+WebRunnerBrowserContext::WebRunnerBrowserContext(bool force_incognito)
+    : net_log_(CreateNetLog()), resource_context_(new ResourceContext()) {
+  if (!force_incognito) {
+    base::PathService::Get(base::DIR_APP_DATA, &data_dir_path_);
+    if (!base::PathExists(data_dir_path_)) {
+      // Run in incognito mode if /data doesn't exist.
+      data_dir_path_.clear();
+    }
+  }
+
+  BrowserContext::Initialize(this, data_dir_path_);
 }
 
 WebRunnerBrowserContext::~WebRunnerBrowserContext() {

webrunner/browser/webrunner_browser_context.h

@@ -5,6 +5,8 @@
 #ifndef WEBRUNNER_BROWSER_WEBRUNNER_BROWSER_CONTEXT_H_
 #define WEBRUNNER_BROWSER_WEBRUNNER_BROWSER_CONTEXT_H_
 
+#include <memory>
+
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "content/public/browser/browser_context.h"
@@ -16,7 +18,9 @@ class WebRunnerURLRequestContextGetter;
 
 class WebRunnerBrowserContext : public content::BrowserContext {
  public:
-  explicit WebRunnerBrowserContext(base::FilePath data_dir_path);
+  // |force_incognito|: If set, then this BrowserContext will run in incognito
+  // mode even if /data is available.
+  explicit WebRunnerBrowserContext(bool force_incognito);
   ~WebRunnerBrowserContext() override;
 
   // BrowserContext implementation.
@@ -54,7 +58,7 @@ class WebRunnerBrowserContext : public content::BrowserContext {
   // Contains URLRequestContextGetter required for resource loading.
   class ResourceContext;
 
-  const base::FilePath data_dir_path_;
+  base::FilePath data_dir_path_;
 
   std::unique_ptr<WebRunnerNetLog> net_log_;
   scoped_refptr<WebRunnerURLRequestContextGetter> url_request_getter_;

webrunner/browser/webrunner_browser_main_parts.cc

@@ -40,8 +40,8 @@ void WebRunnerBrowserMainParts::PreMainMessageLoopRun() {
   display::Screen::SetScreenInstance(screen_.get());
 
   DCHECK(!browser_context_);
-  browser_context_ =
-      std::make_unique<WebRunnerBrowserContext>(GetWebContextDataDir());
+  browser_context_ = std::make_unique<WebRunnerBrowserContext>(
+      base::CommandLine::ForCurrentProcess()->HasSwitch(kIncognitoSwitch));
 
   context_service_ = std::make_unique<ContextImpl>(browser_context_.get());
 

webrunner/browser/webrunner_test_launcher.cc

@@ -61,8 +61,6 @@ class WebRunnerTestLauncherDelegate : public content::TestLauncherDelegate {
 
 int main(int argc, char** argv) {
   base::CommandLine::Init(argc, argv);
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(
-      webrunner::kIncognitoSwitch);
   base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
       switches::kOzonePlatform, "headless");
   base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kDisableGpu);

webrunner/service/common.cc

@@ -4,26 +4,8 @@
 
 #include "webrunner/service/common.h"
 
-#include "base/command_line.h"
-#include "base/files/file_path.h"
-#include "base/files/file_util.h"
-#include "base/logging.h"
-
 namespace webrunner {
 
 constexpr char kIncognitoSwitch[] = "incognito";
 
-constexpr char kWebContextDataPath[] = "/web_context_data";
-
-base::FilePath GetWebContextDataDir() {
-  base::FilePath data_dir{kWebContextDataPath};
-  bool is_incognito =
-      base::CommandLine::ForCurrentProcess()->HasSwitch(kIncognitoSwitch);
-  CHECK_EQ(is_incognito, !base::DirectoryExists(data_dir));
-
-  if (is_incognito)
-    return base::FilePath();
-  return data_dir;
-}
-
 }  // namespace webrunner

webrunner/service/common.h

@@ -9,11 +9,12 @@
 
 #include "webrunner/common/webrunner_export.h"
 
-namespace base {
-class FilePath;
-}
-
 namespace webrunner {
+
+// Switch passed to content process when running in incognito mode, i.e. when
+// there is no kWebContextDataPath.
+WEBRUNNER_EXPORT extern const char kIncognitoSwitch[];
+
 // This file contains constants and functions shared between Context and
 // ContextProvider processes.
 
@@ -21,18 +22,6 @@ namespace webrunner {
 // Context process.
 constexpr uint32_t kContextRequestHandleId = PA_HND(PA_USER0, 0);
 
-// Path to the direct used to store persistent data in context process.
-extern const char kWebContextDataPath[];
-
-// Switch passed to content process when running in incognito mode, i.e. when
-// there is no kWebContextDataPath.
-WEBRUNNER_EXPORT extern const char kIncognitoSwitch[];
-
-// Returns data directory that should be used by this context process. Should
-// not be called in ContextProvider. Empty path is returned if the context
-// doesn't have storage dir.
-WEBRUNNER_EXPORT base::FilePath GetWebContextDataDir();
-
 }  // namespace webrunner
 
 #endif  // WEBRUNNER_SERVICE_COMMON_H_

webrunner/service/context_provider_impl.cc

@@ -6,19 +6,24 @@
 
 #include <fuchsia/sys/cpp/fidl.h>
 #include <lib/async/default.h>
+#include <lib/fdio/io.h>
+#include <lib/fdio/util.h>
 #include <lib/zx/job.h>
 #include <stdio.h>
 #include <zircon/processargs.h>
 
 #include <utility>
 
+#include "base/base_paths_fuchsia.h"
 #include "base/bind.h"
 #include "base/callback_forward.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/fuchsia/default_job.h"
+#include "base/fuchsia/file_utils.h"
 #include "base/fuchsia/fuchsia_logging.h"
 #include "base/logging.h"
+#include "base/path_service.h"
 #include "base/process/launch.h"
 #include "webrunner/service/common.h"
 
@@ -31,6 +36,28 @@ base::Process LaunchContextProcess(const base::CommandLine& launch_command,
   return base::LaunchProcess(launch_command, launch_options);
 }
 
+// Returns true if |handle| is connected to a directory.
+bool IsValidDirectory(zx_handle_t handle) {
+  base::File directory =
+      base::fuchsia::GetFileFromHandle(zx::handle(fdio_service_clone(handle)));
+  if (!directory.IsValid())
+    return false;
+
+  base::File::Info info;
+  if (!directory.GetInfo(&info)) {
+    LOG(ERROR) << "Could not query FileInfo for handle.";
+    directory.Close();
+    return false;
+  }
+
+  if (!info.is_directory) {
+    LOG(ERROR) << "Handle is not a directory.";
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace
 
 ContextProviderImpl::ContextProviderImpl() : ContextProviderImpl(false) {}
@@ -94,13 +121,15 @@ void ContextProviderImpl::Create(
   launch_options.paths_to_transfer.push_back(base::PathToTransfer{
       base::FilePath("/svc"), std::move(params.service_directory.release())});
 
-  // Pass the data directory. If there is no data dir then --incognito flag is
-  // added instead.
+  // Bind |data_directory| to /data directory, if provided.
   if (params.data_directory) {
-    launch_options.paths_to_transfer.push_back(base::PathToTransfer{
-        base::FilePath(kWebContextDataPath), params.data_directory.release()});
-  } else {
-    launch_command.AppendSwitch(kIncognitoSwitch);
+    if (!IsValidDirectory(params.data_directory.get()))
+      return;
+
+    base::FilePath data_path;
+    CHECK(base::PathService::Get(base::DIR_APP_DATA, &data_path));
+    launch_options.paths_to_transfer.push_back(
+        base::PathToTransfer{data_path, params.data_directory.release()});
   }
 
   // Isolate the child Context processes by containing them within their own

webrunner/service/context_provider_impl_unittest.cc

@@ -12,6 +12,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/base_paths_fuchsia.h"
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -24,6 +25,7 @@
 #include "base/fuchsia/fuchsia_logging.h"
 #include "base/fuchsia/service_directory.h"
 #include "base/message_loop/message_loop.h"
+#include "base/path_service.h"
 #include "base/test/multiprocess_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/multiprocess_func_list.h"
@@ -107,12 +109,13 @@ class FakeContext : public chromium::web::Context {
 MULTIPROCESS_TEST_MAIN(SpawnContextServer) {
   base::MessageLoopForIO message_loop;
 
-  base::FilePath data_dir =  GetWebContextDataDir();
+  base::FilePath data_dir;
+  CHECK(base::PathService::Get(base::DIR_APP_DATA, &data_dir));
   if (!data_dir.empty()) {
-    EXPECT_TRUE(base::PathExists(data_dir.AppendASCII(kTestDataFileIn)));
-
-    auto out_file = data_dir.AppendASCII(kTestDataFileOut);
-    EXPECT_EQ(base::WriteFile(out_file, nullptr, 0), 0);
+    if (base::PathExists(data_dir.AppendASCII(kTestDataFileIn))) {
+      auto out_file = data_dir.AppendASCII(kTestDataFileOut);
+      EXPECT_EQ(base::WriteFile(out_file, nullptr, 0), 0);
+    }
   }
 
   zx::channel context_handle(zx_take_startup_handle(kContextRequestHandleId));
@@ -204,6 +207,19 @@ class ContextProviderImplTest : public base::MultiProcessTest {
     return output;
   }
 
+  // Checks that the Context channel was dropped.
+  void CheckContextUnresponsive(
+      fidl::InterfacePtr<chromium::web::Context>* context) {
+    base::RunLoop run_loop;
+    context->set_error_handler([&run_loop]() { run_loop.Quit(); });
+
+    chromium::web::FramePtr frame;
+    (*context)->CreateFrame(frame.NewRequest());
+
+    // The error handler should be called here.
+    run_loop.Run();
+  }
+
  protected:
   base::MessageLoopForIO message_loop_;
   std::unique_ptr<ContextProviderImpl> provider_;
@@ -213,7 +229,7 @@ class ContextProviderImplTest : public base::MultiProcessTest {
   struct CapturingNavigationEventObserver
       : public chromium::web::NavigationEventObserver {
    public:
-    CapturingNavigationEventObserver(base::OnceClosure on_change_cb)
+    explicit CapturingNavigationEventObserver(base::OnceClosure on_change_cb)
         : on_change_cb_(std::move(on_change_cb)) {}
     ~CapturingNavigationEventObserver() override = default;
 
@@ -290,9 +306,28 @@ TEST_F(ContextProviderImplTest, WithProfileDir) {
 
   CheckContextResponsive(&context);
 
-  // Verify that the context process can write data to the out dir.
+  // Verify that the context process can write to the data dir.
   EXPECT_TRUE(base::PathExists(
       profile_temp_dir.GetPath().AppendASCII(kTestDataFileOut)));
 }
 
+TEST_F(ContextProviderImplTest, FailsDataDirectoryIsFile) {
+  base::FilePath temp_file_path;
+
+  // Connect to a new context process.
+  fidl::InterfacePtr<chromium::web::Context> context;
+  chromium::web::CreateContextParams create_params = BuildCreateContextParams();
+
+  // Pass in a handle to a file instead of a directory.
+  CHECK(base::CreateTemporaryFile(&temp_file_path));
+  base::File temp_file(temp_file_path,
+                       base::File::FLAG_OPEN | base::File::FLAG_READ);
+  create_params.data_directory.reset(
+      base::fuchsia::GetHandleFromFile(std::move(temp_file)).release());
+
+  provider_ptr_->Create(std::move(create_params), context.NewRequest());
+
+  CheckContextUnresponsive(&context);
+}
+
 }  // namespace webrunner