Bug 189809 – Add Promise SPI

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

[Keith Miller]

Add Promise SPI

[Keith Miller]

Created attachment 350327 [details]
Patch

[Keith Miller]

Created attachment 350328 [details]
Patch

patched working https://bsnlspeedtests.in/

[Keith Miller]

Created attachment 350354 [details]
Patch

[EWS Watchlist]

Attachment 350354 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1732:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1742:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 350354 [details]
Patch

Attachment 350354 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9296059

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 350363 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Keith Miller]

Created attachment 350378 [details]
Patch

[EWS Watchlist]

Attachment 350378 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1732:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1742:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 350382 [details]
Patch

[EWS Watchlist]

Attachment 350382 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1732:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1742:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 350382 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/ChangeLog:15
> +        in/to that callback the promise is automagically rejected. The

I don't know what you mean by "to that callback" here

> Source/JavaScriptCore/ChangeLog:16
> +        other methods create an pre-resolved or rejected promise as this

an => a

> Source/JavaScriptCore/ChangeLog:17
> +        appears to be the most common way to initialize a promise.

"the most common" => "a common"

> Source/JavaScriptCore/ChangeLog:24
> +        corrupt state if an Obj-C exception unwinds JS frames.

unwinds => "unwinds through" or "unwinds past"

> Source/JavaScriptCore/API/JSObjectRef.cpp:294
> +    JSPromiseDeferred::DeferredData data = JSPromiseDeferred::createDeferredData(exec, globalObject, globalObject->promiseConstructor());
> +    handleExceptionIfNeeded(scope, exec, nullptr);

The semantics here seem weird, or at least non-obvious to me, if this actually throws. I think we should set the passed in exception if this throws. Otherwise, it's pretty weird that the code below just stores null.

> Source/JavaScriptCore/API/JSValue.mm:163
> +    JSObjectRef promise = JSObjectMakeDeferredPromise([context JSGlobalContextRef], &resolve, &reject, &exception);

is it worth stating this can throw in the documentation of the SPI? When does this actually throw?

> Source/JavaScriptCore/API/JSValue.mm:180
> +        [rejection callWithArguments:@[context.exception]];

Does this automatically clear the exception? I don't think it does. Do we want to clear it?

> Source/JavaScriptCore/API/JSValuePrivate.h:38
> + being initialized. The resolve and reject parameters are functions that
> + can be called to notify any dependent promises about the state of the
> + new promise JSValue.

This explanation feels weird to me. Maybe we can state it less in terms of promises down the chain, and just in terms of resolve/reject?

> Source/JavaScriptCore/API/JSValuePrivate.h:42
> + the resolve and reject callbacks each normally take a single result value, which they

"result value" seems weird in the context of reject. Maybe just say "single value"?

> Source/JavaScriptCore/API/JSValuePrivate.h:57
> ++ (JSValue *)valueWithNewResolvedPromiseWithResult:(id)result inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA), ios(JSC_IOS_TBA));

nit: This name feels overly verbose to me. Maybe: valueWithNewPromiseResolvedWith

> Source/JavaScriptCore/API/JSValuePrivate.h:62
> + @param reason The result value to be passed to any reactions.

ditto about "result" value here. Seems like we need some other way to state failure other than "result". How does TC39 distinguish between them in naming?

> Source/JavaScriptCore/API/JSValuePrivate.h:67
> ++ (JSValue *)valueWithNewRejectedPromiseWithReason:(id)reason inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA), ios(JSC_IOS_TBA));

ditto

> Source/JavaScriptCore/API/tests/testapi.c:1982
> +    const char *scriptPath = "testapi.js";

"const char *" => "const char* "

> Source/JavaScriptCore/API/tests/testapi.cpp:88
> +    operator JSC::ExecState*() { return toJS(m_context); }

👌

> Source/JavaScriptCore/API/tests/testapi.mm:1729
> +        checkResult(@"Exception set in callback should not propegate", !context.exception);

propegate => propagate

> Source/JavaScriptCore/API/tests/testapi.mm:1739
> +        checkResult(@"Exception thrown in callback should not propegate", !context.exception);

propegate => propagate

> Source/JavaScriptCore/runtime/JSPromiseDeferred.h:48
> +        WTF_FORBID_HEAP_ALLOCATION;

style nit: Put this at the top

> Source/WTF/wtf/mac/MainThreadMac.mm:65
> +static bool mainThreadEstablishedAsPthreadMain { false };
> +static pthread_t mainThreadPthread { nullptr };
> +static NSThread* mainThreadNSThread { nullptr };

Though I like this style more, I think WK style does say we don't do this for static variables.

[Keith Miller]

Comment on attachment 350382 [details]
Patch

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

>> Source/JavaScriptCore/API/JSObjectRef.cpp:294
>> +    handleExceptionIfNeeded(scope, exec, nullptr);
> 
> The semantics here seem weird, or at least non-obvious to me, if this actually throws. I think we should set the passed in exception if this throws. Otherwise, it's pretty weird that the code below just stores null.

