Follow up on https://bugs.webkit.org/show_bug.cgi?id=192782
Created attachment 359045 [details] Patch
Created attachment 359899 [details] WIP patch
Created attachment 359954 [details] Patch
Created attachment 360008 [details] Patch
<rdar://problem/47514099>
Comment on attachment 360008 [details] Patch Attachment 360008 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/10873229 New failing tests: stress/ftl-get-by-id-getter-exception-interesting-live-state.js.ftl-eager-no-cjit apiTests
Is the jsc EWS failure legit? Seems like it's not.
Comment on attachment 360008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360008&action=review > Source/JavaScriptCore/API/JSScript.h:32 > +namespace JSC { > +class JSSourceCode; > +class Identifier; > +}; > + Please remove. This shouldn't use C++ features and shouldn't expose internal identifiers. > Source/JavaScriptCore/API/JSScript.h:70 > -+ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; > ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inContext:(JSContext *)context withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; > > > /*! > This is deprecated and is equivalent to scriptFromASCIIFile:inVirtualMachine:withCodeSigning:andBytecodeCache:. > */ > -+ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; > ++ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath inContext:(JSContext *)context withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; We shouldn't make this change. If we have to make a context for now that's ok. Our API shouldn't reflect our current VM limitations if we can avoid it. > Source/JavaScriptCore/API/JSScript.mm:53 > + JSScript *result = [[[JSScript alloc] init] autorelease]; Aww man! That's my bad... > Source/JavaScriptCore/runtime/Completion.cpp:119 > + ModuleProgramExecutable* executable = ModuleProgramExecutable::create(exec, source); Why isn't this the UnlinkedModuleProgramExecutable? Why do we need to have the linked module to cache?
Comment on attachment 360008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360008&action=review >> Source/JavaScriptCore/API/JSScript.h:32 >> + > > Please remove. This shouldn't use C++ features and shouldn't expose internal identifiers. Oops >> Source/JavaScriptCore/API/JSScript.h:70 >> ++ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath inContext:(JSContext *)context withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; > > We shouldn't make this change. If we have to make a context for now that's ok. Our API shouldn't reflect our current VM limitations if we can avoid it. Ok, I'll update it. >> Source/JavaScriptCore/runtime/Completion.cpp:119 >> + ModuleProgramExecutable* executable = ModuleProgramExecutable::create(exec, source); > > Why isn't this the UnlinkedModuleProgramExecutable? Why do we need to have the linked module to cache? There's no UnlinkedModuleProgramExecutable, only UnlinkedFunctionExecutable. The linked module is used for generating the bytecode in CodeCache.h, but it seems that it's mostly just used for getting the information in executableInfo(), so maybe we can remove it if you think it's worth it.
Comment on attachment 360008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360008&action=review >>> Source/JavaScriptCore/runtime/Completion.cpp:119 >>> + ModuleProgramExecutable* executable = ModuleProgramExecutable::create(exec, source); >> >> Why isn't this the UnlinkedModuleProgramExecutable? Why do we need to have the linked module to cache? > > There's no UnlinkedModuleProgramExecutable, only UnlinkedFunctionExecutable. The linked module is used for generating the bytecode in CodeCache.h, but it seems that it's mostly just used for getting the information in executableInfo(), so maybe we can remove it if you think it's worth it. I think it's worth it because then we no longer have a dependency on an ExecState.
Created attachment 360040 [details] Patch
Comment on attachment 360040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360040&action=review > Source/JavaScriptCore/API/JSScriptInternal.h:39 > -OBJC_CLASS JSScript; > +namespace JSC { > +class JSSourceCode; > +class Identifier; > +}; > > -const String& getJSScriptSourceCode(JSScript *); > +@interface JSScript(Internal) > + > +- (JSC::JSSourceCode*)jsSourceCode:(JSC::Identifier)moduleKey; > + I think you need to guard all this with #if OBJC_API_ENABLED > Source/JavaScriptCore/parser/SourceProvider.h:176 > + const CachedBytecode* m_cachedBytecode; I think this is wrong. If the JSScript is deallocated before a CachedBytecodeSourceProvider then it will unmap the byteocde underneath this provider. I think you'll need something like JSScriptSourceProvider that has a RetainPtr to the JSScript. Can you add a test for this? > Source/JavaScriptCore/runtime/CodeCache.cpp:180 > + generate(unlinkedCodeBlock->functionDecl(i), CodeForConstruct); I think we should only do CodeForCall. This is exponential and constructors are rare in comparison. It seems like we should be hash-consing the nested unlinked code blocks, which would make this linear instead of exponential. We can do that later, however, if you add a FIXME. > Source/JavaScriptCore/runtime/CodeCache.cpp:184 > + generate(unlinkedCodeBlock->functionExpr(i), CodeForConstruct); Ditto. > Source/JavaScriptCore/runtime/CodeCache.cpp:203 > + char filename[512]; > + int count = snprintf(filename, 512, "%s/%u.cache", cachePath, hash); > + if (count < 0 || count > 512) > + return; you should just use WTF::makeString for this > Source/JavaScriptCore/runtime/CodeCache.h:151 > + munmap(buffer, size); Why are you unmapping the file here? It seems like we want to test closer to what the API is doing?
Comment on attachment 360040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360040&action=review >> Source/JavaScriptCore/parser/SourceProvider.h:176 >> + const CachedBytecode* m_cachedBytecode; > > I think this is wrong. If the JSScript is deallocated before a CachedBytecodeSourceProvider then it will unmap the byteocde underneath this provider. I think you'll need something like JSScriptSourceProvider that has a RetainPtr to the JSScript. > > Can you add a test for this? Yeah, I think you're right, I'll fix this and update the test case >> Source/JavaScriptCore/runtime/CodeCache.h:151 >> + munmap(buffer, size); > > Why are you unmapping the file here? It seems like we want to test closer to what the API is doing? This is code path is not reached when testing the API. The CachedBytecode from the source provider is consumed above in line 110.
Created attachment 360114 [details] Patch
Comment on attachment 360040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360040&action=review >>> Source/JavaScriptCore/parser/SourceProvider.h:176 >>> + const CachedBytecode* m_cachedBytecode; >> >> I think this is wrong. If the JSScript is deallocated before a CachedBytecodeSourceProvider then it will unmap the byteocde underneath this provider. I think you'll need something like JSScriptSourceProvider that has a RetainPtr to the JSScript. >> >> Can you add a test for this? > > Yeah, I think you're right, I'll fix this and update the test case I added the SourceProvider to retain the JSScript, but I'm afraid I couldn't figure out a way to test it. Since the JSScript is passed to the promise resolution callback and the JSSourceCode is only extracted inside the JSAPIGlobalObject, I couldn't find a way to ensure that there's no other references to JSScript left before the accessing the cached bytecode.
Comment on attachment 360114 [details] Patch r=me. I think you can test the unmapping by making a function inside one of the modules that you call after the JSScript is released. Also, can you add a test for a constructor too?
Created attachment 360158 [details] Patch for landing
Comment on attachment 360158 [details] Patch for landing Clearing flags on attachment: 360158 Committed r240511: <https://trac.webkit.org/changeset/240511>
All reviewed patches have been landed. Closing bug.
This should probably actually be tracked with: <rdar://problem/46084932>