fix(runtime): read script caches into per-call buffers, publish them atomically (#468)

* fix(runtime): read script caches into per-call buffers, publish them atomically

tns::ReadBinary and the 3-arg tns::ReadText served every file up to 1 MiB
out of a process-global static buffer. LoadScriptCache handed that pointer
to V8 as CachedData(BufferNotOwned), which the deserializer reads for the
whole of CodeSerializer::Deserialize. Two Worker threads starting at once
both fread their code caches into the same buffer, so one deserialized bytes
the other was overwriting with a different file. Release V8 does not
checksum code caches, so the corrupted payload was followed until a bad
pointer dereferenced: EXC_BAD_ACCESS in Deserializer::ReadObject, seen in
production a few seconds after app launch, always with a second worker in
the same frame.

ReadBinary now returns a caller-owned heap buffer (LoadScriptCache uses
BufferOwned) and ReadText reads straight into its std::string; the shared
Buffer/BinBuffer statics are gone. WriteBinary writes through a mkstemp
file in the cache directory and renames it over the target, stamping the
source's mtime before publishing, so a reader never sees a half-written
cache and two first-time compilers of the same module no longer interleave
writes into one file. Both SaveScriptCache overloads share one helper.

WorkerConcurrentStartupTests starts 12 workers together against warm caches,
each loading six fixture modules in rotated orders. On a Release build of
TestRunner (Debug disables code caches) it fails on main with workers
reporting another module's value and one never reporting; it passes with
this change.

* fix(runtime): guard null code caches and arm the startup spec's timeout early

ScriptCompiler::CreateCodeCache returns nullptr for a script V8 declines to
serialize; both SaveScriptCache overloads dereferenced the result. They now
skip the write instead.

Jasmine 2.0.1 arms a spec's async timeout before invoking it, so raising
DEFAULT_TIMEOUT_INTERVAL inside WorkerConcurrentStartupTests' spec never
applied to that spec (the unfixed-runtime run timed out at exactly the 5 s
default). The interval is now raised in beforeEach and restored in afterEach.

Author: edusperoni

NativeScript/runtime/Helpers.h

@@ -243,10 +243,15 @@ inline bool ToBool(const v8::Local<v8::Value>& value) {
 }
 bool Exists(const char* fullPath);
 v8::Local<v8::String> ReadModule(v8::Isolate* isolate, const std::string& filePath);
-const char* ReadText(const std::string& filePath, long& length, bool& isNew);
 std::string ReadText(const std::string& file);
-uint8_t* ReadBinary(const std::string path, long& length, bool& isNew);
-bool WriteBinary(const std::string& path, const void* data, long length);
+// The whole file in a heap buffer the caller owns (delete[]); nullptr and a
+// zero length when it cannot be read.
+uint8_t* ReadBinary(const std::string& path, long& length);
+// Replaces the file at `path` atomically: readers see the old contents or the
+// new ones, never a partial write. A non-negative `modificationTime` stamps the
+// file's mtime before it is published.
+bool WriteBinary(const std::string& path, const void* data, long length,
+                 time_t modificationTime = -1);
 
 void SetPrivateValue(const v8::Local<v8::Object>& obj, const v8::Local<v8::String>& propName,
                      const v8::Local<v8::Value>& value);

NativeScript/runtime/Helpers.mm

@@ -8,10 +8,12 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/stat.h>
+#include <unistd.h>
 #include <atomic>
-#include <fstream>
+#include <cerrno>
 #include <mutex>
 #include <sstream>
+#include <vector>
 #include "Caches.h"
 #include "ErrorEvents.h"
 #include "NativeScriptException.h"
@@ -20,12 +22,6 @@
 
 using namespace v8;
 
-namespace {
-const int BUFFER_SIZE = 1024 * 1024;
-char* Buffer = new char[BUFFER_SIZE];
-uint8_t* BinBuffer = new uint8_t[BUFFER_SIZE];
-}  // namespace
-
 std::u16string tns::ToUtf16String(Isolate* isolate, const Local<Value>& value) {
   // Read the V8 string's native UTF-16 buffer directly instead of round-tripping
   // through UTF-8, which corrupts lone surrogates (replaced with U+FFFD) and is
@@ -99,88 +95,107 @@
   return str;
 }
 
-const char* tns::ReadText(const std::string& filePath, long& length, bool& isNew) {
+// The file readers below run on the main thread and on every worker's thread
+// at once (workers load shared modules and their code caches while starting),
+// so each call reads into storage of its own; no scratch buffer is shared.
+
+std::string tns::ReadText(const std::string& filePath) {
   FILE* file = fopen(filePath.c_str(), "rb");
   if (file == nullptr) {
     tns::Assert(false);
   }
 
   fseek(file, 0, SEEK_END);
-
-  length = ftell(file);
-  isNew = length > BUFFER_SIZE;
-
+  long length = ftell(file);
   rewind(file);
 
-  if (isNew) {
-    char* newBuffer = new char[length];
-    fread(newBuffer, 1, length, file);
-    fclose(file);
-
-    return newBuffer;
+  std::string result;
+  if (length > 0) {
+    result.resize(length);
+    size_t readBytes = fread(result.data(), 1, length, file);
+    result.resize(readBytes);
   }
-
-  fread(Buffer, 1, length, file);
   fclose(file);
 
-  return Buffer;
-}
-
-std::string tns::ReadText(const std::string& file) {
-  long length;
-  bool isNew;
-  const char* content = tns::ReadText(file, length, isNew);
-
-  std::string result(content, length);
-
-  if (isNew) {
-    delete[] content;
-  }
-
   return result;
 }
 
-uint8_t* tns::ReadBinary(const std::string path, long& length, bool& isNew) {
+uint8_t* tns::ReadBinary(const std::string& path, long& length) {
   length = 0;
-  std::ifstream ifs(path);
-  if (ifs.fail()) {
-    return nullptr;
-  }
-
   FILE* file = fopen(path.c_str(), "rb");
   if (!file) {
     return nullptr;
   }
 
   fseek(file, 0, SEEK_END);
-  length = ftell(file);
+  long fileLength = ftell(file);
   rewind(file);
-
-  isNew = length > BUFFER_SIZE;
-
-  if (isNew) {
-    uint8_t* data = new uint8_t[length];
-    fread(data, sizeof(uint8_t), length, file);
+  if (fileLength <= 0) {
     fclose(file);
-    return data;
+    return nullptr;
   }
 
-  fread(BinBuffer, 1, length, file);
+  uint8_t* data = new uint8_t[fileLength];
+  size_t readBytes = fread(data, sizeof(uint8_t), fileLength, file);
   fclose(file);
+  if (readBytes != static_cast<size_t>(fileLength)) {
+    delete[] data;
+    return nullptr;
+  }
 
-  return BinBuffer;
+  length = fileLength;
+  return data;
 }
 
-bool tns::WriteBinary(const std::string& path, const void* data, long length) {
-  FILE* file = fopen(path.c_str(), "wb");
-  if (!file) {
+bool tns::WriteBinary(const std::string& path, const void* data, long length,
+                      time_t modificationTime) {
+  // Written to a private temp file in the same directory, then renamed over the
+  // target: rename is atomic on the local file system, so a concurrent reader
+  // of `path` never observes a truncated or half-written file, and two writers
+  // racing on the same path leave whichever complete file landed last.
+  std::string tmpTemplate = path + ".XXXXXX";
+  std::vector<char> tmpPath(tmpTemplate.begin(), tmpTemplate.end());
+  tmpPath.push_back('\0');
+  int fd = mkstemp(tmpPath.data());
+  if (fd < 0) {
     return false;
   }
 
-  size_t writtenBytes = fwrite(data, sizeof(uint8_t), length, file);
-  fclose(file);
+  bool ok = true;
+  const uint8_t* cursor = static_cast<const uint8_t*>(data);
+  long remaining = length;
+  while (remaining > 0) {
+    ssize_t written = write(fd, cursor, remaining);
+    if (written < 0) {
+      if (errno == EINTR) {
+        continue;
+      }
+      ok = false;
+      break;
+    }
+    cursor += written;
+    remaining -= written;
+  }
+
+  if (ok && modificationTime >= 0) {
+    struct timespec times[2];
+    times[0].tv_sec = time(nullptr);
+    times[0].tv_nsec = 0;
+    times[1].tv_sec = modificationTime;
+    times[1].tv_nsec = 0;
+    ok = futimens(fd, times) == 0;
+  }
+
+  close(fd);
+
+  if (ok && rename(tmpPath.data(), path.c_str()) != 0) {
+    ok = false;
+  }
+  if (!ok) {
+    unlink(tmpPath.data());
+  }
 
-  return writtenBytes == length;
+  return ok;
 }
 
 void tns::SetPrivateValue(const Local<Object>& obj, const Local<v8::String>& propName,

NativeScript/runtime/ModuleInternal.mm

@@ -3,7 +3,6 @@
 #include <sys/stat.h>
 #include <time.h>
 #include <unistd.h>
-#include <utime.h>
 #include <cmath>
 #include <cstring>
 #include <string>
@@ -1894,44 +1893,39 @@ throw NativeScriptException(
     }
   }
 
-  bool isNew = false;
-  uint8_t* data = tns::ReadBinary(cachePath, length, isNew);
+  uint8_t* data = tns::ReadBinary(cachePath, length);
   if (!data) {
     return nullptr;
   }
 
-  return new ScriptCompiler::CachedData(
-      data, (int)length,
-      isNew ? ScriptCompiler::CachedData::BufferOwned : ScriptCompiler::CachedData::BufferNotOwned);
+  return new ScriptCompiler::CachedData(data, (int)length, ScriptCompiler::CachedData::BufferOwned);
+}
+
+// A blob is published carrying its source file's modification time; that is
+// the freshness check LoadScriptCache applies, so the two must land together.
+static void WriteScriptCache(const std::string& cachePath, const std::string& sourcePath,
+                             const uint8_t* data, long length) {
+  time_t sourceModifiedTime = -1;
+  struct stat result;
+  if (stat(sourcePath.c_str(), &result) == 0) {
+    sourceModifiedTime = result.st_mtime;
+  }
+  tns::WriteBinary(cachePath, data, length, sourceModifiedTime);
 }
 
 void ModuleInternal::SaveScriptCache(const ScriptCompiler::CachedData* cache,
                                      const std::string& path, ScriptCacheKind kind) {
+  // CreateCodeCache yields nullptr for a script V8 declines to serialize.
+  if (cache == nullptr) {
+    return;
+  }
+
   std::string canonicalPath = NormalizePath(path);
   std::string cachePath = ModuleInternal::GetCacheFileName(
       canonicalPath + ScriptCacheSuffix(kind == ScriptCacheKind::kEsModule));
 
-  // std::ofstream ofs(cachePath, std::ios::binary);
-  // if (!ofs) return;  // or throw
-
-  // ofs.write(reinterpret_cast<const char*>(cache->data),
-  //           cache->length);
-  // ofs.close();
-
-  int length = cache->length;
-  tns::WriteBinary(cachePath, cache->data, length);
+  WriteScriptCache(cachePath, canonicalPath, cache->data, cache->length);
   delete cache;
-
-  // make sure cache and js file have the same modification date
-  struct stat result;
-  struct utimbuf new_times;
-  new_times.actime = time(nullptr);
-  new_times.modtime = time(nullptr);
-  if (stat(canonicalPath.c_str(), &result) == 0) {
-    auto jsLastModifiedTime = result.st_mtime;
-    new_times.modtime = jsLastModifiedTime;
-  }
-  utime(cachePath.c_str(), &new_times);
 }
 
 void ModuleInternal::SaveScriptCache(const Local<Script> script, const std::string& path) {
@@ -1944,24 +1938,15 @@ throw NativeScriptException(
   Local<UnboundScript> unboundScript = script->GetUnboundScript();
   // CachedData returned by this function should be owned by the caller (v8 docs)
   ScriptCompiler::CachedData* cachedData = ScriptCompiler::CreateCodeCache(unboundScript);
+  if (cachedData == nullptr) {
+    return;
+  }
 
-  int length = cachedData->length;
   // Always a classic script: this overload takes a v8::Script.
   std::string cachePath =
       ModuleInternal::GetCacheFileName(canonicalPath + ScriptCacheSuffix(false));
-  tns::WriteBinary(cachePath, cachedData->data, length);
+  WriteScriptCache(cachePath, canonicalPath, cachedData->data, cachedData->length);
   delete cachedData;
-
-  // make sure cache and js file have the same modification date
-  struct stat result;
-  struct utimbuf new_times;
-  new_times.actime = time(nullptr);
-  new_times.modtime = time(nullptr);
-  if (stat(canonicalPath.c_str(), &result) == 0) {
-    auto jsLastModifiedTime = result.st_mtime;
-    new_times.modtime = jsLastModifiedTime;
-  }
-  utime(cachePath.c_str(), &new_times);
 }
 
 std::string ModuleInternal::GetCacheFileName(const std::string& path) {

TestRunner/app/tests/WorkerConcurrentStartupTests.js

@@ -0,0 +1,75 @@
+describe("Worker startup", function () {
+    var moduleNames = ["modA", "modB", "modC", "modD", "modE", "modF"];
+    var entryCount = 6;
+    var expected = {};
+    moduleNames.forEach(function (name) {
+        expected[name] = require("./concurrentStartup/" + name).value;
+    });
+
+    // Jasmine arms a spec's async timeout before calling it, so the interval
+    // has to be raised ahead of the spec, not inside it.
+    var originalTimeout;
+    beforeEach(function () {
+        originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL;
+        jasmine.DEFAULT_TIMEOUT_INTERVAL = 30000;
+    });
+    afterEach(function () {
+        jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout;
+    });
+
+    // Every worker thread reads script sources and code caches while it starts;
+    // a batch started together loads the same files at the same moment, which
+    // is how an app that spins up its workers at launch behaves. The entries
+    // load the modules in rotated orders, so concurrent workers are reading
+    // different files at any instant.
+    it("many workers start at once against warm script caches", function (done) {
+        var finished = false;
+        var finish = function () {
+            if (finished) {
+                return;
+            }
+            finished = true;
+            done();
+        };
+
+        var start = function (entry, onValues) {
+            var worker = new Worker("./concurrentStartup/worker" + entry + ".js");
+            worker.onmessage = function (msg) {
+                expect(msg.data.values).toEqual(expected);
+                worker.terminate();
+                onValues();
+            };
+            worker.onerror = function (e) {
+                expect(String(e && e.message ? e.message : e)).toBe("<no worker error>");
+                worker.terminate();
+                finish();
+            };
+        };
+
+        var startBatch = function () {
+            var total = entryCount * 2;
+            var remaining = total;
+            for (var i = 0; i < total; i++) {
+                start(i % entryCount, function () {
+                    remaining--;
+                    if (remaining === 0) {
+                        finish();
+                    }
+                });
+            }
+        };
+
+        // Each entry once, one after another, so every entry's own code cache
+        // exists before the batch.
+        var warm = function (entry) {
+            if (entry === entryCount) {
+                startBatch();
+                return;
+            }
+            start(entry, function () {
+                warm(entry + 1);
+            });
+        };
+        warm(0);
+    });
+});