Skip to content

Commit 17695db

Browse files
saambaratisaambarati
committed
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): Canonical link: https://commits.webkit.org/209726@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242585 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 4331581 commit 17695db

4 files changed

Lines changed: 142 additions & 29 deletions

File tree

‎Source/JavaScriptCore/API/JSScript.mm‎

Lines changed: 42 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ @implementation JSScript {
5151
RetainPtr<NSURL> m_cachePath;
5252
JSC::CachedBytecode m_cachedBytecode;
5353
JSC::Strong<JSC::JSSourceCode> m_jsSourceCode;
54-
UniquedStringImpl* m_moduleKey;
54+
int m_cacheFileDescriptor;
5555
}
5656

5757
+ (instancetype)scriptWithSource:(NSString *)source inVirtualMachine:(JSVirtualMachine *)vm
@@ -174,6 +174,10 @@ - (void)dealloc
174174
{
175175
if (m_cachedBytecode.size() && !m_cachedBytecode.owned())
176176
munmap(const_cast<void*>(m_cachedBytecode.data()), m_cachedBytecode.size());
177+
178+
if (m_cacheFileDescriptor != -1)
179+
close(m_cacheFileDescriptor);
180+
177181
[super dealloc];
178182
}
179183

@@ -182,26 +186,17 @@ - (void)readCache
182186
if (!m_cachePath)
183187
return;
184188

185-
int fd = open([m_cachePath path].UTF8String, O_RDONLY);
186-
if (fd == -1)
189+
m_cacheFileDescriptor = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666);
190+
if (m_cacheFileDescriptor == -1)
187191
return;
188192

189-
int rc = flock(fd, LOCK_SH | LOCK_NB);
190-
if (rc) {
191-
close(fd);
192-
return;
193-
}
194-
195193
struct stat sb;
196-
int res = fstat(fd, &sb);
194+
int res = fstat(m_cacheFileDescriptor, &sb);
197195
size_t size = static_cast<size_t>(sb.st_size);
198-
if (res || !size) {
199-
close(fd);
196+
if (res || !size)
200197
return;
201-
}
202198

203-
void* buffer = mmap(nullptr, size, PROT_READ, MAP_PRIVATE, fd, 0);
204-
close(fd);
199+
void* buffer = mmap(nullptr, size, PROT_READ, MAP_PRIVATE, m_cacheFileDescriptor, 0);
205200

206201
JSC::CachedBytecode cachedBytecode { buffer, size };
207202

@@ -210,6 +205,8 @@ - (void)readCache
210205
JSC::SourceCodeKey key = m_type == kJSScriptTypeProgram ? sourceCodeKeyForSerializedProgram(vm, sourceCode) : sourceCodeKeyForSerializedModule(vm, sourceCode);
211206
if (isCachedBytecodeStillValid(vm, cachedBytecode, key, m_type == kJSScriptTypeProgram ? JSC::SourceCodeType::ProgramType : JSC::SourceCodeType::ModuleType))
212207
m_cachedBytecode = WTFMove(cachedBytecode);
208+
else
209+
ftruncate(m_cacheFileDescriptor, 0);
213210
}
214211

215212
- (BOOL)cacheBytecodeWithError:(NSError **)error
@@ -238,6 +235,16 @@ - (JSScriptType)type
238235

239236
@implementation JSScript(Internal)
240237

