Remove retain cycle from JSScript and also don't keep the cache file descriptor open...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Mar 2019 01:10:41 +0000 (01:10 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Mar 2019 01:10:41 +0000 (01:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195782
<rdar://problem/48880625>

Reviewed by Michael Saboff.

This patch fixes two issues with JSScript API:

1. There was a retain cycle causing us to never destroy a JSScript once it
created a JSSourceCode. The reason for this is that JSScript had a
Strong<JSSourceCode> field. And JSSourceCode transitively had RetainPtr<JSScript>.

This patch fixes this issue by making the "jsSourceCode" accessor return a transient object.

2. r242585 made it so that JSScript would keep the cache file descriptor open
(and locked) for the duration of the lifetime of the JSScript itself. Our
anticipation here is that it would make implementing iterative cache updates
easier. However, this made using the API super limiting in other ways. For
example, if a program had a loop that cached 3000 different JSScripts, it's
likely that such a program would exhaust the open file count limit. This patch
reverts to the behavior prior to r242585 where we just keep open the file descriptor
while we read or write it.

* API/JSAPIGlobalObject.mm:
(JSC::JSAPIGlobalObject::moduleLoaderFetch):
* API/JSContext.mm:
(-[JSContext evaluateJSScript:]):
* API/JSScript.mm:
(-[JSScript dealloc]):
(-[JSScript readCache]):
(-[JSScript init]):
(-[JSScript sourceCode]):
(-[JSScript jsSourceCode]):
(-[JSScript writeCache:]):
(-[JSScript forceRecreateJSSourceCode]): Deleted.
* API/JSScriptInternal.h:
* API/tests/testapi.mm:
(testCanCacheManyFilesWithTheSameVM):
(testObjectiveCAPI):
(testCacheFileIsExclusive): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242980 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/API/JSAPIGlobalObject.mm
Source/JavaScriptCore/API/JSContext.mm
Source/JavaScriptCore/API/JSScript.mm
Source/JavaScriptCore/API/JSScriptInternal.h
Source/JavaScriptCore/API/tests/testapi.mm
Source/JavaScriptCore/ChangeLog

index e41f56f..70a7d24 100644 (file)
@@ -204,7 +204,7 @@ JSInternalPromise* JSAPIGlobalObject::moduleLoaderFetch(JSGlobalObject* globalOb
                 return rejectPromise(makeString("The same JSScript was provided for two different identifiers, previously: ", oldModuleKey, " and now: ", moduleKey.string()));
         } else {
             [jsScript setSourceURL:[NSURL URLWithString:static_cast<NSString *>(moduleKey.string())]];
-            source = [jsScript forceRecreateJSSourceCode];
+            source = [jsScript jsSourceCode];
         }
 
         args.append(source);
index 6608237..41cae4f 100644 (file)
 
     if (script.type == kJSScriptTypeProgram) {
         JSValueRef exceptionValue = nullptr;
-        JSValueRef result = JSEvaluateScriptInternal(locker, exec, m_context, nullptr, [script jsSourceCode]->sourceCode(), &exceptionValue);
+        JSC::SourceCode sourceCode = [script sourceCode];
+        JSValueRef result = JSEvaluateScriptInternal(locker, exec, m_context, nullptr, sourceCode, &exceptionValue);
 
         if (exceptionValue)
             return [self valueFromNotifyException:exceptionValue];
index 01f0dea..e5039ad 100644 (file)
@@ -39,6 +39,7 @@
 #import "Symbol.h"
 #include <sys/stat.h>
 #include <wtf/FileSystem.h>
+#include <wtf/Scope.h>
 
 #if JSC_OBJC_API_ENABLED
 
@@ -50,8 +51,6 @@
     RetainPtr<NSURL> m_sourceURL;
     RetainPtr<NSURL> m_cachePath;
     JSC::CachedBytecode m_cachedBytecode;
-    JSC::Strong<JSC::JSSourceCode> m_jsSourceCode;
-    int m_cacheFileDescriptor;
 }
 
 + (instancetype)scriptWithSource:(NSString *)source inVirtualMachine:(JSVirtualMachine *)vm
@@ -175,9 +174,6 @@ static JSScript *createError(NSString *message, NSError** error)
     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];
 }
 
