Allow startup with missing V8 snapshot file.

gin/v8_initializer.cc

Index: gin/v8_initializer.cc
diff --git a/gin/v8_initializer.cc b/gin/v8_initializer.cc
index 8f79d0d8693b23b01820e9906dad5da48a64bb77..0432e5b5ed9cea1d507a7b91465f2c35da33fc29 100644
--- a/gin/v8_initializer.cc
+++ b/gin/v8_initializer.cc
@@ -50,51 +50,31 @@ const char kSnapshotFileName[] = "snapshot_blob.bin";
 const int kMaxOpenAttempts = 5;
 const int kOpenRetryDelayMillis = 250;
 
-void GetV8FilePaths(base::FilePath* natives_path_out,
-                    base::FilePath* snapshot_path_out) {
+void GetV8FilePath(base::FilePath* path, const char* file_name) {

[rmcilroy, 2015/06/02 15:20:27]

nit - /s/path/path_out and move file_name to first param.

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

 #if !defined(OS_MACOSX)
   base::FilePath data_path;
   PathService::Get(kV8SnapshotBasePathKey, &data_path);
   DCHECK(!data_path.empty());
 
-  *natives_path_out = data_path.AppendASCII(kNativesFileName);
-  *snapshot_path_out = data_path.AppendASCII(kSnapshotFileName);
+  *path = data_path.AppendASCII(file_name);
 #else   // !defined(OS_MACOSX)
   base::ScopedCFTypeRef<CFStringRef> natives_file_name(
-      base::SysUTF8ToCFStringRef(kNativesFileName));
-  *natives_path_out =
-      base::mac::PathForFrameworkBundleResource(natives_file_name);
-  base::ScopedCFTypeRef<CFStringRef> snapshot_file_name(
-      base::SysUTF8ToCFStringRef(kSnapshotFileName));
-  *snapshot_path_out =
-      base::mac::PathForFrameworkBundleResource(snapshot_file_name);
-  DCHECK(!natives_path_out->empty());
-  DCHECK(!snapshot_path_out->empty());
+      base::SysUTF8ToCFStringRef(file_name));
+  *path = base::mac::PathForFrameworkBundleResource(natives_file_name);
 #endif  // !defined(OS_MACOSX)
+  DCHECK(!path->empty());
 }
 