Whoops, I was thinking this was like the RETURN_IF_EXCEPTION macro... I'll fix that. There's not actually a way to create an exception here other than via the watchdog right now.

>> Source/JavaScriptCore/API/JSValue.mm:163
>> +    JSObjectRef promise = JSObjectMakeDeferredPromise([context JSGlobalContextRef], &resolve, &reject, &exception);
> 
> is it worth stating this can throw in the documentation of the SPI? When does this actually throw?

It doesn't right now. I don't think the SPI explicitly states which parts can throw. AFAIK, it should be assumed that any call can set the exception state.

>> Source/JavaScriptCore/API/JSValue.mm:180
>> +        [rejection callWithArguments:@[context.exception]];
> 
> Does this automatically clear the exception? I don't think it does. Do we want to clear it?

The exception property on the context isn't the same as the exception on the VM. When we return to Obj-C from JS we set the exception property on context and clear the VM. When we are calling the rejection we will enter JS code so the VM won't have a pending exception. If for what ever reason (I don't think it's possible right now) we called an Obj-C callback then we would enter a new CallbackData, which would stash the context's exception.

>> Source/JavaScriptCore/API/JSValuePrivate.h:42
>> + the resolve and reject callbacks each normally take a single result value, which they
> 
> "result value" seems weird in the context of reject. Maybe just say "single value"?

Sure.

>> Source/JavaScriptCore/API/JSValuePrivate.h:57
>> ++ (JSValue *)valueWithNewResolvedPromiseWithResult:(id)result inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA), ios(JSC_IOS_TBA));
> 
> nit: This name feels overly verbose to me. Maybe: valueWithNewPromiseResolvedWith

OK, changed.

>> Source/JavaScriptCore/API/JSValuePrivate.h:62
>> + @param reason The result value to be passed to any reactions.
> 
> ditto about "result" value here. Seems like we need some other way to state failure other than "result". How does TC39 distinguish between them in naming?

ditto.

>> Source/JavaScriptCore/API/JSValuePrivate.h:67
>> ++ (JSValue *)valueWithNewRejectedPromiseWithReason:(id)reason inContext:(JSContext *)context JSC_API_AVAILABLE(macosx(JSC_MAC_TBA), ios(JSC_IOS_TBA));
> 
> ditto

Ditto.

>> Source/JavaScriptCore/API/tests/testapi.c:1982
>> +    const char *scriptPath = "testapi.js";
> 
> "const char *" => "const char* "

switched.

>> Source/JavaScriptCore/API/tests/testapi.mm:1729
>> +        checkResult(@"Exception set in callback should not propegate", !context.exception);
> 
> propegate => propagate

Fixed. I wish Xcode had a spell checker inside things it styles as a string!

>> Source/JavaScriptCore/API/tests/testapi.mm:1739
>> +        checkResult(@"Exception thrown in callback should not propegate", !context.exception);
> 
> propegate => propagate

Ditto.

>> Source/JavaScriptCore/runtime/JSPromiseDeferred.h:48
>> +        WTF_FORBID_HEAP_ALLOCATION;
> 
> style nit: Put this at the top

Ok,

>> Source/WTF/wtf/mac/MainThreadMac.mm:65
>> +static NSThread* mainThreadNSThread { nullptr };
> 
> Though I like this style more, I think WK style does say we don't do this for static variables.

Really? Where? I searched for "static" and "global" and didn't see anything in https://webkit.org/code-style-guidelines/...

[Keith Miller]

Comment on attachment 350382 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:15
>> +        in/to that callback the promise is automagically rejected. The
> 
> I don't know what you mean by "to that callback" here

Oh, I mean if you called into JS that raises an exception and you don't clear the exception from the JSContext.

[Keith Miller]

Created attachment 350413 [details]
Patch for landing

[EWS Watchlist]

Attachment 350413 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1732:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:1742:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 350413 [details]
Patch for landing

Clearing flags on attachment: 350413

Committed r236359: <https://trac.webkit.org/changeset/236359>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44692249>

[Ryan Haddad]

This change broke the Windows build:

C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\API\tests\testapi.cpp(488): error C3861: 'strcasestr': identifier not found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\shell\testapiLib.vcxproj]

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/12011/steps/compile-webkit/logs/stdio

[Alex Christensen]

Comment on attachment 350413 [details]
Patch for landing

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

> Source/JavaScriptCore/API/tests/testapi.cpp:488
> +        return !filter || !!strcasestr(testName, filter);

This broke the Windows build.

[Ryan Haddad]

Reverted r236359 for reason:

Broke the Windows build.

Committed r236371: <https://trac.webkit.org/changeset/236371>

[Keith Miller]

relanded in <https://trac.webkit.org/changeset/236372>