Bug 194517

Summary: Update JSScript SPI based on feedback
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 194912    
Attachments:
Description Flags
Patch
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
patch
none
patch
none
patch
none
patch
none
patch
keith_miller: review+
patch
none
patch for landing
none
patch for landing none

Description Keith Miller 2019-02-11 15:35:31 PST
Update JSScript SPI from feedback
Comment 1 Keith Miller 2019-02-11 15:36:24 PST
Created attachment 361720 [details]
Patch
Comment 2 Keith Miller 2019-02-12 17:46:38 PST
*** Bug 194574 has been marked as a duplicate of this bug. ***
Comment 3 Keith Miller 2019-02-12 17:47:04 PST
*** Bug 194573 has been marked as a duplicate of this bug. ***
Comment 4 Keith Miller 2019-02-13 10:37:34 PST
Created attachment 361924 [details]
WIP
Comment 5 Keith Miller 2019-02-13 15:30:40 PST
rdar://problem/47712611
Comment 6 Saam Barati 2019-02-18 11:47:56 PST
Spoke with Keith. I'm going to take this over.
Comment 7 Saam Barati 2019-02-18 15:55:28 PST
Created attachment 362351 [details]
WIP

it compiles
Comment 8 Saam Barati 2019-02-18 16:58:33 PST
Comment on attachment 362351 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=362351&action=review

> Source/JavaScriptCore/API/JSContext.mm:109
> +    JSValueRef result = JSEvaluateScript(context->m_context, source.get(), nullptr, url.get(), 0, &exceptionValue);

We need to make this actually load the cached bytecode script instead of re-evaluating the script.
Comment 9 Saam Barati 2019-02-18 20:01:00 PST
Created attachment 362364 [details]
WIP
Comment 10 Saam Barati 2019-02-18 20:49:38 PST
Created attachment 362365 [details]
WIP

We should actually be running the cached bytecode now, but I haven't tested it yet.
Comment 11 Saam Barati 2019-02-19 12:25:59 PST
Created attachment 362414 [details]
WIP
Comment 12 Saam Barati 2019-02-19 18:01:19 PST
Created attachment 362464 [details]
WIP

Need to add tests and clean up some code, then it should be done.
Comment 13 Saam Barati 2019-02-19 19:46:15 PST
Created attachment 362473 [details]
WIP
Comment 14 Saam Barati 2019-02-20 17:51:12 PST
Created attachment 362579 [details]
WIP
Comment 15 Saam Barati 2019-02-21 12:00:24 PST
Created attachment 362626 [details]
patch
Comment 16 EWS Watchlist 2019-02-21 12:02:47 PST
Attachment 362626 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSAPIGlobalObject.h:60:  The parameter name "script" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSScript.mm:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393:  Declaration has space between * and variable name in char* newCWD  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2276:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Saam Barati 2019-02-21 13:12:29 PST
Created attachment 362635 [details]
patch

Attempt build fixes
Comment 18 EWS Watchlist 2019-02-21 13:14:11 PST
Attachment 362635 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393:  Declaration has space between * and variable name in char* newCWD  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2285:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Saam Barati 2019-02-21 14:16:49 PST
Created attachment 362644 [details]
patch

Try to fix more builds
Comment 20 EWS Watchlist 2019-02-21 14:20:31 PST
Attachment 362644 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393:  Declaration has space between * and variable name in char* newCWD  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2287:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Keith Miller 2019-02-21 14:30:50 PST
Comment on attachment 362635 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362635&action=review

> Source/JavaScriptCore/ChangeLog:33
> +        When writing tests, I also discovered that someone previously broke
> +        testapi. This patch also fixes those failures. They were broken when
> +        we switched to using a testapiScripts directory to hold our test .js
> +        scripts. 

Whoops!

> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:80
> +        URL referrerURL(URL(), referrerString->value(exec));

Don't you need an exception check here?

> Source/JavaScriptCore/API/JSContextPrivate.h:81
> + @discussion If the provided JSScript was created with JSScriptTypeProgram, the script will run synchronously and return the result of evaluation.

Typo: should be kJSScriptTypeProgram, I think