@@ -186,27 +182,30 @@ static JSScript *createError(NSString *message, NSError** error)
     if (!m_cachePath)
         return;
 
-    m_cacheFileDescriptor = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666);
-    if (m_cacheFileDescriptor == -1)
+    int fd = open([m_cachePath path].UTF8String, O_RDONLY | O_EXLOCK | O_NONBLOCK, 0666);
+    if (fd == -1)
         return;
+    auto closeFD = makeScopeExit([&] {
+        close(fd);
+    });
 
     struct stat sb;
-    int res = fstat(m_cacheFileDescriptor, &sb);
+    int res = fstat(fd, &sb);
     size_t size = static_cast<size_t>(sb.st_size);
     if (res || !size)
         return;
 
-    void* buffer = mmap(nullptr, size, PROT_READ, MAP_PRIVATE, m_cacheFileDescriptor, 0);
+    void* buffer = mmap(nullptr, size, PROT_READ, MAP_PRIVATE, fd, 0);
 
     JSC::CachedBytecode cachedBytecode { buffer, size };
 
     JSC::VM& vm = m_virtualMachine.vm;
-    const JSC::SourceCode& sourceCode = [self jsSourceCode]->sourceCode();
+    JSC::SourceCode sourceCode = [self sourceCode];
     JSC::SourceCodeKey key = m_type == kJSScriptTypeProgram ? sourceCodeKeyForSerializedProgram(vm, sourceCode) : sourceCodeKeyForSerializedModule(vm, sourceCode);
     if (isCachedBytecodeStillValid(vm, cachedBytecode, key, m_type == kJSScriptTypeProgram ? JSC::SourceCodeType::ProgramType : JSC::SourceCodeType::ModuleType))
         m_cachedBytecode = WTFMove(cachedBytecode);
     else
-        ftruncate(m_cacheFileDescriptor, 0);
+        ftruncate(fd, 0);
 }
 
 - (BOOL)cacheBytecodeWithError:(NSError **)error
@@ -241,7 +240,6 @@ static JSScript *createError(NSString *message, NSError** error)
     if (!self)
         return nil;
 
-    m_cacheFileDescriptor = -1;
     return self;
 }
 
@@ -260,12 +258,25 @@ static JSScript *createError(NSString *message, NSError** error)
     return &m_cachedBytecode;
 }
 
-- (JSC::JSSourceCode*)jsSourceCode
+- (JSC::SourceCode)sourceCode
 {
-    if (m_jsSourceCode)
-        return m_jsSourceCode.get();
+    JSC::VM& vm = m_virtualMachine.vm;
+    JSC::JSLockHolder locker(vm);
+
+    TextPosition startPosition { };
+    String url = String { [[self sourceURL] absoluteString] };
+    auto type = m_type == kJSScriptTypeModule ? JSC::SourceProviderSourceType::Module : JSC::SourceProviderSourceType::Program;
+    Ref<JSScriptSourceProvider> sourceProvider = JSScriptSourceProvider::create(self, JSC::SourceOrigin(url), URL({ }, url), startPosition, type);
+    JSC::SourceCode sourceCode(WTFMove(sourceProvider), startPosition.m_line.oneBasedInt(), startPosition.m_column.oneBasedInt());
+    return sourceCode;
+}
 
-    return [self forceRecreateJSSourceCode];
+- (JSC::JSSourceCode*)jsSourceCode
+{
+    JSC::VM& vm = m_virtualMachine.vm;
+    JSC::JSLockHolder locker(vm);
+    JSC::JSSourceCode* jsSourceCode = JSC::JSSourceCode::create(vm, [self sourceCode]);
+    return jsSourceCode;
 }
 
 - (BOOL)writeCache:(String&)error
@@ -275,21 +286,28 @@ static JSScript *createError(NSString *message, NSError** error)
         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;
+    if (!m_cachePath) {
+        error = "No cache path was provided during construction of this JSScript."_s;
         return NO;
     }
 
+    int fd = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666);
+    if (fd == -1) {
+        error = makeString("Could not open or lock the bytecode cache file. It's likely another VM or process is already using it. Error: ", strerror(errno));
+        return NO;
+    }
+    auto closeFD = makeScopeExit([&] {
+        close(fd);
+    });
+
     JSC::ParserError parserError;