-static bool MapV8Files(base::File natives_file,
-                       base::File snapshot_file,
-                       base::MemoryMappedFile::Region natives_region =
-                           base::MemoryMappedFile::Region::kWholeFile,
-                       base::MemoryMappedFile::Region snapshot_region =
-                           base::MemoryMappedFile::Region::kWholeFile) {
-  g_mapped_natives = new base::MemoryMappedFile;
-  if (!g_mapped_natives->IsValid()) {
-    if (!g_mapped_natives->Initialize(natives_file.Pass(), natives_region)) {
-      delete g_mapped_natives;
-      g_mapped_natives = NULL;
-      LOG(FATAL) << "Couldn't mmap v8 natives data file";
-      return false;
-    }
-  }
-
-  g_mapped_snapshot = new base::MemoryMappedFile;
-  if (!g_mapped_snapshot->IsValid()) {
-    if (!g_mapped_snapshot->Initialize(snapshot_file.Pass(), snapshot_region)) {
-      delete g_mapped_snapshot;
-      g_mapped_snapshot = NULL;
-      LOG(ERROR) << "Couldn't mmap v8 snapshot data file";
+bool MapV8File(base::MemoryMappedFile** mmapped_file_return,

[rmcilroy, 2015/06/02 15:20:27]

nit - /s/mmapped_file_return/mmapped_file_out and move to end of parameter list
(this will mean we need to the default argument values for region, but we should
probably do that anyway since they are frowned apon by the style guide)

[rmcilroy, 2015/06/02 15:20:27]

Keep as static?

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

+               base::File file,
+               base::MemoryMappedFile::Region region =
+                   base::MemoryMappedFile::Region::kWholeFile) {
+  base::MemoryMappedFile* mmapped_file = *mmapped_file_return =

[rmcilroy, 2015/06/02 15:20:27]

DCHECK(*mmapped_file_out == NULL) before allocating here.

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

+      new base::MemoryMappedFile;
+  if (!mmapped_file->IsValid()) {

[rmcilroy, 2015/06/02 15:20:27]

I don't think this is necessary right? mmapped_file->IsValid() is always going
to be false for a newly allocated MemoryMappedFile.

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

+    if (!mmapped_file->Initialize(file.Pass(), region)) {
+      delete mmapped_file;
+      *mmapped_file_return = NULL;
       return false;
     }
   }
@@ -146,12 +126,12 @@ static bool OpenV8File(const base::FilePath& path,
 }
 
 #if defined(V8_VERIFY_EXTERNAL_STARTUP_DATA)
-bool VerifyV8SnapshotFile(base::MemoryMappedFile* snapshot_file,
-                          const unsigned char* fingerprint) {
+bool VerifyV8StartupFile(base::MemoryMappedFile* file,
+                         const unsigned char* fingerprint) {
   unsigned char output[crypto::kSHA256Length];
   crypto::SHA256HashString(
-      base::StringPiece(reinterpret_cast<const char*>(snapshot_file->data()),
-                        snapshot_file->length()),
+      base::StringPiece(reinterpret_cast<const char*>(file->data()),
+                        file->length()),
       output, sizeof(output));
   return !memcmp(fingerprint, output, sizeof(output));
 }
@@ -172,75 +152,105 @@ extern const unsigned char g_natives_fingerprint[];
 extern const unsigned char g_snapshot_fingerprint[];
 #endif  // V8_VERIFY_EXTERNAL_STARTUP_DATA
 
+enum LoadV8FileResult {
+  V8_LOAD_SUCCESS = 0,
+  V8_LOAD_FAILED_OPEN,
+  V8_LOAD_FAILED_MAP,
+  V8_LOAD_FAILED_VERIFY,
+  V8_LOAD_MAX_VALUE
+};
+
 // static
-bool V8Initializer::LoadV8Snapshot() {
+void V8Initializer::LoadV8Snapshot() {
+  if (g_mapped_snapshot)
+    return;
 
-  enum LoadV8SnapshotResult {
-    SUCCESS = 0,
-    FAILED_OPEN,
-    FAILED_MAP,
-    FAILED_VERIFY,
-    MAX_VALUE
-  };
+  base::FilePath snapshot_data_path;
+  GetV8FilePath(&snapshot_data_path, kSnapshotFileName);
 
-  if (g_mapped_natives && g_mapped_snapshot)
-    return true;
+  base::File snapshot_file;
+  int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
+
+  LoadV8FileResult result;
+  if (!OpenV8File(snapshot_data_path, flags, snapshot_file)) {
+    result = V8_LOAD_FAILED_OPEN;
+  } else if (!MapV8File(&g_mapped_snapshot, snapshot_file.Pass())) {
+    result = V8_LOAD_FAILED_MAP;
+#if defined(V8_VERIFY_EXTERNAL_STARTUP_DATA)
+  } else if (!VerifyV8StartupFile(g_mapped_snapshot, g_snapshot_fingerprint)) {
+    result = V8_LOAD_FAILED_VERIFY;
+#endif  // V8_VERIFY_EXTERNAL_STARTUP_DATA
+  } else {
+    result = V8_LOAD_SUCCESS;
+  }
+  UMA_HISTOGRAM_ENUMERATION("V8.Initializer.LoadV8Snapshot.Result", result,
+                            V8_LOAD_MAX_VALUE);
+}
+
+void V8Initializer::LoadV8Natives() {
+  if (g_mapped_natives)
+    return;
 
   base::FilePath natives_data_path;
-  base::FilePath snapshot_data_path;
-  GetV8FilePaths(&natives_data_path, &snapshot_data_path);
+  GetV8FilePath(&natives_data_path, kNativesFileName);
 
   base::File natives_file;
-  base::File snapshot_file;
   int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
 
-  LoadV8SnapshotResult result;
-  if (!OpenV8File(natives_data_path, flags, natives_file) ||
-      !OpenV8File(snapshot_data_path, flags, snapshot_file)) {
-    result = LoadV8SnapshotResult::FAILED_OPEN;
-  } else if (!MapV8Files(natives_file.Pass(), snapshot_file.Pass())) {
-    result = LoadV8SnapshotResult::FAILED_MAP;
+  LoadV8FileResult result;
+  if (!OpenV8File(natives_data_path, flags, natives_file)) {
+    result = V8_LOAD_FAILED_OPEN;
+  } else if (!MapV8File(&g_mapped_natives, natives_file.Pass())) {
+    result = V8_LOAD_FAILED_MAP;
 #if defined(V8_VERIFY_EXTERNAL_STARTUP_DATA)
-  } else if (!VerifyV8SnapshotFile(g_mapped_natives, g_natives_fingerprint) ||
-             !VerifyV8SnapshotFile(g_mapped_snapshot, g_snapshot_fingerprint)) {
-    result = LoadV8SnapshotResult::FAILED_VERIFY;
+  } else if (!VerifyV8StartupFile(g_mapped_natives, g_natives_fingerprint)) {
+    result = V8_LOAD_FAILED_VERIFY;
 #endif  // V8_VERIFY_EXTERNAL_STARTUP_DATA
   } else {
-    result = LoadV8SnapshotResult::SUCCESS;
+    result = V8_LOAD_SUCCESS;
+  }

[rmcilroy, 2015/06/02 15:20:27]

Could you share the code from lines 168-185 with 194-211 with a helper function
- e.g:

  LoadV8FileResult result = LoadV8StartupFile(&g_mapped_natives, 
kNativesFileName, g_natives_fingerprint);

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

+  if (result != V8_LOAD_SUCCESS) {
+    LOG(FATAL) << "Couldn't mmap v8 natives data file";
   }
-
-  UMA_HISTOGRAM_ENUMERATION("V8.Initializer.LoadV8Snapshot.Result",
-                            result,
-                            LoadV8SnapshotResult::MAX_VALUE);
-  return result == LoadV8SnapshotResult::SUCCESS;
 }
 
 // static
-bool V8Initializer::LoadV8SnapshotFromFD(base::PlatformFile natives_pf,
-                                         int64 natives_offset,
-                                         int64 natives_size,
-                                         base::PlatformFile snapshot_pf,
+bool V8Initializer::LoadV8SnapshotFromFD(base::PlatformFile snapshot_pf,
                                          int64 snapshot_offset,
                                          int64 snapshot_size) {
-  if (g_mapped_natives && g_mapped_snapshot)
+  if (g_mapped_snapshot)
     return true;
 
-  base::MemoryMappedFile::Region natives_region =
-      base::MemoryMappedFile::Region::kWholeFile;
-  if (natives_size != 0 || natives_offset != 0) {
-    natives_region =
-        base::MemoryMappedFile::Region(natives_offset, natives_size);
-  }
+  if (snapshot_pf == -1)
+    return false;
 
   base::MemoryMappedFile::Region snapshot_region =
       base::MemoryMappedFile::Region::kWholeFile;
-  if (natives_size != 0 || natives_offset != 0) {
+  if (snapshot_size != 0 || snapshot_offset != 0) {
     snapshot_region =
         base::MemoryMappedFile::Region(snapshot_offset, snapshot_size);
   }

[picksi, 2015/06/02 16:02:15]

nit: would the following be more readable :

base::MemoryMappedFile::Region snapshot_region;

if (snapshot_size == 0 && snapshot_offset == 0)
{
  snapshot_region = base::MemoryMappedFile::Region::kWholeFile;
}
else
{
 snapshot_region = base::MemoryMappedFile::Region(snapshot_offset,
snapshot_size);
}

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Some compilers are very annoying about uninitialized variables, so I'd rather
avoid them.

 
-  return MapV8Files(base::File(natives_pf), base::File(snapshot_pf),
-                    natives_region, snapshot_region);
+  return MapV8File(&g_mapped_snapshot, base::File(snapshot_pf),

[rmcilroy, 2015/06/02 15:20:27]

Please do the same UMA histograms here as in LoadV8Snapshot (this is actually
the one we care about on Android).

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

+                   snapshot_region);
+}
+
+void V8Initializer::LoadV8NativesFromFD(base::PlatformFile natives_pf,
+                                        int64 natives_offset,
+                                        int64 natives_size) {
+  if (g_mapped_natives)
+    return;
+
+  CHECK_NE(natives_pf, -1);
+
+  base::MemoryMappedFile::Region natives_region =
+      base::MemoryMappedFile::Region::kWholeFile;
+  if (natives_size != 0 || natives_offset != 0) {
+    natives_region =
+        base::MemoryMappedFile::Region(natives_offset, natives_size);
+  }
+
+  CHECK(MapV8File(&g_mapped_natives, base::File(natives_pf), natives_region));

[rmcilroy, 2015/06/02 15:20:27]

LOG(FATAL) here like LoadV8Natives for consistency.

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

 }
 
 // static
@@ -249,19 +259,25 @@ bool V8Initializer::OpenV8FilesForChildProcesses(
     base::PlatformFile* snapshot_fd_out) {
   base::FilePath natives_data_path;
   base::FilePath snapshot_data_path;
-  GetV8FilePaths(&natives_data_path, &snapshot_data_path);
+  GetV8FilePath(&natives_data_path, kNativesFileName);
+  GetV8FilePath(&snapshot_data_path, kSnapshotFileName);
 
   base::File natives_data_file;
   base::File snapshot_data_file;
   int file_flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
 
-  bool success = OpenV8File(natives_data_path, file_flags, natives_data_file) &&
-                 OpenV8File(snapshot_data_path, file_flags, snapshot_data_file);
-  if (success) {
+  bool natives_success =
+      OpenV8File(natives_data_path, file_flags, natives_data_file);
+  if (natives_success) {
     *natives_fd_out = natives_data_file.TakePlatformFile();
+  }
+  bool snapshot_success =
+      OpenV8File(snapshot_data_path, file_flags, snapshot_data_file);
+  if (snapshot_success) {
     *snapshot_fd_out = snapshot_data_file.TakePlatformFile();
   }
-  return success;
+  // We can start up without the snapshot file, but not without the natives.
+  return natives_success;
 }
 
 #endif  // V8_USE_EXTERNAL_STARTUP_DATA
@@ -285,10 +301,12 @@ void V8Initializer::Initialize(gin::IsolateHolder::ScriptMode mode) {
   natives.raw_size = static_cast<int>(g_mapped_natives->length());
   v8::V8::SetNativesDataBlob(&natives);
 
-  v8::StartupData snapshot;
-  snapshot.data = reinterpret_cast<const char*>(g_mapped_snapshot->data());
-  snapshot.raw_size = static_cast<int>(g_mapped_snapshot->length());
-  v8::V8::SetSnapshotDataBlob(&snapshot);
+  if (g_mapped_snapshot != NULL) {
+    v8::StartupData snapshot;
+    snapshot.data = reinterpret_cast<const char*>(g_mapped_snapshot->data());
+    snapshot.raw_size = static_cast<int>(g_mapped_snapshot->length());
+    v8::V8::SetSnapshotDataBlob(&snapshot);
+  }
 #endif  // V8_USE_EXTERNAL_STARTUP_DATA
 
   v8::V8::SetEntropySource(&GenerateEntropy);
@@ -302,15 +320,21 @@ void V8Initializer::GetV8ExternalSnapshotData(const char** natives_data_out,
                                               int* natives_size_out,
                                               const char** snapshot_data_out,
                                               int* snapshot_size_out) {
-  if (!g_mapped_natives || !g_mapped_snapshot) {
-    *natives_data_out = *snapshot_data_out = NULL;
-    *natives_size_out = *snapshot_size_out = 0;
-    return;
+  if (!g_mapped_natives) {

[picksi, 2015/06/02 16:02:15]

nit: remove the ! and swap if/else body?

[Erik Corry Chromium.org, 2015/06/04 11:40:40]

Done.

+    *natives_data_out = NULL;
+    *natives_size_out = 0;
+  } else {
+    *natives_data_out = reinterpret_cast<const char*>(g_mapped_natives->data());
+    *natives_size_out = static_cast<int>(g_mapped_natives->length());
+  }
+  if (!g_mapped_snapshot) {
+    *snapshot_data_out = NULL;
+    *snapshot_size_out = 0;
+  } else {
+    *snapshot_data_out =
+        reinterpret_cast<const char*>(g_mapped_snapshot->data());
+    *snapshot_size_out = static_cast<int>(g_mapped_snapshot->length());
   }
-  *natives_data_out = reinterpret_cast<const char*>(g_mapped_natives->data());
-  *snapshot_data_out = reinterpret_cast<const char*>(g_mapped_snapshot->data());
-  *natives_size_out = static_cast<int>(g_mapped_natives->length());
-  *snapshot_size_out = static_cast<int>(g_mapped_snapshot->length());
 }
 
 }  // namespace gin