Allow startup with missing V8 snapshot file.

gin/v8_initializer.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gin/v8_initializer.h"
 
 #include "base/basictypes.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/files/memory_mapped_file.h"
@@ skipping 32 matching lines @@
 #endif  // !OS_MACOSX
 
 const char kNativesFileName[] = "natives_blob.bin";
 const char kSnapshotFileName[] = "snapshot_blob.bin";
 
 // Constants for snapshot loading retries taken from:
 //   https://support.microsoft.com/en-us/kb/316609.
 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";
+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;
     }
   }
-
-  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";
-      return false;
-    }
-  }
 
   return true;
 }
 
 static bool OpenV8File(const base::FilePath& path,
                        int flags,
                        base::File& file) {
   // Re-try logic here is motivated by http://crbug.com/479537
   // for A/V on Windows (https://support.microsoft.com/en-us/kb/316609).
 
@@ skipping 28 matching lines @@
 
   UMA_HISTOGRAM_ENUMERATION("V8.Initializer.OpenV8File.Result",
                             result,
                             OpenV8FileResult::MAX_VALUE);
 
   return result == OpenV8FileResult::OPENED
          || result == OpenV8FileResult::OPENED_RETRY;
 }
 
 #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));
 }
 #endif  // V8_VERIFY_EXTERNAL_STARTUP_DATA
 #endif  // V8_USE_EXTERNAL_STARTUP_DATA
 
 bool GenerateEntropy(unsigned char* buffer, size_t amount) {
   base::RandBytes(buffer, amount);
   return true;
 }
 
 }  // namespace
 
 #if defined(V8_USE_EXTERNAL_STARTUP_DATA)
 #if defined(V8_VERIFY_EXTERNAL_STARTUP_DATA)
 // Defined in gen/gin/v8_snapshot_fingerprint.cc
 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::FilePath natives_data_path;
-  base::FilePath snapshot_data_path;
-  GetV8FilePaths(&natives_data_path, &snapshot_data_path);
-
-  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(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 (!VerifyV8SnapshotFile(g_mapped_natives, g_natives_fingerprint) ||
-             !VerifyV8SnapshotFile(g_mapped_snapshot, g_snapshot_fingerprint)) {
-    result = LoadV8SnapshotResult::FAILED_VERIFY;
+  } else if (!VerifyV8StartupFile(g_mapped_snapshot, g_snapshot_fingerprint)) {
+    result = V8_LOAD_FAILED_VERIFY;
 #endif  // V8_VERIFY_EXTERNAL_STARTUP_DATA
   } else {
-    result = LoadV8SnapshotResult::SUCCESS;
+    result = V8_LOAD_SUCCESS;
   }
+  UMA_HISTOGRAM_ENUMERATION("V8.Initializer.LoadV8Snapshot.Result", result,
+                            V8_LOAD_MAX_VALUE);
+}
 
-  UMA_HISTOGRAM_ENUMERATION("V8.Initializer.LoadV8Snapshot.Result",
-                            result,
-                            LoadV8SnapshotResult::MAX_VALUE);
-  return result == LoadV8SnapshotResult::SUCCESS;
+void V8Initializer::LoadV8Natives() {
+  if (g_mapped_natives)
+    return;
+
+  base::FilePath natives_data_path;
+  GetV8FilePath(&natives_data_path, kNativesFileName);
+
+  base::File natives_file;
+  int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
+
+  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 (!VerifyV8StartupFile(g_mapped_natives, g_natives_fingerprint)) {
+    result = V8_LOAD_FAILED_VERIFY;
+#endif  // V8_VERIFY_EXTERNAL_STARTUP_DATA
+  } else {
+    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";
+  }
 }
 
 // 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;
 
+  if (snapshot_pf == -1)
+    return false;
+
+  base::MemoryMappedFile::Region snapshot_region =
+      base::MemoryMappedFile::Region::kWholeFile;
+  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 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);
   }
 
-  base::MemoryMappedFile::Region snapshot_region =
-      base::MemoryMappedFile::Region::kWholeFile;
-  if (natives_size != 0 || natives_offset != 0) {
-    snapshot_region =
-        base::MemoryMappedFile::Region(snapshot_offset, snapshot_size);
-  }
-
-  return MapV8Files(base::File(natives_pf), base::File(snapshot_pf),
-                    natives_region, snapshot_region);
+  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
 bool V8Initializer::OpenV8FilesForChildProcesses(
     base::PlatformFile* natives_fd_out,
     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
 
 // static
 void V8Initializer::Initialize(gin::IsolateHolder::ScriptMode mode) {
   static bool v8_is_initialized = false;
   if (v8_is_initialized)
     return;
 
   v8::V8::InitializePlatform(V8Platform::Get());
 
   if (gin::IsolateHolder::kStrictMode == mode) {
     static const char use_strict[] = "--use_strict";
     v8::V8::SetFlagsFromString(use_strict, sizeof(use_strict) - 1);
   }
 
 #if defined(V8_USE_EXTERNAL_STARTUP_DATA)
   v8::StartupData natives;
   natives.data = reinterpret_cast<const char*>(g_mapped_natives->data());
   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);
   v8::V8::Initialize();
 
   v8_is_initialized = true;
 }
 
 // static
 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());
   }
-  *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());
+  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());
+  }
 }
 
 }  // namespace gin