Changeset 242585 in webkit

Timestamp:

Mar 6, 2019, 6:03:39 PM (7 years ago)

Author:

sbarati@apple.com

Message:

JSScript should keep the cache file locked for the duration of its existence and should truncate the cache when it is out of date
​https://bugs.webkit.org/show_bug.cgi?id=195186

Reviewed by Keith Miller.

This patch makes it so that JSScript will keep its bytecode cache file
locked as long as the JSScript is alive. This makes it obvious that it's
safe to update that file, as it will only be used in a single VM, across
all processes, at a single time. We may be able to extend this in the future
if we can atomically update it across VMs/processes. However, we're choosing
more restricted semantics now as it's always easier to extend these semantics
in the future opposed to having to support the more flexible behavior
up front.

This patch also:

• Adds error messages if writing the cache fails. We don't expect this to fail, but previously we would say we cached it even if write() fails.
• Removes the unused m_moduleKey field.
• Makes calling cacheBytecodeWithError with an already non-empty cache file fail. In the future, we should extend this to just fill in the parts of the cache that are not present. But we don't have the ability to do that yet, so we just result in an error for now.

• API/JSScript.mm:

(-[JSScript dealloc]):
(-[JSScript readCache]):
(-[JSScript init]):
(-[JSScript writeCache:]):

• API/JSScriptInternal.h:
• API/tests/testapi.mm:

(testCacheFileIsExclusive):
(testCacheFileFailsWhenItsAlreadyCached):
(testObjectiveCAPI):

Location:

trunk/Source/JavaScriptCore

Files:

• API/JSScript.mm (modified) (7 diffs)
• API/JSScriptInternal.h (modified) (1 diff)
• API/tests/testapi.mm (modified) (2 diffs)
• ChangeLog (modified) (1 diff)

trunk/Source/JavaScriptCore/API/JSScript.mm

@@ -52 +52 @@
     JSC::CachedBytecode m_cachedBytecode;
     JSC::Strong<JSC::JSSourceCode> m_jsSourceCode;
-    UniquedStringImpl* m_moduleKey;
+    int m_cacheFileDescriptor;
 }
 
@@ -175 +175 @@
     if (m_cachedBytecode.size() && !m_cachedBytecode.owned())
         munmap(const_cast<void*>(m_cachedBytecode.data()), m_cachedBytecode.size());
+
+    if (m_cacheFileDescriptor != -1)
+        close(m_cacheFileDescriptor);
+
     [super dealloc];
 }
@@ -183 +187 @@
         return;
 
-    int fd = open([m_cachePath path].UTF8String, O_RDONLY);
-    if (fd == -1)
+    m_cacheFileDescriptor = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666);
+    if (m_cacheFileDescriptor == -1)
         return;
 
-    int rc = flock(fd, LOCK_SH | LOCK_NB);
-    if (rc) {
-        close(fd);
+    struct stat sb;
+    int res = fstat(m_cacheFileDescriptor, &sb);
+    size_t size = static_cast<size_t>(sb.st_size);
+    if (res || !size)
         return;
-    }
-
-    struct stat sb;
-    int res = fstat(fd, &sb);
-    size_t size = static_cast<size_t>(sb.st_size);
-    if (res || !size) {
-        close(fd);
-        return;
-    }
-
-    void* buffer = mmap(nullptr, size, PROT_READ, MAP_PRIVATE, fd, 0);
-    close(fd);
+
+    void* buffer = mmap(nullptr, size, PROT_READ, MAP_PRIVATE, m_cacheFileDescriptor, 0);
 
     JSC::CachedBytecode cachedBytecode { buffer, size };
@@ -211 +206 @@
     if (isCachedBytecodeStillValid(vm, cachedBytecode, key, m_type == kJSScriptTypeProgram ? JSC::SourceCodeType::ProgramType : JSC::SourceCodeType::ModuleType))
         m_cachedBytecode = WTFMove(cachedBytecode);
+    else
+        ftruncate(m_cacheFileDescriptor, 0);
 }
 
@@ -239 +236 @@
 @implementation JSScript(Internal)
 
+- (instancetype)init
+{
+    self = [super init];
+    if (!self)
+        return nil;
+
+    m_cacheFileDescriptor = -1;
+    return self;
+}
+
 - (unsigned)hash
 {
@@ -264 +271 @@
 - (BOOL)writeCache:(String&)error
 {
-    if (m_cachedBytecode.size())
-        return YES;
-
-    if (!m_cachePath) {
-        error = "No cache was path provided during construction of this JSScript."_s;
+    if (m_cachedBytecode.size()) {
+        error = "Cache for JSScript is already non-empty. Can not override it."_s;
+        return NO;
+    }
+
+    if (m_cacheFileDescriptor == -1) {
+        if (!m_cachePath)
+            error = "No cache was path provided during construction of this JSScript."_s;
+        else
+            error = "Could not lock the bytecode cache file. It's likely another VM or process is already using it."_s;
         return NO;
     }
@@ -288 +300 @@
     }
 
-    int fd = open([m_cachePath path].UTF8String, O_CREAT | O_WRONLY, 0666);
-    if (fd == -1) {
-        error = makeString("Unable to open file: ", [m_cachePath path].UTF8String, " due to error: ", strerror(errno));
-        return NO;
-    }
-    int returnCode = flock(fd, LOCK_EX | LOCK_NB);
-    if (returnCode)
-        error = "Unable to lock the cache file; it may already be in use."_s;
-    else
-        write(fd, m_cachedBytecode.data(), m_cachedBytecode.size());
-    close(fd);
-    return !returnCode;
+    ssize_t bytesWritten = write(m_cacheFileDescriptor, m_cachedBytecode.data(), m_cachedBytecode.size());
+    if (bytesWritten == -1) {
+        error = makeString("Could not write cache file to disk: ", strerror(errno));
+        return NO;
+    }
+
+    if (static_cast<size_t>(bytesWritten) != m_cachedBytecode.size()) {
+        ftruncate(m_cacheFileDescriptor, 0);
+        error = makeString("Could not write the full cache file to disk. Only wrote ", String::number(bytesWritten), " of the expected ", String::number(m_cachedBytecode.size()), " bytes.");
+        return NO;
+    }
+
+    return YES;
 }
 

trunk/Source/JavaScriptCore/API/JSScriptInternal.h

@@ -45 +45 @@
 @interface JSScript(Internal)
 
+- (instancetype)init;
 - (unsigned)hash;
 - (const WTF::String&)source;

trunk/Source/JavaScriptCore/API/tests/testapi.mm

@@ -2160 +2160 @@
 }
 