238+
- (instancetype)init
239+
{
240+
self = [super init];
241+
if (!self)
242+
return nil;
243+
244+
m_cacheFileDescriptor = -1;
245+
return self;
246+
}
247+
241248
- (unsigned)hash
242249
{
243250
return m_source.hash();
@@ -263,11 +270,16 @@ - (unsigned)hash
263270

264271
- (BOOL)writeCache:(String&)error
265272
{
266-
if (m_cachedBytecode.size())
267-
return YES;
273+
if (m_cachedBytecode.size()) {
274+
error = "Cache for JSScript is already non-empty. Can not override it."_s;
275+
return NO;
276+
}
268277

269-
if (!m_cachePath) {
270-
error = "No cache was path provided during construction of this JSScript."_s;
278+
if (m_cacheFileDescriptor == -1) {
279+
if (!m_cachePath)
280+
error = "No cache was path provided during construction of this JSScript."_s;
281+
else
282+
error = "Could not lock the bytecode cache file. It's likely another VM or process is already using it."_s;
271283
return NO;
272284
}
273285

@@ -287,18 +299,19 @@ - (BOOL)writeCache:(String&)error
287299
return NO;
288300
}
289301

290-
int fd = open([m_cachePath path].UTF8String, O_CREAT | O_WRONLY, 0666);
291-
if (fd == -1) {
292-
error = makeString("Unable to open file: ", [m_cachePath path].UTF8String, " due to error: ", strerror(errno));
302+
ssize_t bytesWritten = write(m_cacheFileDescriptor, m_cachedBytecode.data(), m_cachedBytecode.size());
303+
if (bytesWritten == -1) {
304+
error = makeString("Could not write cache file to disk: ", strerror(errno));
293305
return NO;
294306
}
295-
int returnCode = flock(fd, LOCK_EX | LOCK_NB);
296-
if (returnCode)
297-
error = "Unable to lock the cache file; it may already be in use."_s;
298-
else
299-
write(fd, m_cachedBytecode.data(), m_cachedBytecode.size());
300-
close(fd);
301-
return !returnCode;
307+
308+
if (static_cast<size_t>(bytesWritten) != m_cachedBytecode.size()) {
309+
ftruncate(m_cacheFileDescriptor, 0);
310+
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.");
311+
return NO;
312+
}
313+
314+
return YES;
302315
}
303316

304317
- (void)setSourceURL:(NSURL *)url

‎Source/JavaScriptCore/API/JSScriptInternal.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ class String;
4444

4545
@interface JSScript(Internal)
4646

47+
- (instancetype)init;
4748
- (unsigned)hash;
4849
- (const WTF::String&)source;
4950
- (nullable const JSC::CachedBytecode*)cachedBytecode;

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2159,6 +2159,67 @@ static void testProgramJSScriptException()
21592159
}
21602160
}
21612161

2162+
static void testCacheFileIsExclusive()
2163+
{
2164+
NSURL* cachePath = tempFile(@"foo.program.cache");
2165+
2166+
@autoreleasepool {
2167+
NSString *source = @"function foo() { return 42; } foo();";
2168+
NSURL* sourceURL = [NSURL URLWithString:@"my-path"];
2169+
JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
2170+
2171+
JSScript *script1 = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
2172+
RELEASE_ASSERT(script1);
2173+
checkResult(@"Should be able to cache the first file", [script1 cacheBytecodeWithError:nil]);
2174+
2175+
JSScript *script2 = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
2176+
RELEASE_ASSERT(script2);
2177+
NSError* error = nil;
2178+
checkResult(@"Should NOT be able to cache the second file", ![script2 cacheBytecodeWithError:&error]);
2179+
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"]);
2180+
}
2181+
2182+
NSFileManager* fileManager = [NSFileManager defaultManager];
2183+
BOOL removedAll = [fileManager removeItemAtURL:cachePath error:nil];
2184+
checkResult(@"Successfully removed cache file", removedAll);
2185+
}
2186+
2187+
static void testCacheFileFailsWhenItsAlreadyCached()
2188+
{
2189+
NSURL* cachePath = tempFile(@"foo.program.cache");
2190+
NSURL* sourceURL = [NSURL URLWithString:@"my-path"];
2191+
NSString *source = @"function foo() { return 42; } foo();";
2192+
2193+
@autoreleasepool {
2194+
JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
2195+
2196+
JSScript *script = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
2197+
RELEASE_ASSERT(script);
2198+
checkResult(@"Should be able to cache the first file", [script cacheBytecodeWithError:nil]);
2199+
}
2200+
2201+
@autoreleasepool {
2202+
JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
2203+
2204+
JSScript *script = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
2205+
RELEASE_ASSERT(script);
2206+
NSError* error = nil;
2207+
checkResult(@"Should not be able to cache the second time because the cache is already present", ![script cacheBytecodeWithError:&error]);
2208+
checkResult(@"Correct error message should be set", [[error description] containsString:@"Cache for JSScript is already non-empty. Can not override it."]);
2209+
2210+
JSContext* context = [[JSContext alloc] initWithVirtualMachine:vm];
2211+
JSC::Options::forceDiskCache() = true;
2212+
JSValue *result = [context evaluateJSScript:script];
2213+
RELEASE_ASSERT(result);
2214+
checkResult(@"Result should be 42", [result isNumber] && [result toInt32] == 42);
2215+
JSC::Options::forceDiskCache() = false;
2216+
}
2217+
2218+
NSFileManager* fileManager = [NSFileManager defaultManager];
2219+
BOOL removedAll = [fileManager removeItemAtURL:cachePath error:nil];
2220+
checkResult(@"Successfully removed cache file", removedAll);
2221+
}
2222+
21622223
@interface JSContextFileLoaderDelegate : JSContext <JSModuleLoaderDelegate>
21632224

