Makes ResourceLoader own handles and return Files

This way it won't leak if destroyed. Also cleaned up html_viewer's use
of ResourceLoader.

R=ben@chromium.org
TBR=ben@chromium.org
BUG=none
TEST=none

Review URL: https://codereview.chromium.org/1132083003

Cr-Commit-Position: refs/heads/master@{#329295}

components/html_viewer/BUILD.gn

@@ -7,6 +7,7 @@ import("//mojo/mojo_application_package.gni")
 import("//testing/test.gni")
 import("//third_party/mojo/src/mojo/public/mojo.gni")
 import("//third_party/mojo/src/mojo/public/mojo_application.gni")
+import("//tools/grit/grit_rule.gni")
 import("//tools/grit/repack.gni")
 
 # Repack this here.
@@ -45,6 +46,14 @@ action("generate_blink_resource_map") {
   ]
 }
 
+grit("html_viewer_resources_grit") {
+  source = "html_viewer_resources.grd"
+  outputs = [
+    "grit/html_viewer_resources.h",
+    "html_viewer_resources.pak",
+    "html_viewer_resources.rc",
+  ]
+}
 source_set("lib") {
   sources = [
     "$target_gen_dir/blink_resource_map.cc",
@@ -166,6 +175,7 @@ mojo_application_package("html_viewer") {
     "ui_setup_android.h",
   ]
   deps = [
+    ":html_viewer_resources_grit",
     ":lib",
     "//components/resource_provider/public/cpp",
     "//components/resource_provider/public/interfaces",
@@ -181,6 +191,7 @@ mojo_application_package("html_viewer") {
   resources = [
     "$root_out_dir/icudtl.dat",
     "$root_out_dir/ui_test.pak",
+    "$target_gen_dir/html_viewer_resources.pak",
   ]
 
   if (v8_use_external_startup_data) {

components/html_viewer/html_viewer.cc

@@ -55,7 +55,7 @@ using mojo::URLResponsePtr;
 using resource_provider::ResourceLoader;
 
 namespace html_viewer {
-
+namespace {
 // Switches for html_viewer.
 
 // Enable MediaRenderer in media pipeline instead of using the internal
@@ -70,6 +70,17 @@ const char kIsHeadless[] = "is-headless";
 
 size_t kDesiredMaxMemory = 20 * 1024 * 1024;
 
+// Paths resources are loaded from.
+const char kResourceIcudtl[] = "icudtl.dat";
+const char kResourceResourcesPak[] = "html_viewer_resources.pak";
+const char kResourceUIPak[] = "ui_test.pak";
+#if defined(V8_USE_EXTERNAL_STARTUP_DATA)
+const char kResourceNativesBlob[] = "natives_blob.bin";
+const char kResourceSnapshotBlob[] = "snapshot_blob.bin";
+#endif
+
+}  // namespace
+
 class HTMLViewer;
 
 class HTMLViewerApplication : public mojo::Application {
@@ -182,13 +193,13 @@ class HTMLViewer : public mojo::ApplicationDelegate,
  private:
   // Overridden from ApplicationDelegate:
   void Initialize(mojo::ApplicationImpl* app) override {
-    // TODO(sky): make this typesafe and not leak.
     std::set<std::string> paths;
-    paths.insert("icudtl.dat");
-    paths.insert("ui_test.pak");
+    paths.insert(kResourceResourcesPak);
+    paths.insert(kResourceIcudtl);
+    paths.insert(kResourceUIPak);
 #if defined(V8_USE_EXTERNAL_STARTUP_DATA)
-    paths.insert("natives_blob.bin");
-    paths.insert("snapshot_blob.bin");
+    paths.insert(kResourceNativesBlob);
+    paths.insert(kResourceSnapshotBlob);
 #endif
     ResourceLoader resource_loader(app->shell(), paths);
     if (!resource_loader.BlockUntilLoaded()) {
@@ -198,8 +209,6 @@ class HTMLViewer : public mojo::ApplicationDelegate,
       return;
     }
 
-    ResourceLoader::ResourceMap resource_map(resource_loader.resource_map());
-
     ui_setup_.reset(new UISetup);
     base::DiscardableMemoryAllocator::SetInstance(
         &discardable_memory_allocator_);
@@ -210,12 +219,15 @@ class HTMLViewer : public mojo::ApplicationDelegate,
 #if defined(V8_USE_EXTERNAL_STARTUP_DATA)
     // Note: this requires file system access.
     CHECK(gin::V8Initializer::LoadV8SnapshotFromFD(
-        resource_map["natives_blob.bin"], 0u, 0u,
-        resource_map["snapshot_blob.bin"], 0u, 0u));
+        resource_loader.ReleaseFile(kResourceNativesBlob).TakePlatformFile(),
+        0u, 0u,
+        resource_loader.ReleaseFile(kResourceSnapshotBlob).TakePlatformFile(),
+        0u, 0u));
 #endif
     blink::initialize(blink_platform_.get());
     base::i18n::InitializeICUWithFileDescriptor(
-        resource_map["icudtl.dat"], base::MemoryMappedFile::Region::kWholeFile);
+        resource_loader.ReleaseFile(kResourceIcudtl).TakePlatformFile(),
+        base::MemoryMappedFile::Region::kWholeFile);
 
     ui::RegisterPathProvider();
 
@@ -232,14 +244,11 @@ class HTMLViewer : public mojo::ApplicationDelegate,
 
     is_headless_ = command_line->HasSwitch(kIsHeadless);
     if (!is_headless_) {
-      // TODO(sky): fix lifetime here.
-      MojoPlatformHandle platform_handle = resource_map["ui_test.pak"];
-      // TODO(sky): the first call should be for strings, not images.
       ui::ResourceBundle::InitSharedInstanceWithPakFileRegion(
-          base::File(platform_handle).Pass(),
+          resource_loader.ReleaseFile(kResourceResourcesPak),
           base::MemoryMappedFile::Region::kWholeFile);
       ui::ResourceBundle::GetSharedInstance().AddDataPackFromFile(
-          base::File(platform_handle).Pass(), ui::SCALE_FACTOR_100P);
+          resource_loader.ReleaseFile(kResourceUIPak), ui::SCALE_FACTOR_100P);
     }
 
     compositor_thread_.Start();

components/html_viewer/html_viewer_resources.grd

+<?xml version="1.0" encoding="UTF-8"?>
+<grit latest_public_release="0" current_release="1">
+  <outputs>
+    <output filename="grit/html_viewer_resources.h" type="rc_header">
+      <emit emit_type='prepend'></emit>
+    </output>
+    <output filename="html_viewer_resources.pak" type="data_package"/>
+    <output filename="html_viewer_resources.rc" type="rc_all" />
+  </outputs>
+  <translations />
+</grit>

components/resource_provider/public/cpp/resource_loader.cc

@@ -5,6 +5,7 @@
 #include "components/resource_provider/public/cpp/resource_loader.h"
 
 #include "base/bind.h"
+#include "base/files/file.h"
 #include "mojo/common/common_type_converters.h"
 #include "mojo/platform_handle/platform_handle_functions.h"
 #include "third_party/mojo/src/mojo/public/cpp/application/connect.h"
@@ -14,7 +15,7 @@ namespace resource_provider {
 
 ResourceLoader::ResourceLoader(mojo::Shell* shell,
                                const std::set<std::string>& paths)
-    : loaded_(false) {
+    : loaded_(false), did_block_(false) {
   shell->ConnectToApplication("mojo:resource_provider",
                               GetProxy(&resource_provider_service_provider_),
                               nullptr);
@@ -31,11 +32,21 @@ ResourceLoader::~ResourceLoader() {
 }
 
 bool ResourceLoader::BlockUntilLoaded() {
-  CHECK(!loaded_);
+  if (did_block_)
+    return loaded_;
+
+  did_block_ = true;
   resource_provider_.WaitForIncomingMethodCall();
   return loaded_;
 }
 
+base::File ResourceLoader::ReleaseFile(const std::string& path) {
+  CHECK(resource_map_.count(path));
+  scoped_ptr<base::File> file_wrapper(resource_map_[path].Pass());
+  resource_map_.erase(path);
+  return file_wrapper->Pass();
+}
+
 void ResourceLoader::OnGotResources(const std::vector<std::string>& paths,
                                     mojo::Array<mojo::ScopedHandle> resources) {
   // We no longer need the connection to ResourceProvider.
@@ -49,7 +60,7 @@ void ResourceLoader::OnGotResources(const std::vector<std::string>& paths,
     MojoPlatformHandle platform_handle;
     CHECK(MojoExtractPlatformHandle(resources[i].release().value(),
                                     &platform_handle) == MOJO_RESULT_OK);
-    resource_map_[paths[i]] = platform_handle;
+    resource_map_[paths[i]].reset(new base::File(platform_handle));
   }
   loaded_ = true;
 }

components/resource_provider/public/cpp/resource_loader.h

@@ -11,12 +11,17 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
 #include "components/resource_provider/public/interfaces/resource_provider.mojom.h"
 #include "mojo/platform_handle/platform_handle.h"
 #include "third_party/mojo/src/mojo/public/cpp/bindings/array.h"
 #include "third_party/mojo/src/mojo/public/cpp/system/handle.h"
 #include "third_party/mojo/src/mojo/public/interfaces/application/service_provider.mojom.h"
 
+namespace base {
+class File;
+}
+
 namespace mojo {
 class Shell;
 }
@@ -29,21 +34,23 @@ namespace resource_provider {
 // have been obtained.
 class ResourceLoader {
  public:
-  using ResourceMap = std::map<std::string, MojoPlatformHandle>;
-
   ResourceLoader(mojo::Shell* shell, const std::set<std::string>& paths);
   ~ResourceLoader();
 
   // Uses WaitForIncomingMessage() to block until the results are available, or
   // an error occurs. Returns true if the resources were obtained, false on
-  // error.
+  // error. This immediately returns if the resources have already been
+  // obtained.
   bool BlockUntilLoaded();
 
-  const ResourceMap& resource_map() const { return resource_map_; }
+  // Releases and returns the file wrapping the handle.
+  base::File ReleaseFile(const std::string& path);
 
   bool loaded() const { return loaded_; }
 
  private:
+  using ResourceMap = std::map<std::string, scoped_ptr<base::File>>;
+
   // Callback when resources have loaded.
   void OnGotResources(const std::vector<std::string>& paths,
                       mojo::Array<mojo::ScopedHandle> resources);
@@ -55,6 +62,7 @@ class ResourceLoader {
   ResourceMap resource_map_;
 
   bool loaded_;
+  bool did_block_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceLoader);
 };

components/resource_provider/resource_provider_apptest.cc

@@ -58,14 +58,12 @@ class ResourceProviderApplicationTest : public mojo::test::ApplicationTestBase {
   ResourceContentsMap GetResources(const std::set<std::string>& paths) {
     ResourceLoader loader(application_impl()->shell(), paths);
     loader.BlockUntilLoaded();
-    const ResourceLoader::ResourceMap& resource_loader_results(
-        loader.resource_map());
 
     // Load the contents of each of the handles.
     ResourceContentsMap results;
-    for (auto& pair : resource_loader_results) {
-      base::File file(pair.second);
-      results[pair.first] = ReadFile(&file);
+    for (auto& path : paths) {
+      base::File file(loader.ReleaseFile(path));
+      results[path] = ReadFile(&file);
     }
     return results;
   }