Bug 191792 – globalFuncImportModule() should return a promise when it clears exceptions.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

<rdar://problem/46090763>

[Mark Lam]

Created attachment 355161 [details]
proposed patch.

[Keith Miller]

Comment on attachment 355161 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:813
> +        if (Exception* exception = catchScope.exception()) {
> +            catchScope.clearException();
> +            promise->reject(exec, exception->value());
> +            CLEAR_AND_RETURN_IF_EXCEPTION(catchScope, JSValue::encode(promise->promise()));
> +        }

nit: You could make this a lambda so you don't duplicate the code.

[Mark Lam]

Comment on attachment 355161 [details]
proposed patch.

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

Thanks for the reviews.

>> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:813
>> +        }
> 
> nit: You could make this a lambda so you don't duplicate the code.

It's only replicated once, and I'm not sure the lambda overhead is necessarily cheaper.  So, I'll keep this as is.

[EWS Watchlist]

Comment on attachment 355161 [details]
proposed patch.

Attachment 355161 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10027602

New failing tests:
stress/global-import-function-should-return-a-promise-when-clearing-exceptions.js.default
stress/global-import-function-should-return-a-promise-when-clearing-exceptions.js.dfg-maximal-flush-validate-no-cjit
stress/global-import-function-should-return-a-promise-when-clearing-exceptions.js.ftl-no-cjit-small-pool
stress/global-import-function-should-return-a-promise-when-clearing-exceptions.js.no-ftl
apiTests

[Mark Lam]

Created attachment 355231 [details]
proposed patch w/ test failure fixes.

Let's test this on the EWS before I get another review.

[EWS Watchlist]

Comment on attachment 355231 [details]
proposed patch w/ test failure fixes.

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html
js/dom/modules/module-incorrect-relative-specifier.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/specifier-error.html

[EWS Watchlist]

Created attachment 355233 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 355231 [details]
proposed patch w/ test failure fixes.

Attachment 355231 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10055664

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html
js/dom/modules/module-incorrect-relative-specifier.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/specifier-error.html

[EWS Watchlist]

Created attachment 355235 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

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

[EWS Watchlist]

Comment on attachment 355231 [details]
proposed patch w/ test failure fixes.

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html
js/dom/modules/module-incorrect-relative-specifier.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/specifier-error.html

[EWS Watchlist]

Created attachment 355236 [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

[EWS Watchlist]

Comment on attachment 355231 [details]
proposed patch w/ test failure fixes.

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

New failing tests:
js/dom/modules/module-incorrect-relative-specifier.html

[EWS Watchlist]

Created attachment 355237 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 355231 [details]
proposed patch w/ test failure fixes.

Attachment 355231 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10055922

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/specifier-error.html
js/dom/modules/module-incorrect-relative-specifier.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html

[EWS Watchlist]

Created attachment 355238 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Mark Lam]

Created attachment 355290 [details]
proposed patch w/ test failure fixes.

[Michael Saboff]

Comment on attachment 355290 [details]
proposed patch w/ test failure fixes.

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

r=me

> Source/JavaScriptCore/ChangeLog:11
> +        be return the empty JSValue in this case: instead, we should return a Promise as

Wording, delete "be".

[Mark Lam]

Thanks for the review.  Landed in r238391: <http://trac.webkit.org/r238391>.