RESOLVED FIXED 203400
[JSC] Remove JSPromiseDeferred
https://bugs.webkit.org/show_bug.cgi?id=203400
Summary [JSC] Remove JSPromiseDeferred
Yusuke Suzuki
Reported 2019-10-24 23:13:46 PDT
[JSC] Remove JSPromiseDeferred
Attachments
Patch (100.81 KB, patch)
2019-10-24 23:15 PDT, Yusuke Suzuki
no flags
Patch (105.78 KB, patch)
2019-10-24 23:55 PDT, Yusuke Suzuki
no flags
Patch (140.75 KB, patch)
2019-10-25 00:24 PDT, Yusuke Suzuki
no flags
Patch (163.75 KB, patch)
2019-10-25 00:34 PDT, Yusuke Suzuki
no flags
Patch (164.26 KB, patch)
2019-10-25 00:37 PDT, Yusuke Suzuki
no flags
Patch (110.95 KB, patch)
2019-10-25 01:12 PDT, Yusuke Suzuki
no flags
Patch (168.03 KB, patch)
2019-10-25 01:14 PDT, Yusuke Suzuki
no flags
Archive of layout-test-results from ews212 for win-future (13.91 MB, application/zip)
2019-10-25 03:31 PDT, EWS Watchlist
no flags
Patch (169.31 KB, patch)
2019-10-25 18:30 PDT, Yusuke Suzuki
no flags
Patch (169.27 KB, patch)
2019-10-25 20:17 PDT, Yusuke Suzuki
no flags
Patch (172.98 KB, patch)
2019-10-25 21:13 PDT, Yusuke Suzuki
no flags
Patch (169.17 KB, patch)
2019-10-25 21:23 PDT, Yusuke Suzuki
no flags
Patch (169.07 KB, patch)
2019-10-25 21:40 PDT, Yusuke Suzuki
keith_miller: review+
Yusuke Suzuki
Comment 1 2019-10-24 23:15:34 PDT
Yusuke Suzuki
Comment 2 2019-10-24 23:55:14 PDT
Yusuke Suzuki
Comment 3 2019-10-25 00:24:07 PDT
Yusuke Suzuki
Comment 4 2019-10-25 00:34:06 PDT
Yusuke Suzuki
Comment 5 2019-10-25 00:37:35 PDT
Yusuke Suzuki
Comment 6 2019-10-25 01:12:28 PDT
Yusuke Suzuki
Comment 7 2019-10-25 01:14:00 PDT
EWS Watchlist
Comment 8 2019-10-25 03:31:28 PDT
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
Comment 9 2019-10-25 03:31:30 PDT
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
Comment 10 2019-10-25 18:30:53 PDT
Yusuke Suzuki
Comment 11 2019-10-25 20:17:26 PDT
Yusuke Suzuki
Comment 12 2019-10-25 21:13:19 PDT
Yusuke Suzuki
Comment 13 2019-10-25 21:16:46 PDT
Yusuke Suzuki
Comment 14 2019-10-25 21:23:34 PDT
Yusuke Suzuki
Comment 15 2019-10-25 21:40:22 PDT
Keith Miller
Comment 16 2019-10-28 11:49:08 PDT
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 17 2019-10-28 13:02:44 PDT
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 18 2019-10-28 16:58:55 PDT
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 19 2019-10-28 18:34:48 PDT
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
Comment 20 2019-10-28 18:37:19 PDT
Yusuke Suzuki
Comment 21 2019-10-29 20:13:33 PDT
*** Bug 201160 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.