21642225
+ (instancetype)newContext;
@@ -2366,6 +2427,8 @@ void testObjectiveCAPI(const char* filter)
23662427
RUN(testBytecodeCacheWithSameCacheFileAndDifferentScript(false));
23672428
RUN(testBytecodeCacheWithSameCacheFileAndDifferentScript(true));
23682429
RUN(testProgramJSScriptException());
2430+
RUN(testCacheFileIsExclusive());
2431+
RUN(testCacheFileFailsWhenItsAlreadyCached());
23692432

23702433
RUN(testLoaderRejectsNilScriptURL());
23712434
RUN(testLoaderRejectsFailedFetch());

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,39 @@
1+
2019-03-06 Saam Barati <sbarati@apple.com>
2+
3+
JSScript should keep the cache file locked for the duration of its existence and should truncate the cache when it is out of date
4+
https://bugs.webkit.org/show_bug.cgi?id=195186
5+
6+
Reviewed by Keith Miller.
7+
8+
This patch makes it so that JSScript will keep its bytecode cache file
9+
locked as long as the JSScript is alive. This makes it obvious that it's
10+
safe to update that file, as it will only be used in a single VM, across
11+
all processes, at a single time. We may be able to extend this in the future
12+
if we can atomically update it across VMs/processes. However, we're choosing
13+
more restricted semantics now as it's always easier to extend these semantics
14+
in the future opposed to having to support the more flexible behavior
15+
up front.
16+
17+
This patch also:
18+
- Adds error messages if writing the cache fails. We don't expect this to
19+
fail, but previously we would say we cached it even if write() fails.
20+
- Removes the unused m_moduleKey field.
21+
- Makes calling cacheBytecodeWithError with an already non-empty cache file fail.
22+
In the future, we should extend this to just fill in the parts of the cache
23+
that are not present. But we don't have the ability to do that yet, so we
24+
just result in an error for now.
25+
26+
* API/JSScript.mm:
27+
(-[JSScript dealloc]):
28+
(-[JSScript readCache]):
29+
(-[JSScript init]):
30+
(-[JSScript writeCache:]):
31+
* API/JSScriptInternal.h:
32+
* API/tests/testapi.mm:
33+
(testCacheFileIsExclusive):
34+
(testCacheFileFailsWhenItsAlreadyCached):
35+
(testObjectiveCAPI):
36+
137
2019-03-06 Christopher Reid <chris.reid@sony.com>
238

339
Followups to (r242306): Use LockHolder instead of std::lock_guard on Remote Inspector Locks

0 commit comments

Comments
 (0)