@@ -204,7 +204,7 @@
                 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);
 
@@ -124,7 +124,8 @@ - (JSValue *)evaluateJSScript:(JSScript *)script
 
     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];
 
@@ -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 @@ @implementation JSScript {
     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 @@ - (void)dealloc
     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 @@ - (void)readCache
     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 @@ - (instancetype)init
     if (!self)
         return nil;
 
-    m_cacheFileDescriptor = -1;
     return self;
 }
 
@@ -260,12 +258,25 @@ - (unsigned)hash
     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 @@ - (BOOL)writeCache:(String&)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 @@ - (BOOL)writeCache:(String&)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 @@ - (void)setSourceURL:(NSURL *)url
     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
@@ -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
 
@@ -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());
 
@@ -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()
Original file line number	Diff line number	Diff line change
`@@ -204,7 +204,7 @@`
`204`	`204`	`return rejectPromise(makeString("The same JSScript was provided for two different identifiers, previously: ", oldModuleKey, " and now: ", moduleKey.string()));`
`205`	`205`	`} else {`
`206`	`206`	`[jsScript setSourceURL:[NSURL URLWithString:static_cast<NSString *>(moduleKey.string())]];`
`207`		`- source = [jsScript forceRecreateJSSourceCode];`
	`207`	`+ source = [jsScript jsSourceCode];`
`208`	`208`	`}`
`209`	`209`
`210`	`210`	`args.append(source);`