Bug 203400 – [JSC] Remove JSPromiseDeferred

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

[Yusuke Suzuki]

[JSC] Remove JSPromiseDeferred

[Yusuke Suzuki]

Created attachment 381884 [details]
Patch

[Yusuke Suzuki]

Created attachment 381888 [details]
Patch

[Yusuke Suzuki]

Created attachment 381890 [details]
Patch

[Yusuke Suzuki]

Created attachment 381891 [details]
Patch

[Yusuke Suzuki]

Created attachment 381892 [details]
Patch

[Yusuke Suzuki]

Created attachment 381893 [details]
Patch

[Yusuke Suzuki]

Created attachment 381894 [details]
Patch

[EWS Watchlist]

Comment on attachment 381894 [details]
Patch

Attachment 381894 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13175853

New failing tests:
js/dom/promise-rejection-event-should-follow-webidl-promise-conversion-rule.html

[EWS Watchlist]

Created attachment 381903 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Yusuke Suzuki]

Created attachment 381984 [details]
Patch

[Yusuke Suzuki]

Created attachment 381990 [details]
Patch

[Yusuke Suzuki]

Created attachment 381993 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/32757152>

[Yusuke Suzuki]

Created attachment 381994 [details]
Patch

[Yusuke Suzuki]

Created attachment 381995 [details]
Patch

[Keith Miller]

Comment on attachment 381995 [details]
Patch

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

> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:175
> -        return deferred->reject(globalObject, exception);
> +        promise->reject(globalObject, exception);
> +        scope.clearException();
> +        return promise;

Wait... does this mean if there's a stack overflow we just return a non-rejected promise? That seems wrong... Shouldn't we propagate that stack overflow to the caller?

[Yusuke Suzuki]

Comment on attachment 381995 [details]
Patch

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

>> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:175
>> +        return promise;
> 
> Wait... does this mean if there's a stack overflow we just return a non-rejected promise? That seems wrong... Shouldn't we propagate that stack overflow to the caller?

Currently, this patch keeps the original behavior. And I think this is not possible in practice to get stack-overflow here since module loader callbacks are driven by microtask callbacks. So this function would be typically called in the bottom of the stack.
I'm planning to add a new C++ function `Promise::rejectedPromise` to offer rejected promise without involving any JS things. This is useful to return a rejected promise when we found `scope.exception()`.
But I filed this in a separate bug for now to make this patch simple (just keeping the current behavior in this patch) https://bugs.webkit.org/show_bug.cgi?id=203402.

[Keith Miller]

Comment on attachment 381995 [details]
Patch

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

r=me but I think the functionality you marked as deprecated is public API so we can't get rid of it. Can we remove the deprecated API naming?

> Source/JavaScriptCore/runtime/Completion.cpp:197
>      scope.releaseAssertNoException();

This seems wrong.

> Source/JavaScriptCore/runtime/JSPromise.h:67
> +    // Deprecated APIs. Remove it once SPIs are removed.

I think we have shipped this API, I don't think we can remove it.

[Yusuke Suzuki]

Comment on attachment 381995 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/Completion.cpp:197
>>      scope.releaseAssertNoException();
> 
> This seems wrong.

Removed, nice!

>> Source/JavaScriptCore/runtime/JSPromise.h:67
>> +    // Deprecated APIs. Remove it once SPIs are removed.
> 
> I think we have shipped this API, I don't think we can remove it.

Yeah, we already shipped it. Remove this comment, and rename `deprecatedCreateDeferredData` to `createDeferredData`.

[Yusuke Suzuki]

Committed r251691: <https://trac.webkit.org/changeset/251691>

[Yusuke Suzuki]

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