> Source/JavaScriptCore/API/JSContextPrivate.h:83
> + Otherwise, if the script was created with JSScriptTypeModule, the module will be run asynchronously and will return a promise resolved when the module and any transitive dependencies are loaded. The module loader will treat the script as if it had been returned from a delegate call to moduleLoaderDelegate. This mirrors the JavaScript dynamic import operation.

Ditto for kJSScriptTypeModule.

> Source/JavaScriptCore/API/JSScript.h:62
> ++ (nullable instancetype)scriptWithSource:(NSString *)source inVirtualMachine:(JSVirtualMachine *)vm;
> +
> +/*!
> + This SPI is deprecated and should not be used. Use "scriptOfType:inVirtualMachine:memoryMappedFromASCIIFile:withSourceURL:andBytecodeCache:error:" instead.
> + */
> ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath;
> +
> +/*!
> + This API is deprecated and should not be used.
> + */
> ++ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath;

Can you add deprecation warnings to these methods?

> Source/JavaScriptCore/API/JSScript.h:76
> ++ (nullable instancetype)scriptOfType:(JSScriptType)type inVirtualMachine:(JSVirtualMachine *)vm withSourceURL:(NSURL *)sourceURL andSource:(NSString *)source andBytecodeCache:(nullable NSURL *)cachePath error:(out NSError * _Nullable * _Nullable)error;

Why does this one have source at the end but the method below have it right after the VM?

I also think that the parts of the method most likely to impact the version you choose should come earlier, which is why I put the source/filePath first originally.

> Source/JavaScriptCore/API/tests/testapi.mm:2066
> +            CRASH();

I think we usually just do a check here.

> Source/JavaScriptCore/API/tests/testapi.mm:2071
> +        RELEASE_ASSERT(result);
> +        RELEASE_ASSERT([result isNumber]);

ditto.

> Source/JavaScriptCore/API/tests/testapi.mm:2098
> +    test(kJSScriptTypeProgram);
> +    test(kJSScriptTypeModule);

Can we make this a parameter and add two lines to the filter?

> Source/JavaScriptCore/API/tests/testapi.mm:2217
> +        size_t afterCount = proc_pidinfo(getpid(), PROC_PIDLISTFDS, 0, 0, 0);

You need another #ifdef here.
Comment 22 Saam Barati 2019-02-21 14:52:21 PST
Comment on attachment 362635 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362635&action=review

>> Source/JavaScriptCore/API/JSScript.h:76
>> ++ (nullable instancetype)scriptOfType:(JSScriptType)type inVirtualMachine:(JSVirtualMachine *)vm withSourceURL:(NSURL *)sourceURL andSource:(NSString *)source andBytecodeCache:(nullable NSURL *)cachePath error:(out NSError * _Nullable * _Nullable)error;
> 
> Why does this one have source at the end but the method below have it right after the VM?
> 
> I also think that the parts of the method most likely to impact the version you choose should come earlier, which is why I put the source/filePath first originally.

I thought ObjC style was to include similar parameters in the same argument index where possible. I could be wrong though, I'm not very familiar with ObjC API style design.

I moved the "source code" parameter to be at the same index for these two new APIs.

>> Source/JavaScriptCore/API/tests/testapi.mm:2066
>> +            CRASH();
> 
> I think we usually just do a check here.

That gets super cumbersome. Also, maybe crashing this script will actually make people realized they broke things. All other comments will be fixed.
Comment 23 Saam Barati 2019-02-21 15:18:53 PST
Created attachment 362653 [details]
patch
Comment 24 Saam Barati 2019-02-21 15:20:20 PST
Comment on attachment 362653 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362653&action=review

> Source/JavaScriptCore/API/tests/testapi.mm:2216
> +        checkResult(@"JSScript should not hold a file descriptor", count == afterCount);

oops, this needs to go above.
Comment 25 Saam Barati 2019-02-21 15:20:47 PST
Created attachment 362654 [details]
patch
Comment 26 EWS Watchlist 2019-02-21 15:24:16 PST
Attachment 362654 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393:  Declaration has space between * and variable name in char* newCWD  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2283:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Keith Miller 2019-02-21 15:33:24 PST
Comment on attachment 362654 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362654&action=review

r=me with comments.

> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:193
> +            args.append(createTypeError(exec, makeString("The JSScript that was provided did not have expected type of kJSScriptTypeModule.")));
> +            call(exec, deferredPromise->JSPromiseDeferred::reject(), args, "This should never be seen...");
> +            return encodedJSUndefined();

Nit: you could probably make a lambda taking the error message that calls reject and returns undefined.

> Source/JavaScriptCore/API/JSScript.h:62
> ++ (nullable instancetype)scriptWithSource:(NSString *)source inVirtualMachine:(JSVirtualMachine *)vm JSC_API_DEPRECATED("Use +scriptOfType:inVirtualMachine:withSource:andSourceURL:andBytecodeCache:error: instead.", macosx(JSC_MAC_TBA, JSC_MAC_TBA), ios(JSC_IOS_TBA, JSC_IOS_TBA));
> +
> +/*!
> + This SPI is deprecated and should not be used. Use "scriptOfType:inVirtualMachine:memoryMappedFromASCIIFile:withSourceURL:andBytecodeCache:error:" instead.
> + */
> ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath JSC_API_DEPRECATED("Use +scriptOfType:inVirtualMachine:memoryMappedFromASCIIFile:withSourceURL:andBytecodeCache:error: instead.", macosx(JSC_MAC_TBA, JSC_MAC_TBA), ios(JSC_IOS_TBA, JSC_IOS_TBA));
> +
> +/*!
> + This API is deprecated and should not be used.
> + */
> ++ (nullable instancetype)scriptFromUTF8File:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath JSC_API_DEPRECATED("Do not use this. Use +scriptOfType:inVirtualMachine:memoryMappedFromASCIIFile:withSourceURL:andBytecodeCache:error: or +scriptOfType:inVirtualMachine:withSource:andSourceURL:andBytecodeCache:error: instead", macosx(JSC_MAC_TBA, JSC_MAC_TBA), ios(JSC_IOS_TBA, JSC_IOS_TBA));

Can you make sure you update the deprecation warnings for the new method names?

> Source/JavaScriptCore/API/JSScript.h:76
> ++ (nullable instancetype)scriptOfType:(JSScriptType)type inVirtualMachine:(JSVirtualMachine *)vm withSource:(NSString *)source andSourceURL:(NSURL *)sourceURL andBytecodeCache:(nullable NSURL *)cachePath error:(out NSError * _Nullable * _Nullable)error;

Per offline discussion we should move inVirtualMachine to the second to last argument. Since, that's not a particularly interesting parameter when choosing between the "overloads".

> Source/JavaScriptCore/API/JSScript.h:92
> ++ (nullable instancetype)scriptOfType:(JSScriptType)type inVirtualMachine:(JSVirtualMachine *)vm memoryMappedFromASCIIFile:(NSURL *)filePath withSourceURL:(NSURL *)sourceURL andBytecodeCache:(nullable NSURL *)cachePath error:(out NSError * _Nullable * _Nullable)error;

ditto.
Comment 28 Saam Barati 2019-02-21 15:39:35 PST
Created attachment 362656 [details]
patch
Comment 29 EWS Watchlist 2019-02-21 15:42:16 PST
Attachment 362656 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393:  Declaration has space between * and variable name in char* newCWD  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2283:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Saam Barati 2019-02-21 15:57:15 PST
Created attachment 362657 [details]
patch for landing
Comment 31 EWS Watchlist 2019-02-21 16:01:57 PST
Attachment 362657 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393:  Declaration has space between * and variable name in char* newCWD  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2283:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Saam Barati 2019-02-21 16:22:23 PST
Created attachment 362663 [details]
patch for landing
Comment 33 EWS Watchlist 2019-02-21 16:26:19 PST
Attachment 362663 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.c:1393:  Declaration has space between * and variable name in char* newCWD  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2283:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 WebKit Commit Bot 2019-02-21 20:21:59 PST
Comment on attachment 362663 [details]
patch for landing

Clearing flags on attachment: 362663

Committed r241929: <https://trac.webkit.org/changeset/241929>
Comment 35 WebKit Commit Bot 2019-02-21 20:22:01 PST
All reviewed patches have been landed.  Closing bug.