+    JSC::SourceCode sourceCode = [self sourceCode];
     switch (m_type) {
     case kJSScriptTypeModule:
-        m_cachedBytecode = JSC::generateModuleBytecode(m_virtualMachine.vm, [self jsSourceCode]->sourceCode(), parserError);
+        m_cachedBytecode = JSC::generateModuleBytecode(m_virtualMachine.vm, sourceCode, parserError);
         break;
     case kJSScriptTypeProgram:
-        m_cachedBytecode = JSC::generateProgramBytecode(m_virtualMachine.vm, [self jsSourceCode]->sourceCode(), parserError);
+        m_cachedBytecode = JSC::generateProgramBytecode(m_virtualMachine.vm, sourceCode, parserError);
         break;
     }
 
@@ -299,14 +317,14 @@ static JSScript *createError(NSString *message, NSError** error)
         return NO;
     }
 
-    ssize_t bytesWritten = write(m_cacheFileDescriptor, m_cachedBytecode.data(), m_cachedBytecode.size());
+    ssize_t bytesWritten = write(fd, 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);
+        ftruncate(fd, 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;
     }
@@ -319,22 +337,6 @@ static JSScript *createError(NSString *message, NSError** error)
     m_sourceURL = url;
 }
 
-- (JSC::JSSourceCode*)forceRecreateJSSourceCode
-{
-    JSC::VM& vm = m_virtualMachine.vm;
-    JSC::JSLockHolder locker(vm);
-
-    TextPosition startPosition { };
-
-    String url = String { [[self sourceURL] absoluteString] };
-    auto type = m_type == kJSScriptTypeModule ? JSC::SourceProviderSourceType::Module : JSC::SourceProviderSourceType::Program;
-    Ref<JSScriptSourceProvider> sourceProvider = JSScriptSourceProvider::create(self, JSC::SourceOrigin(url), URL({ }, url), startPosition, type);
-    JSC::SourceCode sourceCode(WTFMove(sourceProvider), startPosition.m_line.oneBasedInt(), startPosition.m_column.oneBasedInt());
-    JSC::JSSourceCode* jsSourceCode = JSC::JSSourceCode::create(vm, WTFMove(sourceCode));
-    m_jsSourceCode.set(vm, jsSourceCode);
-    return jsSourceCode;
-}
-
 @end
 
 #endif
index 78296b5..ee73573 100644 (file)
@@ -49,9 +49,9 @@ class String;
 - (const WTF::String&)source;
 - (nullable const JSC::CachedBytecode*)cachedBytecode;
 - (JSC::JSSourceCode*)jsSourceCode;
-// FIXME: Remove this once we require sourceURL upon creation: https://bugs.webkit.org/show_bug.cgi?id=194909
-- (JSC::JSSourceCode*)forceRecreateJSSourceCode;
+- (JSC::SourceCode)sourceCode;
 - (BOOL)writeCache:(String&)error;
+// FIXME: Remove this once we require sourceURL upon creation: https://bugs.webkit.org/show_bug.cgi?id=194909
 - (void)setSourceURL:(NSURL *)url;
 
 @end
index 96b1345..4eb6ec4 100644 (file)
@@ -2159,31 +2159,6 @@ static void testProgramJSScriptException()
     }
 }
 
-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");
@@ -2220,6 +2195,48 @@ static void testCacheFileFailsWhenItsAlreadyCached()
     checkResult(@"Successfully removed cache file", removedAll);
 }
 