+static void testCacheFileIsExclusive()
+{
+    NSURL* cachePath = tempFile(@"foo.program.cache");
+
+    @autoreleasepool {
+        NSString *source = @"function foo() { return 42; } foo();";
+        NSURL* sourceURL = [NSURL URLWithString:@"my-path"];
+        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
+
+        JSScript *script1 = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
+        RELEASE_ASSERT(script1);
+        checkResult(@"Should be able to cache the first file", [script1 cacheBytecodeWithError:nil]);
+
+        JSScript *script2 = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
+        RELEASE_ASSERT(script2);
+        NSError* error = nil;
+        checkResult(@"Should NOT be able to cache the second file", ![script2 cacheBytecodeWithError:&error]);
+        checkResult(@"Should NOT be able to cache the second file has the correct error message", [[error description] containsString:@"Could not lock the bytecode cache file. It's likely another VM or process is already using it"]);
+    }
+
+    NSFileManager* fileManager = [NSFileManager defaultManager];
+    BOOL removedAll = [fileManager removeItemAtURL:cachePath error:nil];
+    checkResult(@"Successfully removed cache file", removedAll);
+}
+
+static void testCacheFileFailsWhenItsAlreadyCached()
+{
+    NSURL* cachePath = tempFile(@"foo.program.cache");
+    NSURL* sourceURL = [NSURL URLWithString:@"my-path"];
+    NSString *source = @"function foo() { return 42; } foo();";
+
+    @autoreleasepool {
+        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
+
+        JSScript *script = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
+        RELEASE_ASSERT(script);
+        checkResult(@"Should be able to cache the first file", [script cacheBytecodeWithError:nil]);
+    }
+
+    @autoreleasepool {
+        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
+
+        JSScript *script = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
+        RELEASE_ASSERT(script);
+        NSError* error = nil;
+        checkResult(@"Should not be able to cache the second time because the cache is already present", ![script cacheBytecodeWithError:&error]);
+        checkResult(@"Correct error message should be set", [[error description] containsString:@"Cache for JSScript is already non-empty. Can not override it."]);
+
+        JSContext* context = [[JSContext alloc] initWithVirtualMachine:vm];
+        JSC::Options::forceDiskCache() = true;
+        JSValue *result = [context evaluateJSScript:script];
+        RELEASE_ASSERT(result);
+        checkResult(@"Result should be 42", [result isNumber] && [result toInt32] == 42);
+        JSC::Options::forceDiskCache() = false;
+    }
+
+    NSFileManager* fileManager = [NSFileManager defaultManager];
+    BOOL removedAll = [fileManager removeItemAtURL:cachePath error:nil];
+    checkResult(@"Successfully removed cache file", removedAll);
+}
+
 @interface JSContextFileLoaderDelegate : JSContext <JSModuleLoaderDelegate>
 
@@ -2367 +2428 @@
     RUN(testBytecodeCacheWithSameCacheFileAndDifferentScript(true));
     RUN(testProgramJSScriptException());
+    RUN(testCacheFileIsExclusive());
+    RUN(testCacheFileFailsWhenItsAlreadyCached());
 
     RUN(testLoaderRejectsNilScriptURL());

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-03-06  Saam Barati  <sbarati@apple.com>
+
+        JSScript should keep the cache file locked for the duration of its existence and should truncate the cache when it is out of date
+        https://bugs.webkit.org/show_bug.cgi?id=195186
+
+        Reviewed by Keith Miller.
+
+        This patch makes it so that JSScript will keep its bytecode cache file
+        locked as long as the JSScript is alive. This makes it obvious that it's
+        safe to update that file, as it will only be used in a single VM, across
+        all processes, at a single time. We may be able to extend this in the future
+        if we can atomically update it across VMs/processes. However, we're choosing
+        more restricted semantics now as it's always easier to extend these semantics
+        in the future opposed to having to support the more flexible behavior
+        up front.
+        
+        This patch also:
+        - Adds error messages if writing the cache fails. We don't expect this to
+          fail, but previously we would say we cached it even if write() fails.
+        - Removes the unused m_moduleKey field.
+        - Makes calling cacheBytecodeWithError with an already non-empty cache file fail.
+          In the future, we should extend this to just fill in the parts of the cache
+          that are not present. But we don't have the ability to do that yet, so we
+          just result in an error for now.
+
+        * API/JSScript.mm:
+        (-[JSScript dealloc]):
+        (-[JSScript readCache]):
+        (-[JSScript init]):
+        (-[JSScript writeCache:]):
+        * API/JSScriptInternal.h:
+        * API/tests/testapi.mm:
+        (testCacheFileIsExclusive):
+        (testCacheFileFailsWhenItsAlreadyCached):
+        (testObjectiveCAPI):
+
 2019-03-06  Christopher Reid  <chris.reid@sony.com>