Bug 194517 – Update JSScript SPI based on feedback

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Update JSScript SPI from feedback

[Keith Miller]

Created attachment 361720 [details]
Patch

[Keith Miller]

*** Bug 194574 has been marked as a duplicate of this bug. ***

[Keith Miller]

*** Bug 194573 has been marked as a duplicate of this bug. ***

[Keith Miller]

Created attachment 361924 [details]
WIP

[Keith Miller]

rdar://problem/47712611

[Saam Barati]

Spoke with Keith. I'm going to take this over.

[Saam Barati]

Created attachment 362351 [details]
WIP

it compiles

[Saam Barati]

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.

[Saam Barati]

Created attachment 362364 [details]
WIP

[Saam Barati]

Created attachment 362365 [details]
WIP

We should actually be running the cached bytecode now, but I haven't tested it yet.

[Saam Barati]

Created attachment 362414 [details]
WIP

[Saam Barati]

Created attachment 362464 [details]
WIP

Need to add tests and clean up some code, then it should be done.

[Saam Barati]

Created attachment 362473 [details]
WIP

[Saam Barati]

Created attachment 362579 [details]
WIP

[Saam Barati]

Created attachment 362626 [details]
patch

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 362635 [details]
patch

Attempt build fixes

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 362644 [details]
patch

Try to fix more builds

[EWS Watchlist]

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.

[Keith Miller]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 362653 [details]
patch

[Saam Barati]

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.

[Saam Barati]

Created attachment 362654 [details]
patch

[EWS Watchlist]

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.

[Keith Miller]

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.

[Saam Barati]

Created attachment 362656 [details]
patch

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 362657 [details]
patch for landing

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 362663 [details]
patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 362663 [details]
patch for landing

Clearing flags on attachment: 362663

Committed r241929: <https://trac.webkit.org/changeset/241929>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.