+static void testCanCacheManyFilesWithTheSameVM()
+{
+    NSMutableArray *cachePaths = [[NSMutableArray alloc] init];
+    NSMutableArray *scripts = [[NSMutableArray alloc] init];
+
+    for (unsigned i = 0; i < 10000; ++i)
+        [cachePaths addObject:tempFile([NSString stringWithFormat:@"cache-%d.cache", i])];
+
+    JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
+    bool cachedAll = true;
+    for (NSURL *path : cachePaths) {
+        @autoreleasepool {
+            NSURL *sourceURL = [NSURL URLWithString:@"id"];
+            NSString *source = @"function foo() { return 42; } foo();";
+            JSScript *script = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:path inVirtualMachine:vm error:nil];
+            RELEASE_ASSERT(script);
+
+            [scripts addObject:script];
+            cachedAll &= [script cacheBytecodeWithError:nil];
+        }
+    }
+    checkResult(@"Cached all 10000 scripts", cachedAll);
+
+    JSContext *context = [[JSContext alloc] init];
+    bool all42 = true;
+    for (JSScript *script : scripts) {
+        @autoreleasepool {
+            JSValue *result = [context evaluateJSScript:script];
+            RELEASE_ASSERT(result);
+            all42 &= [result isNumber] && [result toInt32] == 42;
+        }
+    }
+    checkResult(@"All scripts returned 42", all42);
+
+    NSFileManager* fileManager = [NSFileManager defaultManager];
+    bool removedAll = true;
+    for (NSURL *path : cachePaths)
+        removedAll &= [fileManager removeItemAtURL:path error:nil];
+
+    checkResult(@"Removed all cache files", removedAll);
+}
+
 @interface JSContextFileLoaderDelegate : JSContext <JSModuleLoaderDelegate>
 
 + (instancetype)newContext;
@@ -2427,8 +2444,8 @@ void testObjectiveCAPI(const char* filter)
     RUN(testBytecodeCacheWithSameCacheFileAndDifferentScript(false));
     RUN(testBytecodeCacheWithSameCacheFileAndDifferentScript(true));
     RUN(testProgramJSScriptException());
-    RUN(testCacheFileIsExclusive());
     RUN(testCacheFileFailsWhenItsAlreadyCached());
+    RUN(testCanCacheManyFilesWithTheSameVM());
 
     RUN(testLoaderRejectsNilScriptURL());
     RUN(testLoaderRejectsFailedFetch());
index f4d5659..4f15643 100644 (file)
@@ -1,3 +1,46 @@
+2019-03-14  Saam barati  <sbarati@apple.com>
+
+        Remove retain cycle from JSScript and also don't keep the cache file descriptor open so many JSScripts can be cached in a loop
+        https://bugs.webkit.org/show_bug.cgi?id=195782
+        <rdar://problem/48880625>
+
+        Reviewed by Michael Saboff.
+
+        This patch fixes two issues with JSScript API:
+        
+        1. There was a retain cycle causing us to never destroy a JSScript once it
+        created a JSSourceCode. The reason for this is that JSScript had a 
+        Strong<JSSourceCode> field. And JSSourceCode transitively had RetainPtr<JSScript>.
+        
+        This patch fixes this issue by making the "jsSourceCode" accessor return a transient object.
+        
+        2. r242585 made it so that JSScript would keep the cache file descriptor open
+        (and locked) for the duration of the lifetime of the JSScript itself. Our
+        anticipation here is that it would make implementing iterative cache updates
+        easier. However, this made using the API super limiting in other ways. For
+        example, if a program had a loop that cached 3000 different JSScripts, it's
+        likely that such a program would exhaust the open file count limit. This patch
+        reverts to the behavior prior to r242585 where we just keep open the file descriptor
+        while we read or write it.
+
+        * API/JSAPIGlobalObject.mm:
+        (JSC::JSAPIGlobalObject::moduleLoaderFetch):
+        * API/JSContext.mm:
+        (-[JSContext evaluateJSScript:]):
+        * API/JSScript.mm:
+        (-[JSScript dealloc]):
+        (-[JSScript readCache]):
+        (-[JSScript init]):
+        (-[JSScript sourceCode]):
+        (-[JSScript jsSourceCode]):
+        (-[JSScript writeCache:]):
+        (-[JSScript forceRecreateJSSourceCode]): Deleted.
+        * API/JSScriptInternal.h:
+        * API/tests/testapi.mm:
+        (testCanCacheManyFilesWithTheSameVM):
+        (testObjectiveCAPI):
+        (testCacheFileIsExclusive): Deleted.
+
 2019-03-14  Michael Saboff  <msaboff@apple.com>
 
         ASSERTION FAILED: regexp->isValid() or ASSERTION FAILED: !isCompilationThread()